Skip to content

Commit b3877dd

Browse files
committed
Apply enum pre-DDL and placeholder substitution in Table.alter
Table.alter skipped both steps declare performs on adapter output: it never substituted the PostgreSQL schema placeholder, and never issued the adapter's pending CREATE TYPE. Adding an enum attribute therefore emitted a literal "{database}"."enum_<hash>" against a type that was never created. declare.alter now accepts an optional schema_name and returns the pre-DDL alongside the ALTER clauses. The pending types are drained between the two prepare_declare calls, since both register types as a side effect and the old definition's types already exist; what the old parse registers is discarded so it cannot leak into the next declare on the same adapter. Adding a non-primary attribute also emitted MySQL's AFTER positioning clause, which PostgreSQL has no equivalent for. Gate it behind a new supports_column_position adapter property, dropping the position before it can force a statement so a reorder-only change stays a no-op rather than becoming a positionless MODIFY.
1 parent ee22cc0 commit b3877dd

5 files changed

Lines changed: 120 additions & 5 deletions

File tree

src/datajoint/adapters/base.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,21 @@ def supports_inline_indexes(self) -> bool:
617617
"""
618618
return True # Default for MySQL, override in PostgreSQL
619619

620+
@property
621+
def supports_column_position(self) -> bool:
622+
"""
623+
Whether ALTER TABLE can place a column at a position (``AFTER x``).
624+
625+
MySQL supports it. PostgreSQL has no such clause and always appends,
626+
so the position is dropped rather than emitted.
627+
628+
Returns
629+
-------
630+
bool
631+
True for MySQL, False for PostgreSQL.
632+
"""
633+
return True # Default for MySQL, override in PostgreSQL
634+
620635
@property
621636
def auto_indexes_foreign_keys(self) -> bool:
622637
"""

src/datajoint/adapters/postgres.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -720,6 +720,14 @@ def supports_inline_indexes(self) -> bool:
720720
"""
721721
return False
722722

723+
@property
724+
def supports_column_position(self) -> bool:
725+
"""
726+
PostgreSQL has no ``AFTER`` clause in ALTER TABLE; added columns are
727+
always appended.
728+
"""
729+
return False
730+
723731
@property
724732
def auto_indexes_foreign_keys(self) -> bool:
725733
"""

src/datajoint/declare.py

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,10 @@ def _make_attribute_alter(new: list[str], old: list[str], primary_key: list[str]
649649
else:
650650
if idx >= 1 and old_names[idx - 1] != (prev[1] or prev[0]):
651651
after = prev[0]
652+
if not adapter.supports_column_position:
653+
# Without an AFTER clause a reorder-only change has nothing to
654+
# emit, so drop the position before it can force a statement.
655+
after = None
652656
if new_def not in old or after:
653657
# Determine command type
654658
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]
667671
return sql
668672

669673

670-
def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple[list[str], list[str]]:
674+
def alter(
675+
definition: str,
676+
old_definition: str,
677+
context: dict,
678+
adapter,
679+
*,
680+
schema_name: str | None = None,
681+
) -> tuple[list[str], list[str], list[str]]:
671682
"""
672683
Generate SQL ALTER commands for table definition changes.
673684
@@ -681,14 +692,19 @@ def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple
681692
Namespace for resolving foreign key references.
682693
adapter : DatabaseAdapter
683694
Database adapter for backend-specific SQL generation.
695+
schema_name : str, optional
696+
Schema the table lives in. Required to collect pre-DDL for backends that
697+
declare column types separately (PostgreSQL enums); omitting it yields an
698+
empty ``pre_ddl``.
684699
685700
Returns
686701
-------
687702
tuple
688-
Two-element tuple:
703+
Three-element tuple:
689704
690705
- sql : list[str] - SQL ALTER commands
691706
- new_stores : list[str] - New external stores used
707+
- pre_ddl : list[str] - DDL to run before the ALTER (e.g. CREATE TYPE)
692708
693709
Raises
694710
------
@@ -705,6 +721,15 @@ def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple
705721
_fk_attribute_map,
706722
_column_comments,
707723
) = prepare_declare(definition, context, adapter)
724+
725+
# prepare_declare registers backend types (PostgreSQL enums) on the adapter as
726+
# a side effect. Drain them here, between the two parses: the old definition's
727+
# types already exist in the database, so draining after both would emit
728+
# CREATE TYPE for those as well.
729+
pre_ddl = []
730+
if schema_name and hasattr(adapter, "get_pending_enum_ddl"):
731+
pre_ddl.extend(adapter.get_pending_enum_ddl(schema_name))
732+
708733
(
709734
table_comment_,
710735
primary_key_,
@@ -716,6 +741,11 @@ def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple
716741
_column_comments_,
717742
) = prepare_declare(old_definition, context, adapter)
718743

744+
# Discard what the old definition registered, so it cannot leak into the next
745+
# declare() on this adapter (get_pending_enum_ddl clears as it reads).
746+
if schema_name and hasattr(adapter, "get_pending_enum_ddl"):
747+
adapter.get_pending_enum_ddl(schema_name)
748+
719749
# analyze differences between declarations
720750
sql = list()
721751
if primary_key != primary_key_:
@@ -731,7 +761,7 @@ def alter(definition: str, old_definition: str, context: dict, adapter) -> tuple
731761
# For PostgreSQL: would need COMMENT ON TABLE, but that's not an ALTER TABLE clause
732762
# Keep MySQL syntax for now (ALTER TABLE ... COMMENT="...")
733763
sql.append(f'COMMENT="{table_comment}"')
734-
return sql, [e for e in external_stores if e not in external_stores_]
764+
return sql, [e for e in external_stores if e not in external_stores_], pre_ddl
735765

736766

737767
def _parse_index_args(args: str) -> list[str]:

src/datajoint/table.py

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -322,14 +322,33 @@ def alter(self, prompt=True, context=None):
322322
context = dict(frame.f_globals, **frame.f_locals)
323323
del frame
324324
old_definition = self.describe(context=context)
325-
sql, _external_stores = alter(self.definition, old_definition, context, self.connection.adapter)
325+
sql, _external_stores, pre_ddl = alter(
326+
self.definition,
327+
old_definition,
328+
context,
329+
self.connection.adapter,
330+
schema_name=self.database,
331+
)
326332
if not sql:
327333
if prompt:
328334
logger.warning("Nothing to alter.")
329335
else:
330-
sql = "ALTER TABLE {tab}\n\t".format(tab=self.full_table_name) + ",\n\t".join(sql)
336+
# Same two steps declare() performs on its own output: substitute the
337+
# adapter's schema placeholder, and issue any pre-DDL the attribute
338+
# types depend on. The attribute SQL is joined in after the format
339+
# call, so it never passes through str.format.
340+
sql = _substitute_database(
341+
"ALTER TABLE {tab}\n\t".format(tab=self.full_table_name) + ",\n\t".join(sql),
342+
self.database,
343+
)
331344
if not prompt or user_choice(sql + "\n\nExecute?") == "yes":
332345
try:
346+
for ddl in pre_ddl:
347+
try:
348+
self.connection.query(_substitute_database(ddl, self.database))
349+
except Exception:
350+
# Ignore errors (type may already exist)
351+
pass
333352
self.connection.query(sql)
334353
except AccessError:
335354
# skip if no create privilege

tests/integration/test_multi_backend.py

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,3 +167,46 @@ class Commented(dj.Manual):
167167

168168
# Cleanup
169169
schema.drop()
170+
171+
172+
@pytest.mark.backend_agnostic
173+
def test_alter_adds_enum_attribute(connection_by_backend, backend, prefix):
174+
"""Altering a table to add an enum attribute works on both backends.
175+
176+
On PostgreSQL an enum column's type is emitted as a schema-qualified
177+
placeholder and the type must be created before the ALTER runs, so this
178+
exercises both the placeholder substitution and the pre-DDL path.
179+
"""
180+
schema = dj.Schema(
181+
f"{prefix}_multi_backend_{backend}_alter_enum",
182+
connection=connection_by_backend,
183+
)
184+
185+
@schema
186+
class Subject(dj.Manual):
187+
definition = """
188+
subject_id : int32
189+
---
190+
species : enum('mouse', 'rat')
191+
"""
192+
193+
assert Subject.is_declared
194+
195+
# A second enum with different values resolves to a distinct type name, so
196+
# the altered column cannot reuse the type created at declaration.
197+
Subject.definition = """
198+
subject_id : int32
199+
---
200+
species : enum('mouse', 'rat')
201+
status : enum('active', 'retired', 'transferred')
202+
"""
203+
Subject.alter(prompt=False)
204+
205+
heading = Subject().heading
206+
assert "status" in heading.names
207+
208+
# The added column round-trips a value from its own domain.
209+
Subject.insert1({"subject_id": 1, "species": "mouse", "status": "active"})
210+
assert (Subject & {"subject_id": 1}).fetch1("status") == "active"
211+
212+
schema.drop()

0 commit comments

Comments
 (0)