diff --git a/src/datajoint/adapters/base.py b/src/datajoint/adapters/base.py index 14da27c0d..737807731 100644 --- a/src/datajoint/adapters/base.py +++ b/src/datajoint/adapters/base.py @@ -617,6 +617,41 @@ def supports_inline_indexes(self) -> bool: """ return True # Default for MySQL, override in PostgreSQL + def get_pending_enum_ddl(self, schema_name: str) -> list[str]: + """ + DDL for backend types that must exist before the columns using them, + clearing the pending list as it reads. + + Backends that spell column types inline (MySQL) have none. PostgreSQL + overrides this to emit CREATE TYPE for enums registered while parsing. + + Parameters + ---------- + schema_name : str + Schema used to qualify the type names. + + Returns + ------- + list[str] + Empty for MySQL; CREATE TYPE statements for PostgreSQL. + """ + return [] + + @property + def supports_column_position(self) -> bool: + """ + Whether ALTER TABLE can place a column at a position (``AFTER x``). + + MySQL supports it. PostgreSQL has no such clause and always appends, + so the position is dropped rather than emitted. + + Returns + ------- + bool + True for MySQL, False for PostgreSQL. + """ + return True # Default for MySQL, override in PostgreSQL + @property def auto_indexes_foreign_keys(self) -> bool: """ diff --git a/src/datajoint/adapters/postgres.py b/src/datajoint/adapters/postgres.py index fb7fb2cb3..ed7a557f7 100644 --- a/src/datajoint/adapters/postgres.py +++ b/src/datajoint/adapters/postgres.py @@ -720,6 +720,14 @@ def supports_inline_indexes(self) -> bool: """ return False + @property + def supports_column_position(self) -> bool: + """ + PostgreSQL has no ``AFTER`` clause in ALTER TABLE; added columns are + always appended. + """ + return False + @property def auto_indexes_foreign_keys(self) -> bool: """ diff --git a/src/datajoint/declare.py b/src/datajoint/declare.py index 3417183f9..df8e38408 100644 --- a/src/datajoint/declare.py +++ b/src/datajoint/declare.py @@ -649,6 +649,10 @@ def _make_attribute_alter(new: list[str], old: list[str], primary_key: list[str] else: if idx >= 1 and old_names[idx - 1] != (prev[1] or prev[0]): after = prev[0] + if not adapter.supports_column_position: + # Without an AFTER clause a reorder-only change has nothing to + # emit, so drop the position before it can force a statement. + after = None if new_def not in old or after: # Determine command type if (old_name or new_name) not in old_names: @@ -667,7 +671,14 @@ def _make_attribute_alter(new: list[str], old: list[str], primary_key: list[str] return sql -def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple[list[str], list[str]]: +def alter( + definition: str, + old_definition: str, + context: dict, + adapter, + *, + schema_name: str | None = None, +) -> tuple[list[str], list[str], list[str], dict]: """ Generate SQL ALTER commands for table definition changes. @@ -681,14 +692,23 @@ def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple Namespace for resolving foreign key references. adapter : DatabaseAdapter Database adapter for backend-specific SQL generation. + schema_name : str, optional + Schema the table lives in. Required to collect pre-DDL for backends that + declare column types separately (PostgreSQL enums); omitting it yields an + empty ``pre_ddl``. Returns ------- tuple - Two-element tuple: + Four-element tuple: - sql : list[str] - SQL ALTER commands - new_stores : list[str] - New external stores used + - pre_ddl : list[str] - DDL to run before the ALTER (e.g. CREATE TYPE) + - column_comments : dict - Comments to reapply after the ALTER. On + backends that store them out of line these carry the ``:type:`` + prefix that ``heading`` reads back as ``original_type``, so skipping + them silently loses the declared type of an added attribute. Raises ------ @@ -703,8 +723,14 @@ def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple index_sql, external_stores, _fk_attribute_map, - _column_comments, + column_comments, ) = prepare_declare(definition, context, adapter) + + # prepare_declare registers backend types (PostgreSQL enums) on the adapter + # as a side effect, so each parse must be drained separately to tell the two + # apart. Type names are content hashes, making the statements comparable. + new_type_ddl = adapter.get_pending_enum_ddl(schema_name) if schema_name else [] + ( table_comment_, primary_key_, @@ -716,6 +742,13 @@ def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple _column_comments_, ) = prepare_declare(old_definition, context, adapter) + # Whatever the old definition registered already exists in the database, so + # only the difference needs creating. Draining both also leaves nothing + # behind to leak into the next declare() on this adapter, including on the + # NotImplementedError paths below. + old_type_ddl = set(adapter.get_pending_enum_ddl(schema_name)) if schema_name else set() + pre_ddl = [ddl for ddl in new_type_ddl if ddl not in old_type_ddl] + # analyze differences between declarations sql = list() if primary_key != primary_key_: @@ -731,7 +764,7 @@ def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple # For PostgreSQL: would need COMMENT ON TABLE, but that's not an ALTER TABLE clause # Keep MySQL syntax for now (ALTER TABLE ... COMMENT="...") sql.append(f'COMMENT="{table_comment}"') - return sql, [e for e in external_stores if e not in external_stores_] + return sql, [e for e in external_stores if e not in external_stores_], pre_ddl, column_comments def _parse_index_args(args: str) -> list[str]: diff --git a/src/datajoint/table.py b/src/datajoint/table.py index 6406acd24..f7e55c6c3 100644 --- a/src/datajoint/table.py +++ b/src/datajoint/table.py @@ -33,6 +33,18 @@ # Legacy regexp and query kept for reference but no longer used +def _substitute_database(ddl: str, database: str) -> str: + """Replace the adapter-inserted schema placeholder in DDL. + + Matches the exact quoted fragment produced by the PostgreSQL adapter for + enum type qualification (``'"{database}".'`` — see adapters/postgres.py) + rather than the bare token, and uses ``str.replace`` rather than + ``str.format``, so braces in user-supplied comments and enum values — + including a literal ``{database}`` — pass through verbatim. + """ + return ddl.replace('"{database}".', f'"{database}".') + + @dataclass class ValidationResult: """ @@ -158,19 +170,19 @@ def declare(self, context=None): # Call declaration hook for validation (subclasses like AutoPopulate can override) self._declare_check(primary_key, fk_attribute_map) - sql = sql.format(database=self.database) + sql = _substitute_database(sql, self.database) try: # Execute pre-DDL statements (e.g., CREATE TYPE for PostgreSQL enums) for ddl in pre_ddl: try: - self.connection.query(ddl.format(database=self.database)) + self.connection.query(_substitute_database(ddl, self.database)) except Exception: # Ignore errors (type may already exist) pass self.connection.query(sql) # Execute post-DDL statements (e.g., COMMENT ON for PostgreSQL) for ddl in post_ddl: - self.connection.query(ddl.format(database=self.database)) + self.connection.query(_substitute_database(ddl, self.database)) except AccessError: # Only suppress if table already exists (idempotent declaration) # Otherwise raise - user needs to know about permission issues @@ -310,15 +322,45 @@ def alter(self, prompt=True, context=None): context = dict(frame.f_globals, **frame.f_locals) del frame old_definition = self.describe(context=context) - sql, _external_stores = alter(self.definition, old_definition, context, self.connection.adapter) + sql, _external_stores, pre_ddl, column_comments = alter( + self.definition, + old_definition, + context, + self.connection.adapter, + schema_name=self.database, + ) if not sql: if prompt: logger.warning("Nothing to alter.") else: - sql = "ALTER TABLE {tab}\n\t".format(tab=self.full_table_name) + ",\n\t".join(sql) + # Same two steps declare() performs on its own output: substitute the + # adapter's schema placeholder, and issue any pre-DDL the attribute + # types depend on. The attribute SQL is joined in after the format + # call, so it never passes through str.format. + sql = _substitute_database( + "ALTER TABLE {tab}\n\t".format(tab=self.full_table_name) + ",\n\t".join(sql), + self.database, + ) if not prompt or user_choice(sql + "\n\nExecute?") == "yes": try: + for ddl in pre_ddl: + try: + self.connection.query(_substitute_database(ddl, self.database)) + except Exception as error: + # Enum type names are content hashes shared by every + # table in the schema using the same value set, so the + # type may already exist. Logged rather than dropped: + # a genuine failure surfaces on the ALTER below. + logger.debug("pre-DDL skipped (%s): %s", error, ddl) self.connection.query(sql) + # Reapply comments. Where they are stored out of line they + # carry the `:type:` prefix heading reads back as + # original_type, without which describe() loses an added + # attribute's declared type and cannot re-parse the table. + for col_name, comment in column_comments.items(): + comment_ddl = self.connection.adapter.column_comment_ddl(self.full_table_name, col_name, comment) + if comment_ddl: + self.connection.query(_substitute_database(comment_ddl, self.database)) except AccessError: # skip if no create privilege pass diff --git a/tests/integration/test_declare.py b/tests/integration/test_declare.py index 19e711e96..3d3d4998e 100644 --- a/tests/integration/test_declare.py +++ b/tests/integration/test_declare.py @@ -236,6 +236,24 @@ class Part(dj.Part): ] +def test_braces_in_comments(schema_any): + """Braces in table and attribute comments are literal text, not + str.format template fields.""" + + class BraceComment(dj.Manual): + definition = """ + # payload spec: {data, config} + brace_id : int + --- + payload = null : varchar(32) # {data, config} payload + note = null : varchar(64) # mentions {database} literally + """ + + schema_any(BraceComment, context=dict(BraceComment=BraceComment)) + assert BraceComment.heading["payload"].comment == "{data, config} payload" + assert BraceComment.heading["note"].comment == "mentions {database} literally" + + def test_bad_attribute_name(schema_any): class BadName(dj.Manual): definition = """ diff --git a/tests/integration/test_multi_backend.py b/tests/integration/test_multi_backend.py index bf904e362..707b05d20 100644 --- a/tests/integration/test_multi_backend.py +++ b/tests/integration/test_multi_backend.py @@ -119,6 +119,32 @@ class TypeTest(dj.Manual): schema.drop() +@pytest.mark.backend_agnostic +def test_braces_in_comments_by_backend(connection_by_backend, backend, prefix): + """Braces in table and attribute comments are literal text on both + backends — the MySQL path carries them inline in CREATE TABLE, the + PostgreSQL path in post-DDL COMMENT ON statements.""" + schema = dj.Schema( + f"{prefix}_multi_backend_{backend}_braces", + connection=connection_by_backend, + ) + + @schema + class BraceCommented(dj.Manual): + definition = """ + # payload spec: {data, config} + id : int + --- + payload = null : varchar(32) # {data, config} payload + """ + + assert BraceCommented.is_declared + assert BraceCommented.heading["payload"].comment == "{data, config} payload" + + # Cleanup + schema.drop() + + @pytest.mark.backend_agnostic def test_table_comments(connection_by_backend, backend, prefix): """Test that table comments are preserved on both backends.""" @@ -141,3 +167,62 @@ class Commented(dj.Manual): # Cleanup schema.drop() + + +@pytest.mark.backend_agnostic +def test_alter_adds_enum_attribute(connection_by_backend, backend, prefix): + """Altering a table to add an enum attribute works on both backends. + + On PostgreSQL an enum column's type is emitted as a schema-qualified + placeholder and the type must be created before the ALTER runs, so this + exercises both the placeholder substitution and the pre-DDL path. + """ + schema = dj.Schema( + f"{prefix}_multi_backend_{backend}_alter_enum", + connection=connection_by_backend, + ) + + @schema + class Subject(dj.Manual): + definition = """ + subject_id : int32 + --- + species : enum('mouse', 'rat') + """ + + assert Subject.is_declared + + # A second enum with different values resolves to a distinct type name, so + # the altered column cannot reuse the type created at declaration. + Subject.definition = """ + subject_id : int32 + --- + species : enum('mouse', 'rat') + status : enum('active', 'retired', 'transferred') + """ + Subject.alter(prompt=False) + + heading = Subject().heading + assert "status" in heading.names + # `type` is the generated type name on PostgreSQL but the full spelling on + # MySQL; `original_type` is the definition's own text on both. + assert heading["status"].original_type == "enum('active', 'retired', 'transferred')" + + # The added column round-trips a value from its own domain. + Subject.insert1({"subject_id": 1, "species": "mouse", "status": "active"}) + assert (Subject & {"subject_id": 1}).fetch1("status") == "active" + + # Altering again proves the first alter left the type recoverable: describe() + # feeds the next alter, so a column whose declared type was not recorded + # makes the table permanently un-alterable. + Subject.definition = """ + subject_id : int32 + --- + species : enum('mouse', 'rat') + status : enum('active', 'retired', 'transferred') + note = null : varchar(32) + """ + Subject.alter(prompt=False) + assert "note" in Subject().heading.names + + schema.drop() diff --git a/tests/unit/test_ddl_substitution.py b/tests/unit/test_ddl_substitution.py new file mode 100644 index 000000000..e7b47fb01 --- /dev/null +++ b/tests/unit/test_ddl_substitution.py @@ -0,0 +1,16 @@ +"""Unit tests for the DDL schema-placeholder substitution in Table.declare.""" + +from datajoint.table import _substitute_database + + +def test_placeholder_fragment_substituted(): + """The exact adapter-inserted fragment (see adapters/postgres.py enum + qualification) is replaced with the quoted schema name.""" + assert _substitute_database('"{database}".enum_abc NOT NULL', "myschema") == '"myschema".enum_abc NOT NULL' + + +def test_user_braces_pass_through(): + """Brace text outside the adapter fragment — including a bare literal + {database} in a comment — is never touched.""" + ddl = '`payload` varchar(32) COMMENT "{data, config} payload for {database}"' + assert _substitute_database(ddl, "myschema") == ddl