From 30acf3ca791c3562e697f4f22a1d102561b3814b Mon Sep 17 00:00:00 2001 From: sinisaos Date: Thu, 25 Aug 2022 20:41:49 +0200 Subject: [PATCH 01/11] reverse lookup prototype --- piccolo/columns/m2m.py | 2 +- piccolo/columns/reverse_lookup.py | 223 +++++++++++++++++++++++++++ piccolo/query/methods/select.py | 163 ++++++++++++++++++++ tests/columns/test_m2m.py | 7 + tests/columns/test_reverse_lookup.py | 220 ++++++++++++++++++++++++++ 5 files changed, 614 insertions(+), 1 deletion(-) create mode 100644 piccolo/columns/reverse_lookup.py create mode 100644 tests/columns/test_reverse_lookup.py diff --git a/piccolo/columns/m2m.py b/piccolo/columns/m2m.py index db0d61d77..30e3b7a17 100644 --- a/piccolo/columns/m2m.py +++ b/piccolo/columns/m2m.py @@ -14,7 +14,7 @@ ) from piccolo.utils.sync import run_sync -if t.TYPE_CHECKING: +if t.TYPE_CHECKING: # pragma: no cover from piccolo.table import Table diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py new file mode 100644 index 000000000..4f2c22951 --- /dev/null +++ b/piccolo/columns/reverse_lookup.py @@ -0,0 +1,223 @@ +from __future__ import annotations + +import inspect +import typing as t +from dataclasses import dataclass + +from piccolo.columns.base import Selectable +from piccolo.columns.column_types import ( + JSON, + JSONB, + Column, + LazyTableReference, +) + +if t.TYPE_CHECKING: # pragma: no cover + from piccolo.table import Table + + +class ReverseLookupSelect(Selectable): + """ + This is a subquery used within a select to fetch data via an M2M table. + """ + + def __init__( + self, + *columns: Column, + reverse_lookup: ReverseLookup, + as_list: bool = False, + load_json: bool = False, + ): + """ + :param columns: + Which columns to include from the related table. + :param as_list: + If a single column is provided, and ``as_list`` is ``True`` a + flattened list will be returned, rather than a list of objects. + :param load_json: + If ``True``, any JSON strings are loaded as Python objects. + + """ + self.as_list = as_list + self.columns = columns + self.reverse_lookup = reverse_lookup + self.load_json = load_json + + safe_types = [int, str] + + # If the columns can be serialised / deserialise as JSON, then we + # can fetch the data all in one go. + self.serialisation_safe = all( + (column.__class__.value_type in safe_types) + and (type(column) not in (JSON, JSONB)) + for column in columns + ) + + @property + def foreign_key_columns_index(self) -> int: + # If we have multiple FK columns per table, we need to know which + # column we are using for reverse lookup and use that column's index + # TODO - find better way to this because user must set + # reverse_lookup_name (have to be exact table name + # otherwise we get error) + reverse_lookup_table = ( + self.reverse_lookup._meta.resolved_reverse_joining_table + ) + fk_columns = [ + i._meta.name + for i in reverse_lookup_table._meta.foreign_key_columns + ] + return fk_columns.index(self.reverse_lookup._meta.reverse_lookup_name) + + def get_select_string(self, engine_type: str, with_alias=True) -> str: + reverse_lookup_table = ( + self.reverse_lookup._meta.resolved_reverse_joining_table + ) + reverse_lookup_table_name = reverse_lookup_table._meta.tablename + reverse_lookup_pk = reverse_lookup_table._meta.primary_key._meta.name + reverse_lookup_fk_table_name = ( + reverse_lookup_table._meta.foreign_key_columns[ + self.foreign_key_columns_index + ]._meta.name + ) + reverse_lookup_fk = reverse_lookup_table._meta.foreign_key_columns[ + self.foreign_key_columns_index + ]._meta.db_column_name + + reverse_select = f""" + "{reverse_lookup_table_name}" + JOIN "{reverse_lookup_fk_table_name}" "inner_{reverse_lookup_fk_table_name}" ON ( + "{reverse_lookup_table_name}"."{reverse_lookup_fk}" + = "inner_{reverse_lookup_fk_table_name}"."{reverse_lookup_pk}" + ) WHERE "{reverse_lookup_table_name}"."{reverse_lookup_fk}" + = "{reverse_lookup_fk_table_name}"."{reverse_lookup_pk}" + """ # noqa: E501 + + if engine_type == "postgres": + if self.as_list: + column_name = self.columns[0]._meta.db_column_name + return f""" + ARRAY( + SELECT + "{reverse_lookup_table_name}"."{column_name}" + FROM {reverse_select} + ) AS "{reverse_lookup_table_name}s" + """ + elif not self.serialisation_safe: + column_name = reverse_lookup_pk + return f""" + ARRAY( + SELECT + "{reverse_lookup_table_name}"."{column_name}" + FROM {reverse_select} + ) AS "{reverse_lookup_table_name}s" + """ + else: + if len(self.columns) > 0: + column_names = ", ".join( + f'"{reverse_lookup_table_name}"."{column._meta.db_column_name}"' # noqa: E501 + for column in self.columns + ) + else: + column_names = ", ".join( + f'"{reverse_lookup_table_name}"."{column._meta.db_column_name}"' # noqa: E501 + for column in reverse_lookup_table._meta.columns + ) + return f""" + ( + SELECT JSON_AGG("{reverse_lookup_table_name}s") + FROM ( + SELECT {column_names} FROM {reverse_select} + ) AS "{reverse_lookup_table_name}s" + ) AS "{reverse_lookup_table_name}s" + """ + elif engine_type == "sqlite": + if len(self.columns) > 1 or not self.serialisation_safe: + column_name = reverse_lookup_pk + else: + try: + column_name = self.columns[0]._meta.db_column_name + except IndexError: + column_name = reverse_lookup_pk + + return f""" + ( + SELECT group_concat( + "{reverse_lookup_table_name}"."{column_name}" + ) + FROM {reverse_select} + ) + AS "{reverse_lookup_table_name}s [M2M]" + """ + else: + raise ValueError(f"{engine_type} is an unrecognised engine type") + + +@dataclass +class ReverseLookupMeta: + reverse_joining_table: t.Union[t.Type[Table], LazyTableReference] + reverse_lookup_name: str + + @property + def resolved_reverse_joining_table(self) -> t.Type[Table]: + """ + Evaluates the ``reverse_joining_table`` attribute if it's a + ``LazyTableReference``, raising a ``ValueError`` if it fails, + otherwise returns a ``Table`` subclass. + """ + from piccolo.table import Table + + if isinstance(self.reverse_joining_table, LazyTableReference): + return self.reverse_joining_table.resolve() + elif inspect.isclass(self.reverse_joining_table) and issubclass( + self.reverse_joining_table, Table + ): + return self.reverse_joining_table + else: + raise ValueError( + "The reverse_joining_table attribute is neither a Table" + " subclass or a LazyTableReference instance." + ) + + +class ReverseLookup: + def __init__( + self, + reverse_joining_table: t.Union[t.Type[Table], LazyTableReference], + reverse_lookup_name: str, + ): + """ + :param reverse_joining_table: + A ``Table`` for reverse lookup. + :param reverse_lookup_name: + Must be set with reverse table name because if we have + multiple FK columns per table, we need to know which column + we are using for reverse lookup and use that column's index. + """ + self._meta = ReverseLookupMeta( + reverse_joining_table=reverse_joining_table, + reverse_lookup_name=reverse_lookup_name, + ) + + def __call__( + self, *columns: Column, as_list: bool = False, load_json: bool = False + ) -> ReverseLookupSelect: + """ + :param columns: + Which columns to include from the related table. If none are + specified, then all of the columns are returned. + :param as_list: + If a single column is provided, and ``as_list`` is ``True`` a + flattened list will be returned, rather than a list of objects. + :param load_json: + If ``True``, any JSON strings are loaded as Python objects. + """ + + if as_list and len(columns) != 1: + raise ValueError( + "`as_list` is only valid with a single column argument" + ) + + return ReverseLookupSelect( + *columns, reverse_lookup=self, as_list=as_list, load_json=load_json + ) diff --git a/piccolo/query/methods/select.py b/piccolo/query/methods/select.py index 623230ada..3628fee20 100644 --- a/piccolo/query/methods/select.py +++ b/piccolo/query/methods/select.py @@ -9,6 +9,7 @@ from piccolo.columns.column_types import JSON, JSONB, PrimaryKey from piccolo.columns.m2m import M2MSelect from piccolo.columns.readable import Readable +from piccolo.columns.reverse_lookup import ReverseLookupSelect from piccolo.engine.base import Batch from piccolo.query.base import Query from piccolo.query.mixins import ( @@ -330,6 +331,11 @@ async def response_handler(self, response): for i in self.columns_delegate.selected_columns if isinstance(i, M2MSelect) ] + reverse_lookup_selects = [ + i + for i in self.columns_delegate.selected_columns + if isinstance(i, ReverseLookupSelect) + ] for m2m_select in m2m_selects: m2m_name = m2m_select.m2m._meta.name secondary_table = m2m_select.m2m._meta.secondary_table @@ -422,6 +428,163 @@ async def response_handler(self, response): m2m_select, ) + for reverse_lookup_select in reverse_lookup_selects: + reverse_table = ( + reverse_lookup_select.reverse_lookup._meta.resolved_reverse_joining_table # noqa: E501 + ) + reverse_lookup_table_name = reverse_table._meta.tablename + + if self.engine_type == "sqlite": + # With M2M queries in SQLite, we always get the value back as a + # list of strings, so we need to do some type conversion. + value_type = ( + reverse_lookup_select.columns[0].__class__.value_type + if reverse_lookup_select.as_list + and reverse_lookup_select.serialisation_safe + else reverse_table._meta.primary_key.value_type + ) + try: + for row in response: + data = row[f"{reverse_lookup_table_name}s"] + row[f"{reverse_lookup_table_name}s"] = ( + [ + value_type(i) + for i in row[f"{reverse_lookup_table_name}s"] + ] + if data + else [] + ) + except ValueError: + colored_warning( + "Unable to do type conversion for the " + f"{reverse_lookup_table_name}s relation" + ) + + # If the user requested a single column, we just return that + # from the database. Otherwise we request the primary key + # value, so we can fetch the rest of the data in a subsequent + # SQL query - see below. + if reverse_lookup_select.as_list: + if reverse_lookup_select.serialisation_safe: + pass + else: + response = await self._splice_m2m_rows( + response, + reverse_table, + reverse_table._meta.primary_key, + f"{reverse_lookup_table_name}s", + reverse_lookup_select, + as_list=True, + ) + else: + if ( + len(reverse_lookup_select.columns) == 1 + and reverse_lookup_select.serialisation_safe + ): + column_name = reverse_lookup_select.columns[ + 0 + ]._meta.name + for row in response: + if row[f"{reverse_lookup_table_name}s"] is None: + row[f"{reverse_lookup_table_name}s"] = [] + row[f"{reverse_lookup_table_name}s"] = [ + {column_name: i} + for i in row[f"{reverse_lookup_table_name}s"] + ] + elif ( + len(reverse_lookup_select.columns) == 0 + and reverse_lookup_select.serialisation_safe + ): + # if user request all columns + row_ids = list( + set( + itertools.chain( + *[ + row[f"{reverse_lookup_table_name}s"] + for row in response + ] + ) + ) + ) + extra_rows = ( + ( + await reverse_table.select( + *reverse_table._meta.columns, + reverse_table._meta.primary_key.as_alias( + "mapping_key" + ), + ) + .where( + reverse_table._meta.primary_key.is_in( + row_ids + ) + ) + .output( + load_json=reverse_lookup_select.load_json + ) + .run() + ) + if row_ids + else [] + ) + extra_rows_map = { + row["mapping_key"]: { + key: value + for key, value in row.items() + if key != "mapping_key" + } + for row in extra_rows + } + for row in response: + row[f"{reverse_lookup_table_name}s"] = [ + extra_rows_map.get(i) + for i in row[f"{reverse_lookup_table_name}s"] + ] + else: + response = await self._splice_m2m_rows( + response, + reverse_table, + reverse_table._meta.primary_key, + f"{reverse_lookup_table_name}s", + reverse_lookup_select, + as_list=False, + ) + if self.engine_type == "postgres": + if reverse_lookup_select.as_list: + # We get the data back as an array, and can just return it + # unless it's JSON. + if ( + type(reverse_lookup_select.columns[0]) in (JSON, JSONB) + and reverse_lookup_select.load_json + ): + for row in response: + data = row[reverse_lookup_select.columns[0]] + row[reverse_lookup_select.columns[0]] = [ + load_json(i) for i in data + ] + + elif reverse_lookup_select.serialisation_safe: + # If the columns requested can be safely serialised, they + # are returned as a JSON string, so we need to deserialise + # it. + for row in response: + data = row[f"{reverse_lookup_table_name}s"] + row[f"{reverse_lookup_table_name}s"] = ( + load_json(data) if data else [] + ) + else: + # If the data can't be safely serialised as JSON, we get + # back an array of primary key values, and need to + # splice in the correct values using Python. + response = await self._splice_m2m_rows( + response, + reverse_table, + reverse_table._meta.primary_key, + f"{reverse_lookup_table_name}s", + reverse_lookup_select, + as_list=False, + ) + ####################################################################### # If no columns were specified, it's a select *, so we know that diff --git a/tests/columns/test_m2m.py b/tests/columns/test_m2m.py index d3374cfff..1b7e7733f 100644 --- a/tests/columns/test_m2m.py +++ b/tests/columns/test_m2m.py @@ -196,6 +196,13 @@ def test_select_multiple(self): ], ) + def test_select_multiple_as_list_error(self): + + with self.assertRaises(ValueError): + Band.select( + Band.name, Band.genres(Genre.id, Genre.name, as_list=True) + ).run_sync() + def test_select_id(self): response = Band.select( Band.name, Band.genres(Genre.id, as_list=True) diff --git a/tests/columns/test_reverse_lookup.py b/tests/columns/test_reverse_lookup.py new file mode 100644 index 000000000..820d2555a --- /dev/null +++ b/tests/columns/test_reverse_lookup.py @@ -0,0 +1,220 @@ +from unittest import TestCase + +from piccolo.columns.column_types import ( + UUID, + ForeignKey, + LazyTableReference, + Varchar, +) +from piccolo.columns.reverse_lookup import ReverseLookup +from piccolo.table import Table, create_db_tables_sync, drop_db_tables_sync + + +class Manager(Table): + name = Varchar() + bands = ReverseLookup( + LazyTableReference("Band", module_path=__name__), + reverse_lookup_name="manager", + ) + + +class Band(Table): + name = Varchar() + manager = ForeignKey(Manager) + + +SIMPLE_SCHEMA = [Manager, Band] + + +class TestReverseLookup(TestCase): + def setUp(self): + create_db_tables_sync(*SIMPLE_SCHEMA, if_not_exists=True) + + Manager.insert( + Manager(name="Guido"), + Manager(name="Mark"), + Manager(name="John"), + ).run_sync() + + Band.insert( + Band(name="Pythonistas", manager=1), + Band(name="Rustaceans", manager=1), + Band(name="C-Sharps", manager=2), + ).run_sync() + + def tearDown(self): + drop_db_tables_sync(*SIMPLE_SCHEMA) + + def test_select_name(self): + response = Manager.select( + Manager.name, Manager.bands(Band.name, as_list=True) + ).run_sync() + self.assertEqual( + response, + [ + {"name": "Guido", "bands": ["Pythonistas", "Rustaceans"]}, + {"name": "Mark", "bands": ["C-Sharps"]}, + {"name": "John", "bands": []}, + ], + ) + + def test_select_multiple(self): + response = Manager.select( + Manager.name, Manager.bands(Band.id, Band.name) + ).run_sync() + + self.assertEqual( + response, + [ + { + "name": "Guido", + "bands": [ + {"id": 1, "name": "Pythonistas"}, + {"id": 2, "name": "Rustaceans"}, + ], + }, + {"name": "Mark", "bands": [{"id": 3, "name": "C-Sharps"}]}, + { + "name": "John", + "bands": [], + }, + ], + ) + + def test_select_multiple_all_columns(self): + response = Manager.select(Manager.name, Manager.bands()).run_sync() + + self.assertEqual( + response, + [ + { + "name": "Guido", + "bands": [ + {"id": 1, "name": "Pythonistas", "manager": 1}, + {"id": 2, "name": "Rustaceans", "manager": 1}, + ], + }, + { + "name": "Mark", + "bands": [{"id": 3, "name": "C-Sharps", "manager": 2}], + }, + { + "name": "John", + "bands": [], + }, + ], + ) + + def test_select_id(self): + response = Manager.select( + Manager.name, Manager.bands(Band.id, as_list=True) + ).run_sync() + + self.assertEqual( + response, + [ + {"name": "Guido", "bands": [1, 2]}, + {"name": "Mark", "bands": [3]}, + {"name": "John", "bands": []}, + ], + ) + + def test_select_multiple_as_list_error(self): + + with self.assertRaises(ValueError): + Manager.select( + Manager.name, Manager.bands(Band.id, Band.name, as_list=True) + ).run_sync() + + +############################################################################### + +# A schema using custom primary keys + + +class Customer(Table): + uuid = UUID(primary_key=True) + name = Varchar() + concerts = ReverseLookup( + LazyTableReference("Concert", module_path=__name__), + reverse_lookup_name="customer", + ) + + +class Concert(Table): + uuid = UUID(primary_key=True) + name = Varchar() + customer = ForeignKey(Customer) + + +CUSTOM_PK_SCHEMA = [Customer, Concert] + + +class TestReverseLookupCustomPrimaryKey(TestCase): + """ + Make sure the ReverseLookupCustom functionality works correctly + when the tables have custom primary key columns. + """ + + def setUp(self): + create_db_tables_sync(*CUSTOM_PK_SCHEMA, if_not_exists=True) + + def tearDown(self): + drop_db_tables_sync(*CUSTOM_PK_SCHEMA) + + def test_select_custom_primary_key(self): + Customer.objects().create(name="Bob").run_sync() + Customer.objects().create(name="Sally").run_sync() + Customer.objects().create(name="Fred").run_sync() + + bob_pk = ( + Customer.select(Customer.uuid) + .where(Customer.name == "Bob") + .first() + .run_sync() + ) + sally_pk = ( + Customer.select(Customer.uuid) + .where(Customer.name == "Sally") + .first() + .run_sync() + ) + + Concert.objects().create( + name="Rockfest", customer=bob_pk["uuid"] + ).run_sync() + Concert.objects().create( + name="Folkfest", customer=bob_pk["uuid"] + ).run_sync() + Concert.objects().create( + name="Classicfest", customer=sally_pk["uuid"] + ).run_sync() + + response = Customer.select( + Customer.name, Customer.concerts(Concert.name, as_list=True) + ).run_sync() + + self.assertListEqual( + response, + [ + {"name": "Bob", "concerts": ["Rockfest", "Folkfest"]}, + {"name": "Sally", "concerts": ["Classicfest"]}, + {"name": "Fred", "concerts": []}, + ], + ) + + response = Customer.select( + Customer.name, Customer.concerts(Concert.name) + ).run_sync() + + self.assertEqual( + response, + [ + { + "name": "Bob", + "concerts": [{"name": "Rockfest"}, {"name": "Folkfest"}], + }, + {"name": "Sally", "concerts": [{"name": "Classicfest"}]}, + {"name": "Fred", "concerts": []}, + ], + ) From 59403044e9bccf97b92022d5e27dcdb5e2b97203 Mon Sep 17 00:00:00 2001 From: sinisaos Date: Fri, 26 Aug 2022 07:10:45 +0200 Subject: [PATCH 02/11] docstring fix --- piccolo/columns/reverse_lookup.py | 2 +- piccolo/query/methods/select.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py index 4f2c22951..248047948 100644 --- a/piccolo/columns/reverse_lookup.py +++ b/piccolo/columns/reverse_lookup.py @@ -18,7 +18,7 @@ class ReverseLookupSelect(Selectable): """ - This is a subquery used within a select to fetch data via an M2M table. + This is a subquery used within a select to fetch reverse lookup data. """ def __init__( diff --git a/piccolo/query/methods/select.py b/piccolo/query/methods/select.py index 3628fee20..6e40fe584 100644 --- a/piccolo/query/methods/select.py +++ b/piccolo/query/methods/select.py @@ -435,8 +435,9 @@ async def response_handler(self, response): reverse_lookup_table_name = reverse_table._meta.tablename if self.engine_type == "sqlite": - # With M2M queries in SQLite, we always get the value back as a - # list of strings, so we need to do some type conversion. + # With ReverseLookup queries in SQLite, we always get + # the value back as a list of strings, so we need to + # do some type conversion. value_type = ( reverse_lookup_select.columns[0].__class__.value_type if reverse_lookup_select.as_list From c74e89ece0f883884d707a1768d8f2df7d4419df Mon Sep 17 00:00:00 2001 From: sinisaos Date: Sun, 28 Aug 2022 08:36:43 +0200 Subject: [PATCH 03/11] remove reverse_lookup_name --- piccolo/columns/reverse_lookup.py | 28 +++++++++++++--------------- tests/columns/test_reverse_lookup.py | 20 +++++++++++--------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py index 248047948..b9b15a55f 100644 --- a/piccolo/columns/reverse_lookup.py +++ b/piccolo/columns/reverse_lookup.py @@ -27,6 +27,7 @@ def __init__( reverse_lookup: ReverseLookup, as_list: bool = False, load_json: bool = False, + table: t.Type[Table], ): """ :param columns: @@ -42,6 +43,7 @@ def __init__( self.columns = columns self.reverse_lookup = reverse_lookup self.load_json = load_json + self.table = table safe_types = [int, str] @@ -55,11 +57,6 @@ def __init__( @property def foreign_key_columns_index(self) -> int: - # If we have multiple FK columns per table, we need to know which - # column we are using for reverse lookup and use that column's index - # TODO - find better way to this because user must set - # reverse_lookup_name (have to be exact table name - # otherwise we get error) reverse_lookup_table = ( self.reverse_lookup._meta.resolved_reverse_joining_table ) @@ -67,7 +64,7 @@ def foreign_key_columns_index(self) -> int: i._meta.name for i in reverse_lookup_table._meta.foreign_key_columns ] - return fk_columns.index(self.reverse_lookup._meta.reverse_lookup_name) + return fk_columns.index(self.table._meta.tablename) def get_select_string(self, engine_type: str, with_alias=True) -> str: reverse_lookup_table = ( @@ -156,7 +153,6 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: @dataclass class ReverseLookupMeta: reverse_joining_table: t.Union[t.Type[Table], LazyTableReference] - reverse_lookup_name: str @property def resolved_reverse_joining_table(self) -> t.Type[Table]: @@ -184,23 +180,21 @@ class ReverseLookup: def __init__( self, reverse_joining_table: t.Union[t.Type[Table], LazyTableReference], - reverse_lookup_name: str, ): """ :param reverse_joining_table: A ``Table`` for reverse lookup. - :param reverse_lookup_name: - Must be set with reverse table name because if we have - multiple FK columns per table, we need to know which column - we are using for reverse lookup and use that column's index. """ self._meta = ReverseLookupMeta( reverse_joining_table=reverse_joining_table, - reverse_lookup_name=reverse_lookup_name, ) def __call__( - self, *columns: Column, as_list: bool = False, load_json: bool = False + self, + *columns: Column, + as_list: bool = False, + load_json: bool = False, + table: t.Type[Table], ) -> ReverseLookupSelect: """ :param columns: @@ -219,5 +213,9 @@ def __call__( ) return ReverseLookupSelect( - *columns, reverse_lookup=self, as_list=as_list, load_json=load_json + *columns, + reverse_lookup=self, + as_list=as_list, + load_json=load_json, + table=table, ) diff --git a/tests/columns/test_reverse_lookup.py b/tests/columns/test_reverse_lookup.py index 820d2555a..3c5f743c6 100644 --- a/tests/columns/test_reverse_lookup.py +++ b/tests/columns/test_reverse_lookup.py @@ -14,7 +14,6 @@ class Manager(Table): name = Varchar() bands = ReverseLookup( LazyTableReference("Band", module_path=__name__), - reverse_lookup_name="manager", ) @@ -47,7 +46,7 @@ def tearDown(self): def test_select_name(self): response = Manager.select( - Manager.name, Manager.bands(Band.name, as_list=True) + Manager.name, Manager.bands(Band.name, as_list=True, table=Manager) ).run_sync() self.assertEqual( response, @@ -60,7 +59,7 @@ def test_select_name(self): def test_select_multiple(self): response = Manager.select( - Manager.name, Manager.bands(Band.id, Band.name) + Manager.name, Manager.bands(Band.id, Band.name, table=Manager) ).run_sync() self.assertEqual( @@ -82,7 +81,9 @@ def test_select_multiple(self): ) def test_select_multiple_all_columns(self): - response = Manager.select(Manager.name, Manager.bands()).run_sync() + response = Manager.select( + Manager.name, Manager.bands(table=Manager) + ).run_sync() self.assertEqual( response, @@ -107,7 +108,7 @@ def test_select_multiple_all_columns(self): def test_select_id(self): response = Manager.select( - Manager.name, Manager.bands(Band.id, as_list=True) + Manager.name, Manager.bands(Band.id, as_list=True, table=Manager) ).run_sync() self.assertEqual( @@ -123,7 +124,8 @@ def test_select_multiple_as_list_error(self): with self.assertRaises(ValueError): Manager.select( - Manager.name, Manager.bands(Band.id, Band.name, as_list=True) + Manager.name, + Manager.bands(Band.id, Band.name, as_list=True, table=Manager), ).run_sync() @@ -137,7 +139,6 @@ class Customer(Table): name = Varchar() concerts = ReverseLookup( LazyTableReference("Concert", module_path=__name__), - reverse_lookup_name="customer", ) @@ -191,7 +192,8 @@ def test_select_custom_primary_key(self): ).run_sync() response = Customer.select( - Customer.name, Customer.concerts(Concert.name, as_list=True) + Customer.name, + Customer.concerts(Concert.name, as_list=True, table=Customer), ).run_sync() self.assertListEqual( @@ -204,7 +206,7 @@ def test_select_custom_primary_key(self): ) response = Customer.select( - Customer.name, Customer.concerts(Concert.name) + Customer.name, Customer.concerts(Concert.name, table=Customer) ).run_sync() self.assertEqual( From 2839931b27d873c640b2e43922db1f919243427a Mon Sep 17 00:00:00 2001 From: sinisaos Date: Sun, 28 Aug 2022 09:59:01 +0200 Subject: [PATCH 04/11] update docstring --- piccolo/columns/reverse_lookup.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py index b9b15a55f..a6f8d5ad2 100644 --- a/piccolo/columns/reverse_lookup.py +++ b/piccolo/columns/reverse_lookup.py @@ -37,6 +37,8 @@ def __init__( flattened list will be returned, rather than a list of objects. :param load_json: If ``True``, any JSON strings are loaded as Python objects. + :param table: + Parent table for reverse lookup. """ self.as_list = as_list @@ -205,6 +207,9 @@ def __call__( flattened list will be returned, rather than a list of objects. :param load_json: If ``True``, any JSON strings are loaded as Python objects. + :param table: + Parent table for reverse lookup. + """ if as_list and len(columns) != 1: From f98e0b360b4b8b54547bbe3d4e548320e31a1768 Mon Sep 17 00:00:00 2001 From: powellnorma <101364699+powellnorma@users.noreply.github.com> Date: Fri, 16 Dec 2022 21:22:10 +0100 Subject: [PATCH 05/11] reverse lookup: some fixes Make it work when the ForeignKey is named differently than the referenced Table. --- piccolo/columns/reverse_lookup.py | 100 +++++++++++++++--------------- piccolo/query/methods/select.py | 37 +++++------ piccolo/table.py | 6 +- 3 files changed, 74 insertions(+), 69 deletions(-) diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py index a6f8d5ad2..5038e500b 100644 --- a/piccolo/columns/reverse_lookup.py +++ b/piccolo/columns/reverse_lookup.py @@ -27,7 +27,6 @@ def __init__( reverse_lookup: ReverseLookup, as_list: bool = False, load_json: bool = False, - table: t.Type[Table], ): """ :param columns: @@ -45,7 +44,6 @@ def __init__( self.columns = columns self.reverse_lookup = reverse_lookup self.load_json = load_json - self.table = table safe_types = [int, str] @@ -57,39 +55,22 @@ def __init__( for column in columns ) - @property - def foreign_key_columns_index(self) -> int: - reverse_lookup_table = ( - self.reverse_lookup._meta.resolved_reverse_joining_table - ) - fk_columns = [ - i._meta.name - for i in reverse_lookup_table._meta.foreign_key_columns - ] - return fk_columns.index(self.table._meta.tablename) - def get_select_string(self, engine_type: str, with_alias=True) -> str: - reverse_lookup_table = ( - self.reverse_lookup._meta.resolved_reverse_joining_table - ) - reverse_lookup_table_name = reverse_lookup_table._meta.tablename - reverse_lookup_pk = reverse_lookup_table._meta.primary_key._meta.name - reverse_lookup_fk_table_name = ( - reverse_lookup_table._meta.foreign_key_columns[ - self.foreign_key_columns_index - ]._meta.name - ) - reverse_lookup_fk = reverse_lookup_table._meta.foreign_key_columns[ - self.foreign_key_columns_index - ]._meta.db_column_name + reverse_lookup_name = self.reverse_lookup._meta.name + + table1 = self.reverse_lookup._meta.table + table1_pk = table1._meta.primary_key._meta.name + table1_name = table1._meta.tablename + + table2 = self.reverse_lookup._meta.resolved_reverse_joining_table + table2_name = table2._meta.tablename + table2_pk = table2._meta.primary_key._meta.name + table2_fk = self.reverse_lookup._meta.reverse_fk reverse_select = f""" - "{reverse_lookup_table_name}" - JOIN "{reverse_lookup_fk_table_name}" "inner_{reverse_lookup_fk_table_name}" ON ( - "{reverse_lookup_table_name}"."{reverse_lookup_fk}" - = "inner_{reverse_lookup_fk_table_name}"."{reverse_lookup_pk}" - ) WHERE "{reverse_lookup_table_name}"."{reverse_lookup_fk}" - = "{reverse_lookup_fk_table_name}"."{reverse_lookup_pk}" + "{table2_name}" + WHERE "{table2_name}"."{table2_fk}" + = "{table1_name}"."{table1_pk}" """ # noqa: E501 if engine_type == "postgres": @@ -98,55 +79,55 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: return f""" ARRAY( SELECT - "{reverse_lookup_table_name}"."{column_name}" + "{table2_name}"."{column_name}" FROM {reverse_select} - ) AS "{reverse_lookup_table_name}s" + ) AS "{reverse_lookup_name}s" """ elif not self.serialisation_safe: - column_name = reverse_lookup_pk + column_name = table2_pk return f""" ARRAY( SELECT - "{reverse_lookup_table_name}"."{column_name}" + "{table2_name}"."{column_name}" FROM {reverse_select} - ) AS "{reverse_lookup_table_name}s" + ) AS "{reverse_lookup_name}s" """ else: if len(self.columns) > 0: column_names = ", ".join( - f'"{reverse_lookup_table_name}"."{column._meta.db_column_name}"' # noqa: E501 + f'"{table2_name}"."{column._meta.db_column_name}"' # noqa: E501 for column in self.columns ) else: column_names = ", ".join( - f'"{reverse_lookup_table_name}"."{column._meta.db_column_name}"' # noqa: E501 - for column in reverse_lookup_table._meta.columns + f'"{table2_name}"."{column._meta.db_column_name}"' # noqa: E501 + for column in table2._meta.columns ) return f""" ( - SELECT JSON_AGG("{reverse_lookup_table_name}s") + SELECT JSON_AGG("{table2_name}s") FROM ( SELECT {column_names} FROM {reverse_select} - ) AS "{reverse_lookup_table_name}s" - ) AS "{reverse_lookup_table_name}s" + ) AS "{table2_name}s" + ) AS "{reverse_lookup_name}s" """ elif engine_type == "sqlite": if len(self.columns) > 1 or not self.serialisation_safe: - column_name = reverse_lookup_pk + column_name = table2_pk else: try: column_name = self.columns[0]._meta.db_column_name except IndexError: - column_name = reverse_lookup_pk + column_name = table2_pk return f""" ( SELECT group_concat( - "{reverse_lookup_table_name}"."{column_name}" + "{table2_name}"."{column_name}" ) FROM {reverse_select} ) - AS "{reverse_lookup_table_name}s [M2M]" + AS "{table2_name}s [M2M]" """ else: raise ValueError(f"{engine_type} is an unrecognised engine type") @@ -155,6 +136,27 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: @dataclass class ReverseLookupMeta: reverse_joining_table: t.Union[t.Type[Table], LazyTableReference] + reverse_fk: str + + # Set by the Table Metaclass: + _name: t.Optional[str] = None + _table: t.Optional[t.Type[Table]] = None + + @property + def name(self) -> str: + if not self._name: + raise ValueError( + "`_name` isn't defined - the Table Metaclass should set it." + ) + return self._name + + @property + def table(self) -> t.Type[Table]: + if not self._table: + raise ValueError( + "`_table` isn't defined - the Table Metaclass should set it." + ) + return self._table @property def resolved_reverse_joining_table(self) -> t.Type[Table]: @@ -182,6 +184,7 @@ class ReverseLookup: def __init__( self, reverse_joining_table: t.Union[t.Type[Table], LazyTableReference], + reverse_fk: str, ): """ :param reverse_joining_table: @@ -189,6 +192,7 @@ def __init__( """ self._meta = ReverseLookupMeta( reverse_joining_table=reverse_joining_table, + reverse_fk=reverse_fk, ) def __call__( @@ -196,7 +200,6 @@ def __call__( *columns: Column, as_list: bool = False, load_json: bool = False, - table: t.Type[Table], ) -> ReverseLookupSelect: """ :param columns: @@ -222,5 +225,4 @@ def __call__( reverse_lookup=self, as_list=as_list, load_json=load_json, - table=table, ) diff --git a/piccolo/query/methods/select.py b/piccolo/query/methods/select.py index e41c59464..73bc4b708 100644 --- a/piccolo/query/methods/select.py +++ b/piccolo/query/methods/select.py @@ -437,10 +437,11 @@ async def response_handler(self, response): ) for reverse_lookup_select in reverse_lookup_selects: + reverse_lookup = reverse_lookup_select.reverse_lookup reverse_table = ( - reverse_lookup_select.reverse_lookup._meta.resolved_reverse_joining_table # noqa: E501 + reverse_lookup._meta.resolved_reverse_joining_table # noqa: E501 ) - reverse_lookup_table_name = reverse_table._meta.tablename + reverse_lookup_name = reverse_lookup._meta.name if self.engine_type == "sqlite": # With ReverseLookup queries in SQLite, we always get @@ -454,11 +455,11 @@ async def response_handler(self, response): ) try: for row in response: - data = row[f"{reverse_lookup_table_name}s"] - row[f"{reverse_lookup_table_name}s"] = ( + data = row[f"{reverse_lookup_name}s"] + row[f"{reverse_lookup_name}s"] = ( [ value_type(i) - for i in row[f"{reverse_lookup_table_name}s"] + for i in row[f"{reverse_lookup_name}s"] ] if data else [] @@ -466,7 +467,7 @@ async def response_handler(self, response): except ValueError: colored_warning( "Unable to do type conversion for the " - f"{reverse_lookup_table_name}s relation" + f"{reverse_lookup_name}s relation" ) # If the user requested a single column, we just return that @@ -481,7 +482,7 @@ async def response_handler(self, response): response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_table_name}s", + f"{reverse_lookup_name}s", reverse_lookup_select, as_list=True, ) @@ -494,11 +495,11 @@ async def response_handler(self, response): 0 ]._meta.name for row in response: - if row[f"{reverse_lookup_table_name}s"] is None: - row[f"{reverse_lookup_table_name}s"] = [] - row[f"{reverse_lookup_table_name}s"] = [ + if row[f"{reverse_lookup_name}s"] is None: + row[f"{reverse_lookup_name}s"] = [] + row[f"{reverse_lookup_name}s"] = [ {column_name: i} - for i in row[f"{reverse_lookup_table_name}s"] + for i in row[f"{reverse_lookup_name}s"] ] elif ( len(reverse_lookup_select.columns) == 0 @@ -509,7 +510,7 @@ async def response_handler(self, response): set( itertools.chain( *[ - row[f"{reverse_lookup_table_name}s"] + row[f"{reverse_lookup_name}s"] for row in response ] ) @@ -545,16 +546,16 @@ async def response_handler(self, response): for row in extra_rows } for row in response: - row[f"{reverse_lookup_table_name}s"] = [ + row[f"{reverse_lookup_name}s"] = [ extra_rows_map.get(i) - for i in row[f"{reverse_lookup_table_name}s"] + for i in row[f"{reverse_lookup_name}s"] ] else: response = await self._splice_m2m_rows( response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_table_name}s", + f"{reverse_lookup_name}s", reverse_lookup_select, as_list=False, ) @@ -577,8 +578,8 @@ async def response_handler(self, response): # are returned as a JSON string, so we need to deserialise # it. for row in response: - data = row[f"{reverse_lookup_table_name}s"] - row[f"{reverse_lookup_table_name}s"] = ( + data = row[f"{reverse_lookup_name}s"] + row[f"{reverse_lookup_name}s"] = ( load_json(data) if data else [] ) else: @@ -589,7 +590,7 @@ async def response_handler(self, response): response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_table_name}s", + f"{reverse_lookup_name}s", reverse_lookup_select, as_list=False, ) diff --git a/piccolo/table.py b/piccolo/table.py index a6cdc64c4..c5da36531 100644 --- a/piccolo/table.py +++ b/piccolo/table.py @@ -24,6 +24,7 @@ M2MGetRelated, M2MRemoveRelated, ) +from piccolo.columns.reverse_lookup import ReverseLookup from piccolo.columns.readable import Readable from piccolo.columns.reference import LAZY_COLUMN_REFERENCES from piccolo.engine import Engine, engine_finder @@ -281,10 +282,11 @@ def __init_subclass__( if column._meta.auto_update is not ...: auto_update_columns.append(column) - if isinstance(attribute, M2M): + if isinstance(attribute, (M2M, ReverseLookup)): attribute._meta._name = attribute_name attribute._meta._table = cls - m2m_relationships.append(attribute) + if isinstance(attribute, M2M): + m2m_relationships.append(attribute) if not primary_key: primary_key = cls._create_serial_primary_key() From 9a5ba3e2c725877942494ada060cfeb765e3c884 Mon Sep 17 00:00:00 2001 From: powellnorma <101364699+powellnorma@users.noreply.github.com> Date: Sun, 18 Dec 2022 14:36:08 +0100 Subject: [PATCH 06/11] reverse_lookup.py: update docstrings --- piccolo/columns/reverse_lookup.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py index 5038e500b..c90449317 100644 --- a/piccolo/columns/reverse_lookup.py +++ b/piccolo/columns/reverse_lookup.py @@ -36,8 +36,6 @@ def __init__( flattened list will be returned, rather than a list of objects. :param load_json: If ``True``, any JSON strings are loaded as Python objects. - :param table: - Parent table for reverse lookup. """ self.as_list = as_list @@ -189,6 +187,8 @@ def __init__( """ :param reverse_joining_table: A ``Table`` for reverse lookup. + :param reverse_fk: + The ForeignKey to be used for the reverse lookup. """ self._meta = ReverseLookupMeta( reverse_joining_table=reverse_joining_table, @@ -210,8 +210,6 @@ def __call__( flattened list will be returned, rather than a list of objects. :param load_json: If ``True``, any JSON strings are loaded as Python objects. - :param table: - Parent table for reverse lookup. """ From d6c64531246e20c00657e0dbd570268b548668ca Mon Sep 17 00:00:00 2001 From: powellnorma <101364699+powellnorma@users.noreply.github.com> Date: Mon, 19 Dec 2022 00:20:28 +0100 Subject: [PATCH 07/11] reverse lookup: remove excess 's' --- piccolo/columns/reverse_lookup.py | 6 +++--- piccolo/query/methods/select.py | 32 +++++++++++++++---------------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py index c90449317..4c13f4afe 100644 --- a/piccolo/columns/reverse_lookup.py +++ b/piccolo/columns/reverse_lookup.py @@ -79,7 +79,7 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: SELECT "{table2_name}"."{column_name}" FROM {reverse_select} - ) AS "{reverse_lookup_name}s" + ) AS "{reverse_lookup_name}" """ elif not self.serialisation_safe: column_name = table2_pk @@ -88,7 +88,7 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: SELECT "{table2_name}"."{column_name}" FROM {reverse_select} - ) AS "{reverse_lookup_name}s" + ) AS "{reverse_lookup_name}" """ else: if len(self.columns) > 0: @@ -107,7 +107,7 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: FROM ( SELECT {column_names} FROM {reverse_select} ) AS "{table2_name}s" - ) AS "{reverse_lookup_name}s" + ) AS "{reverse_lookup_name}" """ elif engine_type == "sqlite": if len(self.columns) > 1 or not self.serialisation_safe: diff --git a/piccolo/query/methods/select.py b/piccolo/query/methods/select.py index 73bc4b708..8eb5c8042 100644 --- a/piccolo/query/methods/select.py +++ b/piccolo/query/methods/select.py @@ -455,11 +455,11 @@ async def response_handler(self, response): ) try: for row in response: - data = row[f"{reverse_lookup_name}s"] - row[f"{reverse_lookup_name}s"] = ( + data = row[f"{reverse_lookup_name}"] + row[f"{reverse_lookup_name}"] = ( [ value_type(i) - for i in row[f"{reverse_lookup_name}s"] + for i in row[f"{reverse_lookup_name}"] ] if data else [] @@ -467,7 +467,7 @@ async def response_handler(self, response): except ValueError: colored_warning( "Unable to do type conversion for the " - f"{reverse_lookup_name}s relation" + f"{reverse_lookup_name} relation" ) # If the user requested a single column, we just return that @@ -482,7 +482,7 @@ async def response_handler(self, response): response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_name}s", + f"{reverse_lookup_name}", reverse_lookup_select, as_list=True, ) @@ -495,11 +495,11 @@ async def response_handler(self, response): 0 ]._meta.name for row in response: - if row[f"{reverse_lookup_name}s"] is None: - row[f"{reverse_lookup_name}s"] = [] - row[f"{reverse_lookup_name}s"] = [ + if row[f"{reverse_lookup_name}"] is None: + row[f"{reverse_lookup_name}"] = [] + row[f"{reverse_lookup_name}"] = [ {column_name: i} - for i in row[f"{reverse_lookup_name}s"] + for i in row[f"{reverse_lookup_name}"] ] elif ( len(reverse_lookup_select.columns) == 0 @@ -510,7 +510,7 @@ async def response_handler(self, response): set( itertools.chain( *[ - row[f"{reverse_lookup_name}s"] + row[f"{reverse_lookup_name}"] for row in response ] ) @@ -546,16 +546,16 @@ async def response_handler(self, response): for row in extra_rows } for row in response: - row[f"{reverse_lookup_name}s"] = [ + row[f"{reverse_lookup_name}"] = [ extra_rows_map.get(i) - for i in row[f"{reverse_lookup_name}s"] + for i in row[f"{reverse_lookup_name}"] ] else: response = await self._splice_m2m_rows( response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_name}s", + f"{reverse_lookup_name}", reverse_lookup_select, as_list=False, ) @@ -578,8 +578,8 @@ async def response_handler(self, response): # are returned as a JSON string, so we need to deserialise # it. for row in response: - data = row[f"{reverse_lookup_name}s"] - row[f"{reverse_lookup_name}s"] = ( + data = row[f"{reverse_lookup_name}"] + row[f"{reverse_lookup_name}"] = ( load_json(data) if data else [] ) else: @@ -590,7 +590,7 @@ async def response_handler(self, response): response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_name}s", + f"{reverse_lookup_name}", reverse_lookup_select, as_list=False, ) From b3a0e27caab819ec97d742e9089fd3786acde70e Mon Sep 17 00:00:00 2001 From: sinisaos Date: Mon, 19 Dec 2022 21:21:24 +0100 Subject: [PATCH 08/11] fix tests and linter errors --- piccolo/columns/reverse_lookup.py | 4 +- piccolo/table.py | 2 +- tests/columns/test_reverse_lookup.py | 87 +++++++++++++++++++++------- 3 files changed, 69 insertions(+), 24 deletions(-) diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py index 4c13f4afe..4d622a02d 100644 --- a/piccolo/columns/reverse_lookup.py +++ b/piccolo/columns/reverse_lookup.py @@ -69,9 +69,9 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: "{table2_name}" WHERE "{table2_name}"."{table2_fk}" = "{table1_name}"."{table1_pk}" - """ # noqa: E501 + """ - if engine_type == "postgres": + if engine_type in ("postgres", "cockroach"): if self.as_list: column_name = self.columns[0]._meta.db_column_name return f""" diff --git a/piccolo/table.py b/piccolo/table.py index c5da36531..3d2941f80 100644 --- a/piccolo/table.py +++ b/piccolo/table.py @@ -24,9 +24,9 @@ M2MGetRelated, M2MRemoveRelated, ) -from piccolo.columns.reverse_lookup import ReverseLookup from piccolo.columns.readable import Readable from piccolo.columns.reference import LAZY_COLUMN_REFERENCES +from piccolo.columns.reverse_lookup import ReverseLookup from piccolo.engine import Engine, engine_finder from piccolo.query import ( Alter, diff --git a/tests/columns/test_reverse_lookup.py b/tests/columns/test_reverse_lookup.py index 3c5f743c6..83bda120e 100644 --- a/tests/columns/test_reverse_lookup.py +++ b/tests/columns/test_reverse_lookup.py @@ -8,12 +8,17 @@ ) from piccolo.columns.reverse_lookup import ReverseLookup from piccolo.table import Table, create_db_tables_sync, drop_db_tables_sync +from tests.base import engine_is, engines_skip class Manager(Table): name = Varchar() bands = ReverseLookup( - LazyTableReference("Band", module_path=__name__), + LazyTableReference( + "Band", + module_path=__name__, + ), + reverse_fk="manager", ) @@ -29,24 +34,46 @@ class TestReverseLookup(TestCase): def setUp(self): create_db_tables_sync(*SIMPLE_SCHEMA, if_not_exists=True) - Manager.insert( - Manager(name="Guido"), - Manager(name="Mark"), - Manager(name="John"), - ).run_sync() + if engine_is("cockroach"): + managers = ( + Manager.insert( + Manager(name="Guido"), + Manager(name="Mark"), + Manager(name="John"), + ) + .returning(Manager.id) + .run_sync() + ) + + Band.insert( + Band(name="Pythonistas", manager=managers[0]["id"]), + Band(name="Rustaceans", manager=managers[0]["id"]), + Band(name="C-Sharps", manager=managers[1]["id"]), + ).returning(Band.id).run_sync() + + else: + Manager.insert( + Manager(name="Guido"), + Manager(name="Mark"), + Manager(name="John"), + ).run_sync() - Band.insert( - Band(name="Pythonistas", manager=1), - Band(name="Rustaceans", manager=1), - Band(name="C-Sharps", manager=2), - ).run_sync() + Band.insert( + Band(name="Pythonistas", manager=1), + Band(name="Rustaceans", manager=1), + Band(name="C-Sharps", manager=2), + ).run_sync() def tearDown(self): drop_db_tables_sync(*SIMPLE_SCHEMA) + @engines_skip("cockroach") def test_select_name(self): + """ + 🐛 Cockroach bug: https://github.com/cockroachdb/cockroach/issues/71908 "could not decorrelate subquery" error under asyncpg + """ # noqa: E501 response = Manager.select( - Manager.name, Manager.bands(Band.name, as_list=True, table=Manager) + Manager.name, Manager.bands(Band.name, as_list=True) ).run_sync() self.assertEqual( response, @@ -57,9 +84,13 @@ def test_select_name(self): ], ) + @engines_skip("cockroach") def test_select_multiple(self): + """ + 🐛 Cockroach bug: https://github.com/cockroachdb/cockroach/issues/71908 "could not decorrelate subquery" error under asyncpg + """ # noqa: E501 response = Manager.select( - Manager.name, Manager.bands(Band.id, Band.name, table=Manager) + Manager.name, Manager.bands(Band.id, Band.name) ).run_sync() self.assertEqual( @@ -80,10 +111,12 @@ def test_select_multiple(self): ], ) + @engines_skip("cockroach") def test_select_multiple_all_columns(self): - response = Manager.select( - Manager.name, Manager.bands(table=Manager) - ).run_sync() + """ + 🐛 Cockroach bug: https://github.com/cockroachdb/cockroach/issues/71908 "could not decorrelate subquery" error under asyncpg + """ # noqa: E501 + response = Manager.select(Manager.name, Manager.bands()).run_sync() self.assertEqual( response, @@ -106,9 +139,13 @@ def test_select_multiple_all_columns(self): ], ) + @engines_skip("cockroach") def test_select_id(self): + """ + 🐛 Cockroach bug: https://github.com/cockroachdb/cockroach/issues/71908 "could not decorrelate subquery" error under asyncpg + """ # noqa: E501 response = Manager.select( - Manager.name, Manager.bands(Band.id, as_list=True, table=Manager) + Manager.name, Manager.bands(Band.id, as_list=True) ).run_sync() self.assertEqual( @@ -125,7 +162,7 @@ def test_select_multiple_as_list_error(self): with self.assertRaises(ValueError): Manager.select( Manager.name, - Manager.bands(Band.id, Band.name, as_list=True, table=Manager), + Manager.bands(Band.id, Band.name, as_list=True), ).run_sync() @@ -138,7 +175,11 @@ class Customer(Table): uuid = UUID(primary_key=True) name = Varchar() concerts = ReverseLookup( - LazyTableReference("Concert", module_path=__name__), + LazyTableReference( + "Concert", + module_path=__name__, + ), + reverse_fk="customer", ) @@ -163,7 +204,11 @@ def setUp(self): def tearDown(self): drop_db_tables_sync(*CUSTOM_PK_SCHEMA) + @engines_skip("cockroach") def test_select_custom_primary_key(self): + """ + 🐛 Cockroach bug: https://github.com/cockroachdb/cockroach/issues/71908 "could not decorrelate subquery" error under asyncpg + """ # noqa: E501 Customer.objects().create(name="Bob").run_sync() Customer.objects().create(name="Sally").run_sync() Customer.objects().create(name="Fred").run_sync() @@ -193,7 +238,7 @@ def test_select_custom_primary_key(self): response = Customer.select( Customer.name, - Customer.concerts(Concert.name, as_list=True, table=Customer), + Customer.concerts(Concert.name, as_list=True), ).run_sync() self.assertListEqual( @@ -206,7 +251,7 @@ def test_select_custom_primary_key(self): ) response = Customer.select( - Customer.name, Customer.concerts(Concert.name, table=Customer) + Customer.name, Customer.concerts(Concert.name) ).run_sync() self.assertEqual( From b42daf7d2d07fb617a96589c349754842d3e9e4f Mon Sep 17 00:00:00 2001 From: sinisaos Date: Mon, 9 Dec 2024 20:54:14 +0100 Subject: [PATCH 09/11] update the branch code --- piccolo/columns/reverse_lookup.py | 24 +++++++---- piccolo/query/methods/select.py | 41 +++++++++---------- piccolo/table.py | 9 ++-- .../apps/asgi/commands/files/dummy_server.py | 4 +- 4 files changed, 43 insertions(+), 35 deletions(-) diff --git a/piccolo/columns/reverse_lookup.py b/piccolo/columns/reverse_lookup.py index 4d622a02d..4feea4766 100644 --- a/piccolo/columns/reverse_lookup.py +++ b/piccolo/columns/reverse_lookup.py @@ -4,7 +4,7 @@ import typing as t from dataclasses import dataclass -from piccolo.columns.base import Selectable +from piccolo.columns.base import QueryString, Selectable from piccolo.columns.column_types import ( JSON, JSONB, @@ -53,7 +53,9 @@ def __init__( for column in columns ) - def get_select_string(self, engine_type: str, with_alias=True) -> str: + def get_select_string( + self, engine_type: str, with_alias=True + ) -> QueryString: reverse_lookup_name = self.reverse_lookup._meta.name table1 = self.reverse_lookup._meta.table @@ -74,22 +76,26 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: if engine_type in ("postgres", "cockroach"): if self.as_list: column_name = self.columns[0]._meta.db_column_name - return f""" + return QueryString( + f""" ARRAY( SELECT "{table2_name}"."{column_name}" FROM {reverse_select} ) AS "{reverse_lookup_name}" """ + ) elif not self.serialisation_safe: column_name = table2_pk - return f""" + return QueryString( + f""" ARRAY( SELECT "{table2_name}"."{column_name}" FROM {reverse_select} ) AS "{reverse_lookup_name}" """ + ) else: if len(self.columns) > 0: column_names = ", ".join( @@ -101,7 +107,8 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: f'"{table2_name}"."{column._meta.db_column_name}"' # noqa: E501 for column in table2._meta.columns ) - return f""" + return QueryString( + f""" ( SELECT JSON_AGG("{table2_name}s") FROM ( @@ -109,6 +116,7 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: ) AS "{table2_name}s" ) AS "{reverse_lookup_name}" """ + ) elif engine_type == "sqlite": if len(self.columns) > 1 or not self.serialisation_safe: column_name = table2_pk @@ -118,15 +126,17 @@ def get_select_string(self, engine_type: str, with_alias=True) -> str: except IndexError: column_name = table2_pk - return f""" + return QueryString( + f""" ( SELECT group_concat( "{table2_name}"."{column_name}" ) FROM {reverse_select} ) - AS "{table2_name}s [M2M]" + AS "{reverse_lookup_name} [M2M]" """ + ) else: raise ValueError(f"{engine_type} is an unrecognised engine type") diff --git a/piccolo/query/methods/select.py b/piccolo/query/methods/select.py index 4d77b3bf1..995bf4530 100644 --- a/piccolo/query/methods/select.py +++ b/piccolo/query/methods/select.py @@ -250,7 +250,7 @@ async def _splice_m2m_rows( secondary_table: t.Type[Table], secondary_table_pk: Column, m2m_name: str, - m2m_select: M2MSelect, + m2m_select: t.Union[M2MSelect, ReverseLookupSelect], as_list: bool = False, ): row_ids = list( @@ -287,7 +287,7 @@ async def _splice_m2m_rows( row[m2m_name] = [extra_rows_map.get(i) for i in row[m2m_name]] return response - async def response_handler(self, response): + async def response_handler(self, response: t.List[t.Dict[str, t.Any]]): m2m_selects = [ i for i in self.columns_delegate.selected_columns @@ -409,12 +409,9 @@ async def response_handler(self, response): ) try: for row in response: - data = row[f"{reverse_lookup_name}"] - row[f"{reverse_lookup_name}"] = ( - [ - value_type(i) - for i in row[f"{reverse_lookup_name}"] - ] + data = row[reverse_lookup_name] + row[reverse_lookup_name] = ( + [value_type(i) for i in row[reverse_lookup_name]] if data else [] ) @@ -436,7 +433,7 @@ async def response_handler(self, response): response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_name}", + reverse_lookup_name, reverse_lookup_select, as_list=True, ) @@ -449,11 +446,11 @@ async def response_handler(self, response): 0 ]._meta.name for row in response: - if row[f"{reverse_lookup_name}"] is None: - row[f"{reverse_lookup_name}"] = [] - row[f"{reverse_lookup_name}"] = [ + if row[reverse_lookup_name] is None: + row[reverse_lookup_name] = [] + row[reverse_lookup_name] = [ {column_name: i} - for i in row[f"{reverse_lookup_name}"] + for i in row[reverse_lookup_name] ] elif ( len(reverse_lookup_select.columns) == 0 @@ -464,7 +461,7 @@ async def response_handler(self, response): set( itertools.chain( *[ - row[f"{reverse_lookup_name}"] + row[reverse_lookup_name] for row in response ] ) @@ -500,16 +497,16 @@ async def response_handler(self, response): for row in extra_rows } for row in response: - row[f"{reverse_lookup_name}"] = [ + row[reverse_lookup_name] = [ extra_rows_map.get(i) - for i in row[f"{reverse_lookup_name}"] + for i in row[reverse_lookup_name] ] else: response = await self._splice_m2m_rows( response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_name}", + reverse_lookup_name, reverse_lookup_select, as_list=False, ) @@ -522,8 +519,8 @@ async def response_handler(self, response): and reverse_lookup_select.load_json ): for row in response: - data = row[reverse_lookup_select.columns[0]] - row[reverse_lookup_select.columns[0]] = [ + data = row[str(reverse_lookup_select.columns[0])] + row[str(reverse_lookup_select.columns[0])] = [ load_json(i) for i in data ] @@ -532,8 +529,8 @@ async def response_handler(self, response): # are returned as a JSON string, so we need to deserialise # it. for row in response: - data = row[f"{reverse_lookup_name}"] - row[f"{reverse_lookup_name}"] = ( + data = row[reverse_lookup_name] + row[reverse_lookup_name] = ( load_json(data) if data else [] ) else: @@ -544,7 +541,7 @@ async def response_handler(self, response): response, reverse_table, reverse_table._meta.primary_key, - f"{reverse_lookup_name}", + reverse_lookup_name, reverse_lookup_select, as_list=False, ) diff --git a/piccolo/table.py b/piccolo/table.py index 120e1cbf1..bb4683803 100644 --- a/piccolo/table.py +++ b/piccolo/table.py @@ -89,7 +89,9 @@ class TableMeta: tags: t.List[str] = field(default_factory=list) help_text: t.Optional[str] = None _db: t.Optional[Engine] = None - m2m_relationships: t.List[M2M] = field(default_factory=list) + m2m_relationships: t.List[t.Union[M2M, ReverseLookup]] = field( + default_factory=list + ) schema: t.Optional[str] = None # Records reverse foreign key relationships - i.e. when the current table @@ -279,7 +281,7 @@ def __init_subclass__( email_columns: t.List[Email] = [] auto_update_columns: t.List[Column] = [] primary_key: t.Optional[Column] = None - m2m_relationships: t.List[M2M] = [] + m2m_relationships: t.List[t.Union[M2M, ReverseLookup]] = [] attribute_names = itertools.chain( *[i.__dict__.keys() for i in reversed(cls.__mro__)] @@ -330,8 +332,7 @@ def __init_subclass__( if isinstance(attribute, (M2M, ReverseLookup)): attribute._meta._name = attribute_name attribute._meta._table = cls - if isinstance(attribute, M2M): - m2m_relationships.append(attribute) + m2m_relationships.append(attribute) if not primary_key: primary_key = cls._create_serial_primary_key() diff --git a/tests/apps/asgi/commands/files/dummy_server.py b/tests/apps/asgi/commands/files/dummy_server.py index 9b83470a3..a4807aa66 100644 --- a/tests/apps/asgi/commands/files/dummy_server.py +++ b/tests/apps/asgi/commands/files/dummy_server.py @@ -3,7 +3,7 @@ import sys import typing as t -from httpx import AsyncClient +from httpx import ASGITransport, AsyncClient async def dummy_server(app: t.Union[str, t.Callable] = "app:app"): @@ -24,7 +24,7 @@ async def dummy_server(app: t.Union[str, t.Callable] = "app:app"): module = importlib.import_module(path) app = t.cast(t.Callable, getattr(module, app_name)) - async with AsyncClient(app=app) as client: + async with AsyncClient(transport=ASGITransport(app=app)) as client: response = await client.get("http://localhost:8000") if response.status_code != 200: sys.exit("The app isn't callable!") From bacb08d953c77e2c02a6404f6bd0a6e0a6905692 Mon Sep 17 00:00:00 2001 From: sinisaos Date: Tue, 10 Dec 2024 11:37:47 +0100 Subject: [PATCH 10/11] add docs --- docs/src/piccolo/schema/index.rst | 1 + docs/src/piccolo/schema/reverse_lookup.rst | 105 +++++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 docs/src/piccolo/schema/reverse_lookup.rst diff --git a/docs/src/piccolo/schema/index.rst b/docs/src/piccolo/schema/index.rst index ec9b887e6..bdda8bc4c 100644 --- a/docs/src/piccolo/schema/index.rst +++ b/docs/src/piccolo/schema/index.rst @@ -9,5 +9,6 @@ The schema is how you define your database tables, columns and relationships. ./defining ./column_types ./m2m + ./reverse_lookup ./one_to_one ./advanced diff --git a/docs/src/piccolo/schema/reverse_lookup.rst b/docs/src/piccolo/schema/reverse_lookup.rst new file mode 100644 index 000000000..eb85366a7 --- /dev/null +++ b/docs/src/piccolo/schema/reverse_lookup.rst @@ -0,0 +1,105 @@ +.. currentmodule:: piccolo.columns.reverse_lookup + +############## +Reverse Lookup +############## + +For example, we might have our ``Manager`` table, and we want to +get all the bands associated with the same manager. +For this we can use reverse foreign key lookup. + +We create it in Piccolo like this: + +.. code-block:: python + + from piccolo.columns.column_types import ( + ForeignKey, + LazyTableReference, + Varchar + ) + from piccolo.columns.reverse_lookup import ReverseLookup + from piccolo.table import Table + + + class Manager(Table): + name = Varchar() + bands = ReverseLookup( + LazyTableReference( + "Band", + module_path=__name__, + ), + reverse_fk="manager", + ) + + + class Band(Table): + name = Varchar() + manager = ForeignKey(Manager) + +------------------------------------------------------------------------------- + +Select queries +============== + +If we want to select each manager, along with a list of associated band names, +we can do this: + +.. code-block:: python + + >>> await Manager.select(Manager.name, Manager.bands(Band.name, as_list=True)) + [ + {'name': 'John', 'bands': ['C-Sharps']}, + {'name': 'Guido', 'bands': ['Pythonistas', 'Rustaceans']}, + ] + +You can request whichever column you like from the reverse lookup: + +.. code-block:: python + + >>> await Manager.select(Manager.name, Manager.bands(Band.id, as_list=True)) + [ + {'name': 'John', 'bands': [3]}, + {'name': 'Guido', 'bands': [1, 2]}, + ] + +You can also request multiple columns from the reverse lookup: + +.. code-block:: python + + >>> await Manager.select(Manager.name, Manager.bands(Band.id, Band.name)) + [ + { + 'name': 'John', + 'bands': [ + {'id': 3, 'name': 'C-Sharps'}, + ] + }, + { + 'name': 'Guido', + 'bands': [ + {'id': 1, 'name': 'Pythonistas'}, + {'id': 2, 'name': 'Rustaceans'}, + ] + } + ] + +If you omit the columns argument, then all of the columns are returned. + +.. code-block:: python + + >>> await Manager.select(Manager.name, Manager.bands()) + [ + { + 'name': 'John', + 'bands': [ + {'id': 3, 'name': 'C-Sharps'}, + ] + }, + { + 'name': 'Guido', + 'bands': [ + {'id': 1, 'name': 'Pythonistas'}, + {'id': 2, 'name': 'Rustaceans'}, + ] + } + ] \ No newline at end of file From 70b183afc7892381cbc8dc4302ee565e86b6e52d Mon Sep 17 00:00:00 2001 From: sinisaos Date: Wed, 11 Dec 2024 07:08:07 +0100 Subject: [PATCH 11/11] fix indentation in docs --- docs/src/piccolo/schema/reverse_lookup.rst | 20 +++++++-------- piccolo/query/methods/select.py | 30 ++++++++++++---------- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/docs/src/piccolo/schema/reverse_lookup.rst b/docs/src/piccolo/schema/reverse_lookup.rst index eb85366a7..6f71d5ee9 100644 --- a/docs/src/piccolo/schema/reverse_lookup.rst +++ b/docs/src/piccolo/schema/reverse_lookup.rst @@ -22,19 +22,19 @@ We create it in Piccolo like this: class Manager(Table): - name = Varchar() - bands = ReverseLookup( - LazyTableReference( - "Band", - module_path=__name__, - ), - reverse_fk="manager", - ) + name = Varchar() + bands = ReverseLookup( + LazyTableReference( + "Band", + module_path=__name__, + ), + reverse_fk="manager", + ) class Band(Table): - name = Varchar() - manager = ForeignKey(Manager) + name = Varchar() + manager = ForeignKey(Manager) ------------------------------------------------------------------------------- diff --git a/piccolo/query/methods/select.py b/piccolo/query/methods/select.py index 995bf4530..5acbb1c5f 100644 --- a/piccolo/query/methods/select.py +++ b/piccolo/query/methods/select.py @@ -244,33 +244,33 @@ def lock_rows( ) return self - async def _splice_m2m_rows( + async def _splice_related_rows( self, response: t.List[t.Dict[str, t.Any]], secondary_table: t.Type[Table], secondary_table_pk: Column, - m2m_name: str, - m2m_select: t.Union[M2MSelect, ReverseLookupSelect], + related_name: str, + related_select: t.Union[M2MSelect, ReverseLookupSelect], as_list: bool = False, ): row_ids = list( - set(itertools.chain(*[row[m2m_name] for row in response])) + set(itertools.chain(*[row[related_name] for row in response])) ) extra_rows = ( ( await secondary_table.select( - *m2m_select.columns, + *related_select.columns, secondary_table_pk.as_alias("mapping_key"), ) .where(secondary_table_pk.is_in(row_ids)) - .output(load_json=m2m_select.load_json) + .output(load_json=related_select.load_json) .run() ) if row_ids else [] ) if as_list: - column_name = m2m_select.columns[0]._meta.name + column_name = related_select.columns[0]._meta.name extra_rows_map = { row["mapping_key"]: row[column_name] for row in extra_rows } @@ -284,7 +284,9 @@ async def _splice_m2m_rows( for row in extra_rows } for row in response: - row[m2m_name] = [extra_rows_map.get(i) for i in row[m2m_name]] + row[related_name] = [ + extra_rows_map.get(i) for i in row[related_name] + ] return response async def response_handler(self, response: t.List[t.Dict[str, t.Any]]): @@ -333,7 +335,7 @@ async def response_handler(self, response: t.List[t.Dict[str, t.Any]]): if m2m_select.serialisation_safe: pass else: - response = await self._splice_m2m_rows( + response = await self._splice_related_rows( response, secondary_table, secondary_table_pk, @@ -352,7 +354,7 @@ async def response_handler(self, response: t.List[t.Dict[str, t.Any]]): {column_name: i} for i in row[m2m_name] ] else: - response = await self._splice_m2m_rows( + response = await self._splice_related_rows( response, secondary_table, secondary_table_pk, @@ -382,7 +384,7 @@ async def response_handler(self, response: t.List[t.Dict[str, t.Any]]): # If the data can't be safely serialised as JSON, we get # back an array of primary key values, and need to # splice in the correct values using Python. - response = await self._splice_m2m_rows( + response = await self._splice_related_rows( response, secondary_table, secondary_table_pk, @@ -429,7 +431,7 @@ async def response_handler(self, response: t.List[t.Dict[str, t.Any]]): if reverse_lookup_select.serialisation_safe: pass else: - response = await self._splice_m2m_rows( + response = await self._splice_related_rows( response, reverse_table, reverse_table._meta.primary_key, @@ -502,7 +504,7 @@ async def response_handler(self, response: t.List[t.Dict[str, t.Any]]): for i in row[reverse_lookup_name] ] else: - response = await self._splice_m2m_rows( + response = await self._splice_related_rows( response, reverse_table, reverse_table._meta.primary_key, @@ -537,7 +539,7 @@ async def response_handler(self, response: t.List[t.Dict[str, t.Any]]): # If the data can't be safely serialised as JSON, we get # back an array of primary key values, and need to # splice in the correct values using Python. - response = await self._splice_m2m_rows( + response = await self._splice_related_rows( response, reverse_table, reverse_table._meta.primary_key,