diff --git a/docs/src/piccolo/api_reference/index.rst b/docs/src/piccolo/api_reference/index.rst index 6159548a2..7e4f58770 100644 --- a/docs/src/piccolo/api_reference/index.rst +++ b/docs/src/piccolo/api_reference/index.rst @@ -60,7 +60,6 @@ LazyTableReference .. currentmodule:: piccolo.columns .. autoclass:: LazyTableReference - :members: ------------------------------------------------------------------------------- diff --git a/piccolo/apps/migrations/auto/serialisation.py b/piccolo/apps/migrations/auto/serialisation.py index 320cf74f7..26964cac7 100644 --- a/piccolo/apps/migrations/auto/serialisation.py +++ b/piccolo/apps/migrations/auto/serialisation.py @@ -15,7 +15,7 @@ from piccolo.columns import Column from piccolo.columns.defaults.base import Default from piccolo.columns.reference import LazyTableReference -from piccolo.table import Table +from piccolo.table import Table, create_table_class from piccolo.utils.repr import repr_class_instance from .serialisation_legacy import deserialise_legacy_params @@ -483,6 +483,31 @@ def __repr__(self): ############################################################################### +def _primary_key_params(primary_key: Column) -> SerialisedParams: + """ + Returns the extra imports and definitions required for a primary column. + """ + primary_key_class = primary_key.__class__ + + pk_serialised_params: SerialisedParams = serialise_params( + params=primary_key._meta.params + ) + + pk_serialised_params.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, + ), + ) + ) + + return pk_serialised_params + + def serialise_params(params: t.Dict[str, t.Any]) -> SerialisedParams: """ When writing column params to a migration file, we need to serialise some @@ -664,12 +689,24 @@ def serialise_params(params: t.Dict[str, t.Any]) -> SerialisedParams: expect_conflict_with_global_name=UniqueGlobalNames.TABLE, ) ) + pk_serialised_params = _primary_key_params( + table_type._meta.primary_key + ) + extra_imports.extend(pk_serialised_params.extra_imports) + extra_definitions.extend(pk_serialised_params.extra_definitions) continue - # Replace any Table class values into class and table names - if inspect.isclass(value) and issubclass(value, Table): - params[key] = SerialisedCallable(callable_=value) - extra_definitions.append(SerialisedTableType(table_type=value)) + if key == "references" and isinstance(value, str): + # This isn't perfect - it's better if the user uses + # LazyTableReference, but do the best we can. + + referenced_table_class = create_table_class( + class_name=value.rsplit(".")[-1] + ) + + extra_definitions.append( + SerialisedTableType(table_type=referenced_table_class) + ) extra_imports.append( Import( module=Table.__module__, @@ -678,23 +715,26 @@ def serialise_params(params: t.Dict[str, t.Any]) -> SerialisedParams: ) ) - primary_key_class = value._meta.primary_key.__class__ + pk_serialised_params = _primary_key_params( + referenced_table_class._meta.primary_key + ) + extra_imports.extend(pk_serialised_params.extra_imports) + extra_definitions.extend(pk_serialised_params.extra_definitions) + continue + + # Replace any Table class values into class and table names + if inspect.isclass(value) and issubclass(value, Table): + params[key] = SerialisedCallable(callable_=value) + extra_definitions.append(SerialisedTableType(table_type=value)) 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, - ), + module=Table.__module__, + target=UniqueGlobalNames.TABLE, + expect_conflict_with_global_name=UniqueGlobalNames.TABLE, ) ) - # Include the extra imports and definitions required for the - # primary column params. - pk_serialised_params: SerialisedParams = serialise_params( - params=value._meta.primary_key._meta.params - ) + + pk_serialised_params = _primary_key_params(value._meta.primary_key) extra_imports.extend(pk_serialised_params.extra_imports) extra_definitions.extend(pk_serialised_params.extra_definitions) diff --git a/piccolo/columns/base.py b/piccolo/columns/base.py index 8c5b84064..29e02e061 100644 --- a/piccolo/columns/base.py +++ b/piccolo/columns/base.py @@ -79,7 +79,7 @@ def __repr__(self): @dataclass class ForeignKeyMeta: - references: t.Union[t.Type[Table], LazyTableReference] + references: t.Union[t.Type[Table], LazyTableReference, str] on_delete: OnDelete on_update: OnUpdate target_column: t.Union[Column, str, None] @@ -95,7 +95,14 @@ def resolved_references(self) -> t.Type[Table]: from piccolo.table import Table if isinstance(self.references, LazyTableReference): - return self.references.resolve() + if self.references.ready: + return self.references.resolve() + else: + raise ValueError( + f"The {self.references.table_class_name} table hasn't " + "fully initialised yet - please refactor your code so all " + "tables have imported before using them." + ) elif inspect.isclass(self.references) and issubclass( self.references, Table ): diff --git a/piccolo/columns/column_types.py b/piccolo/columns/column_types.py index 9934eebed..873cf0c81 100644 --- a/piccolo/columns/column_types.py +++ b/piccolo/columns/column_types.py @@ -1716,15 +1716,27 @@ class Musician(Table): In certain situations, you may be unable to reference a ``Table`` class if it causes a circular dependency. Try and avoid these by refactoring - your code. If unavoidable, you can specify a lazy reference. If the - ``Table`` is defined in the same file: + your code. If unavoidable, you can specify a lazy reference using + :class:`LazyTableReference `. + If the ``Table`` is defined in the same file: .. code-block:: python + from piccolo.columns.reference import LazyTableReference + class Band(Table): - manager = ForeignKey(references='Manager') + manager = ForeignKey( + references=LazyTableReference( + table_class_name="Manager", + module_path=__name__, + ) + # Alternatively, Piccolo will interpret this string the + # same as above: + # references="Manager" + ) - If the ``Table`` is defined in a Piccolo app: + Or if the ``Table`` is defined in a different file: + Python module: .. code-block:: python @@ -1733,12 +1745,15 @@ class Band(Table): class Band(Table): manager = ForeignKey( references=LazyTableReference( - table_class_name="Manager", app_name="my_app", + table_class_name="Manager", + module_path="some_module.tables", ) + # Alternatively, Piccolo will interpret this string the + # same as above: + # references="some_module.tables.Manager" ) - If you aren't using Piccolo apps, you can specify a ``Table`` in any - Python module: + If the ``Table`` is defined in a Piccolo app, you can also use this: .. code-block:: python @@ -1747,12 +1762,8 @@ class Band(Table): class Band(Table): manager = ForeignKey( references=LazyTableReference( - table_class_name="Manager", - module_path="some_module.tables", + table_class_name="Manager", app_name="my_app", ) - # Alternatively, Piccolo will interpret this string as - # the same as above: - # references="some_module.tables.Manager" ) :param on_delete: @@ -1887,19 +1898,38 @@ def __init__( # The ``TableMetaclass``` sets the actual value for # ``ForeignKeyMeta.references``, if the user passed in a string. self._foreign_key_meta = ForeignKeyMeta( - references=Table if isinstance(references, str) else references, + references=references, on_delete=on_delete, on_update=on_update, target_column=target_column, ) - def _setup(self, table_class: t.Type[Table]) -> ForeignKeySetupResponse: + def _on_ready(self): + """ + Once we know the table being referred to has been loaded, we call this + method. + """ + # Record the reverse relationship on the target table. + referenced_table = self._foreign_key_meta.resolved_references + if self not in referenced_table._meta._foreign_key_references: + referenced_table._meta._foreign_key_references.append(self) + # Allow columns on the referenced table to be accessed via + # auto completion. + self.set_proxy_columns() + + def _setup( + self, + table_class: t.Type[Table], + loaded_table_classes: t.List[t.Type[Table]] = [], + ) -> ForeignKeySetupResponse: """ This is called by the ``TableMetaclass``. A ``ForeignKey`` column can only be completely setup once it's parent ``Table`` is known. :param table_class: The parent ``Table`` class for this column. + :param loaded_table_classes: + Any ``Table`` classes which have already been loaded. """ from piccolo.table import Table @@ -1929,26 +1959,30 @@ def _setup(self, table_class: t.Type[Table]) -> ForeignKeySetupResponse: module_path=module_path, ) + self._foreign_key_meta.references = references + is_lazy = isinstance(references, LazyTableReference) + + if is_lazy: + # We should check if the referenced tables are already + # available. + if references.check_ready( + table_classes=loaded_table_classes + ).was_changed: + self._on_ready() + is_table_class = inspect.isclass(references) and issubclass( references, Table ) - if is_lazy or is_table_class: - self._foreign_key_meta.references = references - else: + if not (is_lazy or is_table_class): raise ValueError( "Error - ``references`` must be a ``Table`` subclass, or " "a ``LazyTableReference`` instance." ) if is_table_class: - # Record the reverse relationship on the target table. - references._meta._foreign_key_references.append(self) - - # Allow columns on the referenced table to be accessed via - # auto completion. - self.set_proxy_columns() + self._on_ready() return ForeignKeySetupResponse(is_lazy=is_lazy) @@ -2084,22 +2118,6 @@ def __getattribute__(self, name: str) -> t.Union[Column, t.Any]: case a copy is returned with an updated call_chain (which records the joins required). """ - # If the ForeignKey is using a lazy reference, we need to set the - # attributes here. Attributes starting with a double underscore are - # unlikely to be column names. - if not name.startswith("__"): - try: - _foreign_key_meta = object.__getattribute__( - self, "_foreign_key_meta" - ) - except AttributeError: - pass - else: - if _foreign_key_meta.proxy_columns == [] and isinstance( - _foreign_key_meta.references, LazyTableReference - ): - object.__getattribute__(self, "set_proxy_columns")() - try: value = object.__getattribute__(self, name) except AttributeError: diff --git a/piccolo/columns/reference.py b/piccolo/columns/reference.py index 841545eeb..85fa87453 100644 --- a/piccolo/columns/reference.py +++ b/piccolo/columns/reference.py @@ -3,8 +3,6 @@ """ from __future__ import annotations -import importlib -import inspect import typing as t from dataclasses import dataclass, field @@ -13,12 +11,19 @@ from piccolo.table import Table +@dataclass +class CheckReadyResponse: + is_ready: bool + was_changed: bool + + @dataclass class LazyTableReference: """ Holds a reference to a :class:`Table ` subclass. Used to avoid circular dependencies in the ``references`` argument of :class:`ForeignKey ` columns. + Pass in either ``app_name`` OR ``module_path``. :param table_class_name: The name of the ``Table`` subclass. For example, ``'Manager'``. @@ -44,35 +49,67 @@ def __post_init__(self): raise ValueError( "Specify either app_name or module_path - not both." ) + # We should only try resolving it once it is ready. We know it's ready + # because the table metaclass sets this to `True` when the + # corresponding table has been initialised. + self.ready = False + self._resolved: t.Optional[t.Table] = None + + def check_ready( + self, table_classes: t.List[t.Type[Table]] + ) -> CheckReadyResponse: + """ + Checks to see if the table being referenced is in the provided list, + meaning it has successfully loaded. + """ + if self.ready: + return CheckReadyResponse(is_ready=True, was_changed=False) + else: + for table_class in table_classes: + if self.table_class_name == table_class.__name__: + if ( + self.module_path + and self.module_path == table_class.__module__ + ) or ( + self.app_name + and self.app_name == table_class._meta.app_name + ): + self.ready = True + return CheckReadyResponse( + is_ready=True, was_changed=True + ) + + return CheckReadyResponse(is_ready=False, was_changed=False) def resolve(self) -> t.Type[Table]: + if self._resolved is not None: + return self._resolved + if self.app_name is not None: from piccolo.conf.apps import Finder finder = Finder() - return finder.get_table_with_name( + table_class = finder.get_table_with_name( app_name=self.app_name, table_class_name=self.table_class_name ) + self._resolved = table_class + return table_class if self.module_path: - module = importlib.import_module(self.module_path) - table: t.Optional[t.Type[Table]] = getattr( - module, self.table_class_name, None - ) + from piccolo.table import TABLE_REGISTRY - from piccolo.table import Table + for table_class in TABLE_REGISTRY: + if ( + table_class.__name__ == self.table_class_name + and table_class.__module__ == self.module_path + ): + self._resolved = table_class + return table_class - if ( - table is not None - and inspect.isclass(table) - and issubclass(table, Table) - ): - return table - else: - raise ValueError( - "Can't find a Table subclass called " - f"{self.table_class_name} in {self.module_path}" - ) + raise ValueError( + "Can't find a Table subclass called " + f"{self.table_class_name} in {self.module_path}" + ) raise ValueError("You must specify either app_name or module_path.") @@ -87,8 +124,23 @@ def __str__(self): @dataclass class LazyColumnReferenceStore: + # Foreign key columns which use LazyTableReference. foreign_key_columns: t.List[ForeignKey] = field(default_factory=list) + def check_ready(self, table_class: t.Type[Table]): + """ + The ``Table`` metaclass calls this once a ``Table`` has been imported. + It tells each ``LazyTableReference`` which references that table that + it's ready. + """ + for foreign_key_column in self.foreign_key_columns: + reference = t.cast( + LazyTableReference, + foreign_key_column._foreign_key_meta.references, + ) + if reference.check_ready(table_classes=[table_class]).was_changed: + foreign_key_column._on_ready() + def for_table(self, table: t.Type[Table]) -> t.List[ForeignKey]: return [ i diff --git a/piccolo/conf/apps.py b/piccolo/conf/apps.py index b88d50ba2..b452446de 100644 --- a/piccolo/conf/apps.py +++ b/piccolo/conf/apps.py @@ -156,6 +156,9 @@ def __post_init__(self): i if isinstance(i, Command) else Command(i) for i in self.commands ] + for table_class in self.table_classes: + table_class._meta.app_name = self.app_name + if isinstance(self.migrations_folder_path, pathlib.Path): self.migrations_folder_path = str(self.migrations_folder_path) diff --git a/piccolo/table.py b/piccolo/table.py index ddbe04fa6..ecf32cffb 100644 --- a/piccolo/table.py +++ b/piccolo/table.py @@ -74,6 +74,7 @@ class TableMeta: """ tablename: str = "" + app_name: t.Optional[str] = None columns: t.List[Column] = field(default_factory=list) default_columns: t.List[Column] = field(default_factory=list) non_default_columns: t.List[Column] = field(default_factory=list) @@ -353,14 +354,16 @@ def __init_subclass__( # ForeignKey columns require additional setup based on their # parent Table. foreign_key_setup_response = foreign_key_column._setup( - table_class=cls + table_class=cls, loaded_table_classes=TABLE_REGISTRY ) + if foreign_key_setup_response.is_lazy: LAZY_COLUMN_REFERENCES.foreign_key_columns.append( foreign_key_column ) TABLE_REGISTRY.append(cls) + LAZY_COLUMN_REFERENCES.check_ready(table_class=cls) def __init__( self, diff --git a/tests/apps/migrations/auto/integration/test_migrations.py b/tests/apps/migrations/auto/integration/test_migrations.py index dbcdbf032..c5c814593 100644 --- a/tests/apps/migrations/auto/integration/test_migrations.py +++ b/tests/apps/migrations/auto/integration/test_migrations.py @@ -954,25 +954,56 @@ def test_m2m(self): ############################################################################### -@engines_only("postgres", "cockroach") -class TestForeignKeys(MigrationTestCase): - def setUp(self): - class TableA(Table): - pass +class TableA(Table): + pass + + +class TableB(Table): + fk = ForeignKey(TableA) + + +class TableC(Table): + fk = ForeignKey(TableB) + + +class TableD(Table): + fk = ForeignKey(TableC) + + +class TableE(Table): + fk_2 = ForeignKey(LazyTableReference("TableF", module_path=__name__)) + fk_3 = ForeignKey(LazyTableReference("TableG", module_path=__name__)) + fk_5 = ForeignKey("TableG") - class TableB(Table): - fk = ForeignKey(TableA) - class TableC(Table): - fk = ForeignKey(TableB) +class TableF(Table): + """ + A table with a custom PK. + """ - class TableD(Table): - fk = ForeignKey(TableC) + id = UUID(primary_key=True) - class TableE(Table): - fk = ForeignKey(TableD) - self.table_classes = [TableA, TableB, TableC, TableD, TableE] +class TableG(Table): + """ + A table with the default Serial PK. + """ + + pass + + +@engines_only("postgres", "cockroach") +class TestForeignKeys(MigrationTestCase): + def setUp(self): + self.table_classes = [ + TableA, + TableB, + TableC, + TableD, + TableE, + TableF, + TableG, + ] def tearDown(self): drop_db_tables_sync(Migration, *self.table_classes) @@ -994,6 +1025,13 @@ def test_foreign_keys(self): for table_class in table_classes: self.assertTrue(table_class.table_exists().run_sync()) + def test_subset(self): + """ + Test a smaller subset, just in case there's a bug with importing + dependencies. + """ + self._test_migrations(table_snapshots=[[TableE, TableF, TableG]]) + @engines_only("postgres", "cockroach") class TestTargetColumn(MigrationTestCase): diff --git a/tests/apps/migrations/auto/test_serialisation.py b/tests/apps/migrations/auto/test_serialisation.py index 2920b72c9..3ba68150b 100644 --- a/tests/apps/migrations/auto/test_serialisation.py +++ b/tests/apps/migrations/auto/test_serialisation.py @@ -19,6 +19,7 @@ from piccolo.columns.column_types import Varchar from piccolo.columns.defaults import UUID4, DateNow, TimeNow, TimestampNow from piccolo.columns.reference import LazyTableReference +from tests.base import engine_is class TestUniqueGlobalNamesMeta: @@ -234,23 +235,27 @@ def test_lazy_table_reference(self): serialised.params["references"].__repr__() == "Manager" ) - self.assertTrue(len(serialised.extra_imports) == 1) - self.assertEqual( - serialised.extra_imports[0].__str__(), - "from piccolo.table import Table", + self.assertListEqual( + sorted([i.__str__() for i in serialised.extra_imports]), + [ + "from piccolo.columns.column_types import Serial", + "from piccolo.columns.indexes import IndexMethod", + "from piccolo.table import Table", + ], ) self.assertTrue(len(serialised.extra_definitions) == 1) - self.assertEqual( - serialised.extra_definitions[0].__str__(), - ( - 'class Manager(Table, tablename="manager", schema=None): ' - "id = Serial(null=False, primary_key=True, unique=False, " - "index=False, index_method=IndexMethod.btree, " - "choices=None, db_column_name='id', secret=False)" - ), - ) + if engine_is("postgres") or engine_is("cockroach"): + self.assertEqual( + serialised.extra_definitions[0].__str__(), + ( + 'class Manager(Table, tablename="manager", schema=None): ' # noqa: E501 + "id = Serial(null=False, primary_key=True, unique=False, " # noqa: E501 + "index=False, index_method=IndexMethod.btree, " + "choices=None, db_column_name='id', secret=False)" + ), + ) def test_function(self): serialised = serialise_params(params={"default": example_function})