Repository navigation
piccolo migrations new --auto crashes when an altered column's previous class was a custom Column subclass #1428
Description
Summary
SchemaDiffer.alter_columns looks up UniqueGlobalNames.COLUMN_<NAME> for both the new and the old column class. The lookup for the new class passes a None default; the one for the old class doesn't. Altering a column whose previous class was a user-defined Column subclass therefore raises AttributeError, and piccolo migrations new --auto dies before writing anything.
Reproduction
No database needed:
from piccolo.apps.migrations.auto.diffable_table import DiffableTable
from piccolo.apps.migrations.auto.schema_differ import SchemaDiffer
from piccolo.columns.column_types import Varchar
class CustomVarchar(Varchar):
pass
old = CustomVarchar(length=10)
old._meta.name = "name"
new = Varchar(length=20)
new._meta.name = "name"
differ = SchemaDiffer(
schema=[DiffableTable(class_name="Band", tablename="band", columns=[new])],
schema_snapshot=[
DiffableTable(class_name="Band", tablename="band", columns=[old])
],
auto_input="y",
)
print(differ.alter_columns.statements)Expected: one manager.alter_column(...) statement.
Actual:
Traceback (most recent call last):
File "<stdin>", line 16, in <module>
File ".../piccolo/apps/migrations/auto/schema_differ.py", line 516, in alter_columns
expect_conflict_with_global_name=getattr(
^^^^^^^^
AttributeError: type object 'UniqueGlobalNames' has no attribute 'COLUMN_CUSTOMVARCHAR'. Did you mean: 'COLUMN_VARCHAR'?
Cause
schema_differ.py makes six getattr(UniqueGlobalNames, f"COLUMN_{...}") lookups. Five pass None as the default (lines 503, 594, 635, 725, 767 on master). The sixth - the old_column_class branch at line 516, twelve lines below its column_class twin - does not:
if alter_column.column_class is not None:
...
expect_conflict_with_global_name=getattr(
UniqueGlobalNames,
f"COLUMN_{alter_column.column_class.__name__.upper()}",
None, # <- present
),
if alter_column.old_column_class is not None:
...
expect_conflict_with_global_name=getattr(
UniqueGlobalNames,
f"COLUMN_{alter_column.old_column_class.__name__.upper()}",
), # <- missingUniqueGlobalNames only carries the built-in column classes, so a miss is the expected outcome for any subclass a user defines - which the five other call sites already treat as normal.
Impact
Subclassing a column type is a supported and fairly common thing to do - wrapping Varchar to fix a length, or Timestamptz to add a server-side default, so the constraint lives in one place rather than at every declaration. Any project that does this loses --auto entirely the first time such a column changes, with a traceback that points at global-name bookkeeping rather than at the column involved. The workaround is to hand-write the migration.
Environment
- Piccolo
master@6ef37514, and the 1.36.0 release - Python 3.12
- Database-independent:
SchemaDifferraises before any engine is used
Suggested fix
Add the None default so both branches behave identically. I have a branch with that one-line change plus a regression test in tests/apps/migrations/auto/test_schema_differ.py (styled on the neighbouring test_alter_column_precision); tests/apps/migrations and tests/columns pass, as do isort, black and flake8.
See #1429
Activity
Don't crash when an altered column's old class is a custom Column sub…