Open
Conversation
Contributor
Reviewer's GuideIntroduce a new strawberry_django.federation module that provides Django-aware Apollo Federation decorators for types, interfaces, and fields, auto-generating resolve_reference methods and integrating with the query optimizer, along with updated federation documentation, tests, and release notes. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
|
Thanks for adding the Below is the changelog that will be used for the release. Add native federation support via New decorators that combine
Example usage: import strawberry
import strawberry_django
from strawberry.federation import Schema
@strawberry_django.federation.type(models.Product, keys=["upc"])
class Product:
upc: strawberry.auto
name: strawberry.auto
price: strawberry.auto
# resolve_reference is automatically generated!
schema = Schema(query=Query)The auto-generated This release was contributed by @bellini666 in #858 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #858 +/- ##
==========================================
+ Coverage 91.38% 91.60% +0.22%
==========================================
Files 45 49 +4
Lines 4421 4514 +93
==========================================
+ Hits 4040 4135 +95
+ Misses 381 379 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
generate_resolve_referenceyou wrapresolve_referencewith bothtypes.MethodTypeandclassmethod, which causes theclsargument to be effectively passed twice (andinfoto receive the class when called positionally); you can simplify and avoid this subtle bug by returningclassmethod(resolve_reference)directly instead ofclassmethod(types.MethodType(...)). - The
_get_keys_from_directiveshelper claims to return a list of unique key field names but currently just concatenates all fields from all@keydirectives; consider either deduplicating the list or updating the docstring to match the actual behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `generate_resolve_reference` you wrap `resolve_reference` with both `types.MethodType` and `classmethod`, which causes the `cls` argument to be effectively passed twice (and `info` to receive the class when called positionally); you can simplify and avoid this subtle bug by returning `classmethod(resolve_reference)` directly instead of `classmethod(types.MethodType(...))`.
- The `_get_keys_from_directives` helper claims to return a list of unique key field names but currently just concatenates all fields from all `@key` directives; consider either deduplicating the list or updating the docstring to match the actual behavior.
## Individual Comments
### Comment 1
<location> `strawberry_django/federation/type.py:209-128` </location>
<code_context>
+ keys: Iterable[Key | str] = (),
+ extend: bool = False,
+ shareable: bool = False,
+ inaccessible: bool | object = UNSET,
+ authenticated: bool = False,
+ policy: list[list[str]] | None = None,
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing `inaccessible=False` still adds the `@inaccessible` directive, which likely contradicts the API intent.
Given the `bool | object = UNSET` type, this parameter appears to support three states: UNSET (no directive), True (add `@inaccessible`), and False (explicitly do not add it). With the current `if inaccessible is not UNSET:` check, both True and False add the directive. Please either (a) change the type to `bool = False` to match the current behavior, or (b) update the condition to check specifically for `True` so that `False` acts as an explicit opt‑out.
</issue_to_address>
### Comment 2
<location> `strawberry_django/federation/type.py:164-167` </location>
<code_context>
+ return
+
+ # Don't override existing resolve_reference
+ if hasattr(cls, "resolve_reference"):
+ # Check if it's actually defined on this class, not inherited
+ for klass in cls.__mro__:
+ if "resolve_reference" in klass.__dict__:
+ return
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The `resolve_reference` override check prevents auto-generation even when the method is only inherited, which contradicts the comment.
The docstring and inline comment promise auto-generation unless this class defines `resolve_reference`, yet looping over the full MRO returns as soon as any ancestor defines it, so inherited implementations also block auto-generation. To match the intent, inspect only `cls.__dict__` (or otherwise ignore base classes) instead of walking the entire MRO.
</issue_to_address>
### Comment 3
<location> `strawberry_django/federation/resolve.py:67-68` </location>
<code_context>
+ # If optimizer extension is enabled, optimize this queryset
+ qs = ext.optimize(qs, info=info)
+
+ def _get_result() -> _M:
+ return qs.get()
+
+ return django_resolver(_get_result)()
</code_context>
<issue_to_address>
**issue (bug_risk):** `resolve_model_reference` promises `Model | None` but `qs.get()` will raise on missing records.
Because the function advertises `Model | None`, callers will expect graceful failure. Please either catch `DoesNotExist` (and decide on `MultipleObjectsReturned`) to return `None`, or update the type hint and docstring to document the propagated exceptions so the contract is accurate.
</issue_to_address>
### Comment 4
<location> `strawberry_django/federation/resolve.py:105` </location>
<code_context>
+ return resolve_model_reference(cls_, info=info, **filtered_kwargs)
+
+ # Make it a classmethod bound to the class
+ return classmethod(types.MethodType(resolve_reference, cls)) # type: ignore[arg-type]
</code_context>
<issue_to_address>
**issue:** Using `types.MethodType` inside `classmethod` is unnecessary and likely changes how `cls_` is bound.
Double-binding via MethodType before classmethod can yield unexpected call signatures and interfere with subclass overrides; simply return classmethod(resolve_reference) so Python handles binding once.
</issue_to_address>
### Comment 5
<location> `tests/federation/test_resolve_reference.py:134-153` </location>
<code_context>
+ assert result is not None
+ assert result.name == "testuser"
+
+ def test_resolve_reference_with_multiple_keys(self):
+ """Test type with multiple @key directives uses first key's fields."""
+
+ @strawberry_django.federation.type(
+ models.Fruit,
+ keys=["id", "name"],
+ )
+ class FruitType:
+ id: strawberry.auto
+ name: strawberry.auto
+
+ fruit = models.Fruit.objects.create(name="multi-key-fruit")
+
+ # Should work with id
+ result1 = FruitType.resolve_reference(id=fruit.id)
+ assert result1.name == "multi-key-fruit"
+
+ # Should also work with name
+ result2 = FruitType.resolve_reference(name="multi-key-fruit")
+ assert result2.id == fruit.id
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test ensuring `resolve_reference` ignores non-key kwargs instead of failing
Because federation gateways often send extra representation fields, add a test that calls `FruitType.resolve_reference` with a valid key plus a non-key kwarg (e.g., `extra="ignored"`) and asserts resolution still succeeds, locking in this tolerant behavior.
```suggestion
def test_resolve_reference_with_multiple_keys(self):
"""Test type with multiple @key directives uses first key's fields."""
@strawberry_django.federation.type(
models.Fruit,
keys=["id", "name"],
)
class FruitType:
id: strawberry.auto
name: strawberry.auto
fruit = models.Fruit.objects.create(name="multi-key-fruit")
# Should work with id
result1 = FruitType.resolve_reference(id=fruit.id)
assert result1.name == "multi-key-fruit"
# Should also work with name
result2 = FruitType.resolve_reference(name="multi-key-fruit")
assert result2.id == fruit.id
def test_resolve_reference_ignores_non_key_kwargs(self):
"""Ensure non-key kwargs in representations are ignored, not cause failure."""
@strawberry_django.federation.type(
models.Fruit,
keys=["id", "name"],
)
class FruitType:
id: strawberry.auto
name: strawberry.auto
fruit = models.Fruit.objects.create(name="extra-field-fruit")
# Use a valid key plus an extra non-key field; resolution should still succeed
result = FruitType.resolve_reference(id=fruit.id, extra="ignored")
assert result is not None
assert result.id == fruit.id
assert result.name == "extra-field-fruit"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive native federation support to strawberry-django, enabling seamless integration with Apollo Federation. The implementation provides Django-aware federation decorators that automatically generate resolve_reference methods for entity types, similar to how strawberry_django.type auto-generates Relay resolve_node methods.
Changes:
- Adds
strawberry_django.federationmodule withtype,interface, andfielddecorators that support federation directives - Implements auto-generation of
resolve_referencemethods for entity resolution in federated graphs - Provides comprehensive test coverage with 30+ test cases covering various federation scenarios
- Includes detailed documentation with examples for using federation with Django models
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
strawberry_django/federation/__init__.py |
Module initialization with exports for type, interface, field decorators and utility functions |
strawberry_django/federation/type.py |
Federation-aware type and interface decorators with automatic resolve_reference generation |
strawberry_django/federation/resolve.py |
Utility functions for resolving federation entity references using Django ORM |
strawberry_django/federation/field.py |
Federation-aware field decorator supporting @external, @requires, @provides directives |
strawberry_django/__init__.py |
Updated to export the new federation module |
docs/integrations/federation.md |
Comprehensive documentation with examples for federation integration |
tests/federation/__init__.py |
Test module initialization |
tests/federation/test_type.py |
Tests for federation type decorators, directives, and resolve_reference generation |
tests/federation/test_schema.py |
Integration tests for federation schema generation and _entities resolver |
tests/federation/test_resolve_reference.py |
Tests for auto-generated and custom resolve_reference implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bellini666
force-pushed
the
feat/federation-integration
branch
2 times, most recently
from
f671ca8 to
7aa9ef1
Compare
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
federation.field, thegraphql_typeargument is always passed throughStrawberryAnnotation.from_annotation(graphql_type)(with a default ofNone), which may bypass Strawberry’s usual type inference from annotations; consider mirroring the existingstrawberry_django.fieldimplementation so that whengraphql_typeis not explicitly provided, the field’s type is inferred from the annotated attribute/resolver instead of being forced toNone. - The auto-generated
resolve_referenceuses a flat union of all@keydirective fields and filters by whatever subset is present inkwargs; if you intend to support composite keys or multiple alternative keys, it may be worth explicitly documenting this behavior and/or grouping fields per key so you can validate that all fields for a chosen key are present in a representation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `federation.field`, the `graphql_type` argument is always passed through `StrawberryAnnotation.from_annotation(graphql_type)` (with a default of `None`), which may bypass Strawberry’s usual type inference from annotations; consider mirroring the existing `strawberry_django.field` implementation so that when `graphql_type` is not explicitly provided, the field’s type is inferred from the annotated attribute/resolver instead of being forced to `None`.
- The auto-generated `resolve_reference` uses a flat union of all `@key` directive fields and filters by whatever subset is present in `kwargs`; if you intend to support composite keys or multiple alternative keys, it may be worth explicitly documenting this behavior and/or grouping fields per key so you can validate that all fields for a chosen key are present in a representation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bellini666
force-pushed
the
feat/federation-integration
branch
from
7aa9ef1 to
d95dc32
Compare
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_get_keys_from_directiveshelper currently splitsKey.fieldson whitespace, which will misbehave for nested or more complexFieldSetvalues (e.g. introduce{/}as key names); consider either restricting support to flat keys explicitly or using Strawberry’sFieldSetparsing APIs to extract only valid top-level field names. - In
generate_resolve_reference, the generated resolver treats the union of all key fields across multiple@keydirectives as valid filters; if the intent is to respect the semantics of multiple distinct keys, you may want to pick a single key set (or detect which is satisfied from kwargs) instead of merging them into one field list.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_get_keys_from_directives` helper currently splits `Key.fields` on whitespace, which will misbehave for nested or more complex `FieldSet` values (e.g. introduce `{`/`}` as key names); consider either restricting support to flat keys explicitly or using Strawberry’s `FieldSet` parsing APIs to extract only valid top-level field names.
- In `generate_resolve_reference`, the generated resolver treats the union of all key fields across multiple `@key` directives as valid filters; if the intent is to respect the semantics of multiple distinct keys, you may want to pick a single key set (or detect which is satisfied from kwargs) instead of merging them into one field list.
## Individual Comments
### Comment 1
<location> `RELEASE.md:30` </location>
<code_context>
+schema = Schema(query=Query, enable_federation_2=True)
+```
+
+The auto-generated `resolve_reference` methods support composite keys, multiple keys, and integrate with the query optimizer.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adjusting the sentence for parallel structure and grammatical clarity.
The phrase "support composite keys, multiple keys, and integrate with the query optimizer" mixes verb forms. Consider rewording to "support composite keys, multiple keys, and integration with the query optimizer" or "support composite keys and multiple keys, and they integrate with the query optimizer" to keep the structure consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bellini666
force-pushed
the
feat/federation-integration
branch
2 times, most recently
from
2a2307c to
4915cc8
Compare
bellini666
force-pushed
the
feat/federation-integration
branch
4 times, most recently
from
d5cf822 to
568827c
Compare
bellini666
force-pushed
the
feat/federation-integration
branch
2 times, most recently
from
33a4dac to
fb354e2
Compare
bellini666
force-pushed
the
feat/federation-integration
branch
from
565c07f to
93a8621
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bellini666
force-pushed
the
feat/federation-integration
branch
from
93a8621 to
a9a5f62
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #774
Fix #732
Fix #59
Summary by Sourcery
Add native Apollo Federation integration for strawberry-django via a new federation module and update federation documentation.
New Features:
Enhancements:
Documentation:
Tests:
Chores: