Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/datajoint/adapters/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
"""
Expand Down
8 changes: 8 additions & 0 deletions src/datajoint/adapters/postgres.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
"""
Expand Down
41 changes: 37 additions & 4 deletions src/datajoint/declare.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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.

Expand All @@ -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
------
Expand All @@ -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_,
Expand All @@ -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_:
Expand All @@ -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]:
Expand Down
52 changes: 47 additions & 5 deletions src/datajoint/table.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
"""
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions tests/integration/test_declare.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = """
Expand Down
85 changes: 85 additions & 0 deletions tests/integration/test_multi_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand All @@ -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()
16 changes: 16 additions & 0 deletions tests/unit/test_ddl_substitution.py
Original file line number Diff line number Diff line change
@@ -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
Loading