Skip to content

fixed missing primary key import for LazyTableReference #1231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fixed missing primary key import for LazyTableReference #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c435c38
fixed missing primary key import for FK migration using LazyTableRefe…
sinisaos Jul 28, 2025
e386495
Merge branch 'piccolo-orm:master' into fix_lazytable_migration
sinisaos Jul 29, 2025
cb70a52
Merge branch 'piccolo-orm:master' into fix_lazytable_migration
sinisaos Jul 29, 2025
8b26a5a
Merge branch 'piccolo-orm:master' into fix_lazytable_migration
sinisaos Aug 6, 2025
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
15 changes: 15 additions & 0 deletions piccolo/apps/migrations/auto/serialisation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,21 @@ def serialise_params(params: dict[str, Any]) -> SerialisedParams:
expect_conflict_with_global_name=UniqueGlobalNames.TABLE,
)
)
# also add missing primary key to extra_imports when creating a
# migration with a ForeignKey that uses a LazyTableReference
# https://github.com/piccolo-orm/piccolo/issues/865
primary_key_class = table_type._meta.primary_key.__class__
extra_imports.append(
Import(
module=primary_key_class.__module__,
target=primary_key_class.__name__,
expect_conflict_with_global_name=getattr(
UniqueGlobalNames,
f"COLUMN_{primary_key_class.__name__.upper()}",
None,
),
)
)
Comment on lines +670 to +684
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this - I tested it, and it works.

I think the problem goes even deeper though.

Let's say you have this (note the UUID primary key):

class TableA(Table):
    table_b = ForeignKey(LazyTableReference('TableB', module_path=__module__))

class TableB(Table):
    id = UUID(primary_key=True)

If you create the migration, it now has the import for UUID, but it's missing the import for UUID4 (the default value for UUID).

The problem is, we use SerialisedTableType, which itself calls serialise_params. We need to access the extra_imports from that serialise_params call.

Sorry, it sounds very confusing.

But basically, I think the ideal solution would be this:

serialised_table_type = SerialisedTableType(table_type=table_type)
extra_definitions.append(serialised_table_type)
extra_imports.extend(serialised_table_type.extra_imports)

So we need to refactor SerialisedTableType slightly so can access its extra imports.

Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my case everything works fine because I already tried this PR with custom primary key. UUID4() is imported along with UUID with tables like this.

class TableA(Table):
    id = UUID(primary_key=True)
    table_b = ForeignKey(LazyTableReference("TableB", module_path=__module__))

class TableB(Table):
    id = UUID(primary_key=True)
Migration file 
from piccolo.apps.migrations.auto.migration_manager import MigrationManager
from piccolo.columns.base import OnDelete
from piccolo.columns.base import OnUpdate
from piccolo.columns.column_types import ForeignKey
from piccolo.columns.column_types import UUID # <- correct import
from piccolo.columns.defaults.uuid import UUID4 # <- correct import
from piccolo.columns.indexes import IndexMethod
from piccolo.table import Table


class TableB(Table, tablename="table_b", schema=None):
    id = UUID( # <- correct
        default=UUID4(), # <- correct
        null=False,
        primary_key=True,
        unique=False,
        index=False,
        index_method=IndexMethod.btree,
        choices=None,
        db_column_name=None,
        secret=False,
    )


ID = "2025-07-31T09:41:33:395378"
VERSION = "1.28.0"
DESCRIPTION = ""


async def forwards():
    manager = MigrationManager(
        migration_id=ID, app_name="home", description=DESCRIPTION
    )

    manager.add_table(
        class_name="TableB", tablename="table_b", schema=None, columns=None
    )

    manager.add_table(
        class_name="TableA", tablename="table_a", schema=None, columns=None
    )

    manager.add_column(
        table_class_name="TableB",
        tablename="table_b",
        column_name="id",
        db_column_name="id",
        column_class_name="UUID",
        column_class=UUID, # <- correct
        params={
            "default": UUID4(), # <- correct
            "null": False,
            "primary_key": True,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
        schema=None,
    )

    manager.add_column(
        table_class_name="TableA",
        tablename="table_a",
        column_name="id",
        db_column_name="id",
        column_class_name="UUID",
        column_class=UUID, # <- correct
        params={
            "default": UUID4(), # <- correct
            "null": False,
            "primary_key": True,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
        schema=None,
    )

    manager.add_column(
        table_class_name="TableA",
        tablename="table_a",
        column_name="table_b",
        db_column_name="table_b",
        column_class_name="ForeignKey",
        column_class=ForeignKey,
        params={
            "references": TableB,
            "on_delete": OnDelete.cascade,
            "on_update": OnUpdate.cascade,
            "target_column": None,
            "null": True,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
        schema=None,
    )

    return manager

If you think the SerialisedTableType should be changed, feel free to implement it in this PR.

continue

# Replace any Table class values into class and table names
Expand Down
10 changes: 8 additions & 2 deletions tests/apps/migrations/auto/test_serialisation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,16 @@ def test_lazy_table_reference(self):
self.assertTrue(
serialised.params["references"].__repr__() == "Manager"
)
# sorted extra_imports for consistency between tests
sorted_extra_imports = sorted(serialised.extra_imports)

self.assertTrue(len(serialised.extra_imports) == 1)
self.assertTrue(len(serialised.extra_imports) == 2)
self.assertEqual(
serialised.extra_imports[0].__str__(),
sorted_extra_imports[0].__str__(),
"from piccolo.columns.column_types import Serial",
)
self.assertEqual(
sorted_extra_imports[1].__str__(),
"from piccolo.table import Table",
)

Expand Down