feat: prefetch aliased fields with filters/ordering#882
Open
rcybulski1122012 wants to merge 9 commits intostrawberry-graphql:mainfrom
Open
feat: prefetch aliased fields with filters/ordering#882rcybulski1122012 wants to merge 9 commits intostrawberry-graphql:mainfrom
rcybulski1122012 wants to merge 9 commits intostrawberry-graphql:mainfrom
Conversation
Contributor
Reviewer's GuideExtends the Django optimizer to support prefetching aliased relation fields that use different filters and/or ordering by assigning each alias a dedicated to_attr, while ensuring connection/paginated fields are not incorrectly optimized, and updates field resolution plus tests accordingly. File-Level Changes
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. Adjust the optimizer to handle if the same field with different arguments using aliases is selected and the field doesn't have a custom resolver. This release was contributed by @rcybulski1122012 in #882 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #882 +/- ##
==========================================
+ Coverage 91.59% 91.62% +0.02%
==========================================
Files 51 51
Lines 4618 4631 +13
==========================================
+ Hits 4230 4243 +13
Misses 388 388 ☔ 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 2 issues, and left some high level feedback:
- In
Field.get_result, the lookup for aliased prefetch usesinfo._raw_info.path.keyand a hard-coded"_strawberry_alias_"prefix; consider centralizing this convention (e.g., a small helper shared with the optimizer) and avoiding direct reliance on_raw_infointernals if possible, so changes in the underlying GraphQL library or path structure don’t break the mapping. - In
_get_hints_from_django_relation, whento_attris requested for connection/paginated fields you skip optimization entirely; it may be worth narrowing this condition or falling back to a non-to_attrprefetch where safe so that alias-based prefetching doesn’t silently disable otherwise valid optimizations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Field.get_result`, the lookup for aliased prefetch uses `info._raw_info.path.key` and a hard-coded `"_strawberry_alias_"` prefix; consider centralizing this convention (e.g., a small helper shared with the optimizer) and avoiding direct reliance on `_raw_info` internals if possible, so changes in the underlying GraphQL library or path structure don’t break the mapping.
- In `_get_hints_from_django_relation`, when `to_attr` is requested for connection/paginated fields you skip optimization entirely; it may be worth narrowing this condition or falling back to a non-`to_attr` prefetch where safe so that alias-based prefetching doesn’t silently disable otherwise valid optimizations.
## Individual Comments
### Comment 1
<location path="tests/test_optimizer.py" line_range="1233" />
<code_context>
}
[email protected]_db(transaction=True)
+def test_query_prefetch_aliases_with_different_filters(
+ db, gql_client: GraphQLTestClient
</code_context>
<issue_to_address>
**issue (testing):** Add tests covering aliased fields on connection/paginated relations where optimization is intentionally skipped
The optimizer now intentionally skips `to_attr` when fields are connections/paginated, but the current tests only cover `milestones` on `projectsPaginated.results`. They don’t cover a connection/paginated field that is itself aliased with different filters/ordering.
Please add an integration test that:
- Uses a connection/paginated relation as the aliased field, with different filters/ordering on the same Django relation.
- Verifies each alias returns the correct results.
- Asserts query counts to confirm we are *not* merging these into a single prefetch (i.e., it’s acceptable if query counts are higher with the optimizer enabled).
This will directly exercise the new `is_connection`/`is_paginated` conditional and protect against regressions when alias-based prefetching is used on connection/paginated fields.
</issue_to_address>
### Comment 2
<location path="tests/test_optimizer.py" line_range="1345-1347" />
<code_context>
+
+
[email protected]_db(transaction=True)
+def test_query_prefetch_aliases_with_same_filters(db, gql_client: GraphQLTestClient):
+ # even though these have the same filters, query is not optimized
+ query = """
+ query TestQuery {
</code_context>
<issue_to_address>
**suggestion (testing):** Clarify and/or assert the intended optimization behavior for aliases with identical filters
The test comment states that queries are not optimized for identical filters, but the test only asserts result shape and reuses the standard `assert_num_queries(3 if DjangoOptimizerExtension.enabled.get() else 5)` pattern. This doesn’t actually demonstrate whether optimization is disabled here or if some merging still occurs.
To better capture the intent, please either:
- Change the expected query count in the `enabled` case to match the “no optimization/merging” requirement, or
- Reword the comment to describe the expected optimization behavior and, ideally, add a companion test that contrasts query counts with an equivalent non-aliased query.
As written, the test may give a misleading impression of what is or isn’t optimized.
```suggestion
@pytest.mark.django_db(transaction=True)
def test_query_prefetch_aliases_with_same_filters(db, gql_client: GraphQLTestClient):
# Aliased milestones with identical filters are currently treated as separate fields.
# This test guards the existing query count so that any future merging/optimization is explicit.
```
</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.
Hey - I've found 5 issues, and left some high level feedback:
- The alias-based
to_attrnaming (ALIAS_PREFIX + alias) is global per model instance, so reusing the same alias for different relations on the same model will result in attribute collisions; consider including the field name or path in the generatedto_attrto keep them unique per field. - In
test_query_prefetch_aliases_with_same_filtersthe inline comment says the query is not optimized, but theassert_num_queries(3 if DjangoOptimizerExtension.enabled.get() else 5)expectation implies optimization still applies; it would be clearer to align the comment or assertion with the intended behavior so readers understand whether this case should be optimized or not.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The alias-based `to_attr` naming (`ALIAS_PREFIX + alias`) is global per model instance, so reusing the same alias for different relations on the same model will result in attribute collisions; consider including the field name or path in the generated `to_attr` to keep them unique per field.
- In `test_query_prefetch_aliases_with_same_filters` the inline comment says the query is not optimized, but the `assert_num_queries(3 if DjangoOptimizerExtension.enabled.get() else 5)` expectation implies optimization still applies; it would be clearer to align the comment or assertion with the intended behavior so readers understand whether this case should be optimized or not.
## Individual Comments
### Comment 1
<location path="strawberry_django/optimizer.py" line_range="1061-1072" />
<code_context>
+ # expect data in _prefetched_objects_cache, not as a plain list attribute.
+ # If to_attr was requested but can't be used, skip optimization entirely
+ # to avoid merging conflicting prefetches for the same path.
+ if to_attr and (
+ getattr(field, "is_connection", False) or getattr(field, "is_paginated", False)
+ ):
+ return store
+ field_prefetch = Prefetch(path, queryset=field_qs, to_attr=to_attr)
field_prefetch._optimizer_sentinel = _sentinel # type: ignore
</code_context>
<issue_to_address>
**suggestion (performance):** Skipping the entire optimization when `to_attr` is incompatible drops non-prefetch hints as well.
For connection/paginated fields, this early return prevents adding the prefetch but also discards other collected optimizations in `field_store` (`only`, `select_related`, nested prefetches that don’t rely on `to_attr`). If the goal is just to avoid conflicting `to_attr` prefetches, consider still building `field_qs` and merging its non-`to_attr` hints into `store`, while omitting only the `Prefetch(..., to_attr=...)` for this field.
```suggestion
field_qs = field_store.apply(base_qs, info=field_info, config=config)
# Don't use to_attr for connection/paginated fields - their resolvers
# expect data in _prefetched_objects_cache, not as a plain list attribute.
# If to_attr was requested but can't be used, still apply other
# optimizations but omit the to_attr argument from the Prefetch so we
# don't create conflicting prefetches for the same path.
use_to_attr = to_attr and not (
getattr(field, "is_connection", False) or getattr(field, "is_paginated", False)
)
if use_to_attr:
field_prefetch = Prefetch(path, queryset=field_qs, to_attr=to_attr)
else:
field_prefetch = Prefetch(path, queryset=field_qs)
field_prefetch._optimizer_sentinel = _sentinel # type: ignore
store.prefetch_related.append(field_prefetch)
```
</issue_to_address>
### Comment 2
<location path="strawberry_django/fields/field.py" line_range="227-231" />
<code_context>
+
+ # Check for to_attr-based prefetch from optimizer (aliased field with filters)
+ if info is not None:
+ response_key = info._raw_info.path.key
+ alias_attr = f"{ALIAS_PREFIX}{response_key}"
+ prefetched = getattr(source, alias_attr, None)
+ if prefetched is not None:
+ return prefetched
+
attr = getattr(source.__class__, attname, None)
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `path.key` for alias lookup may not distinguish between identical aliases at different nesting levels.
Because `alias_attr` uses only `path.key`, different fields at different depths that share the same response key (aliased or not) will all use the same `_strawberry_alias_<key>` attribute. Since the prefetch is stored on the Django model instance, these nested selections can overwrite or read each other’s prefetched values. To avoid collisions, consider basing `to_attr` / `alias_attr` on a stable representation of the full GraphQL path rather than just the leaf key.
</issue_to_address>
### Comment 3
<location path="tests/test_optimizer.py" line_range="1289-1317" />
<code_context>
+
+
[email protected]_db(transaction=True)
+def test_query_prefetch_alias_and_field_with_different_filters(
+ db, gql_client: GraphQLTestClient
+):
+ query = """
+ query TestQuery {
+ projectsPaginated{
+ results {
+ id
+ milestones(filters: { name: {contains: "a"}}) {
+ id
+ }
+ b: milestones(filters: { name: {contains: "b"}}) {
+ id
+ }
+ }
+ }
+ }
+ """
+
+ project_1 = ProjectFactory.create()
+ milestone_1a = MilestoneFactory.create(project=project_1, name="a")
+ milestone_1b = MilestoneFactory.create(project=project_1, name="b")
+ project_2 = ProjectFactory.create()
+ milestone_2a = MilestoneFactory.create(project=project_2, name="a")
+ milestone_2b = MilestoneFactory.create(project=project_2, name="b")
+
+ with assert_num_queries(3 if DjangoOptimizerExtension.enabled.get() else 5):
+ res = gql_client.query(query)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case where one alias has filters and another has no filters to cover mixed filtered/unfiltered aliases
We already test alias vs alias with different filters and alias vs field with different filters. What’s missing is a case where one alias uses arguments (filters/order) and another alias or the base field uses none (the default relation). Please add a test like `milestones` (no filters) plus `a: milestones(filters: ...)` to verify `to_attr` handling and prefetch merging when filtered and unfiltered variants of the same field are requested together.
```suggestion
@pytest.mark.django_db(transaction=True)
def test_query_prefetch_alias_and_field_with_different_filters(
db, gql_client: GraphQLTestClient
):
query = """
query TestQuery {
projectsPaginated{
results {
id
milestones(filters: { name: {contains: "a"}}) {
id
}
b: milestones(filters: { name: {contains: "b"}}) {
id
}
}
}
}
"""
project_1 = ProjectFactory.create()
milestone_1a = MilestoneFactory.create(project=project_1, name="a")
milestone_1b = MilestoneFactory.create(project=project_1, name="b")
project_2 = ProjectFactory.create()
milestone_2a = MilestoneFactory.create(project=project_2, name="a")
milestone_2b = MilestoneFactory.create(project=project_2, name="b")
with assert_num_queries(3 if DjangoOptimizerExtension.enabled.get() else 5):
res = gql_client.query(query)
assert res.data["projectsPaginated"]["results"] == [
{
"id": to_base64("ProjectType", project_1.pk),
"milestones": [
{"id": to_base64("MilestoneType", milestone_1a.pk)},
],
"b": [
{"id": to_base64("MilestoneType", milestone_1b.pk)},
],
},
{
"id": to_base64("ProjectType", project_2.pk),
"milestones": [
{"id": to_base64("MilestoneType", milestone_2a.pk)},
],
"b": [
{"id": to_base64("MilestoneType", milestone_2b.pk)},
],
},
]
@pytest.mark.django_db(transaction=True)
def test_query_prefetch_mixed_filtered_and_unfiltered_aliases(
db, gql_client: GraphQLTestClient
):
query = """
query TestQuery {
projectsPaginated {
results {
id
milestones {
id
}
a: milestones(filters: { name: { contains: "a" } }) {
id
}
}
}
}
"""
project_1 = ProjectFactory.create()
milestone_1a = MilestoneFactory.create(project=project_1, name="a")
milestone_1b = MilestoneFactory.create(project=project_1, name="b")
project_2 = ProjectFactory.create()
milestone_2a = MilestoneFactory.create(project=project_2, name="a")
milestone_2b = MilestoneFactory.create(project=project_2, name="b")
with assert_num_queries(3 if DjangoOptimizerExtension.enabled.get() else 5):
res = gql_client.query(query)
assert res.data["projectsPaginated"]["results"] == [
{
"id": to_base64("ProjectType", project_1.pk),
"milestones": [
{"id": to_base64("MilestoneType", milestone_1a.pk)},
{"id": to_base64("MilestoneType", milestone_1b.pk)},
],
"a": [
{"id": to_base64("MilestoneType", milestone_1a.pk)},
],
},
{
"id": to_base64("ProjectType", project_2.pk),
"milestones": [
{"id": to_base64("MilestoneType", milestone_2a.pk)},
{"id": to_base64("MilestoneType", milestone_2b.pk)},
],
"a": [
{"id": to_base64("MilestoneType", milestone_2a.pk)},
],
},
]
```
</issue_to_address>
### Comment 4
<location path="tests/test_optimizer.py" line_range="1346-1347" />
<code_context>
+
+
[email protected]_db(transaction=True)
+def test_query_prefetch_aliases_with_same_filters(db, gql_client: GraphQLTestClient):
+ # even though these have the same filters, query is not optimized
+ query = """
+ query TestQuery {
</code_context>
<issue_to_address>
**issue (testing):** The comment states queries are not optimized, but the test only checks counts; consider asserting optimizer behavior more explicitly or rephrasing
The test only checks `assert_num_queries(3 if DjangoOptimizerExtension.enabled.get() else 5)` and the response payload, which doesn’t actually show whether alias merging occurred. Please either make the assertions demonstrate the lack of optimization (e.g., show a higher query count than in optimized cases or explicitly assert optimizer state if possible), or reword the inline comment so it doesn’t claim behavior we don’t verify. Otherwise, future readers may assume the test guarantees something it doesn’t.
</issue_to_address>
### Comment 5
<location path="RELEASE.md" line_range="5" />
<code_context>
+release type: minor
+---
+
+Adjust the optimizer to handle if the same filed with different arguments using aliases is selected and the field doesn't have a custom resolver.
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo "filed" → "field" and consider slightly rephrasing the sentence for clarity.
You could rephrase to something like: "Adjust the optimizer to handle cases where the same field with different arguments using aliases is selected and the field doesn't have a custom resolver."
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Adjust the optimizer so it works if someone uses aliases to retrieve a field (with default resolver) few times with different argument
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Support optimized prefetching for aliased Django relation fields with differing filters and ordering in GraphQL queries.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Optimize Django relation prefetching for aliased GraphQL fields that reuse the same underlying field with different arguments.
New Features:
Enhancements:
Tests: