Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 19 additions & 3 deletions beanie/odm/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,11 @@ def __modify_schema__(cls, field_schema: Dict[str, Any]):
}
)

def serialize(self):
if isinstance(self, Link):
return self.to_dict()
return self.dict()

def to_ref(self):
return self.ref

Expand All @@ -524,7 +529,7 @@ def to_dict(self):


if not IS_PYDANTIC_V2:
ENCODERS_BY_TYPE[Link] = lambda o: o.to_dict()
ENCODERS_BY_TYPE[Link] = lambda o: o.serialize()


class BackLink(Generic[T]):
Expand All @@ -535,6 +540,12 @@ def __init__(self, document_class: Type[T]):

if IS_PYDANTIC_V2:

@staticmethod
def serialize(value: Union[BackLink[T], BaseModel]):
if isinstance(value, BackLink):
return value.to_dict()
return value.model_dump(mode="json")
Copy link
Member

Choose a reason for hiding this comment

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

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.


@classmethod
def wrapped_validate(
cls, source_type: Type[Any], handler: GetCoreSchemaHandler
Expand Down Expand Up @@ -565,7 +576,7 @@ def __get_pydantic_core_schema__(
values_schema=core_schema.any_schema(),
),
serialization=core_schema.plain_serializer_function_ser_schema(
lambda instance: cls.to_dict(instance),
lambda instance: cls.serialize(instance),
Copy link
Member

Choose a reason for hiding this comment

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

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_schema=core_schema.dict_schema(),
when_used="json-unless-none",
),
Expand Down Expand Up @@ -610,13 +621,18 @@ def __modify_schema__(cls, field_schema: Dict[str, Any]):
}
)

def serialize(self):
if isinstance(self, BackLink):
return self.to_dict()
return self.dict()

def to_dict(self) -> dict[str, str]:
document_class = DocsRegistry.evaluate_fr(self.document_class) # type: ignore
return {"collection": document_class.get_collection_name()}


if not IS_PYDANTIC_V2:
ENCODERS_BY_TYPE[BackLink] = lambda o: o.to_dict()
ENCODERS_BY_TYPE[BackLink] = lambda o: o.serialize()


class IndexModelField:
Expand Down
5 changes: 5 additions & 0 deletions tests/fastapi/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,8 @@ async def create_house_new(house: House = Body(...)):
await house.save(link_rule=WriteRules.WRITE)
await house.sync()
return house


@house_router.get("/person/{id}", response_model=Person)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

async def get_person(id: PydanticObjectId):
return await Person.get(id, fetch_links=True, nesting_depth=1)
19 changes: 19 additions & 0 deletions tests/fastapi/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,22 @@ async def test_create_house_new(api_client):
assert resp_json["name"] == payload["name"]
assert resp_json["owner"]["name"] == payload["owner"]["name"][-3:]
assert resp_json["owner"]["house"]["collection"] == "House"


async def test_get_person(api_client):
payload = {
"name": "FreshHouse",
"owner": {"name": "will_be_overridden_to_Bob"},
}
resp = await api_client.post("/v1/house", json=payload)
resp_json = resp.json()

person_id = resp_json["owner"].get("id")
if person_id is None:
person_id = resp_json["owner"].get("_id")
assert person_id is not None

resp2 = await api_client.get(f"/v1/person/{person_id}")

resp2_json = resp2.json()
assert resp2_json["name"] == payload["owner"]["name"][-3:]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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...

12 changes: 12 additions & 0 deletions tests/odm/test_relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,18 @@ async def test_with_chaining_aggregation(self):

assert addresses_count[0] == {"count": 10}

async def test_dump_model_with_fetched_backlink(
self, link_and_backlink_doc_pair
):
link_doc, back_link_doc = link_and_backlink_doc_pair

document_with_fetched_backlinks = await DocumentWithBackLink.get(
back_link_doc.id, fetch_links=True, nesting_depth=1
)

assert document_with_fetched_backlinks is not None
document_with_fetched_backlinks.model_dump_json()

async def test_with_chaining_aggregation_and_text_search(self):
# ARRANGE
NUM_DOCS = 10
Expand Down
Loading