Skip to content

changes to support non-primary key in Piccolo Admin #134

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
sinisaos wants to merge 11 commits into from
Closed

changes to support non-primary key in Piccolo Admin #134

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6df9c07
changes to support non-primary key in Piccolo Admin
sinisaos Jan 21, 2022
d899cd1
fix for multiple reference non primary keys
sinisaos Jan 22, 2022
1d9b3b2
fix filter search
sinisaos Jan 28, 2022
b7e402d
correct type for Postgres
sinisaos Jan 29, 2022
346f3ee
cleaning and renaming variables
sinisaos Feb 1, 2022
a828396
final fix for multiple non-primary key per table
sinisaos Feb 8, 2022
a6f2cca
index moved to target_column_fk_name
sinisaos Feb 9, 2022
c94facb
Merge branch 'master' into reference_non_primary_key
sinisaos Mar 8, 2023
ac16f2b
fix linter errors
sinisaos Mar 8, 2023
2c0a351
Merge branch 'piccolo-orm:master' into reference_non_primary_key
sinisaos Apr 4, 2023
2cd11bb
Merge branch 'piccolo-orm:master' into reference_non_primary_key
sinisaos Jun 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions piccolo_api/crud/endpoints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Copy Markdown
Member

@dantownsend dantownsend Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see the idea. I think the problem though is it's having to do [0]. If the _foreign_key_references contains more than 1 item, we're guessing that the first one is correct.

I think we're going to have to add a GET parameter so the Piccolo Admin UI can explicitly tell us which column it's interested in.

Copy link
Copy Markdown
Member Author

@sinisaos sinisaos Jan 21, 2022

Choose a reason for hiding this comment

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

Yes. That make sense. I will try to add a GET parameter.

Copy link
Copy Markdown
Member

@dantownsend dantownsend Jan 22, 2022

Choose a reason for hiding this comment

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

Cool, thanks. I think another advantage of the GET param is if someone doesn't pass it we can return the same response as before, so there are no breaking API changes.

Copy link
Copy Markdown
Member Author

@sinisaos sinisaos Jan 22, 2022

Choose a reason for hiding this comment

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

I didn't get far, but we can pass target_column as a query param

target_column = request.query_params.get("target_column")
    if target_column is not None:
        return JSONResponse({i["readable"]: i["readable"] for i in values})

and return the result like this

# ./api/tables/movie/ids/?target_column=
{
   'Star Wars': 'Star Wars',
   'Return of the Jedi: 'Return of the Jedi,
   ...
}

(we don't need to pass anything to query param and return all results because the KeySearchModal already needs all results)
or without the target_column return standard response

# ./api/tables/movie/ids/
{
   1: 'Star Wars',
   2: 'Return of the Jedi,
   ...
}

I don't know yet how to trigger Vue frontend based on the existence of the column_name query parameter. I don't know is this ok, or I completely miss the point.

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
}
)

###########################################################################
Expand Down Expand Up @@ -871,7 +889,16 @@ 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:
Copy link
Copy Markdown
Member

@dantownsend dantownsend Jan 23, 2022

Choose a reason for hiding this comment

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

I think we can move the logic around a bit here.

target = i._meta.params.get("target_column")

if target is None:
    try:
        row_id = self.table._meta.primary_key.value_type(row_id)
     except ValueError:
        return Response("The ID is invalid", status_code=400)
else:
    # insert your new logic here

Copy link
Copy Markdown
Member Author

@sinisaos sinisaos Jan 24, 2022

Choose a reason for hiding this comment

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

Unfortunately this also does not work if we have multiple non-primary keys per single table. e.g if we add director name as non-primary key (or mix primary or non-primary keys) to review reviewer this does not work. we should to leave this as is.

target = i._meta.params["target_column"]
Copy link
Copy Markdown
Member

@dantownsend dantownsend Jan 23, 2022

Choose a reason for hiding this comment

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

This should be i._meta.params.get("target_column") in case the key doesn't exist.

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]
Copy link
Copy Markdown
Member

@dantownsend dantownsend Jan 23, 2022

Choose a reason for hiding this comment

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

We can do this slightly easier:

reference_target_pk = (
    await self.table.select(self.table._meta.primary_key)
    .where(target == row_id)
    .output(as_list=True)
    .first()
    .run()
)[0]

Copy link
Copy Markdown
Member Author

@sinisaos sinisaos Jan 24, 2022

Choose a reason for hiding this comment

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

Unfortunately this does not work if we have multiple non-primary keys per single table.


if (
not await self.table.exists()
Expand Down
9 changes: 4 additions & 5 deletions tests/crud/test_crud_endpoints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand All @@ -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):
"""
Expand All @@ -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")
Expand All @@ -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):
Expand Down
7 changes: 1 addition & 6 deletions tests/crud/test_custom_pk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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")
41 changes: 41 additions & 0 deletions tests/crud/test_target_column_pk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"])
2 changes: 1 addition & 1 deletion tests/fastapi/test_fastapi_endpoints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down