Fix JSON serialization error for UUID primary keys when excluded from list #553
Conversation
starlette_admin/views.py
Outdated
| async def get_serialized_pk_value( | ||
| self, request: Request, obj: Any, action: RequestAction | ||
| ) -> Any: |
There was a problem hiding this comment.
The request action is now accessible via request.state.action. I'd prefer new methods to use request.state.action directly
| async def get_serialized_pk_value( | |
| self, request: Request, obj: Any, action: RequestAction | |
| ) -> Any: | |
| async def get_serialized_pk_value( | |
| self, request: Request, obj: Any | |
| ) -> Any: |
| def req(app: Starlette): | ||
| return Request( | ||
| { | ||
| "app": app, | ||
| "router": app.router, | ||
| "type": "http", | ||
| "headers": [], | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_ensuring_pk(req: Request, user_view: UserView): | ||
| """ | ||
| Ensures PK is present in the serialized data and properly serialized as a string. | ||
| """ | ||
|
|
||
| user_id = uuid.uuid1() | ||
| user = User(id=user_id, name="Jack") | ||
|
|
||
| data = await user_view.serialize(user, req, RequestAction.LIST) | ||
|
|
||
| assert data["id"] == str(user_id) |
There was a problem hiding this comment.
Starlette recommends testclient to avoid creating the request object. I think It's better to simply call the listing API ,which will, in turn, invoke the serialize method. Here is an example ->
There was a problem hiding this comment.
This poses the question, why the serialization is dependent on the HTTP request object? Maybe consider decoupling them via repackaging parts of the request the serialization is dependent on into a separate DTO (the serialization context). This would give you exactly what I've done -- testing serialization thoroughly without executing fake HTTP requests.
I'll see how doing it through testclient looks for now.
There was a problem hiding this comment.
The request is used to generate some metadata as you can see here
starlette-admin/starlette_admin/views.py
Line 760 in 13e902f
Maybe consider decoupling them via repackaging parts of the request the serialization is dependent on into a separate DTO (the serialization context).
This a good idea, can you open a discussion to present your approach?
There was a problem hiding this comment.
Hm, I'm seeing your commits on this. I was cleaning it up when seen them. )
In fact, I already implemented simple UUID support for Sqlite in tests.
There was a problem hiding this comment.
Hi, sorry I thought you were struggling with the test, and I was trying to help. your implementation with the platform-independent UUID is better, the only thing missing is the relationship (for foreign_model.get_ser...) that you can copy and paste from my commit. Thanks
There was a problem hiding this comment.
Oh, ok. I was just busy yesterday. Will finish the PR changes today.
starlette_admin/views.py
Outdated
| # Make sure the primary key is always available | ||
| pk_attr = not_none(self.pk_attr) | ||
| if pk_attr not in obj_serialized: | ||
| pk_value = await self.get_serialized_pk_value(request, obj, action) |
There was a problem hiding this comment.
We need to use get_serialized_pk_value at these lines as well.
starlette-admin/starlette_admin/views.py
Line 722 in 8c6f58e
starlette-admin/starlette_admin/views.py
Line 732 in 8c6f58e
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #553 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 75 76 +1
Lines 5817 5897 +80
=========================================
+ Hits 5817 5897 +80 ☔ View full report in Codecov by Sentry. |
|
@jowilf here's the gist of my changes that are now conflicting with yours -- https://gist.github.com/alg/b03907a4edcd44fc5adbfd27c90fcf76 Let me know how you want to deal with it. Are you taking over, or me integrating your changes? |
alg
force-pushed
the
serializable-pk
branch
2 times, most recently
from
d58e84d
to
2cef782
Compare
|
@jowilf Pushed the updates to tests incorporating your changes. Please review. |
|
I guess, I need to provide the UUID for MySQL now for the tests of serialization to pass. :) See what I meant? Will have a look. |
When PK is not listed among fields it needs proper serialization in case it's not of a JSON-serializable type already (for example, UUID).
|
Fixed the issue with tests and verified locally by running tests for MySQL. |
for more information, see https://pre-commit.ci
jowilf
changed the title
Addresses #552
When PK is not listed among fields it needs proper serialization in case it's not of a JSON-serializable type already (for example, UUID).
starlette_admin.BaseModelViewreturns the PK value unchanged (for backwards compatibility)starlette_admin.contrib.sqla.ModelViewuses PK field for proper serialization