fix: braces in comments crash declaration, and alter() drops type metadata

[MilagrosMarin]

Summary

A {...} sequence in any table or attribute comment crashed table declaration with an opaque KeyError — e.g.:

payload : json   # {data, config} payload

  File ".../datajoint/table.py", line 161, in declare
    sql = sql.format(database=self.database)
KeyError: 'data, config'

The assembled DDL — including verbatim user comments — was passed through str.format(), which interprets any brace pair as a template field. On the PostgreSQL path, brace-containing enum values additionally made the pre-DDL CREATE TYPE raise inside the except Exception: pass guard, so the statement was silently never sent and CREATE TABLE later failed with a baffling "type does not exist" — that swallowed pre-DDL error, not the KeyError itself, is what made this hard to diagnose.

Fix

The only intended placeholder in the DDL is inserted by the PostgreSQL adapter for enum type qualification, always as the exact fragment '"{database}".' (adapters/postgres.py). The three format(database=...) sites in Table.declare now str.replace that exact fragment instead, so braces in user comments and enum values — including a bare literal {database} — pass through verbatim.

The same defect was live on Table.alter, which turned out to be missing rather more than the substitution. Four things were wrong, each visible only after fixing the one before it:

1. No placeholder substitution. Adding an enum attribute on PostgreSQL emitted a literal "{database}"."enum_<hash>". declare.alter() now takes an optional schema_name; Table.alter runs its output through the same _substitute_database helper as declare.

2. No CREATE TYPE. The pending-type drain lived in declare(), so alter() issued none and substitution alone would have traded one error for another. declare.alter() now returns the pre-DDL. It drains between the two prepare_declare calls and subtracts what the old definition registered rather than discarding it — both parses register types as a side effect, so the new definition's drain otherwise includes types that already exist, making every alter of a table with any pre-existing enum issue a statement certain to fail. Type names are content hashes, so the statements compare directly. The guard around the loop stays (a hash is shared by every table in the schema using the same value set, so a genuine collision is possible) but now logs instead of discarding.

3. MySQL-only positioning. Adding a non-primary attribute emitted AFTER "col", which PostgreSQL has no equivalent for. A supports_column_position adapter property gates it, following supports_inline_indexes / auto_indexes_foreign_keys. The position is dropped before it can force a statement, so a reorder-only change stays a no-op rather than becoming a positionless MODIFY.

4. Type metadata silently lost. original_type travels in the column comment as :type:comment; PostgreSQL stores that with COMMENT ON COLUMN, which declare() emits as post-DDL and alter() discarded. An added attribute therefore came back as its generated type name, describe() stopped round-tripping, and — since describe() is the input to the next alter() — the table became permanently un-alterable, failing with Unsupported attribute type enum_<hash>. declare.alter() now returns the column comments and Table.alter reapplies them after the ALTER.

This makes ADD and DROP work on PostgreSQL (COLUMN is optional there for both). Still MySQL-only and out of scope: MODIFY and CHANGE (PostgreSQL needs ALTER COLUMN … TYPE and RENAME COLUMN), and the table-comment clause at declare.py:761, which the surrounding code already flags.

Two alternatives were considered and rejected:

• Escaping braces at comment-emission time in declare.py: the alter path parses COMMENT "{ name }" as the attribute-rename convention through the same compile_attribute, and alter's DDL never goes through the format call — escaping there would corrupt renames.
• Replacing the bare {database} token: a user comment legitimately containing that literal text would be silently rewritten (the old format had the same defect); matching the full quote-wrapped, dot-suffixed fragment shrinks the collision surface to a string that cannot appear in a sane comment.

Tests

• tests/integration/test_declare.py::test_braces_in_comments — braces in table and attribute comments (including a bare literal {database}) round-trip through the heading verbatim. Fails on master with the KeyError.
• tests/integration/test_multi_backend.py::test_braces_in_comments_by_backend — same invariant on both backends, exercising the PostgreSQL COMMENT ON path the MySQL-only test cannot reach.
• tests/integration/test_multi_backend.py::test_alter_adds_enum_attribute — declares a table with an enum, alters it to add a second enum with a different value set, then asserts the column exists, that original_type is the declared spelling, that a value round-trips, and that the table can be altered again. The last two are what catch (4): a presence-only assertion passes against a column whose declared type was lost.
• tests/unit/test_ddl_substitution.py — pins the mechanism: the adapter fragment is replaced, user brace text is untouched.

Full test_declare.py + test_alter.py + test_multi_backend.py pass, and the full integration + unit suites are green.

[dimitri-yatsenko]

Approving. I verified the two claims the fix rests on rather than reading the reasoning, and both hold.

The fragment really is the only one. '"{database}".' at adapters/postgres.py:368 is the sole producer of that placeholder anywhere in src/ for DDL purposes — every other {database} hit is either an already-interpolated f-string or a schemas.py logger message. So matching the full quote-wrapped, dot-suffixed fragment is not merely narrower than the bare token, it is exact.

The three sites are all of them. table.py:161, :166, :173 were the only format(database= consumers in the package, and all three are converted. Nothing else in the declare path needed touching.

The alter-path reasoning checks out too, for a reason worth stating because it is not obvious from the diff: alter() does "ALTER TABLE {tab}".format(tab=...) on the literal prefix and then concatenates the joined user SQL after the format call, so user content never passes through str.format. Your rejected alternative — escaping at comment-emission time — would indeed have corrupted the rename convention, since alter routes COMMENT "{ name }" through the same compile_attribute.

Good instinct pinning the mechanism in a unit test rather than only the symptom. And the PostgreSQL detail in the description is the useful part of this report: an exception swallowed by except Exception: pass in the pre-DDL loop, surfacing much later as "type does not exist", is a failure mode nobody would have found from the KeyError alone.

One latent bug this uncovers — follow-up, not a change request

alter() never substitutes the placeholder at all, and it can emit it. declare.alter() builds its attribute SQL through _make_attribute_alter → compile_attribute, which for a PostgreSQL enum returns '"{database}".' + quote_identifier(type_name). That string is joined into the ALTER TABLE statement after the only .format() call, so it reaches the server as the literal text "{database}".enumtype.

Pre-existing, out of scope here, and your _substitute_database helper is exactly what it needs — one call at table.py:317, plus a test altering a table to add an enum attribute on Postgres. Worth its own issue so it does not get lost; happy to file it if you would rather keep moving.

[dimitri-yatsenko]

Follow-up: alter() has the same defect, plus two more behind it

Recording this here rather than as a separate issue, since _substitute_database is most of the fix and the context is this diff. Pre-existing, not introduced here — no change requested on this PR.

Altering a table to add or modify an enum attribute on PostgreSQL is broken in three compounding ways.

1. The placeholder is never substituted

alter() builds its statement at table.py:330 (this branch):

sql = "ALTER TABLE {tab}\n\t".format(tab=self.full_table_name) + ",\n\t".join(sql)

The .format() applies to the literal prefix only, and the joined attribute SQL is concatenated after it. That is exactly why braces in alter'd comments were never affected by the bug this PR fixes — and also why nothing substitutes {database} on this path.

The attribute SQL comes from prepare_declare() (declare.py:707, :718), the same function declare() uses, so for a PostgreSQL enum it contains '"{database}".' + quote_identifier(type_name) from adapters/postgres.py:368. That reaches the server as the literal text "{database}"."enum_a1b2c3d4".

2. CREATE TYPE is never issued, so substitution alone is not sufficient

The pending-enum drain lives in declare(), not in prepare_declare():

# declare.py:534 — inside def declare(), which starts at :445
if schema_name and hasattr(adapter, "get_pending_enum_ddl"):
    pre_ddl.extend(adapter.get_pending_enum_ddl(schema_name))

alter() returns tuple[list[str], list[str]] — sql and external_stores. There is no pre_ddl in its signature and no caller to drain into one. So the type the altered column references is never created. Fixing only §1 turns a nonsense-identifier error into "type does not exist".

3. The pending types leak into the next declaration

adapter._pending_enum_types is populated as a side effect of sql_type() and cleared only by get_pending_enum_ddl(). Since alter() never drains it, entries accumulate and the next declare() in the same session emits CREATE TYPE for types belonging to a table it is not declaring. Mostly invisible today, because those statements land in the except Exception: pass pre-DDL loop — which is the same guard that hid the failure in this PR's PostgreSQL story.

Suggested shape

1. declare.alter() returns pre_ddl as a third element, draining get_pending_enum_ddl(schema_name) the way declare() does.
2. Table.alter() runs those statements before the ALTER TABLE, and passes the statement through _substitute_database from this PR.
3. Consider narrowing the except Exception: pass in the pre-DDL loop to the already-exists case. Both this bug and the one this PR fixes were made harder to find by that guard swallowing unrelated errors — worth its own decision, since idempotent redeclaration is a real requirement.

Test that would have caught it

Declare a table with an enum attribute on PostgreSQL, then alter() it to add a second enum attribute with different values, and assert the new column exists with the right type. On master this fails at the literal {database}; with §1 alone it fails on the missing type.

Worth noting the two bugs share a root cause beyond the mechanics: declare() and alter() both assemble DDL from prepare_declare(), but only declare() performs the two steps that output requires — substitution and pre-DDL. Any future adapter that emits a placeholder or a pre-DDL statement will break on alter() the same way.

[dimitri-yatsenko]

Switching to request-changes. To be clear about what this is and is not: the code in this diff is correct — I verified the fragment is the only producer, the three sites are all the consumers, and the alternatives were rightly rejected. The request is about scope, not correctness.

1. Apply the fix to alter() in this PR

Detailed in the comment above. The short version: alter() assembles DDL from the same prepare_declare() and never substitutes the placeholder, so a PostgreSQL enum attribute reaches the server as the literal "{database}"."enum_a1b2c3d4". And because the pending-enum drain lives in declare() rather than prepare_declare(), no CREATE TYPE is issued either — so substitution alone converts one bad error into another.

The reason to do it here rather than in a follow-up: _substitute_database is the fix, and it exists as of this diff. Merging declare-only leaves the identical defect live on the sibling path, with the helper sitting right next to it unused. The next person to hit it will find a function named for exactly their problem and wonder why it was not applied — which is worse than the bug.

Concretely:

• declare.alter() returns pre_ddl as a third element, draining get_pending_enum_ddl(schema_name) as declare() does at declare.py:534.
• Table.alter() runs those statements before the ALTER TABLE, and passes the statement through _substitute_database.
• One test: declare a table with an enum attribute on PostgreSQL, alter() it to add a second enum with different values, assert the column exists with the right type.

If that turns out to be more than a small change — the NotImplementedError guards in alter() suggest that path has sharp edges I have not explored — say so and I will approve the declare-only fix with a linked issue instead. I would rather be told the scope is wrong than have it forced.

2. Correct one claim in the description

At default log level the error is swallowed by the logging excepthook ("Uncaught exception", no traceback)

Not on master. logging.py:58 states that DataJoint intentionally installs no process-wide sys.excepthook, removed in #1516 specifically so uncaught exceptions get Python's full traceback. That was true before #1516 and is not now.

This matters only because the PR description is the decision record. As written, a future reader debugging a similar swallowed error will go looking for a mechanism that no longer exists. The diagnosis difficulty on the PostgreSQL path is real and well told — it is the except Exception: pass in the pre-DDL loop, not an excepthook.

Still worth saying

The bug itself is a good find, and the report is unusually thorough — the enum-values-in-pre_ddl path degrading into "type does not exist" much later is the kind of detail that only comes from actually chasing it. Pinning the substitution mechanism in a unit test rather than only the symptom is the right instinct.

[MilagrosMarin]

Thanks @dimitri-yatsenko — both addressed.

✅ alter() pre-DDL + substitution. declare.alter() now takes an optional schema_name and returns the pending CREATE TYPE DDL as a third element; Table.alter runs it and the ALTER statement through the same _substitute_database helper declare uses. The drain happens between the two prepare_declare calls — both populate _pending_enum_types as a side effect, and the old definition's types already exist in the database, so draining after both would re-emit CREATE TYPE for them. What the old parse registers is discarded so it can't leak into the next declare() on that adapter.

✅ Test. test_multi_backend.py::test_alter_adds_enum_attribute — declares a table with an enum, alters it to add a second enum with a different value set (distinct type name, so it can't reuse the declaration-time type), asserts the column exists and round-trips a value. Passes on both backends.

Writing the test surfaced a second blocker, which is why this branch grew a bit: with the placeholder and pre-DDL fixed, the PostgreSQL ALTER then failed on syntax error at or near "AFTER". _make_attribute_alter was emitting MySQL's positioning clause unconditionally. Added a supports_column_position adapter property (True for MySQL, False for PostgreSQL) alongside the existing supports_inline_indexes / auto_indexes_foreign_keys flags, and dropped the position before it can force a statement so a reorder-only change is a no-op rather than a positionless MODIFY.

That covers ADD and DROP on PostgreSQL. MODIFY and CHANGE are still MySQL-only syntax — PG needs ALTER COLUMN ... TYPE and RENAME COLUMN — so attribute type changes and renames via alter() remain unsupported there. I left that alone as out of scope; happy to open a separate issue if you want it tracked.

✅ PR description. Removed the excepthook claim — it was wrong. DataJoint deliberately installs no process-wide sys.excepthook (logging.py, per #1516). The KeyError surfaces with a normal traceback; what actually made this hard to diagnose was the swallowed CREATE TYPE inside the except Exception: pass guard on the PostgreSQL path. Reworded to say that.

Full test_alter.py + test_declare.py + test_multi_backend.py pass (40 tests).

[MilagrosMarin]

Thanks @dimitri-yatsenko — both addressed. Taking your scope question seriously, though: it grew, and I want to be straight about how much.

✅ Description. Excepthook claim removed. You're right — logging.py says DataJoint deliberately installs none, per #1516. Reworded to attribute the diagnosis difficulty to the swallowed CREATE TYPE, which is what it actually was.

✅ alter(). Done, but it needed four fixes rather than the two we scoped, each only visible after the previous one landed:

1. Placeholder substitution — as you specified.
2. pre_ddl returned and issued — as you specified, with one wrinkle. Draining the new definition's types also picks up types the old definition already created, so every alter of a table with a pre-existing enum emitted a CREATE TYPE certain to fail. I subtract the old parse's drain instead of discarding it; names are content hashes so the statements compare directly. The guard stays — a hash is shared by every table in the schema using the same value set — but it logs now rather than discarding, since adding a fresh except Exception: pass in the fix for one seemed like poor form.
3. _make_attribute_alter emitted MySQL's AFTER clause unconditionally; PostgreSQL failed with syntax error at or near "AFTER". Gated behind a supports_column_position property next to supports_inline_indexes / auto_indexes_foreign_keys, dropping the position before it can force a statement so a reorder-only change stays a no-op.
4. The one worth flagging. original_type travels in the column comment as :type:comment, which PostgreSQL stores via COMMENT ON COLUMN — post-DDL that declare() emits and alter() discarded. So an added attribute came back as its generated type name, describe() stopped round-tripping, and since describe() feeds the next alter(), the table became permanently un-alterable (Unsupported attribute type enum_<hash>). alter() now returns the column comments and reapplies them.

Your test spec is what found (4). My first version asserted the column existed and a value round-tripped — both pass against a column whose declared type has been lost. Writing "with the right type" as you actually worded it failed on assert None == "enum('active', 'retired', 'transferred')". The test now also alters a second time, which is the behavioral form of the same invariant.

On the exit you offered. This is past "a small change" — alter() on PostgreSQL was substantially unimplemented rather than slightly broken. I kept it here because the four are interdependent and splitting means re-deriving the chain, but if you'd rather see the declare-only fix merged and this as a separate PR, say so and I'll split it — the offer was fair and I don't want to have forced the scope by attrition.

Still MySQL-only, out of scope, happy to file as one issue: MODIFY and CHANGE (PostgreSQL needs ALTER COLUMN … TYPE and RENAME COLUMN), and the table-comment clause at declare.py:761 that the code already flags.

Full integration + unit suites green on both backends.