Skip to content

Commit d0e8a80

Browse files
fix(#1425): strict write gate no longer consumes one-shot row iterables
The strict-provenance write gate ran assert_write_allowed(self, rows) before insert materialized rows, and its key-consistency check did 'for row in rows'. For a one-shot iterable (generator, map, iter), that exhausted it, so the downstream insert saw an empty iterator and wrote zero rows with no error — silent data loss on compliant self.insert(<generator>) code (common for Part inserts). insert(<query_expression>) was also double-executed. Split the gate: assert_write_allowed(target) now does the target-only check (needs no rows) in Table.insert; a new per-row assert_row_key_allowed(row) runs in Table._insert_rows as each row is materialized — the single point reached by both the chunked and single-batch paths, so streaming/chunking is preserved and the caller's iterable is never consumed early. The QueryExpression (INSERT ... SELECT) path is no longer iterated by the gate, fixing the double-execution; per-row key consistency does not apply there (rows never materialize client-side), governed by the target check only. Adds regression tests: a generator insert into a Part must land all rows, and the per-row key check still fires for generator-sourced rows. Fixes the blocking bug flagged by @ttngu207 in review. Read-gate coverage (len/bool/in, restriction-by-table) is tracked separately pending the best-effort-vs-close decision.
1 parent f60495b commit d0e8a80

3 files changed

Lines changed: 138 additions & 30 deletions

File tree

src/datajoint/provenance.py

Lines changed: 38 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,11 @@
44
When the flag is enabled, this module's context (set by ``AutoPopulate._populate_one``)
55
tracks which tables and primary key the currently-executing ``make()`` is
66
allowed to read and write. The read gate in :func:`assert_read_allowed`
7-
fires inside ``QueryExpression.cursor``; the write gate in
8-
:func:`assert_write_allowed` fires inside ``Table.insert``.
7+
fires inside ``QueryExpression.cursor``. The write gate has two parts: the
8+
target check in :func:`assert_write_allowed` fires inside ``Table.insert``
9+
(before rows are materialized), and the per-row key-consistency check in
10+
:func:`assert_row_key_allowed` fires inside ``Table._insert_rows`` as each row
11+
is materialized — so the gate never consumes the caller's ``rows`` iterable.
912
1013
The contract is documented in
1114
``datajoint-docs/src/reference/specs/provenance.md`` §3.
@@ -122,29 +125,30 @@ def assert_read_allowed(query_expression) -> None:
122125
)
123126

124127

125-
def assert_write_allowed(target_table, rows) -> None:
128+
def assert_write_allowed(target_table) -> None:
126129
"""
127-
Verify an insert is allowed under the active strict-make context.
130+
Verify the *target* of an insert is allowed under the active strict-make context.
128131
129-
Called from ``Table.insert`` after the existing ``_allow_insert`` check.
130-
No-op when no strict-make context is active.
132+
Called from ``Table.insert`` after the existing ``_allow_insert`` check and
133+
before any rows are materialized. No-op when no strict-make context is active.
131134
132-
Allowed writes:
135+
Allowed targets:
133136
134-
- Target is the current ``make()`` target (``self``) or one of its Part
135-
tables.
136-
- Every row's primary-key columns that overlap with the current ``key``
137-
must equal ``key``'s values.
137+
- The current ``make()`` target (``self``) or one of its Part tables.
138138
139-
Anything else raises ``DataJointError``.
139+
Per-row key consistency is checked separately by :func:`assert_row_key_allowed`
140+
as rows are materialized, so this gate never consumes the caller's ``rows``
141+
iterable — a one-shot generator must survive to reach ``insert``.
142+
143+
Raises ``DataJointError`` if the target is not permitted.
140144
"""
141145
ctx = _active_strict_make.get()
142146
if ctx is None:
143147
return
144148

145-
make_target, _allowed_tables, key = ctx
149+
make_target, _allowed_tables, _key = ctx
146150

147-
# 1. Target must be `make_target` (self) or one of its Parts.
151+
# Target must be `make_target` (self) or one of its Parts.
148152
target_name = getattr(target_table, "full_table_name", None)
149153
target_set = {make_target.full_table_name}
150154
# Collect Part tables of make_target via class __dict__ (not dir/getattr,
@@ -169,17 +173,26 @@ def assert_write_allowed(target_table, rows) -> None:
169173
f"table and its Part tables may be written."
170174
)
171175

172-
# 2. Each row's key columns that overlap with the current key must match.
173-
if isinstance(rows, dict):
174-
_check_row_key(rows, key)
175-
else:
176-
try:
177-
for row in rows:
178-
if isinstance(row, dict):
179-
_check_row_key(row, key)
180-
# Non-dict rows (tuples, etc.) bypass — older API; can't check.
181-
except TypeError:
182-
pass # not iterable; let downstream code handle
176+
177+
def assert_row_key_allowed(row) -> None:
178+
"""
179+
Verify a single insert row's key columns match the active ``make()`` key.
180+
181+
Called per row from ``Table._insert_rows`` as rows are materialized, so the
182+
check sees a concrete row without the write gate having to consume the
183+
caller's ``rows`` iterable. No-op when no strict-make context is active or
184+
when ``row`` is not a dict (numpy records / bare sequences carry no field
185+
names to check by — same as the previous behavior).
186+
187+
Raises ``DataJointError`` on a mismatch.
188+
"""
189+
ctx = _active_strict_make.get()
190+
if ctx is None:
191+
return
192+
if not isinstance(row, dict):
193+
return
194+
_make_target, _allowed_tables, key = ctx
195+
_check_row_key(row, key)
183196

184197

185198
def _check_row_key(row: dict, current_key: dict) -> None:

src/datajoint/table.py

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -797,16 +797,23 @@ def insert(
797797
" To override, set keyword argument allow_direct_insert=True."
798798
)
799799

800-
# Strict-provenance write gate. No-op outside make() or when the
801-
# config flag is off. See src/datajoint/provenance.py.
800+
# Strict-provenance write gate (target check only). No-op outside make()
801+
# or when the config flag is off. Deliberately does NOT touch `rows` —
802+
# the per-row key-consistency check happens in `_insert_rows` as rows are
803+
# materialized, so a one-shot iterable (generator) is not consumed here.
804+
# See src/datajoint/provenance.py.
802805
from .provenance import assert_write_allowed
803806

804-
assert_write_allowed(self, rows)
807+
assert_write_allowed(self)
805808

806809
if inspect.isclass(rows) and issubclass(rows, QueryExpression):
807810
rows = rows() # instantiate if a class
808811
if isinstance(rows, QueryExpression):
809-
# insert from select - chunk_size not applicable
812+
# insert from select - chunk_size not applicable.
813+
# Note: this INSERT ... SELECT runs entirely server-side, so under
814+
# strict_provenance the per-row key-consistency check does not apply
815+
# (row values are never materialized client-side). The target check
816+
# in assert_write_allowed above still governs which table is written.
810817
if chunk_size is not None:
811818
raise DataJointError("chunk_size is not supported for QueryExpression inserts")
812819
if not ignore_extra_fields:
@@ -861,7 +868,17 @@ def _insert_rows(self, rows, replace, skip_duplicates, ignore_extra_fields):
861868
"""
862869
# collects the field list from first row (passed by reference)
863870
field_list = []
864-
rows = list(self.__make_row_to_insert(row, field_list, ignore_extra_fields) for row in rows)
871+
# Strict-provenance per-row key check runs here, as each row is
872+
# materialized — no-op outside make()/when the flag is off. Placing it in
873+
# this single materialization point (reached by both the chunked and
874+
# single-batch paths) avoids consuming the caller's `rows` iterable early.
875+
from .provenance import assert_row_key_allowed
876+
877+
def _make_row(row):
878+
assert_row_key_allowed(row)
879+
return self.__make_row_to_insert(row, field_list, ignore_extra_fields)
880+
881+
rows = list(_make_row(row) for row in rows)
865882
if rows:
866883
try:
867884
# Handle empty field_list (all-defaults insert)

tests/integration/test_strict_provenance.py

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,84 @@ def make(self, key):
212212
assert len(Master.Bin & {"subject_id": 1}) == 3
213213

214214

215+
def test_strict_generator_insert_not_dropped(prefix, connection_test, strict_mode):
216+
"""Regression (#1474 bug 1): a one-shot generator of compliant rows must not
217+
be consumed by the write gate. Before the fix, assert_write_allowed iterated
218+
`rows` for its key check, exhausting the generator so insert saw zero rows and
219+
silently wrote nothing."""
220+
schema = dj.Schema(f"{prefix}_strict_generator", connection=connection_test)
221+
222+
@schema
223+
class Subject(dj.Lookup):
224+
definition = """
225+
subject_id : int32
226+
"""
227+
contents = [(1,), (2,)]
228+
229+
@schema
230+
class Spectrum(dj.Computed):
231+
definition = """
232+
-> Subject
233+
---
234+
n : int32
235+
"""
236+
237+
class Bin(dj.Part):
238+
definition = """
239+
-> master
240+
bin_id : int32
241+
---
242+
energy : float64
243+
"""
244+
245+
def make(self, key):
246+
n = 5
247+
self.insert1({**key, "n": n})
248+
# one-shot generator (not a list) — must survive the write gate
249+
self.Bin.insert({**key, "bin_id": i, "energy": float(i)} for i in range(n))
250+
251+
Spectrum.populate()
252+
for sid in (1, 2):
253+
assert (Spectrum & {"subject_id": sid}).fetch1("n") == 5
254+
# The core assertion: all 5 generated rows landed, none silently dropped.
255+
assert len(Spectrum.Bin & {"subject_id": sid}) == 5
256+
257+
258+
def test_strict_generator_insert_mismatched_key_still_caught(prefix, connection_test, strict_mode):
259+
"""The per-row key check still fires when rows come from a generator — a row
260+
whose key disagrees with the current make() key raises, not silently passes."""
261+
schema = dj.Schema(f"{prefix}_strict_gen_mismatch", connection=connection_test)
262+
263+
@schema
264+
class Subject(dj.Lookup):
265+
definition = """
266+
subject_id : int32
267+
"""
268+
contents = [(1,)]
269+
270+
@schema
271+
class Derived(dj.Computed):
272+
definition = """
273+
-> Subject
274+
---
275+
val : int32
276+
"""
277+
278+
class Bin(dj.Part):
279+
definition = """
280+
-> master
281+
bin_id : int32
282+
"""
283+
284+
def make(self, key):
285+
self.insert1({**key, "val": 0})
286+
# generator whose 3rd row carries a bogus subject_id
287+
self.Bin.insert({**({**key, "subject_id": 999} if i == 2 else key), "bin_id": i} for i in range(4))
288+
289+
with pytest.raises(DataJointError, match="does not match the current make"):
290+
Derived.populate()
291+
292+
215293
def test_strict_off_by_default_no_change(prefix, connection_test):
216294
"""With strict_provenance unset (default False), existing patterns work unchanged."""
217295
schema = dj.Schema(f"{prefix}_strict_default_off", connection=connection_test)

0 commit comments

Comments
 (0)