Skip to content

Commit fa51011

Browse files
fix(provenance): exclude job tables, and never let source break an insert
Both from review of #1555. Job tables were getting `_prov`. `is_entry_table` excluded the other tiers' prefixes one at a time -- `_`, `#`, and `__` for parts -- and `~` was not among them, so `~~analysis`, `~jobs` and `~lineage` all passed. The column really landed and every job row carried a payload; on a large queue that is a lot of JSON nobody asked for. It only surfaced after `jobs.refresh()` materialises the table, which is why a plain populate() in the first round of tests missed it. Now matched against `Manual.tier_regexp`. Enumerating what to exclude makes every tier added later an Entry table until someone remembers this function; matching the tier definition inverts that, so a name is an Entry table only if the library says it is. A `source` that json could not render broke every insert. `config.provenance.source` is deployment-supplied and typed `dict[str, Any]`, so a `date` in it raised `TypeError: Object of type date is not JSON serializable` from inside every insert into every Entry table, naming neither provenance nor the setting responsible. Three layers close it: - `serialize` passes `default=str`, so ordinary values a deployment would actually set -- dates, paths -- record correctly rather than failing; - a validator on the field rejects what remains (non-string keys, cycles) at assignment, where the error belongs; - `_attach_provenance` catches and logs, so recording where a row came from can never stop the row being written. That guarantee previously covered only the version call. Also fixes the `datajoint.migrate.add_prov_column` reference in the settings description -- it lives in `deploy`, and that string shows up in config help. Regression tests fail against the previous code: 4 of them, including the integration one that drives a real job queue.
1 parent fb4e15a commit fa51011

5 files changed

Lines changed: 130 additions & 12 deletions

File tree

‎src/datajoint/provenance.py‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import contextvars
3030
import datetime
3131
import json
32+
import re
3233
from typing import Any
3334

3435
#: Name of the hidden attribute. Hidden attributes are excluded from
@@ -92,12 +93,18 @@ def _jsonable(value):
9293
def is_entry_table(table_name):
9394
"""Whether a stripped table name denotes an Entry (``dj.Manual``) table.
9495
95-
Entry tables carry no tier prefix. ``#`` marks Lookup, ``_`` Imported and
96-
``__`` Computed, while a part table carries ``__`` between its master's name
97-
and its own. A part inherits its master's provenance and gets no slot of
98-
its own.
96+
Matched against ``Manual.tier_regexp``, the definition the rest of the
97+
library uses, rather than by excluding the prefixes of the other tiers.
98+
Enumerating prefixes means every tier added later is an Entry table until
99+
someone remembers this function -- which is how job tables (``~``) first
100+
acquired the slot.
101+
102+
A part table carries its master's name and ``__`` before its own, so it
103+
fails the match and inherits its master's provenance, which is what we want.
99104
"""
100-
return not table_name.startswith(("_", "#")) and "__" not in table_name
105+
from .user_tables import Manual
106+
107+
return re.fullmatch(Manual.tier_regexp, table_name) is not None
101108

102109

103110
def build_payload(connection, config=None):
@@ -141,5 +148,11 @@ def build_payload(connection, config=None):
141148

142149

143150
def serialize(payload):
144-
"""Render a payload for the ``json`` column."""
145-
return json.dumps(payload)
151+
"""Render a payload for the ``json`` column.
152+
153+
``default=str`` because ``config.provenance.source`` is deployment-supplied
154+
and typed ``dict[str, Any]``: a ``date`` or a ``Path`` in it would otherwise
155+
raise from inside every insert into every Entry table, with an error naming
156+
neither provenance nor the setting that caused it.
157+
"""
158+
return json.dumps(payload, default=str)

‎src/datajoint/settings.py‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ class ProvenanceSettings(BaseSettings):
326326
default=True,
327327
description="Add the hidden `_prov` attribute to Entry (dj.Manual) tables at declaration "
328328
"and fill it on insert. Tables declared while this is False never receive the column; "
329-
"use datajoint.migrate.add_prov_column to add it to an existing table.",
329+
"use datajoint.deploy.add_prov_column to add it to an existing table.",
330330
)
331331
source: dict[str, Any] = Field(
332332
default_factory=dict,
@@ -336,6 +336,24 @@ class ProvenanceSettings(BaseSettings):
336336
"No author supplies this at the insert call site.",
337337
)
338338

339+
@field_validator("source")
340+
@classmethod
341+
def _source_must_be_json_serializable(cls, value: dict) -> dict:
342+
"""Reject a source that cannot be recorded, at the point it is set.
343+
344+
Every insert into an Entry table serializes this. Without the check the
345+
failure surfaces from inside an unrelated insert, naming neither
346+
provenance nor the setting responsible.
347+
"""
348+
try:
349+
json.dumps(value, default=str)
350+
except (TypeError, ValueError) as error:
351+
raise ValueError(
352+
f"provenance.source must be JSON-serializable; it is recorded on every row "
353+
f"entering an Entry table. {error.__class__.__name__}: {error}"
354+
) from error
355+
return value
356+
339357

340358
class Config(BaseSettings):
341359
"""

‎src/datajoint/table.py‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -966,13 +966,23 @@ def _attach_provenance(self, rows, field_list):
966966
if not rows or not self.connection._config.provenance.capture:
967967
return
968968
if not self._has_prov_attribute():
969-
# Declared before capture was enabled. datajoint.migrate.add_prov_column
969+
# Declared before capture was enabled. datajoint.deploy.add_prov_column
970970
# adds the slot to such a table.
971971
return
972-
payload = provenance.build_payload(self.connection, self.connection._config)
973-
if payload is None:
972+
try:
973+
payload = provenance.build_payload(self.connection, self.connection._config)
974+
value = provenance.serialize(payload) if payload is not None else None
975+
except Exception as error:
976+
# Recording where a row came from must never stop it being written.
977+
# `source` is deployment-supplied and typed `dict[str, Any]`, so this
978+
# is reachable from configuration alone; the validator on that field
979+
# catches the common case at assignment, and this covers the rest.
980+
logger.warning(
981+
f"Provenance not recorded for insert into {self.full_table_name}: " f"{error.__class__.__name__}: {error}"
982+
)
983+
return
984+
if value is None:
974985
return
975-
value = provenance.serialize(payload)
976986
for row in rows:
977987
row["names"] = list(row["names"]) + [provenance.PROV_ATTRIBUTE]
978988
row["placeholders"] = list(row["placeholders"]) + ["%s"]

‎tests/integration/test_entry_provenance.py‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,3 +219,30 @@ class Legacy(dj.Manual):
219219
finally:
220220
schema.drop()
221221
dj.config.provenance.capture = original
222+
223+
224+
def test_job_tables_do_not_get_prov(schema_prov):
225+
"""Regression for the #1555 review: `~` passed the old prefix test.
226+
227+
The job table is only materialised by a refresh, so a plain populate() does
228+
not surface this -- which is how it survived the first round of tests.
229+
"""
230+
schema, t = schema_prov
231+
t["RecordingFile"].insert1({"file_id": 11, "path": "/data/b.tif"})
232+
t["Ingest"].jobs.refresh()
233+
t["Ingest"].populate(reserve_jobs=True)
234+
235+
conn = t["Ingest"]().connection
236+
tables = [r[0] for r in conn.query(f"SHOW TABLES IN `{schema.database}`").fetchall()]
237+
job_tables = [name for name in tables if name.startswith("~")]
238+
assert job_tables, "no job table was created; the test would pass vacuously"
239+
240+
for name in job_tables:
241+
columns = {
242+
r[0]
243+
for r in conn.query(
244+
"SELECT COLUMN_NAME FROM information_schema.COLUMNS " "WHERE TABLE_SCHEMA = %s AND TABLE_NAME = %s",
245+
args=(schema.database, name),
246+
).fetchall()
247+
}
248+
assert provenance.PROV_ATTRIBUTE not in columns, f"{name} carries {provenance.PROV_ATTRIBUTE}"

‎tests/unit/test_provenance.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ def config():
4040
("subject__detail", False), # Part of an Entry master
4141
("__analysis__unit", False), # Part of a Computed master
4242
("_ingest__row", False), # Part of an Imported master
43+
("~jobs", False), # job table
44+
("~~analysis", False), # per-table job queue
45+
("~lineage", False), # lineage table
4346
],
4447
)
4548
def test_is_entry_table(table_name, expected):
@@ -125,3 +128,50 @@ def test_settings_come_from_the_environment(monkeypatch):
125128
settings = ProvenanceSettings()
126129
assert settings.capture is False
127130
assert settings.source == {"system": "PyRat", "endpoint": "https://x"}
131+
132+
133+
def test_entry_test_follows_the_tier_definition_not_a_prefix_list():
134+
"""Regression for #1555 review: `~` tables were Entry tables.
135+
136+
Excluding the other tiers' prefixes one by one means any tier added later is
137+
an Entry table until someone remembers this function. Matching
138+
`Manual.tier_regexp` inverts that: a name is an Entry table only if the
139+
library says it is.
140+
"""
141+
import re
142+
143+
from datajoint.user_tables import Computed, Imported, Lookup, Manual, Part
144+
145+
assert provenance.is_entry_table("subject")
146+
for tier in (Lookup, Imported, Computed, Part):
147+
sample = {Lookup: "#param", Imported: "_ingest", Computed: "__analysis", Part: "subject__detail"}[tier]
148+
assert re.fullmatch(tier.tier_regexp, sample), f"{sample} is not a {tier.__name__}"
149+
assert not provenance.is_entry_table(sample)
150+
# The job prefix belongs to no user tier at all, which is how it slipped through.
151+
assert not any(re.fullmatch(t.tier_regexp, "~~analysis") for t in (Manual, Lookup, Imported, Computed, Part))
152+
assert not provenance.is_entry_table("~~analysis")
153+
154+
155+
def test_serialize_survives_a_deployment_supplied_source(config):
156+
"""`source` is dict[str, Any]; a date in it must not break an insert."""
157+
import datetime
158+
import pathlib
159+
160+
payload = {
161+
"time": "t",
162+
"source": {"when": datetime.date(2026, 1, 1), "where": pathlib.Path("/mnt/raw")},
163+
}
164+
rendered = json.loads(provenance.serialize(payload))
165+
assert rendered["source"] == {"when": "2026-01-01", "where": "/mnt/raw"}
166+
167+
168+
def test_source_must_be_serializable_at_assignment():
169+
"""The error belongs where the setting is made, not inside an unrelated insert."""
170+
from pydantic import ValidationError
171+
172+
from datajoint.settings import ProvenanceSettings
173+
174+
cyclic: dict = {}
175+
cyclic["self"] = cyclic
176+
with pytest.raises(ValidationError, match="JSON-serializable"):
177+
ProvenanceSettings(source=cyclic)

0 commit comments

Comments
 (0)