From 49fd1e44c3214a4a3d6002a44c855334b33546f6 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Mon, 10 Aug 2026 09:10:24 -0500 Subject: [PATCH] fix: validate attribute types at declaration and insert (#1527, #1528, #1529, #1530) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 , 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. --- src/datajoint/declare.py | 27 ++++- src/datajoint/heading.py | 6 +- src/datajoint/migrate.py | 32 ++++-- src/datajoint/table.py | 14 ++- tests/integration/test_declare.py | 70 +++++++++++++ tests/unit/test_type_patterns.py | 160 ++++++++++++++++++++++++++++++ 6 files changed, 294 insertions(+), 15 deletions(-) create mode 100644 tests/unit/test_type_patterns.py diff --git a/src/datajoint/declare.py b/src/datajoint/declare.py index dfd4c85df..4fc127921 100644 --- a/src/datajoint/declare.py +++ b/src/datajoint/declare.py @@ -64,12 +64,17 @@ # Core DataJoint types **{name.upper(): pattern for name, (pattern, _) in CORE_TYPES.items()}, # Native SQL types (passthrough with warning for non-standard use) - INTEGER=r"((tiny|small|medium|big|)int|integer)(\s*\(.+\))?(\s+unsigned)?(\s+auto_increment)?|serial$", - NUMERIC=r"numeric(\s*\(.+\))?(\s+unsigned)?$", # numeric is SQL alias, use decimal instead + INTEGER=r"(((tiny|small|medium|big|)int|integer)(\s*\(.+\))?(\s+unsigned)?(\s+auto_increment)?|serial)$", + # decimal/numeric/dec/fixed are the same SQL type. The canonical core spelling + # decimal(M,D) is matched above; anything carrying a modifier or an alias falls + # through to here and passes as a native type, as int/float unsigned do. + NUMERIC=r"(decimal|numeric|dec|fixed)(\s*\(.+\))?(\s+unsigned)?(\s+zerofill)?$", FLOAT=r"(double|float|real)(\s*\(.+\))?(\s+unsigned)?$", STRING=r"(var)?char\s*\(.+\)$", # Catches char/varchar not matched by core types TEMPORAL=r"(time|timestamp|year)(\s*\(.+\))?$", # time, timestamp, year (not date/datetime) - NATIVE_BLOB=r"(tiny|small|medium|long)blob$", # Specific blob variants + # Size prefix is optional: bare `blob` is a MySQL type in its own right, and is + # already recognized by migrate.BLOB_TYPES. + NATIVE_BLOB=r"(tiny|small|medium|long)?blob$", NATIVE_TEXT=r"(tiny|small|medium|long)?text$", # Native text types (not portable) # Codecs use angle brackets CODEC=r"<.+>$", @@ -107,8 +112,12 @@ def match_type(attribute_type: str) -> str: DataJointError If the type string doesn't match any known pattern. """ + # fullmatch, not match: a declared type must be consumed in its entirety. With + # a prefix match, a misspelling such as `int24` or `tinyinteger` classifies as a + # native integer and is emitted into the DDL verbatim, so the failure surfaces as + # a SQL syntax error from the server rather than as an unsupported-type error here. try: - return next(category for category, pattern in TYPE_PATTERN.items() if pattern.match(attribute_type)) + return next(category for category, pattern in TYPE_PATTERN.items() if pattern.fullmatch(attribute_type)) except StopIteration: raise DataJointError("Unsupported attribute type {type}".format(type=attribute_type)) @@ -913,6 +922,16 @@ def compile_attribute( # Core types and Codecs are recorded in comment for reconstruction match["comment"] = ":{type}:{comment}".format(**match) substitute_special_type(match, category, foreign_key_sql, context, adapter) + elif category == "NATIVE_BLOB": + # A native blob is a raw binary column, not a DataJoint blob. Say so plainly: + # the generic portability warning below reads as cosmetic, and users migrating + # from pre-2.0 expect `longblob` to serialize objects for them. + logger.warning( + f"Native type '{match['type']}' is used in attribute '{match['name']}'. " + "It stores raw bytes only — values are not serialized on write or " + "deserialized on read. Use '' to store arrays and other Python " + "objects, or 'bytes' if raw binary is intended." + ) elif category in NATIVE_TYPES: # Native type - warn user logger.warning( diff --git a/src/datajoint/heading.py b/src/datajoint/heading.py index 722c412f4..a405ea27d 100644 --- a/src/datajoint/heading.py +++ b/src/datajoint/heading.py @@ -484,7 +484,11 @@ def _init_from_database(self) -> None: in_key=(attr["key"] == "PRI"), nullable=attr["nullable"], # Already boolean from parse_column_info autoincrement=bool(re.search(r"auto_increment", attr["extra"], flags=re.I)), - numeric=any(TYPE_PATTERN[t].match(attr["type"]) for t in ("DECIMAL", "INTEGER", "FLOAT")), + # NOTE: these use prefix matching, unlike match_type() which requires a + # full match. attr["type"] is reported by the server and is not + # normalized, so PostgreSQL spellings such as "double precision" and + # "timestamp without time zone" are matched on their leading word. + numeric=any(TYPE_PATTERN[t].match(attr["type"]) for t in ("DECIMAL", "NUMERIC", "INTEGER", "FLOAT")), string=any(TYPE_PATTERN[t].match(attr["type"]) for t in ("ENUM", "TEMPORAL", "STRING")), is_blob=any(TYPE_PATTERN[t].match(attr["type"]) for t in ("BYTES", "NATIVE_BLOB")), uuid=False, diff --git a/src/datajoint/migrate.py b/src/datajoint/migrate.py index 1f174ccfd..f20d6ee43 100644 --- a/src/datajoint/migrate.py +++ b/src/datajoint/migrate.py @@ -61,18 +61,25 @@ # Column Type Migration (Phase 2) # ============================================================================= -# Mapping from MySQL native types to DataJoint core types +# Mapping from MySQL native types to DataJoint core types. +# +# Every value here must be a type that `declare.match_type` accepts: the values are +# written into column comments as `::` markers and read back by +# `heading.Heading._init_from_database`. DataJoint 2.0 provides no unsigned integer +# types, so unsigned columns widen to the next signed type that holds their full range. +# See `test_migrate_core_types_are_declarable` for the assertion that enforces this. NATIVE_TO_CORE_TYPE = { - # Unsigned integers - "tinyint unsigned": "uint8", - "smallint unsigned": "uint16", - "mediumint unsigned": "uint24", - "int unsigned": "uint32", - "bigint unsigned": "uint64", + # Unsigned integers widen to the next signed type. bigint unsigned is the one case + # with no lossless target: values above 2**63-1 do not fit in int64. + "tinyint unsigned": "int16", + "smallint unsigned": "int32", + "mediumint unsigned": "int32", + "int unsigned": "int64", + "bigint unsigned": "int64", # Signed integers "tinyint": "int8", "smallint": "int16", - "mediumint": "int24", + "mediumint": "int32", # int24 is not a core type "int": "int32", "bigint": "int64", # Floats @@ -191,6 +198,13 @@ def analyze_columns(schema: Schema) -> dict: # Handle numeric types elif lookup_type in NATIVE_TO_CORE_TYPE: col_info["core_type"] = NATIVE_TO_CORE_TYPE[lookup_type] + if lookup_type == "bigint unsigned": + logger.warning( + f"Column `{col_info['table']}`.`{col_info['column']}` is " + "`bigint unsigned` and will be labeled `int64`. DataJoint 2.0 " + "provides no unsigned integer types; values above 2**63-1 do " + "not fit in int64. Verify the stored range before migrating." + ) result["needs_migration"].append(col_info) # Types that don't need migration (varchar, date, datetime, json, etc.) # are silently skipped @@ -211,7 +225,7 @@ def migrate_columns( Migrates: - - Numeric types: int unsigned → :uint32:, smallint → :int16:, etc. + - Numeric types: int unsigned → :int64:, smallint → :int16:, etc. - Blob types: longblob → :: Does NOT migrate external storage columns (external-*, attach@*, diff --git a/src/datajoint/table.py b/src/datajoint/table.py index 6406acd24..a97616133 100644 --- a/src/datajoint/table.py +++ b/src/datajoint/table.py @@ -1415,7 +1415,19 @@ def __make_placeholder(self, name, value, ignore_extra_fields=False, row=None): # Numeric - convert to string elif attr.numeric: value = str(int(value) if isinstance(value, (bool, np.bool_)) else value) - # Blob - pass through as bytes (use for automatic serialization) + # Native blob - raw bytes only. Anything else is rejected here rather than + # handed to the driver: PyMySQL has no encoder for objects such as ndarray + # and falls back to str(value), which stores the text repr of the object + # (elided in the middle, for a large array) with no error on insert or on + # fetch. Use for serialization. + elif attr.is_blob and attr.codec is None: + if not isinstance(value, (bytes, bytearray, memoryview)): + raise DataJointError( + f"Attribute `{name}` is declared as the native binary type " + f"`{attr.type}`, which stores raw bytes, but a value of type " + f"`{type(value).__name__}` was given. Declare the attribute as " + "'' to store arrays and other Python objects, or pass bytes." + ) return name, placeholder, value diff --git a/tests/integration/test_declare.py b/tests/integration/test_declare.py index 19e711e96..7db8c5ce6 100644 --- a/tests/integration/test_declare.py +++ b/tests/integration/test_declare.py @@ -470,3 +470,73 @@ class Metadata(dj.Lookup): # Description should show just the secondary attribute assert "info" in description # _singleton is hidden, implementation detail + + +def test_native_blob_rejects_non_bytes(schema_any): + """ + A native blob column stores raw bytes. Handing it an object silently stored + str(object) before the guard in __make_placeholder; it must raise instead. + """ + import numpy as np + + @schema_any + class RawBinary(dj.Manual): + definition = """ + id : int32 + --- + data : longblob + """ + + with pytest.raises(dj.DataJointError, match="native binary type"): + RawBinary.insert1({"id": 1, "data": np.arange(500, dtype="float32")}) + with pytest.raises(dj.DataJointError, match="native binary type"): + RawBinary.insert1({"id": 2, "data": [1, 2, 3]}) + with pytest.raises(dj.DataJointError, match="native binary type"): + RawBinary.insert1({"id": 3, "data": "text"}) + + # bytes are what the column is for, and must round-trip untouched + RawBinary.insert1({"id": 4, "data": b"\x00\x01\x02"}) + assert (RawBinary & "id=4").fetch1("data") == b"\x00\x01\x02" + assert len(RawBinary()) == 1 # none of the rejected inserts landed + RawBinary.drop_quick() + + +def test_bare_blob_datatype(schema_any): + """`blob` without a size prefix is a MySQL type in its own right.""" + + @schema_any + class BareBlob(dj.Manual): + definition = """ + id : int32 + --- + data : blob + """ + + BareBlob.insert1({"id": 1, "data": b"abc"}) + assert (BareBlob & "id=1").fetch1("data") == b"abc" + BareBlob.drop_quick() + + +def test_decimal_with_modifiers(schema_any): + """ + decimal(M,D) carrying `unsigned`, or spelled with a single argument, was valid in + 0.14.x and must remain declarable as a native type. + """ + from decimal import Decimal + + @schema_any + class Proportion(dj.Manual): + definition = """ + id : int32 + --- + on_proportion : decimal(2, 2) unsigned + scale : decimal(5) + """ + + Proportion.insert1({"id": 1, "on_proportion": Decimal("0.25"), "scale": 42}) + row = (Proportion & "id=1").fetch1() + assert row["on_proportion"] == Decimal("0.25") + assert int(row["scale"]) == 42 + # the modified decimal must be recognized as numeric, not fall through as unsupported + assert Proportion.heading["on_proportion"].numeric + Proportion.drop_quick() diff --git a/tests/unit/test_type_patterns.py b/tests/unit/test_type_patterns.py new file mode 100644 index 000000000..30396b48b --- /dev/null +++ b/tests/unit/test_type_patterns.py @@ -0,0 +1,160 @@ +""" +Type-pattern consistency and strictness. + +These tests cover the seam between the three places that name a type: the core type +table in ``declare``, the native passthrough patterns beside it, and the legacy mapping +in ``migrate``. Each of the bugs exercised here was a spelling that one of those three +accepted and another did not. +""" + +import pytest + +from datajoint.declare import CORE_TYPES, TYPE_PATTERN, match_type +from datajoint.errors import DataJointError +from datajoint.migrate import NATIVE_TO_CORE_TYPE + + +class TestCrossReferences: + """Every type named in one module must be recognized by the others.""" + + def test_migrate_core_types_are_declarable(self): + """ + Values in NATIVE_TO_CORE_TYPE are written into column comments as ``:type:`` + markers and read back by Heading. A value that match_type rejects produces a + marker that cannot be resolved on load. + """ + for native, core in NATIVE_TO_CORE_TYPE.items(): + if core.startswith("<"): + continue # codecs are resolved by the codec registry, not match_type + assert match_type(core), f"{native!r} maps to {core!r}, which match_type rejects" + + def test_migrate_native_types_are_recognized(self): + """The keys are types read from a legacy database and must classify too.""" + for native in NATIVE_TO_CORE_TYPE: + assert match_type(native) + + def test_core_type_names_are_declarable(self): + """Each core type's own canonical spelling must classify as that core type.""" + canonical = { + "float32": "float32", + "float64": "float64", + "int64": "int64", + "int32": "int32", + "int16": "int16", + "int8": "int8", + "bool": "bool", + "uuid": "uuid", + "json": "json", + "bytes": "bytes", + "date": "date", + "datetime": "datetime", + "char": "char(8)", + "varchar": "varchar(8)", + "enum": "enum('a','b')", + "decimal": "decimal(6,2)", + } + assert set(canonical) == set(CORE_TYPES), "CORE_TYPES changed; update this map" + for name, spelling in canonical.items(): + assert match_type(spelling) == name.upper() + + +class TestStrictness: + """A declared type must be consumed in its entirety, not merely prefixed.""" + + @pytest.mark.parametrize( + "spelling", + [ + "int24", # plausible-looking, and formerly emitted by migrate.py + "intbanana", + "integerish", + "tinyinteger", + "uint32", # unsigned core types are deliberately not provided + "uint8", + "float64x", + "completely_invalid_type_xyz", + ], + ) + def test_near_miss_spellings_are_rejected(self, spelling): + with pytest.raises(DataJointError, match="Unsupported attribute type"): + match_type(spelling) + + def test_serial_alternative_stays_anchored(self): + """`serial` is an alternative inside the INTEGER pattern; it must not leak.""" + assert match_type("serial") == "INTEGER" + with pytest.raises(DataJointError): + match_type("serialize") + + +class TestNumericAliases: + """decimal/numeric/dec/fixed are one SQL type and must be treated alike.""" + + def test_canonical_decimal_is_a_core_type(self): + assert match_type("decimal(6,2)") == "DECIMAL" + assert match_type("decimal(2, 2)") == "DECIMAL" + + @pytest.mark.parametrize( + "spelling", + [ + "decimal(2,2) unsigned", + "decimal(2, 2) unsigned", + "DECIMAL(2,2) UNSIGNED", + "decimal(2,2) zerofill", + "decimal(5)", + "decimal", + "dec(2,2)", + "fixed(2,2)", + "numeric(2,2)", + "numeric(2,2) unsigned", + ], + ) + def test_modified_and_aliased_forms_pass_as_native(self, spelling): + """ + These were valid in 0.14.x and must remain declarable. They pass as native + types with a warning rather than as the core decimal type. + """ + assert match_type(spelling) == "NUMERIC" + + def test_numeric_is_recognized_as_numeric_by_heading(self): + """ + Heading derives its `numeric` flag from the same patterns. A decimal carrying a + modifier must not fall through as neither numeric nor string. + """ + for spelling in ("decimal(2,2) unsigned", "numeric(6,2)", "decimal(5)"): + assert any(TYPE_PATTERN[t].match(spelling) for t in ("DECIMAL", "NUMERIC", "INTEGER", "FLOAT")) + + +class TestNativeSpellingsStillAccepted: + """Guard against the strictness change rejecting legitimate native types.""" + + @pytest.mark.parametrize( + "spelling,category", + [ + ("int", "INTEGER"), + ("int(11)", "INTEGER"), + ("int unsigned", "INTEGER"), + ("int(11) unsigned", "INTEGER"), + ("bigint unsigned auto_increment", "INTEGER"), + ("integer", "INTEGER"), + ("tinyint", "INTEGER"), + ("smallint", "INTEGER"), + ("double", "FLOAT"), + ("float", "FLOAT"), + ("real", "FLOAT"), + ("double unsigned", "FLOAT"), + ("varchar(255)", "VARCHAR"), + ("char(4)", "CHAR"), + ("timestamp", "TEMPORAL"), + ("time", "TEMPORAL"), + ("year", "TEMPORAL"), + ("longblob", "NATIVE_BLOB"), + ("mediumblob", "NATIVE_BLOB"), + ("blob", "NATIVE_BLOB"), # bare blob is a MySQL type too + ("longtext", "NATIVE_TEXT"), + ("text", "NATIVE_TEXT"), + ("", "CODEC"), + ("", "CODEC"), + ("", "CODEC"), + ], + ) + def test_spelling_classifies(self, spelling, category): + assert match_type(spelling) == category