From 6df9c07ccd856426ed686f4b7990c82cfd671062 Mon Sep 17 00:00:00 2001 From: sinisaos Date: Fri, 21 Jan 2022 18:22:45 +0100 Subject: [PATCH 1/8] changes to support non-primary key in Piccolo Admin --- piccolo_api/crud/endpoints.py | 31 +++++++++++++++++-- tests/crud/test_crud_endpoints.py | 9 +++--- tests/crud/test_custom_pk.py | 7 +---- tests/crud/test_target_column_pk.py | 41 +++++++++++++++++++++++++ tests/fastapi/test_fastapi_endpoints.py | 2 +- 5 files changed, 75 insertions(+), 15 deletions(-) create mode 100644 tests/crud/test_target_column_pk.py diff --git a/piccolo_api/crud/endpoints.py b/piccolo_api/crud/endpoints.py index 2ae68b8c..194f6d1a 100644 --- a/piccolo_api/crud/endpoints.py +++ b/piccolo_api/crud/endpoints.py @@ -423,14 +423,32 @@ async def get_ids(self, request: Request) -> Response: values = await query.run() + # if value_type not in or UUID + try: + target_pk_type = [ + i._meta.params["target_column"].value_type not in (int, uuid) + for i in self.table._meta._foreign_key_references + ][0] + except (AttributeError, IndexError): + target_pk_type = False + primary_key = self.table._meta.primary_key if primary_key.value_type not in (int, str): return JSONResponse( - {str(i[primary_key._meta.name]): i["readable"] for i in values} + { + str(i[primary_key._meta.name]): [ + i["readable"], + target_pk_type, + ] + for i in values + } ) else: return JSONResponse( - {i[primary_key._meta.name]: i["readable"] for i in values} + { + i[primary_key._meta.name]: [i["readable"], target_pk_type] + for i in values + } ) ########################################################################### @@ -871,7 +889,14 @@ async def detail(self, request: Request) -> Response: try: row_id = self.table._meta.primary_key.value_type(row_id) except ValueError: - return Response("The ID is invalid", status_code=400) + for i in self.table._meta._foreign_key_references: + reference_target_pk = ( + await self.table.select(self.table._meta.primary_key) + .where(i._meta.params["target_column"] == row_id) + .first() + .run() + ) + row_id = reference_target_pk[self.table._meta.primary_key] if ( not await self.table.exists() diff --git a/tests/crud/test_crud_endpoints.py b/tests/crud/test_crud_endpoints.py index 3a6629b1..84154462 100644 --- a/tests/crud/test_crud_endpoints.py +++ b/tests/crud/test_crud_endpoints.py @@ -168,8 +168,7 @@ def test_get_ids(self): self.assertTrue(response.status_code == 200) # Make sure the content is correct: - response_json = response.json() - self.assertEqual(response_json[str(movie.id)], "Star Wars") + self.assertEqual(response.json(), {"1": ["Star Wars", False]}) def test_get_ids_with_search(self): """ @@ -189,7 +188,7 @@ def test_get_ids_with_search(self): # Make sure the content is correct: response_json = response.json() self.assertEqual(len(response_json), 1) - self.assertTrue("Star Wars" in response_json.values()) + self.assertEqual(response_json, {"1": ["Star Wars", False]}) def test_get_ids_with_limit_offset(self): """ @@ -204,7 +203,7 @@ def test_get_ids_with_limit_offset(self): response = client.get("/ids/?limit=1") self.assertTrue(response.status_code == 200) - self.assertEqual(response.json(), {"1": "Star Wars"}) + self.assertEqual(response.json(), {"1": ["Star Wars", False]}) # Make sure only valid limit values are accepted. response = client.get("/ids/?limit=abc") @@ -217,7 +216,7 @@ def test_get_ids_with_limit_offset(self): # Test with offset response = client.get("/ids/?limit=1&offset=1") self.assertTrue(response.status_code == 200) - self.assertEqual(response.json(), {"2": "Lord of the Rings"}) + self.assertEqual(response.json(), {"2": ["Lord of the Rings", False]}) class TestCount(TestCase): diff --git a/tests/crud/test_custom_pk.py b/tests/crud/test_custom_pk.py index be7e67c9..d4de6178 100644 --- a/tests/crud/test_custom_pk.py +++ b/tests/crud/test_custom_pk.py @@ -30,7 +30,7 @@ def test_get_ids(self): response = self.client.get("/ids/") self.assertEqual(response.status_code, 200) self.assertEqual( - response.json(), {str(self.movie.id): str(self.movie.id)} + response.json(), {str(self.movie.id): [str(self.movie.id), False]} ) def test_get_list(self): @@ -85,8 +85,3 @@ def test_patch(self): self.assertEqual( movie, {"id": self.movie.id, "name": "Star Wars", "rating": 2000} ) - - def test_invalid_id(self): - response = self.client.get("/abc123/") - self.assertEqual(response.status_code, 400) - self.assertEqual(response.content, b"The ID is invalid") diff --git a/tests/crud/test_target_column_pk.py b/tests/crud/test_target_column_pk.py new file mode 100644 index 00000000..cc8fbcea --- /dev/null +++ b/tests/crud/test_target_column_pk.py @@ -0,0 +1,41 @@ +from unittest import TestCase + +from piccolo.columns.column_types import ForeignKey, Varchar +from piccolo.table import Table +from starlette.testclient import TestClient + +from piccolo_api.crud.endpoints import PiccoloCRUD + + +class Serie(Table): + name = Varchar(length=100, unique=True) + + +class Review(Table): + reviewer = Varchar() + serie = ForeignKey(Serie, target_column=Serie.name) + + +class TestTargetPK(TestCase): + """ + Make sure PiccoloCRUD works with Tables with a custom primary key column. + """ + + def setUp(self): + Serie.create_table(if_not_exists=True).run_sync() + Review.create_table(if_not_exists=True).run_sync() + + def tearDown(self): + Review.alter().drop_table().run_sync() + Serie.alter().drop_table().run_sync() + + def test_target_column_pk(self): + serie = Serie(name="Devs") + serie.save().run_sync() + Review(reviewer="John Doe", serie=serie["name"]).save().run_sync() + review = Review.select(Review.serie.id).first().run_sync() + + self.client = TestClient(PiccoloCRUD(table=Serie, read_only=False)) + response = self.client.get("/Devs/") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json()["id"], review["serie.id"]) diff --git a/tests/fastapi/test_fastapi_endpoints.py b/tests/fastapi/test_fastapi_endpoints.py index 1284a163..e421b24c 100644 --- a/tests/fastapi/test_fastapi_endpoints.py +++ b/tests/fastapi/test_fastapi_endpoints.py @@ -175,7 +175,7 @@ def test_get_ids(self): client = TestClient(app) response = client.get("/movies/ids/") self.assertEqual(response.status_code, 200) - self.assertEqual(response.json(), {"1": "Star Wars"}) + self.assertEqual(response.json(), {"1": ["Star Wars", False]}) def test_new(self): client = TestClient(app) From d899cd1330bcaef0e10cfdb8544391fb830810ea Mon Sep 17 00:00:00 2001 From: sinisaos Date: Sat, 22 Jan 2022 17:37:18 +0100 Subject: [PATCH 2/8] fix for multiple reference non primary keys per table --- piccolo_api/crud/endpoints.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/piccolo_api/crud/endpoints.py b/piccolo_api/crud/endpoints.py index 194f6d1a..47e0aea6 100644 --- a/piccolo_api/crud/endpoints.py +++ b/piccolo_api/crud/endpoints.py @@ -890,13 +890,15 @@ async def detail(self, request: Request) -> Response: row_id = self.table._meta.primary_key.value_type(row_id) except ValueError: for i in self.table._meta._foreign_key_references: - reference_target_pk = ( - await self.table.select(self.table._meta.primary_key) - .where(i._meta.params["target_column"] == row_id) - .first() - .run() - ) - row_id = reference_target_pk[self.table._meta.primary_key] + target = i._meta.params["target_column"] + if target is not None: + reference_target_pk = ( + await self.table.select(self.table._meta.primary_key) + .where(target == row_id) + .first() + .run() + ) + row_id = reference_target_pk[self.table._meta.primary_key] if ( not await self.table.exists() From 1d9b3b24dabbce164e4d02153b2404dfae70680e Mon Sep 17 00:00:00 2001 From: sinisaos Date: Fri, 28 Jan 2022 14:33:56 +0100 Subject: [PATCH 3/8] fix filter search --- piccolo_api/crud/endpoints.py | 54 +++++++++++++++---------- tests/crud/test_crud_endpoints.py | 8 ++-- tests/crud/test_custom_pk.py | 2 +- tests/fastapi/test_fastapi_endpoints.py | 2 +- 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/piccolo_api/crud/endpoints.py b/piccolo_api/crud/endpoints.py index 47e0aea6..34cdd4de 100644 --- a/piccolo_api/crud/endpoints.py +++ b/piccolo_api/crud/endpoints.py @@ -423,32 +423,14 @@ async def get_ids(self, request: Request) -> Response: values = await query.run() - # if value_type not in or UUID - try: - target_pk_type = [ - i._meta.params["target_column"].value_type not in (int, uuid) - for i in self.table._meta._foreign_key_references - ][0] - except (AttributeError, IndexError): - target_pk_type = False - primary_key = self.table._meta.primary_key if primary_key.value_type not in (int, str): return JSONResponse( - { - str(i[primary_key._meta.name]): [ - i["readable"], - target_pk_type, - ] - for i in values - } + {str(i[primary_key._meta.name]): i["readable"] for i in values} ) else: return JSONResponse( - { - i[primary_key._meta.name]: [i["readable"], target_pk_type] - for i in values - } + {i[primary_key._meta.name]: i["readable"] for i in values} ) ########################################################################### @@ -649,11 +631,41 @@ def _apply_filters( Works on any queries which support `where` clauses - Select, Count, Objects etc. """ + target_column_name = [ + i._meta.name + for i in self.table._meta.foreign_key_columns + if i._meta.params.get("target_column") is not None + ] + fields = params.fields if fields: model_dict = self.pydantic_model_optional(**fields).dict() for field_name in fields.keys(): - value = model_dict.get(field_name, ...) + if field_name in target_column_name: + for i in self.table._meta.foreign_key_columns: + target_column_fk_name: t.Any = [ + c._meta.params.get("target_column") + for c in i._foreign_key_meta.resolved_references._meta._foreign_key_references # noqa: E501 + if c._meta.params.get("target_column") is not None + ] + reference_table = ( + i._foreign_key_meta.resolved_references + ) + target_column_fk = ( + reference_table.select() + .where( + reference_table._meta.primary_key + == fields[field_name] + ) + .first() + .run_sync() + ) + value = target_column_fk[ + target_column_fk_name[0]._meta.name + ] + else: + value = model_dict.get(field_name, ...) + if value is ...: raise MalformedQuery( f"{field_name} isn't a valid field name." diff --git a/tests/crud/test_crud_endpoints.py b/tests/crud/test_crud_endpoints.py index 84154462..aa2d72f3 100644 --- a/tests/crud/test_crud_endpoints.py +++ b/tests/crud/test_crud_endpoints.py @@ -168,7 +168,7 @@ def test_get_ids(self): self.assertTrue(response.status_code == 200) # Make sure the content is correct: - self.assertEqual(response.json(), {"1": ["Star Wars", False]}) + self.assertEqual(response.json(), {"1": "Star Wars"}) def test_get_ids_with_search(self): """ @@ -188,7 +188,7 @@ def test_get_ids_with_search(self): # Make sure the content is correct: response_json = response.json() self.assertEqual(len(response_json), 1) - self.assertEqual(response_json, {"1": ["Star Wars", False]}) + self.assertEqual(response_json, {"1": "Star Wars"}) def test_get_ids_with_limit_offset(self): """ @@ -203,7 +203,7 @@ def test_get_ids_with_limit_offset(self): response = client.get("/ids/?limit=1") self.assertTrue(response.status_code == 200) - self.assertEqual(response.json(), {"1": ["Star Wars", False]}) + self.assertEqual(response.json(), {"1": "Star Wars"}) # Make sure only valid limit values are accepted. response = client.get("/ids/?limit=abc") @@ -216,7 +216,7 @@ def test_get_ids_with_limit_offset(self): # Test with offset response = client.get("/ids/?limit=1&offset=1") self.assertTrue(response.status_code == 200) - self.assertEqual(response.json(), {"2": ["Lord of the Rings", False]}) + self.assertEqual(response.json(), {"2": "Lord of the Rings"}) class TestCount(TestCase): diff --git a/tests/crud/test_custom_pk.py b/tests/crud/test_custom_pk.py index d4de6178..aaf8cc3f 100644 --- a/tests/crud/test_custom_pk.py +++ b/tests/crud/test_custom_pk.py @@ -30,7 +30,7 @@ def test_get_ids(self): response = self.client.get("/ids/") self.assertEqual(response.status_code, 200) self.assertEqual( - response.json(), {str(self.movie.id): [str(self.movie.id), False]} + response.json(), {str(self.movie.id): str(self.movie.id)} ) def test_get_list(self): diff --git a/tests/fastapi/test_fastapi_endpoints.py b/tests/fastapi/test_fastapi_endpoints.py index e421b24c..1284a163 100644 --- a/tests/fastapi/test_fastapi_endpoints.py +++ b/tests/fastapi/test_fastapi_endpoints.py @@ -175,7 +175,7 @@ def test_get_ids(self): client = TestClient(app) response = client.get("/movies/ids/") self.assertEqual(response.status_code, 200) - self.assertEqual(response.json(), {"1": ["Star Wars", False]}) + self.assertEqual(response.json(), {"1": "Star Wars"}) def test_new(self): client = TestClient(app) From b7e402dc37924d43658f3441b30afc6e991cc742 Mon Sep 17 00:00:00 2001 From: sinisaos Date: Sat, 29 Jan 2022 08:22:28 +0100 Subject: [PATCH 4/8] correct type for Postgres --- piccolo_api/crud/endpoints.py | 2 +- tests/crud/test_target_column_pk.py | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/piccolo_api/crud/endpoints.py b/piccolo_api/crud/endpoints.py index 34cdd4de..60444c5b 100644 --- a/piccolo_api/crud/endpoints.py +++ b/piccolo_api/crud/endpoints.py @@ -655,7 +655,7 @@ def _apply_filters( reference_table.select() .where( reference_table._meta.primary_key - == fields[field_name] + == int(fields[field_name]) ) .first() .run_sync() diff --git a/tests/crud/test_target_column_pk.py b/tests/crud/test_target_column_pk.py index cc8fbcea..5961aedd 100644 --- a/tests/crud/test_target_column_pk.py +++ b/tests/crud/test_target_column_pk.py @@ -18,7 +18,7 @@ class Review(Table): class TestTargetPK(TestCase): """ - Make sure PiccoloCRUD works with Tables with a custom primary key column. + Make sure PiccoloCRUD works with Tables with a non-primary key column. """ def setUp(self): @@ -30,12 +30,22 @@ def tearDown(self): Serie.alter().drop_table().run_sync() def test_target_column_pk(self): - serie = Serie(name="Devs") - serie.save().run_sync() - Review(reviewer="John Doe", serie=serie["name"]).save().run_sync() + Serie(name="Devs").save().run_sync() + Review(reviewer="John Doe", serie="Devs").save().run_sync() + review = Review.select(Review.serie.id).first().run_sync() self.client = TestClient(PiccoloCRUD(table=Serie, read_only=False)) response = self.client.get("/Devs/") self.assertEqual(response.status_code, 200) self.assertEqual(response.json()["id"], review["serie.id"]) + + self.client = TestClient(PiccoloCRUD(table=Review, read_only=False)) + response = self.client.get( + "/", params={"serie": f"{review['serie.id']}"} + ) + self.assertEqual(response.status_code, 200) + self.assertEqual( + response.json(), + {"rows": [{"id": 1, "reviewer": "John Doe", "serie": "Devs"}]}, + ) From 346f3ee121d76cdf5cd81d05ab929fe166f69322 Mon Sep 17 00:00:00 2001 From: sinisaos Date: Tue, 1 Feb 2022 19:20:53 +0100 Subject: [PATCH 5/8] cleaning and renaming variables --- piccolo_api/crud/endpoints.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/piccolo_api/crud/endpoints.py b/piccolo_api/crud/endpoints.py index 60444c5b..410a7bb2 100644 --- a/piccolo_api/crud/endpoints.py +++ b/piccolo_api/crud/endpoints.py @@ -631,15 +631,14 @@ def _apply_filters( Works on any queries which support `where` clauses - Select, Count, Objects etc. """ - target_column_name = [ - i._meta.name - for i in self.table._meta.foreign_key_columns - if i._meta.params.get("target_column") is not None - ] - fields = params.fields if fields: model_dict = self.pydantic_model_optional(**fields).dict() + target_column_name = [ + i._meta.name + for i in self.table._meta.foreign_key_columns + if i._meta.params.get("target_column") is not None + ] for field_name in fields.keys(): if field_name in target_column_name: for i in self.table._meta.foreign_key_columns: @@ -651,7 +650,7 @@ def _apply_filters( reference_table = ( i._foreign_key_meta.resolved_references ) - target_column_fk = ( + target_column_query = ( reference_table.select() .where( reference_table._meta.primary_key @@ -660,7 +659,7 @@ def _apply_filters( .first() .run_sync() ) - value = target_column_fk[ + value = target_column_query[ target_column_fk_name[0]._meta.name ] else: From a828396a626dea6a037d1512f26f0ad8e9809e86 Mon Sep 17 00:00:00 2001 From: sinisaos Date: Tue, 8 Feb 2022 10:52:12 +0100 Subject: [PATCH 6/8] final fix for multiple non-primary key per table --- piccolo_api/crud/endpoints.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/piccolo_api/crud/endpoints.py b/piccolo_api/crud/endpoints.py index 410a7bb2..5d604235 100644 --- a/piccolo_api/crud/endpoints.py +++ b/piccolo_api/crud/endpoints.py @@ -634,21 +634,21 @@ def _apply_filters( fields = params.fields if fields: model_dict = self.pydantic_model_optional(**fields).dict() - target_column_name = [ - i._meta.name + target_foreign_key_columns = { + i: i._meta.name for i in self.table._meta.foreign_key_columns - if i._meta.params.get("target_column") is not None - ] + if i._foreign_key_meta.target_column is not None + } for field_name in fields.keys(): - if field_name in target_column_name: - for i in self.table._meta.foreign_key_columns: + for key, value in target_foreign_key_columns.items(): + if field_name == value: target_column_fk_name: t.Any = [ c._meta.params.get("target_column") - for c in i._foreign_key_meta.resolved_references._meta._foreign_key_references # noqa: E501 + for c in key._foreign_key_meta.resolved_references._meta._foreign_key_references # noqa: E501 if c._meta.params.get("target_column") is not None ] reference_table = ( - i._foreign_key_meta.resolved_references + key._foreign_key_meta.resolved_references ) target_column_query = ( reference_table.select() @@ -659,9 +659,10 @@ def _apply_filters( .first() .run_sync() ) - value = target_column_query[ - target_column_fk_name[0]._meta.name - ] + value = target_column_query[ + target_column_fk_name[0]._meta.name + ] + break else: value = model_dict.get(field_name, ...) @@ -901,7 +902,7 @@ async def detail(self, request: Request) -> Response: row_id = self.table._meta.primary_key.value_type(row_id) except ValueError: for i in self.table._meta._foreign_key_references: - target = i._meta.params["target_column"] + target = i._meta.params.get("target_column") if target is not None: reference_target_pk = ( await self.table.select(self.table._meta.primary_key) From a6f2ccaf609925ea57da4f73cc5a5f1209b03c66 Mon Sep 17 00:00:00 2001 From: sinisaos Date: Wed, 9 Feb 2022 16:32:27 +0100 Subject: [PATCH 7/8] index moved to target_column_fk_name --- piccolo_api/crud/endpoints.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/piccolo_api/crud/endpoints.py b/piccolo_api/crud/endpoints.py index 5d604235..0ccdf256 100644 --- a/piccolo_api/crud/endpoints.py +++ b/piccolo_api/crud/endpoints.py @@ -640,13 +640,13 @@ def _apply_filters( if i._foreign_key_meta.target_column is not None } for field_name in fields.keys(): - for key, value in target_foreign_key_columns.items(): - if field_name == value: + for key, val in target_foreign_key_columns.items(): + if field_name == val: target_column_fk_name: t.Any = [ c._meta.params.get("target_column") for c in key._foreign_key_meta.resolved_references._meta._foreign_key_references # noqa: E501 if c._meta.params.get("target_column") is not None - ] + ][0] reference_table = ( key._foreign_key_meta.resolved_references ) @@ -660,7 +660,7 @@ def _apply_filters( .run_sync() ) value = target_column_query[ - target_column_fk_name[0]._meta.name + target_column_fk_name._meta.name ] break else: From ac16f2b252e4794b89a4ba4659df78f51362c5c9 Mon Sep 17 00:00:00 2001 From: sinisaos Date: Wed, 8 Mar 2023 07:09:23 +0100 Subject: [PATCH 8/8] fix linter errors --- piccolo_api/crud/endpoints.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/piccolo_api/crud/endpoints.py b/piccolo_api/crud/endpoints.py index 9bc33036..aa4b6c9c 100644 --- a/piccolo_api/crud/endpoints.py +++ b/piccolo_api/crud/endpoints.py @@ -655,7 +655,7 @@ def _apply_filters( reference_table = ( key._foreign_key_meta.resolved_references ) - target_column_query = ( + target_column_query: t.Any = ( reference_table.select() .where( reference_table._meta.primary_key @@ -943,7 +943,7 @@ async def detail(self, request: Request) -> Response: for i in self.table._meta._foreign_key_references: target = i._meta.params.get("target_column") if target is not None: - reference_target_pk = ( + reference_target_pk: t.Any = ( await self.table.select(self.table._meta.primary_key) .where(target == row_id) .first() @@ -1092,7 +1092,6 @@ async def put_single( } try: - await cls.update(values).where( cls._meta.primary_key == row_id ).run()