diff --git a/src/datajoint/adapters/base.py b/src/datajoint/adapters/base.py index 32f591a53..5b4a7ab72 100644 --- a/src/datajoint/adapters/base.py +++ b/src/datajoint/adapters/base.py @@ -1317,48 +1317,6 @@ def enum_type_ddl(self, type_name: str, values: list[str]) -> str | None: """ ... - @abstractmethod - def job_metadata_columns(self) -> list[str]: - """ - Return job metadata column definitions for Computed/Imported tables. - - Returns - ------- - list[str] - List of column definition strings (fully formatted with quotes). - - Examples - -------- - MySQL: - ["`_job_start_time` datetime(3) DEFAULT NULL", - "`_job_duration` float DEFAULT NULL", - "`_job_version` varchar(64) DEFAULT ''"] - PostgreSQL: - ['"_job_start_time" timestamp DEFAULT NULL', - '"_job_duration" real DEFAULT NULL', - '"_job_version" varchar(64) DEFAULT \'\''] - """ - ... - - @abstractmethod - def provenance_columns(self) -> list[str]: - """ - Return the hidden extrinsic-provenance column for Entry tables. - - Returns - ------- - list[str] - List of column definition strings (fully formatted with quotes). - - Examples - -------- - MySQL: - ["`_prov` json DEFAULT NULL"] - PostgreSQL: - ['"_prov" jsonb DEFAULT NULL'] - """ - ... - # ========================================================================= # Error Translation # ========================================================================= diff --git a/src/datajoint/adapters/mysql.py b/src/datajoint/adapters/mysql.py index 082b55041..2ef9baf84 100644 --- a/src/datajoint/adapters/mysql.py +++ b/src/datajoint/adapters/mysql.py @@ -996,34 +996,6 @@ def enum_type_ddl(self, type_name: str, values: list[str]) -> str | None: """ return None # MySQL uses inline enum - def job_metadata_columns(self) -> list[str]: - """ - Return MySQL-specific job metadata column definitions. - - Examples - -------- - >>> adapter.job_metadata_columns() - ["`_job_start_time` datetime(3) DEFAULT NULL", - "`_job_duration` float DEFAULT NULL", - "`_job_version` varchar(64) DEFAULT ''"] - """ - return [ - "`_job_start_time` datetime(3) DEFAULT NULL", - "`_job_duration` float DEFAULT NULL", - "`_job_version` varchar(64) DEFAULT ''", - ] - - def provenance_columns(self) -> list[str]: - """ - Return the MySQL extrinsic-provenance column definition. - - Examples - -------- - >>> adapter.provenance_columns() - ["`_prov` json DEFAULT NULL"] - """ - return ["`_prov` json DEFAULT NULL"] - # ========================================================================= # Error Translation # ========================================================================= diff --git a/src/datajoint/adapters/postgres.py b/src/datajoint/adapters/postgres.py index a08b887f8..3a55d8eb8 100644 --- a/src/datajoint/adapters/postgres.py +++ b/src/datajoint/adapters/postgres.py @@ -1349,34 +1349,6 @@ def get_pending_enum_ddl(self, schema_name: str) -> list[str]: self._pending_enum_types = {} return ddl_statements - def job_metadata_columns(self) -> list[str]: - """ - Return PostgreSQL-specific job metadata column definitions. - - Examples - -------- - >>> adapter.job_metadata_columns() - ['"_job_start_time" timestamp DEFAULT NULL', - '"_job_duration" real DEFAULT NULL', - '"_job_version" varchar(64) DEFAULT \\'\\''] - """ - return [ - '"_job_start_time" timestamp DEFAULT NULL', - '"_job_duration" real DEFAULT NULL', - "\"_job_version\" varchar(64) DEFAULT ''", - ] - - def provenance_columns(self) -> list[str]: - """ - Return the PostgreSQL extrinsic-provenance column definition. - - Examples - -------- - >>> adapter.provenance_columns() - ['"_prov" jsonb DEFAULT NULL'] - """ - return ['"_prov" jsonb DEFAULT NULL'] - # ========================================================================= # Error Translation # ========================================================================= diff --git a/src/datajoint/declare.py b/src/datajoint/declare.py index c63bb6ebb..8e10c7e3c 100644 --- a/src/datajoint/declare.py +++ b/src/datajoint/declare.py @@ -156,7 +156,10 @@ def build_attribute_parser() -> pp.ParserElement: """ quoted = pp.QuotedString('"') ^ pp.QuotedString("'") colon = pp.Literal(":").suppress() - attribute_name = pp.Word(pp.srange("[a-z]"), pp.srange("[a-z0-9_]")).set_results_name("name") + # A leading underscore is permitted by the grammar so the framework can + # declare its own columns in this notation. Whether a *user* may is policy, + # enforced in prepare_declare where a user's definition is parsed. + attribute_name = pp.Word(pp.srange("[a-z_]"), pp.srange("[a-z0-9_]")).set_results_name("name") data_type = ( pp.Combine(pp.Word(pp.alphas) + pp.SkipTo("#", ignore=quoted)) ^ pp.QuotedString("<", end_quote_char=">", unquote_results=False) @@ -334,12 +337,52 @@ def compile_foreign_key( fk_index_candidates.append(fk_attrs) +# ============================================================================= +# Platform-managed attributes +# +# Written in DataJoint notation and compiled by the same `compile_attribute` as +# any user attribute, so the backend type mapping, the `:type:` comment and the +# column-comment bookkeeping all come from the one path that owns them. +# ============================================================================= + +#: Primary key for a table that declares none of its own. +SINGLETON_DEFINITION = "_singleton = 1 : bool # singleton primary key" + +#: Per-row execution record on the auto-populated tiers. +JOB_METADATA_DEFINITION = ( + "_job_start_time = null : datetime(3) # when computation began", + "_job_duration = null : float32 # computation duration in seconds", + '_job_version = "" : varchar(64) # code version', +) + +#: Extrinsic provenance on Entry tables, where rows arrive from outside. +PROV_DEFINITION = "_prov = null : json # extrinsic provenance for a row that entered from outside" + + +def _tier_attributes(table_name: str | None, config) -> tuple[str, ...]: + """The platform-managed attributes a tier receives, in DataJoint notation.""" + from .user_tables import Computed, Imported, Manual, is_tier + + if table_name is None: # `alter` compares two definitions; neither declares these + return () + if config.jobs.add_job_metadata and (is_tier(table_name, Computed) or is_tier(table_name, Imported)): + return JOB_METADATA_DEFINITION + if config.provenance.capture and is_tier(table_name, Manual): + return (PROV_DEFINITION,) + return () + + def prepare_declare( - definition: str, context: dict, adapter + definition: str, context: dict, adapter, *, table_name: str | None = None, config=None ) -> tuple[str, list[str], list[str], list[str], list[str], list[str], dict[str, tuple[str, str]], dict[str, str]]: """ Parse a table definition into its components. + The platform-managed attributes are added here too, once the user's lines are + parsed and by the same ``add_attribute``. That is what lets ``_singleton`` + be decided from the primary key the parse produced, and leaves the loop + iterating user lines only, where a user-declared hidden one is refused. + Parameters ---------- definition : str @@ -348,6 +391,13 @@ def prepare_declare( Namespace for resolving foreign key references. adapter : DatabaseAdapter Database adapter for backend-specific SQL generation. + table_name : str, optional + Stripped table name, which carries the tier. Given one, the tier's + platform attributes are added and ``config`` is required; ``alter`` + passes neither, since it compares two definitions and neither side + declares them. + config : Config, optional + Read for ``jobs.add_job_metadata`` and ``provenance.capture``. Returns ------- @@ -380,6 +430,18 @@ def prepare_declare( column_comments = {} # column_name -> comment (for PostgreSQL COMMENT ON) fk_index_candidates = [] # PostgreSQL: FK column-lists that may need a support index (#1512) + def add_attribute(line: str, in_key: bool) -> None: + name, sql, store, comment = compile_attribute(line, in_key, foreign_key_sql, context, adapter) + if store: + external_stores.append(store) + if in_key and name not in primary_key: + primary_key.append(name) + if name not in attributes: + attributes.append(name) + attribute_sql.append(sql) + if comment: + column_comments[name] = comment + for line in definition: if not line or line.startswith("#"): # ignore additional comments pass @@ -400,17 +462,27 @@ def prepare_declare( ) elif re.match(r"^(unique\s+)?index\s*\(.*\)\s*(#.*)?$", line, re.I): # index compile_index(re.sub(r"\s*#.*$", "", line), index_sql, adapter) + elif line.startswith("_"): + # Policy, not grammar: the parser accepts the name so that the + # framework can declare its own columns in the same notation. Only + # user lines reach here -- the platform's are added after the loop. + raise DataJointError( + f'Attribute name in line "{line}" starts with an underscore. ' + "Names with leading underscore are reserved for platform-managed " + "columns (e.g. _job_start_time, _singleton). Use a regular " + "attribute name; if you need to control visibility at the call " + "site, use proj()." + ) else: - name, sql, store, comment = compile_attribute(line, in_key, foreign_key_sql, context, adapter) - if store: - external_stores.append(store) - if in_key and name not in primary_key: - primary_key.append(name) - if name not in attributes: - attributes.append(name) - attribute_sql.append(sql) - if comment: - column_comments[name] = comment + add_attribute(line, in_key) + + # A table that declares no primary key of its own gets the sentinel, which is + # what makes it hold at most one row. Only the parse knows whether it does: + # `index (...)` contributes no attribute and `-> Parent` may contribute many. + if not primary_key: + add_attribute(SINGLETON_DEFINITION, True) + for line in _tier_attributes(table_name, config): + add_attribute(line, False) # Foreign-key support indexes (PostgreSQL; #1512). Now that the whole # definition is parsed, emit an index on each candidate foreign key's columns @@ -485,7 +557,7 @@ def declare( Raises ------ DataJointError - If table name exceeds max length or has no primary key. + If the table name exceeds the backend's max length. """ # Parse table name using adapter (handles backend-specific quoting) schema_name, table_name = adapter.split_full_table_name(full_table_name) @@ -497,6 +569,11 @@ def declare( ) ) + if config is None: + from .settings import config as _config + + config = _config + ( table_comment, primary_key, @@ -506,49 +583,7 @@ def declare( external_stores, fk_attribute_map, column_comments, - ) = prepare_declare(definition, context, adapter) - - # Add hidden job metadata for Computed/Imported tables (not parts) - if config is None: - from .settings import config as _config - - config = _config - if config.jobs.add_job_metadata: - # Check if this is a Computed (__) or Imported (_) table, but not a Part (contains __ in middle) - is_computed = table_name.startswith("__") and "__" not in table_name[2:] - is_imported = table_name.startswith("_") and not table_name.startswith("__") - if is_computed or is_imported: - job_metadata_sql = adapter.job_metadata_columns() - attribute_sql.extend(job_metadata_sql) - - # Add the hidden extrinsic-provenance slot to Entry tables, where rows enter - # from outside the pipeline. Computed and Imported tables have no use for - # it -- their provenance is entailed by the foreign-key graph -- and a part - # inherits its master's. - # Matched against the Manual tier itself, not by excluding the other tiers' - # prefixes: enumerating exclusions makes every tier added later an Entry - # table by default, which is how job tables (`~`) first acquired the slot. - # Imported here rather than at module scope: user_tables imports table, - # which imports this module. - from .user_tables import Manual - - if config.provenance.capture and re.fullmatch(Manual.tier_regexp, table_name): - attribute_sql.extend(adapter.provenance_columns()) - - if not primary_key: - # Singleton table: add hidden sentinel attribute - primary_key = ["_singleton"] - singleton_comment = ":bool:singleton primary key" - sql_type = adapter.core_type_to_sql("bool") - singleton_sql = adapter.format_column_definition( - name="_singleton", - sql_type=sql_type, - nullable=False, - default="NOT NULL DEFAULT TRUE", - comment=singleton_comment, - ) - attribute_sql.insert(0, singleton_sql) - column_comments["_singleton"] = singleton_comment + ) = prepare_declare(definition, context, adapter, table_name=table_name, config=config) pre_ddl = [] # DDL to run BEFORE CREATE TABLE (e.g., CREATE TYPE for enums) post_ddl = [] # DDL to run AFTER CREATE TABLE (e.g., COMMENT ON) @@ -958,14 +993,6 @@ def compile_attribute( DataJointError If syntax is invalid, primary key is nullable, or blob has invalid default. """ - if line.lstrip().startswith("_"): - raise DataJointError( - f'Attribute name in line "{line}" starts with an underscore. ' - "Names with leading underscore are reserved for platform-managed " - "columns (e.g. _job_start_time, _singleton). Use a regular " - "attribute name; if you need to control visibility at the call " - "site, use proj()." - ) try: match = attribute_parser.parse_string(line + "#", parse_all=True) except pp.ParseException as err: diff --git a/src/datajoint/deploy.py b/src/datajoint/deploy.py index bb6a0245a..3aff87710 100644 --- a/src/datajoint/deploy.py +++ b/src/datajoint/deploy.py @@ -226,13 +226,18 @@ def add_prov_column(target: "TargetType", dry_run: bool = True) -> dict: a part table inherits its master's. - Rows already present keep ``NULL``. Provenance is recorded at insert and is never reconstructed after the fact. + - **Takes effect on the next schema load.** A ``Heading`` is read from the + database once and memoized, so a table already in use keeps its pre-ALTER + attributes for the life of the process and its inserts go on recording + nothing. This is a deploy-time operation: run it before the workers that + will write through it, as with :func:`set_replica_identity`. """ - import re from . import provenance + from .declare import PROV_DEFINITION, compile_attribute from .schemas import _Schema from .table import Table - from .user_tables import Manual + from .user_tables import Manual, is_tier if isinstance(target, _Schema): connection = target.connection @@ -258,8 +263,14 @@ def add_prov_column(target: "TargetType", dry_run: bool = True) -> dict: raise DataJointError("Cannot add the provenance column: the target has no database.") adapter = connection.adapter - column_sql = adapter.provenance_columns()[0] - + # Compiled by the same path that declares it on a new table, so a retrofitted + # column is identical to a freshly declared one. + _name, column_sql, _store, prov_comment = compile_attribute( + PROV_DEFINITION, in_key=False, foreign_key_sql=[], context={}, adapter=adapter + ) + + # `tables_modified` and `columns_added` count work actually done, as + # set_replica_identity does; a dry run reports through `ddl` and `details`. result: dict[str, Any] = { "tables_analyzed": 0, "tables_modified": 0, @@ -269,7 +280,7 @@ def add_prov_column(target: "TargetType", dry_run: bool = True) -> dict: } for table_name in table_names: - if not re.fullmatch(Manual.tier_regexp, table_name): + if not is_tier(table_name, Manual): continue result["tables_analyzed"] += 1 @@ -287,11 +298,24 @@ def add_prov_column(target: "TargetType", dry_run: bool = True) -> dict: ddl = ( f"ALTER TABLE {adapter.quote_identifier(database)}.{adapter.quote_identifier(table_name)} ADD COLUMN {column_sql}" ) - result["ddl"].append(ddl) + statements = [ddl] + # On PostgreSQL the comment is not part of the column definition; without + # this the retrofitted column loses the `:type:` marker that a freshly + # declared one carries, and reads back with no original_type. + comment_ddl = adapter.column_comment_ddl( + f"{adapter.quote_identifier(database)}.{adapter.quote_identifier(table_name)}", + provenance.PROV_ATTRIBUTE, + prov_comment, + ) + if comment_ddl: + statements.append(comment_ddl) + + result["ddl"].extend(statements) result["details"].append({"table": f"{database}.{table_name}", "status": "pending" if dry_run else "added"}) if not dry_run: - connection.query(ddl) - result["tables_modified"] += 1 - result["columns_added"] += 1 + for statement in statements: + connection.query(statement) + result["tables_modified"] += 1 + result["columns_added"] += 1 return result diff --git a/src/datajoint/migrate.py b/src/datajoint/migrate.py index 1d1045e50..b78f00750 100644 --- a/src/datajoint/migrate.py +++ b/src/datajoint/migrate.py @@ -595,12 +595,25 @@ def check_migration_status(schema: Schema) -> dict: # Job Metadata Migration # ============================================================================= -# Hidden job metadata columns added by config.jobs.add_job_metadata -JOB_METADATA_COLUMNS = [ - ("_job_start_time", "datetime(3) DEFAULT NULL"), - ("_job_duration", "float DEFAULT NULL"), - ("_job_version", "varchar(64) DEFAULT ''"), -] + +def _job_metadata_ddl(adapter) -> list[tuple[str, str, str]]: + """Compile the job-metadata columns the way a fresh declaration does. + + Returns ``(name, column_sql, comment)`` per column. Going through + ``compile_attribute`` rather than a hand-written string is what makes a + retrofitted column identical to a declared one: the same backend type + mapping -- ``datetime(3)`` on MySQL, ``timestamp(3)`` on PostgreSQL -- and + the same ``:type:`` marker. + """ + from .declare import JOB_METADATA_DEFINITION, compile_attribute + + return [ + (name, column_sql, comment) + for name, column_sql, _store, comment in ( + compile_attribute(line, in_key=False, foreign_key_sql=[], context={}, adapter=adapter) + for line in JOB_METADATA_DEFINITION + ) + ] def _get_existing_columns(connection, database: str, table_name: str) -> set[str]: @@ -617,16 +630,10 @@ def _get_existing_columns(connection, database: str, table_name: str) -> set[str def _is_autopopulated_table(table_name: str) -> bool: - """Check if a table name indicates a Computed or Imported table.""" - # Computed tables start with __ (but not part tables which have __ in middle) - # Imported tables start with _ (but not __) - if table_name.startswith("__"): - # Computed table if no __ after the prefix - return "__" not in table_name[2:] - elif table_name.startswith("_"): - # Imported table - return True - return False + """Whether a table name denotes a Computed or Imported table.""" + from .user_tables import Computed, Imported, is_tier + + return is_tier(table_name, Computed) or is_tier(table_name, Imported) def add_job_metadata_columns(target, dry_run: bool = True) -> dict: @@ -719,8 +726,9 @@ def add_job_metadata_columns(target, dry_run: bool = True) -> dict: continue # Check which columns need to be added + adapter = connection.adapter existing_columns = _get_existing_columns(connection, database, table_name) - columns_to_add = [(name, definition) for name, definition in JOB_METADATA_COLUMNS if name not in existing_columns] + columns_to_add = [column for column in _job_metadata_ddl(adapter) if column[0] not in existing_columns] if not columns_to_add: result["details"].append( @@ -740,13 +748,20 @@ def add_job_metadata_columns(target, dry_run: bool = True) -> dict: "sql_statements": [], } - for col_name, col_definition in columns_to_add: - sql = f"ALTER TABLE `{database}`.`{table_name}` ADD COLUMN `{col_name}` {col_definition}" - table_detail["sql_statements"].append(sql) + qualified = f"{adapter.quote_identifier(database)}.{adapter.quote_identifier(table_name)}" + for col_name, column_sql, comment in columns_to_add: + statements = [f"ALTER TABLE {qualified} ADD COLUMN {column_sql}"] + # PostgreSQL keeps comments out of the column definition, so without + # this the retrofitted column reads back with no original_type. + comment_ddl = adapter.column_comment_ddl(qualified, col_name, comment) + if comment_ddl: + statements.append(comment_ddl) + table_detail["sql_statements"].extend(statements) if not dry_run: try: - connection.query(sql) + for statement in statements: + connection.query(statement) logger.info(f"Added column {col_name} to {database}.{table_name}") except Exception as e: logger.error(f"Failed to add column {col_name} to {database}.{table_name}: {e}") diff --git a/src/datajoint/schemas.py b/src/datajoint/schemas.py index fa934d569..9f2cc1497 100644 --- a/src/datajoint/schemas.py +++ b/src/datajoint/schemas.py @@ -22,7 +22,7 @@ from .heading import Heading from .jobs import Job from .table import FreeTable, lookup_class_name -from .user_tables import Computed, Imported, Lookup, Manual, Part, _get_tier +from .user_tables import Computed, Imported, Lookup, Manual, Part, _get_tier, is_tier from .utils import to_camel_case, user_choice logger = logging.getLogger(__name__.split(".")[0]) @@ -377,9 +377,9 @@ def make_classes(self, into: dict[str, Any] | None = None) -> None: class_name = to_camel_case(table_name) if class_name not in into: try: - cls = next(cls for cls in master_classes if re.fullmatch(cls.tier_regexp, table_name)) + cls = next(cls for cls in master_classes if is_tier(table_name, cls)) except StopIteration: - if re.fullmatch(Part.tier_regexp, table_name): + if is_tier(table_name, Part): part_tables.append(table_name) else: # declare and decorate master table classes diff --git a/src/datajoint/user_tables.py b/src/datajoint/user_tables.py index 938ac4352..c497d69e5 100644 --- a/src/datajoint/user_tables.py +++ b/src/datajoint/user_tables.py @@ -278,6 +278,11 @@ def alter(self, prompt=True, context=None): user_table_classes = (Manual, Lookup, Computed, Imported, Part) +def is_tier(table_name: str, tier) -> bool: + """Whether a stripped table name belongs to ``tier``.""" + return re.fullmatch(tier.tier_regexp, table_name) is not None + + def _get_tier(table_name): """given the table name, return the user table class.""" # Handle both MySQL backticks and PostgreSQL double quotes @@ -290,6 +295,6 @@ def _get_tier(table_name): else: return None try: - return next(tier for tier in user_table_classes if re.fullmatch(tier.tier_regexp, extracted_name)) + return next(tier for tier in user_table_classes if is_tier(extracted_name, tier)) except StopIteration: return None diff --git a/tests/integration/test_declare.py b/tests/integration/test_declare.py index a32994414..d0b904df1 100644 --- a/tests/integration/test_declare.py +++ b/tests/integration/test_declare.py @@ -474,6 +474,34 @@ class PipelineConfig(dj.Lookup): assert result["max_workers"] == 4 assert result["debug_mode"] == 0 # bool stored as tinyint + def test_singleton_with_an_index(self, schema_any): + """An index line is not an attribute, and must not suppress `_singleton`. + + Whether a table needs the sentinel is decided after the parse, from the + primary key it produced, rather than by reading the definition text. + Deciding it from the text had to guess what each line contributes, and + read an `index (...)` line in the key section as an attribute -- which + left the table with no primary key and a `PRIMARY KEY ()` the server + rejects. + """ + + @schema_any + class IndexedConfig(dj.Lookup): + definition = """ + index (label) + --- + label : varchar(32) + """ + + IndexedConfig.heading.attributes # trigger the lazy load + assert "_singleton" in IndexedConfig.heading._attributes + assert IndexedConfig.heading.primary_key == [] + + IndexedConfig.insert1({"label": "only"}) + assert IndexedConfig.fetch1()["label"] == "only" + with pytest.raises(dj.errors.DuplicateError): + IndexedConfig.insert1({"label": "second"}) + def test_singleton_describe(self, schema_any): """Describe should show the singleton nature.""" diff --git a/tests/integration/test_entry_provenance.py b/tests/integration/test_entry_provenance.py index ffec5d1f5..5f490e08e 100644 --- a/tests/integration/test_entry_provenance.py +++ b/tests/integration/test_entry_provenance.py @@ -195,7 +195,9 @@ class Legacy(dj.Manual): Legacy.insert1({"legacy_id": 1, "note": "before"}) preview = add_prov_column(Legacy, dry_run=True) - assert preview["columns_added"] == 1 and preview["ddl"] + # A dry run reports through `ddl`; the counters record work actually done. + assert preview["ddl"] and preview["columns_added"] == 0 + assert preview["details"][0]["status"] == "pending" applied = add_prov_column(Legacy, dry_run=False) assert applied["columns_added"] == 1 @@ -203,6 +205,9 @@ class Legacy(dj.Manual): # idempotent assert add_prov_column(Legacy, dry_run=False)["columns_added"] == 0 + # add_prov_column is a deploy-time operation and does not reach into the + # in-process cache; the schema is reloaded before anything writes through + # it. Reload here for the same reason a deployment would. Legacy().heading._init_from_database() assert provenance.PROV_ATTRIBUTE in Legacy().heading._attributes diff --git a/tests/integration/test_migrate_job_metadata.py b/tests/integration/test_migrate_job_metadata.py new file mode 100644 index 000000000..41d3d1dc5 --- /dev/null +++ b/tests/integration/test_migrate_job_metadata.py @@ -0,0 +1,135 @@ +"""The retrofit path for `_job_*` must reach the same column a declaration does. + +`migrate.add_job_metadata_columns` writes the three hidden columns onto tables +declared before `config.jobs.add_job_metadata` was on. It used to build the +SQL by hand -- backtick-quoted and typed `datetime(3)` -- which is a syntax +error on PostgreSQL and the wrong type there besides. It now compiles the same +`JOB_METADATA_DEFINITION` lines `declare()` uses, so what these tests pin is +that the two paths converge: a migrated table and a declared one are +indistinguishable in the catalog. +""" + +import pytest + +import datajoint as dj +from datajoint.migrate import add_job_metadata_columns + +JOB_COLUMNS = ("_job_start_time", "_job_duration", "_job_version") + + +def _column_types(table): + """Backend type and recovered `original_type` per hidden column, from the catalog.""" + heading = table().heading + heading._init_from_database() + return { + name: (heading._attributes[name].type, heading._attributes[name].original_type) + for name in JOB_COLUMNS + if name in heading._attributes + } + + +@pytest.fixture +def declared_and_migrated(connection_by_backend, backend, prefix): + """Two Computed tables with the same definition: one declared with the + columns, one declared without them and then migrated.""" + original = dj.config.jobs.add_job_metadata + + dj.config.jobs.add_job_metadata = False + off = dj.Schema(f"{prefix}_jobmeta_off_{backend}", connection=connection_by_backend) + + @off + class Source(dj.Lookup): + definition = """ + source_id : int16 + --- + value : float32 + """ + contents = [(1, 1.0)] + + @off + class Legacy(dj.Computed): + definition = """ + -> Source + --- + result : float32 + """ + + def make(self, key): + self.insert1({**key, "result": 2.0}) + + dj.config.jobs.add_job_metadata = True + on = dj.Schema(f"{prefix}_jobmeta_on_{backend}", connection=connection_by_backend) + + @on + class Source2(dj.Lookup): + definition = """ + source_id : int16 + --- + value : float32 + """ + contents = [(1, 1.0)] + + @on + class Fresh(dj.Computed): + definition = """ + -> Source2 + --- + result : float32 + """ + + def make(self, key): + self.insert1({**key, "result": 2.0}) + + try: + yield Legacy, Fresh + finally: + off.drop() + on.drop() + dj.config.jobs.add_job_metadata = original + + +def test_migrated_columns_match_declared_ones(declared_and_migrated): + """The point of routing the migration through `compile_attribute`.""" + legacy, fresh = declared_and_migrated + + assert _column_types(legacy) == {}, "declared with metadata off, so nothing to compare yet" + assert set(_column_types(fresh)) == set(JOB_COLUMNS) + + preview = add_job_metadata_columns(legacy, dry_run=True) + assert preview["columns_added"] == 3 + assert preview["details"][0]["status"] == "pending" + + applied = add_job_metadata_columns(legacy, dry_run=False) + assert applied["columns_added"] == 3 + + # Identical type *and* `:type:` marker -- the marker is what carries + # `original_type`, and on PostgreSQL it needs its own COMMENT ON statement. + assert _column_types(legacy) == _column_types(fresh) + + +def test_migration_is_idempotent(declared_and_migrated): + legacy, _ = declared_and_migrated + add_job_metadata_columns(legacy, dry_run=False) + again = add_job_metadata_columns(legacy, dry_run=False) + assert again["columns_added"] == 0 + assert again["details"][0]["status"] == "already_migrated" + + +def test_populate_fills_the_migrated_columns(declared_and_migrated): + """A migrated table must be writable by the same code that writes a declared one.""" + legacy, _ = declared_and_migrated + add_job_metadata_columns(legacy, dry_run=False) + legacy().heading._init_from_database() + + legacy.populate() + rows = ( + legacy() + .connection.query( + f"SELECT {', '.join(legacy().connection.adapter.quote_identifier(c) for c in JOB_COLUMNS)} " + f"FROM {legacy().full_table_name}" + ) + .fetchall() + ) + assert rows, "populate() inserted nothing; the test would pass vacuously" + start_time, duration, _version = rows[0] + assert start_time is not None and duration is not None diff --git a/tests/unit/test_adapters.py b/tests/unit/test_adapters.py index 5b7e6a96e..bd0afa9e0 100644 --- a/tests/unit/test_adapters.py +++ b/tests/unit/test_adapters.py @@ -410,7 +410,6 @@ def test_adapter_implements_interface(self, backend): "table_comment_ddl", "column_comment_ddl", "enum_type_ddl", - "job_metadata_columns", "translate_error", "validate_native_type", ] @@ -476,15 +475,20 @@ def test_enum_type_ddl_mysql(self, adapter): assert result is None def test_job_metadata_columns_mysql(self, adapter): - """Test MySQL job metadata columns.""" - result = adapter.job_metadata_columns() - assert len(result) == 3 - assert "_job_start_time" in result[0] - assert "datetime(3)" in result[0] - assert "_job_duration" in result[1] - assert "float" in result[1] - assert "_job_version" in result[2] - assert "varchar(64)" in result[2] + """Job metadata is declared in DataJoint notation, compiled like any attribute.""" + from datajoint.declare import compile_attribute + from datajoint.declare import JOB_METADATA_DEFINITION + + compiled = [ + compile_attribute(line, in_key=False, foreign_key_sql=[], context={}, adapter=adapter) + for line in JOB_METADATA_DEFINITION + ] + names = [c[0] for c in compiled] + sql = [c[1] for c in compiled] + assert names == ["_job_start_time", "_job_duration", "_job_version"] + assert "datetime(3)" in sql[0] and ":datetime(3):" in sql[0] + assert "float" in sql[1] + assert "varchar(64)" in sql[2] class TestPostgreSQLDDLMethods: @@ -550,12 +554,21 @@ def test_replica_identity_ddl_invalid_mode(self, postgres_adapter): postgres_adapter.replica_identity_ddl('"schema"."table"', "nothing") def test_job_metadata_columns_postgres(self, postgres_adapter): - """Test PostgreSQL job metadata columns.""" - result = postgres_adapter.job_metadata_columns() - assert len(result) == 3 - assert "_job_start_time" in result[0] - assert "timestamp" in result[0] - assert "_job_duration" in result[1] - assert "real" in result[1] - assert "_job_version" in result[2] - assert "varchar(64)" in result[2] + """Precision is the point: a bare `timestamp` gave PostgreSQL microseconds. + + The hand-written column declared `timestamp`, so PostgreSQL applied its + default precision while MySQL got `datetime(3)`. Compiling the DataJoint + type keeps the two backends on the same declared precision -- #1566. + """ + from datajoint.declare import compile_attribute + from datajoint.declare import JOB_METADATA_DEFINITION + + sql = [ + compile_attribute(line, in_key=False, foreign_key_sql=[], context={}, adapter=postgres_adapter)[1] + for line in JOB_METADATA_DEFINITION + ] + assert "timestamp(3)" in sql[0], "must match MySQL's datetime(3)" + assert "real" in sql[1] + assert "varchar(64)" in sql[2] + # PostgreSQL carries no inline comment; it is applied via COMMENT ON. + assert "COMMENT" not in sql[0].upper() diff --git a/tests/unit/test_declare_hidden_attribute.py b/tests/unit/test_declare_hidden_attribute.py index 0a1db6555..60e20c0c4 100644 --- a/tests/unit/test_declare_hidden_attribute.py +++ b/tests/unit/test_declare_hidden_attribute.py @@ -1,21 +1,38 @@ -"""Unit tests for the leading-underscore guard in attribute declarations. +"""Grammar permits a leading underscore; policy forbids a user declaring one. -Regression coverage for issue #1433: declarations like ``_hidden: bool`` -previously failed with a cryptic ``pyparsing.ParseException``. The framework -intentionally does not support user-defined hidden attributes — those names -are reserved for platform-managed columns (e.g. ``_job_start_time``, -``_singleton``) which DataJoint injects programmatically after parsing. +Issue #1433 asked that `_hidden: bool` fail with a clear error rather than a +parser-internals exception. That is still the contract, but it now lives in a +different place. -This test ensures the user gets a clear ``DataJointError`` pointing to the -right alternative, not a parser-internals error. +The framework declares its own hidden columns -- `_job_start_time`, +`_singleton`, `_prov` -- in the same DataJoint notation a user writes, so the +grammar has to be able to spell them. What must stay forbidden is a *user* +declaring one, and that check is a branch of `prepare_declare`'s line loop, +which reaches it only after blanks, comments, `---`, foreign keys and indexes +have each been dispatched -- and which iterates nothing but the user's lines, +the framework's own being added after the loop. + +Keeping the two separate is what lets platform columns go through one path while +`heading`'s visible/hidden split keeps meaning what it says. """ import pytest -from datajoint.declare import attribute_parser, compile_attribute +from datajoint.adapters.mysql import MySQLAdapter +from datajoint.declare import attribute_parser, compile_attribute, prepare_declare from datajoint.errors import DataJointError +def parse(definition, table_name=None, config=None): + """Run a definition through the real entry point.""" + return prepare_declare(definition, {}, MySQLAdapter(), table_name=table_name, config=config) + + +def attribute_names(definition, table_name=None, config=None): + _comment, _pk, attribute_sql, *_rest = parse(definition, table_name, config) + return [sql.split()[0].strip("`") for sql in attribute_sql] + + @pytest.mark.parametrize( "line", [ @@ -24,28 +41,106 @@ " _leading_whitespace: int32", ], ) -def test_compile_attribute_rejects_leading_underscore(line): - """The leading-underscore guard fires before the parser, so adapter is unused.""" +def test_user_definition_rejects_leading_underscore(line): + """The user-facing guarantee, checked on the path a user actually takes.""" with pytest.raises(DataJointError, match="reserved for platform-managed"): - compile_attribute(line, in_key=False, foreign_key_sql=[], context={}, adapter=None) + parse(f"id : int32\n---\n{line}") + + +def test_rejection_message_is_unchanged(): + """#1433's point was the message, not only the failure.""" + with pytest.raises(DataJointError) as exc: + parse("id : int32\n---\n_hidden : bool") + message = str(exc.value) + assert "starts with an underscore" in message + assert "_job_start_time" in message and "_singleton" in message + assert "proj()" in message + + +def test_indented_declarations_are_refused_too(): + """Definitions arrive indented from a class body; lines are stripped first.""" + with pytest.raises(DataJointError, match="starts with an underscore"): + parse(" id : int32\n ---\n _indented : int32") + + +def test_ordinary_definitions_pass(): + """A name with an interior underscore is ordinary and must not trip the check.""" + assert attribute_names("subject_id : int32\n---\nspecies_name : varchar(32)") == [ + "subject_id", + "species_name", + ] + + +def test_comments_and_indexes_are_not_attribute_lines(): + assert attribute_names("# _not_an_attribute\nid : int32\n---\nvalue : int32\nindex (value)") == [ + "id", + "value", + ] + +def test_grammar_accepts_what_policy_forbids(): + """The parser must spell a hidden name; refusing is policy, applied later.""" + parsed = attribute_parser.parse_string("_prov = null : json#", parse_all=True) + assert parsed["name"] == "_prov" -def test_parser_still_rejects_leading_underscore(): - """Parser regex itself remains strict; the helpful error fires before the parser.""" - import pyparsing as pp - with pytest.raises(pp.ParseException): - attribute_parser.parse_string("_hidden: bool#", parse_all=True) +@pytest.mark.parametrize( + "line, expect_in_sql", + [ + ("_job_start_time = null : datetime(3) # began", ":datetime(3):"), + ("_prov = null : json # extrinsic provenance", ":json:"), + ], +) +def test_framework_can_compile_its_own_columns(line, expect_in_sql): + """The framework's own declarations go through the ordinary compile path.""" + name, sql, _store, comment = compile_attribute(line, in_key=False, foreign_key_sql=[], context={}, adapter=MySQLAdapter()) + assert name.startswith("_") + assert expect_in_sql in sql + assert expect_in_sql in comment + + +class _Config: + """Minimal stand-in for the settings object the helper reads.""" + + class jobs: + add_job_metadata = True + + class provenance: + capture = True + + +@pytest.mark.parametrize( + "table_name, expected, unexpected", + [ + ("subject", ["_prov"], ["_job_start_time"]), # Entry + ("__analysis", ["_job_start_time"], ["_prov"]), # Computed + ("_ingest", ["_job_start_time"], ["_prov"]), # Imported + ("#param", [], ["_prov", "_job_start_time"]), # Lookup + ("subject__detail", [], ["_prov", "_job_start_time"]), # Part of an Entry + ("~~analysis", [], ["_prov", "_job_start_time"]), # job table + ], +) +def test_platform_attributes_added_per_tier(table_name, expected, unexpected): + """Each tier gets exactly the hidden columns it should, and no others.""" + names = attribute_names("id : int32\n---\nvalue : int32", table_name, _Config) + for name in expected: + assert name in names, f"{table_name} should receive {name}" + for name in unexpected: + assert name not in names, f"{table_name} should not receive {name}" -def test_parser_still_accepts_plain_names(): - match = attribute_parser.parse_string("name: varchar(40)#", parse_all=True) - assert match["name"] == "name" +def test_platform_attributes_are_secondary(): + """They are nullable, so landing in the key section would be rejected. + Compiling them after the parse is what settles this: a definition whose + attributes are all primary key has no separator to sit behind, and does not + need one. + """ + _comment, primary_key, *_rest = parse("id : int32", "subject", _Config) + assert primary_key == ["id"] + assert attribute_names("id : int32", "subject", _Config) == ["id", "_prov"] -def test_parser_rejects_digit_start(): - """Numeric leading char remains invalid (preserved behavior).""" - import pyparsing as pp - with pytest.raises(pp.ParseException): - attribute_parser.parse_string("1bad: int32#", parse_all=True) +def test_no_tier_means_no_secondary_attributes(): + """`alter` passes no table name, and compares two definitions that declare none.""" + assert attribute_names("id : int32\n---\nvalue : int32") == ["id", "value"]