Repository navigation
fix: validate attribute types at declaration and insert (#1527, #1528, #1529, #1530) - #1531
dimitri-yatsenko merged 4 commits into
Conversation
Sorry, something went wrong.
…#1529, #1530) Four bugs with one shape: the type system accepted spellings it could not honour, and failed late — at the driver or the server — instead of at declaration. #1527 A native blob column accepted any object. DataJoint passed it to the driver untouched; PyMySQL has no encoder for ndarray and falls back to str(value), so an array was stored as its text repr, elided in the middle for large arrays, with no error on insert or on fetch. __make_placeholder now requires bytes for a codec-less blob attribute. The declaration warning for native blob types now says the column stores raw bytes and names <blob>, rather than the generic portability note. #1528 decimal(M,D) carrying `unsigned` or `zerofill`, spelled `dec`/`fixed`, or given a single argument was rejected outright, where 0.14.x accepted it. 2.x promoted decimal to a core type with a strict pattern and left the old permissive branch under the name NUMERIC, which kept `numeric` and dropped `decimal` — so `numeric(2,2) unsigned` passed while the canonical spelling of the same SQL type did not. NUMERIC covers all four aliases again; the core DECIMAL pattern still wins for decimal(M,D). Heading derives its `numeric` flag from NUMERIC too, so a modified decimal is no longer classified as neither numeric nor string. #1529 NATIVE_TO_CORE_TYPE mapped unsigned integers to uint8/uint16/uint24/ uint32/uint64 and mediumint to int24, none of which exist as core types. Phase 2 migration wrote :uint32: markers that Heading then silently dropped. Unsigned columns now widen to the next signed type that holds their range, as the migration guide already documented. bigint unsigned has no lossless target and logs a warning. #1530 match_type used re.match, and the $ in the INTEGER pattern bound to the `serial` alternative only, so `int24`, `tinyinteger` and `intbanana` all classified as native integers and were emitted into the DDL verbatim. Now fullmatch, with the INTEGER alternation anchored as a whole. Also: bare `blob` is a MySQL type and was rejected while the sized variants were accepted, though migrate.BLOB_TYPES already recognized it. Found by the new cross-reference test. tests/unit/test_type_patterns.py asserts that every type named in NATIVE_TO_CORE_TYPE round-trips through match_type, that near-miss spellings raise, and that the native spellings still accepted are not regressed by the strictness change.
The migration guide promised `decimal(M,D) → decimal(M,D) # unchanged`, which is true only of the canonical two-argument form. The same SQL type carrying a modifier or spelled with an alias is a native type, not a core type, and until datajoint/datajoint-python#1528 it was rejected outright — so readers with an unsigned decimal hit an error immediately after reading that it was unchanged. - state that decimal is a core type only as decimal(M,D), and that decimal(M,D) unsigned / decimal(M) / numeric / dec / fixed pass as native - complete the unsigned-integer table: add mediumint, and the signed rows that were implied but missing - call out bigint unsigned as the one lossy widening, since values above 2**63-1 do not fit in int64; dj.migrate warns per column - add bare `blob` and `tinyblob` to the codec mapping — both are MySQL types and both migrate to <blob> Documents the behaviour of datajoint/datajoint-python#1531.
MilagrosMarin
left a comment
There was a problem hiding this comment.
Verified the four fixes. The fullmatch change is safe — match_type() is only reached from declare.py:857/:957, both on the declaration path, never on server-reported types, and the patterns involved already end in $. The insert guard correctly gates on attr.is_blob and attr.codec is None.
On the bigint unsigned call you flagged — traced it through:
migrate_columns emits MODIFY COLUMN {native_type} … COMMENT, so the physical column is preserved and stored data is untouched; values above 2**63-1 still fetch fine. The effect is descriptive: heading.py:517 sets original_type = "int64", and Table.describe() renders original_type or attr.type — so describe() reports int64 for a column that is physically bigint unsigned.
That's where it bites: the describe→recreate→copy path produces a signed bigint, and values above 2**63-1 don't survive that copy. Nothing is lost at migration time, and the warning is well-placed, but the label is what describe() echoes downstream.
Worth weighing as an alternative: leave bigint unsigned out of NATIVE_TO_CORE_TYPE and give it an explicit warn-and-skip branch. The column then stays honestly native and describe() keeps reporting bigint unsigned. It would need the explicit branch — unmapped types currently fall through the "silently skipped" path, and silence would be worse than what you have now. Your call; both are defensible, but the describe() round-trip is the consequence I'd weigh it against.
Smaller: the new heading.py comment says prefix matching is what lets PostgreSQL spellings like "double precision" and "timestamp without time zone" match on their leading word. Testing both directly, re.match returns None for each — FLOAT and TEMPORAL both end in $, and every pattern in those tuples is anchored, so .match() and .fullmatch() are equivalent there. No runtime effect; just a rationale future maintainers would take at face value.
Sorry, something went wrong.
…-trip) and the lossless escape hatch (decimal(20,0)); fix the match-vs-fullmatch comment
|
Addressed the review:
|
Sorry, something went wrong.
|
Rebased onto master (merge commit — the branch already carried one) and re-running CI. Picks up the two PRs that landed just now, #1549 and #1548; no conflicts despite #1548 touching the same declare.py and table.py. All checks green, and 387 unit tests pass locally. @ttngu207 this is the last item queued for the 2.3.3 tag. One thing worth your explicit sign-off before it goes in, since downstream consumers will pin >=2.3.3 soon after the release: The #1527 fix turns a silent corruption into a hard error. Inserting an ndarray into a native blob attribute (no codec) previously stored str(array) — the text repr, elided in the middle for anything large — with no error on insert and none on fetch. It now raises, naming <blob> as the fix. That is the correct behavior and it is what the issue asks for, but it is a new exception in a patch release. On PostgreSQL the same insert already raised can't adapt type, so this only changes MySQL behavior; the guard makes the two backends agree. Ask: confirm no production pipeline you know of inserts arrays into a native blob rather than a <blob> codec attribute. Anything that does has been writing corrupt data and will now fail loudly on its next insert — better, but it will fail at an inconvenient moment. If you would rather not take that on in this release, say so and it moves to 2.3.4; nothing here blocks the rest of 2.3.3. |
Sorry, something went wrong.
Closes #1527, closes #1528, closes #1529, closes #1530.
Four bugs with one shape: the type system accepted spellings it could not honour, and
failed late — at the driver or at the server — instead of at declaration. Fixing them
separately would have meant four passes over the same twenty lines, so they are here
together.
#1527 — native blob silently stored str(array)
__make_placeholder handed the value to the driver untouched for a codec-less blob
attribute. PyMySQL has no encoder for ndarray and falls back to escape_str, so an
array was stored as its text repr — elided in the middle for anything large — with no
error on insert and none on fetch. It now requires bytes/bytearray/memoryview
and names <blob> in the error. This is the one with actual data loss.
The declaration warning for native blob types also now says the column stores raw
bytes and points at <blob>, instead of the generic "consider a core DataJoint type
for better portability", which reads as cosmetic.
Note the failure was MySQL-specific: on PostgreSQL the same insert already raised
can't adapt type, because that adapter registers numpy scalars only. The guard makes
the two backends agree.
#1528 — decimal(M,D) unsigned rejected outright
A regression from 0.14.x, which had one permissive pattern for both spellings. 2.x
promoted decimal to a core type with a strict pattern and left the old permissive
branch under the name NUMERIC — which kept numeric and dropped decimal. The
result was that numeric(2,2) unsigned passed while the canonical spelling of the
same SQL type raised. NUMERIC now covers decimal/numeric/dec/fixed with
unsigned/zerofill; the core DECIMAL pattern is still matched first, so
decimal(M,D) remains a core type.
Heading derives its numeric flag from these same patterns and did not consult
NUMERIC, so a modified decimal was classified as neither numeric nor string. Fixed
in the same place.
Found in element-optogenetics, which declares four decimal(2, 2) unsigned
proportions and cannot be declared at all on 2.x.
#1529 — migration wrote core-type markers that do not exist
NATIVE_TO_CORE_TYPE mapped unsigned integers to uint8…uint64 and mediumint to
int24, none of which match_type accepts. Phase 2 stamped :uint32: into column
comments and Heading then silently dropped the marker, so the effect was cosmetic
rather than destructive — but the labels meant nothing, and the guide documented a
different mapping than the code performed.
Unsigned columns now widen to the next signed type that holds their full range, which
is what migrate-to-v20.md already said. bigint unsigned is the one case with no
lossless target — values above 2**63-1 do not fit in int64 — so it maps to
int64 and logs a warning naming the column. That is the decision most worth a second
opinion in this PR.
#1530 — misspelled types classified as native integers
match_type used re.match, and the $ in the INTEGER pattern bound to the
serial alternative only, so int24, tinyinteger and intbanana all matched on
their prefix and were emitted into the DDL verbatim — producing a MySQL syntax error
instead of Unsupported attribute type. Now fullmatch, with the alternation anchored
as a whole.
tests/integration/test_declare.py::test_unsupported_int_datatype already asserted
this behaviour and was passing only because the server rejected the DDL; it now passes
for the right reason.
I did not move the TYPE_PATTERN call sites in heading.py to fullmatch, which
#1530 suggested. Those match against types reported by the server, which are not
normalized — PostgreSQL returns double precision and timestamp without time zone,
and both currently classify on their leading word. fullmatch there would break the
PostgreSQL backend. Normalizing adapter-reported types is the real fix and is a larger
change; I left a comment at the call site saying so.
Also: bare blob
NATIVE_BLOB required a size prefix, so blob — a MySQL type in its own right, and
one migrate.BLOB_TYPES already recognized — was rejected while tinyblob and
longblob were accepted. The size prefix is now optional. This was found by the new
cross-reference test rather than by reading, and it is real: element-moseq declares
three bare blob attributes.
Testing
tests/unit/test_type_patterns.py covers the seam between the three places that name
a type. The assertions that would have caught these:
strictness change cannot quietly narrow what is accepted
Plus integration tests for the insert guard, a bare blob round-trip, and declaring
and reading back decimal(2, 2) unsigned.
Full suite green on both backends: 1021 passed, 10 skipped. ruff, ruff-format and
mypy clean at the versions pinned in .pre-commit-config.yaml.
Follow-up
The documentation half needs a datajoint-docs PR: migrate-to-v20.md:1180 currently
promises decimal(M,D) → decimal(M,D) # unchanged, and :1080 documents the
unsigned-integer mapping this PR aligns the code to. type-system.md should also say
what happens to native modifiers. Not in this repo, so not in this PR.