fix backlink serialization when link data is fetched#1171
fix backlink serialization when link data is fetched#1171alexdlukens wants to merge 3 commits intoBeanieODM:mainfrom
Conversation
alexdlukens
force-pushed
the
backlink-fastapi-validation-error
branch
2 times, most recently
from
69cc101 to
2454f8e
Compare
alexdlukens
force-pushed
the
backlink-fastapi-validation-error
branch
from
2454f8e to
751180e
Compare
staticxterm
left a comment
staticxterm
left a comment
There was a problem hiding this comment.
Hi,
thank you for the PR.
Roman previously noted that BackLinks should only be "virtual" fields, and not be serializable.
Please provide a meaningful use case scenario where one could benefit from the serialization of a BackLink. Later, if there really is a valid use case for this, it should be added to the Beanie docs as well.
| ), | ||
| serialization=core_schema.plain_serializer_function_ser_schema( | ||
| lambda instance: cls.to_dict(instance), | ||
| lambda instance: cls.serialize(instance), |
There was a problem hiding this comment.
I've placed a comment here on line 569 noting to other developers that BackLinks should not be serialized (as I've understood from Roman's response in some GH issue comment).
| return house | ||
|
|
||
|
|
||
| @house_router.get("/person/{id}", response_model=Person) |
There was a problem hiding this comment.
This (response_model=Person) is not supported as BackLinks should not be serializable.
BackLink fetching (e.g. Person.get(fetch_links=True)) was never implemented nor specified as supported in the docs:
https://beanie-odm.dev/tutorial/relations/#back-links
It is not possible to fetch() this virtual link after the initial search.
One problem being that it would lead to infinite recursion if "nesting_depth" was not specified, as you've done below.
There was a problem hiding this comment.
I am okay with not having the backlink field in the serialized result, but I believe serialization should not raise an error if the backlink field was fetched. Please see the minimal test I have added showing this behavior in cf4a18d
| resp2 = await api_client.get(f"/v1/person/{person_id}") | ||
|
|
||
| resp2_json = resp2.json() | ||
| assert resp2_json["name"] == payload["owner"]["name"][-3:] |
There was a problem hiding this comment.
The assertion is kind of meaningless here. What is the point in fetching the related House "owner" from the document in a Person collection?
I struggle to see the real benefit of this functionality from this test alone.
There was a problem hiding this comment.
This example is a bit weak, but I think making CRUD routes for a database model is a common use case. As in routes that would insert, get, update, delete a database model. This flow is broken if we cannot serialize a model when a backlink has been fetched
I guess to make this work one would have to not include backlinks in the model at all, or never fetch_links...
| def serialize(value: Union[BackLink[T], BaseModel]): | ||
| if isinstance(value, BackLink): | ||
| return value.to_dict() | ||
| return value.model_dump(mode="json") |
There was a problem hiding this comment.
What is the use case for this "else if not a BackLink" scenario? Please provide a test case showcasing this, similar to the test_id_types_serialized_when_dumping_to_json test case which does it for the Link type.
|
Hello, please find the minimal test I have added to exhibit the problem I was seeing. This test raises an error when executed on the main branch: |
|
Hello, I see the issue with the recursion as you mention. I still think the behavior I am seeing is a bug, but I understand why dumping the fetched model will not work as I have written. GIven this, I am not sure what the expected result should be. If we want to return Best, |
|
This PR is stale because it has been open 45 days with no activity. |
|
Superceded by PR #1207 |
2 participants
Hello,
I have noticed in FastAPI, if using a model with a BackLink as the return type for a route, the data will fail validation due to an error in validating the BackLink field.
On further inspection, it seems that there was no serialization handling for the BackLink when the field contains valid data from the linked model.
I took the similar approach from the Link field, using the
serializemethod. Find the Link implementation hereAdded a test that checks for this to the FastAPI tests.
Best,
Alex