fix: handle BackLink types in fetch_link and fetch_all_links#1286
fix: handle BackLink types in fetch_link and fetch_all_links#1286roman-right wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes fetch_link()/fetch_all_links() so BackLink fields are actually resolved instead of being silently skipped, addressing #885.
Changes:
- Added BackLink resolution logic to
Document.fetch_link()for direct and list back-links. - Added regression tests covering
fetch_link()for single/list BackLinks andfetch_all_links()behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/odm/test_relations.py | Adds regression tests ensuring BackLink fields are populated by fetch_link()/fetch_all_links(). |
| beanie/odm/documents.py | Implements BackLink fetching by querying the reverse collection using the lookup field $id. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| query = {f"{link_info.lookup_field_name}.$id": self.id} | ||
| result = await link_info.document_class.find_one(query) |
There was a problem hiding this comment.
BackLink queries currently match only on ...$id == self.id, but DBRef-like storage is typically {field: DBRef(collection, id)} and may include $ref/$db parts. Matching only $id can produce false positives if the same id value exists for different referenced collections (or if ids are not globally unique across collections). Consider querying on the whole DBRef (e.g., {lookup_field_name: self.to_ref()}) or including $ref in the predicate (e.g., {f\"{lookup_field_name}.$ref\": <collection>, f\"{lookup_field_name}.$id\": self.id}) to ensure the reverse lookup is collection-safe.
There was a problem hiding this comment.
The query already targets a specific collection via link_info.document_class, so cross-collection false positives are not possible. This is the same {lookup_field_name}.$id}pattern used in all existing$lookupstages inbeanie/odm/utils/find.py`.
| results = await link_info.document_class.find( | ||
| {f"{link_info.lookup_field_name}.$id": self.id}, | ||
| ).to_list() |
There was a problem hiding this comment.
Same issue as the BACK_DIRECT case: filtering only on ...$id can match references from the wrong collection. Prefer querying by full DBRef (or at least $ref + $id) when resolving BACK_LIST/OPTIONAL_BACK_LIST to avoid incorrect BackLink populations.
There was a problem hiding this comment.
Same as above. The collection is already scoped by document_class.
| await back_link_doc.fetch_link("back_link") | ||
| assert len(back_link_doc.back_link) == 1 |
There was a problem hiding this comment.
For list BackLinks, the order of find(...).to_list() is unspecified unless a sort is applied. If multiple documents can match in real scenarios, this test (and the implementation) may become flaky or hide ordering expectations. Consider either asserting set membership/ids without relying on ordering, or updating the implementation to apply a deterministic sort (and then assert the order in the test).
There was a problem hiding this comment.
The test creates exactly one matching document, so ordering is not relevant here. The assertion checks length and id, not position.
staticxterm
left a comment
staticxterm
left a comment
There was a problem hiding this comment.
Regarding this When a user calls doc.fetch_link("back_link") or doc.fetch_all_links() on a document with BackLink fields, those fields remain as unresolved BackLink objects instead of being populated with the actual linked documents., what if doing so results in an endless loop if the referenced BackLink also includes a reference to the "current" document from which we are fetching the "BackLink"?
| ): | ||
| if self.id is None: | ||
| return | ||
| query = {f"{link_info.lookup_field_name}.$id": self.id} |
There was a problem hiding this comment.
Not related to this PR but I believe this should be investigated and hopefully fixed with a follow-up PR.
There is one open issue on this matter: #1131
And I've also verified this myself that the index is not being used but instead a full collection scan is being made.
Also, why does the Link.fetch_list() utilize a different query on the "_id" field?
2 participants
Problem
fetch_link()andfetch_all_links()silently skip BackLink fields. When a user callsdoc.fetch_link("back_link")ordoc.fetch_all_links()on a document with BackLink fields, those fields remain as unresolvedBackLinkobjects instead of being populated with the actual linked documents.Fix
Added handling for
BACK_DIRECT,OPTIONAL_BACK_DIRECT,BACK_LIST, andOPTIONAL_BACK_LISTlink types infetch_link(). For single back-links, query the reverse collection with{lookup_field_name}.$id: self.id}and set the result. For list back-links, query and collect all matching documents.fetch_all_links()already delegates tofetch_link()per field, so it works automatically.Closes #885