feat: support aliasing fields with optimizer hint callables - #930
feat: support aliasing fields with optimizer hint callables#930rcybulski1122012 wants to merge 37 commits into
Conversation
…ith-fields-and-ordering
When a field with annotate/prefetch_related callables is selected multiple times via aliases, resolve the callables once per response key with an alias-scoped info and store callable annotations under an alias-scoped label so they don't clash. Adds three public helpers to keep the hint and the resolver in sync: - optimizer_hint_key(info): unique attribute name for the current selection, identical during hint collection and at resolve time - get_hint_value(source, info, default_attr, *, default): read the hint-produced value back inside the resolver - get_field_arguments(info): resolve the current selection's argument values inside hint callables The synthetic resolve info built during hint collection now uses the response key (alias) as its path key, which is what makes optimizer_hint_key consistent between both phases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…multiple values Dict annotations with callable values now get their custom labels prefixed with the alias-scoped hint key when the field is selected under multiple response keys, and get_hint_value probes that scoped label first. A resolver can therefore read several per-alias annotated values and combine them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for adding the Below is the changelog that will be used for the release. Adjust the optimizer to handle when the same field is selected multiple times via aliases with different arguments and the field doesn't have a custom resolver. Also add support for aliasing custom fields with
Note: inside an optimizer hint callable (and any This release was contributed by @rcybulski1122012 in #930 Additional contributors: @Copilot |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #930 +/- ##
==========================================
+ Coverage 91.78% 91.87% +0.09%
==========================================
Files 50 50
Lines 4709 4801 +92
==========================================
+ Hits 4322 4411 +89
- Misses 387 390 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
StrawberryDjangoField.get_result, the alias-based prefetch lookup manually reconstructs the alias attribute name usingALIAS_PREFIXandinfo._raw_info.path.key; consider delegating this tooptimizer_hint_key(info)to avoid duplicating the naming logic and relying on a private_raw_infoattribute. - The updated
_get_field_argumentsnow usesprint_astto compare arguments, which may introduce unnecessary overhead on hot optimizer paths; you might simplify this by comparing normalized argument values (e.g. viaget_argument_valuesand a stable representation) rather than serializing ASTs to strings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `StrawberryDjangoField.get_result`, the alias-based prefetch lookup manually reconstructs the alias attribute name using `ALIAS_PREFIX` and `info._raw_info.path.key`; consider delegating this to `optimizer_hint_key(info)` to avoid duplicating the naming logic and relying on a private `_raw_info` attribute.
- The updated `_get_field_arguments` now uses `print_ast` to compare arguments, which may introduce unnecessary overhead on hot optimizer paths; you might simplify this by comparing normalized argument values (e.g. via `get_argument_values` and a stable representation) rather than serializing ASTs to strings.
## Individual Comments
### Comment 1
<location path="tests/test_optimizer.py" line_range="1233-1242" />
<code_context>
}
+@pytest.mark.django_db(transaction=True)
+def test_query_prefetch_aliases_with_different_filters(
+ db, gql_client: GraphQLTestClient
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that exercises `get_hint_value`'s failure path when no hint/default is available
Current tests only cover the successful paths (optimizer on with alias-scoped annotations/prefetches, or default computation when the optimizer is off). Please also cover the failure path by adding a focused test that:
- asserts `get_hint_value` raises `AttributeError` when neither alias-scoped nor `default_attr` attributes exist and no `default` is provided;
- checks that the error message includes the selection key and mentions `DjangoOptimizerExtension` being disabled.
A small unit-style test (e.g., around `issues_count_filtered` or `issues_filtered` with the optimizer disabled and no default) should be enough to guard future changes to this logic and message.
Suggested implementation:
```python
@pytest.mark.django_db(transaction=True)
def test_query_prefetch_aliases_with_different_filters(
db, gql_client: GraphQLTestClient
):
query = """
query TestQuery {
projectsPaginated{
results {
id
a: milestones(filters: { name: {contains: "a"}}) {
id
}
b: milestones(filters: { name: {contains: "b"}}) {
```
`):
<file_operations>
<file_operation operation="edit" file_path="tests/test_optimizer.py">
<<<<<<< SEARCH
@pytest.mark.django_db(transaction=True)
def test_query_prefetch_aliases_with_different_filters(
db, gql_client: GraphQLTestClient
):
query = """
query TestQuery {
projectsPaginated{
results {
id
a: milestones(filters: { name: {contains: "a"}}) {
id
}
b: milestones(filters: { name: {contains: "b"}}) {
=======
@pytest.mark.django_db(transaction=True)
def test_query_prefetch_aliases_with_different_filters(
db, gql_client: GraphQLTestClient
):
query = """
query TestQuery {
projectsPaginated{
results {
id
a: milestones(filters: { name: {contains: "a"}}) {
id
}
b: milestones(filters: { name: {contains: "b"}}) {
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
Please add a new unit-style test near the existing tests that exercise `get_hint_value`, using the same imports and fixtures as the rest of `tests/test_optimizer.py`. The test should look conceptually like this (adapt names/signatures to your actual code):
```python
import pytest
from strawberry_django.optimizer import get_hint_value # or wherever it's imported from
@pytest.mark.django_db(transaction=True)
def test_get_hint_value_raises_when_no_hint_no_default(
db,
gql_client: GraphQLTestClient,
settings, # or whatever you use to configure extensions
):
# Ensure DjangoOptimizerExtension is disabled for this test
# (adapt this to your project-specific way of disabling it)
settings.STRAWBERRY_EXTENSIONS = [
ext
for ext in settings.STRAWBERRY_EXTENSIONS
if "DjangoOptimizerExtension" not in getattr(ext, "__name__", "")
]
# Use a selection key that exists in your schema, e.g. "issuesFiltered" or "issuesCountFiltered"
selection_key = "issuesFiltered"
# Build or obtain a selection object that goes through get_hint_value.
# A simple pattern is to execute a GraphQL query and inspect the selection tree,
# but if your tests already have a helper/wrapper around get_hint_value, reuse it instead.
#
# Example (pseudo-ish, adapt to your actual API):
#
# selection = build_selection_for_field(selection_key)
# with pytest.raises(AttributeError) as exc_info:
# get_hint_value(
# selection=selection,
# default_attr="issues_filtered", # something that does NOT exist on the root
# default=None, # IMPORTANT: no default to force failure
# )
with pytest.raises(AttributeError) as exc_info:
# This call signature needs to match your actual get_hint_value
# function; adapt parameters accordingly.
get_hint_value(
selection=None, # TODO: pass a real selection
default_attr="issues_filtered", # non-existent attr on the root object
default=None, # no default provided, to force failure
)
# Assert that the error message includes the selection key
message = str(exc_info.value)
assert selection_key in message
# and that it mentions the DjangoOptimizerExtension being disabled
assert "DjangoOptimizerExtension" in message
```
You will need to:
1. **Import `get_hint_value`** from the correct module (often `strawberry_django.optimizer`), or reuse any helper already used in `tests/test_optimizer.py`.
2. **Construct a real `selection` object** that routes through `get_hint_value`:
- Prefer reusing existing utilities in this test file (for example, any helpers that walk or build the selection tree) so the test mirrors how production code calls `get_hint_value`.
- Target a field like `issuesFiltered` or `issuesCountFiltered` where the test comment refers to.
3. **Disable `DjangoOptimizerExtension`** in the same way other tests do when testing the non-optimized path:
- If there’s a fixture or helper (e.g., a context manager or a client factory that disables extensions), reuse that instead of manipulating `settings` directly.
4. **Ensure `default_attr` and `default` are chosen to force the failure path**:
- `default_attr` must refer to an attribute that does not exist on the root object (or whatever object `get_hint_value` introspects).
- `default` must be `None` or omitted so that `get_hint_value` has no fallback and must raise `AttributeError`.
Position this new test near related tests that already assert `get_hint_value`’s success behavior to keep the file organized. Adjust names and fixtures to match the conventions already present in `tests/test_optimizer.py`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Reuse optimizer_hint_key in StrawberryDjangoField.get_result instead of manually rebuilding the alias attribute from ALIAS_PREFIX and the private _raw_info path. - Compare aliased selections by coerced argument values (resolving variables and defaults via get_argument_values) instead of printed ASTs, falling back to print_ast only for meta fields absent from the parent type's field map. Semantically equal arguments now merge even when spelled differently (e.g. literal vs variable). - Add a test covering get_hint_value's failure path: AttributeError naming the selection key and pointing at DjangoOptimizerExtension, and the default= escape hatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Extends Strawberry Django’s query optimizer to correctly handle aliased selections for fields that use callable optimizer hints (annotate=lambda info: ..., prefetch_related=lambda info: ...) by scoping hint resolution and hint-produced attributes/labels per response key (alias).
Changes:
- Add public helpers (
optimizer_hint_key,get_hint_value,get_field_arguments) to make alias-scoped hint production and resolver consumption deterministic. - Update optimizer selection merging to treat callable-hint fields once per response key, and to use
to_attrfor aliased relation prefetches with differing arguments (while avoidingto_attrfor paginated/connection fields). - Add/extend schema, docs, release notes, and tests covering aliasing with different arguments (filters/ordering/pagination, nested selections, variables, annotate + prefetch callables).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_optimizer.py | Adds integration/unit tests covering aliased selections with callable hints (annotate/prefetch), variables, nesting, ordering/filtering/pagination, and get_hint_value behavior. |
| tests/projects/snapshots/schema.gql | Snapshot update to include newly-added MilestoneType fields used by optimizer aliasing tests. |
| tests/projects/snapshots/schema_with_inheritance.gql | Snapshot update to include the same new fields on inherited milestone types. |
| tests/projects/schema.py | Adds example fields on MilestoneType using callable annotate/prefetch_related hints and the new helper APIs. |
| strawberry_django/optimizer.py | Core implementation: alias-scoped hint keying, argument coercion for selection merging, callable-hint alias handling, and new public helper functions. |
| strawberry_django/fields/field.py | Default resolver path reads to_attr-prefetched aliased relation results using the alias-scoped hint key. |
| strawberry_django/init.py | Re-exports the new optimizer helper functions as part of the public API. |
| RELEASE.md | Release note documenting the new aliasing behavior and helper APIs. |
| docs/guide/optimizer.md | Documentation describing how to use aliasing with callable hints and how resolvers should read alias-scoped hint results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
bellini666
left a comment
There was a problem hiding this comment.
Claude raised this concern to me:
I ran a four-pass self-review over this PR (correctness, regressions, performance, over-engineering) and left the findings inline. Three of them are blockers and all three are reproduced against this branch with a diff against the merge base -- the prefetch to_attr name breaking on aliases containing __ or starting with _, callable prefetch_related hints raising ValueError on two response keys, and dict-form annotate labels breaking the documented root._label resolver pattern.
Two of those three collapse into one change: make the _field_has_callable_hints split conditional on the coerced arguments actually differing, and disambiguate callable prefetch_related the way annotate already is.
Three notes that couldn't be anchored inline, since the lines fall just outside the diff hunks:
_generate_selection_resolve_info (~line 842) -- info.path inside hint callables now reports the alias rather than the field name:
{ milestoneList { a: graphqlPath b: graphqlPath } }
main: {'a': 'milestoneList,0,graphqlPath', 'b': 'milestoneList,0,graphqlPath'}
PR: {'a': 'milestoneList,0,a', 'b': 'milestoneList,0,b'}
That's the intent of the change, but it's an undocumented break for anyone keying cache or permission logic on info.path inside a hint callable or a get_queryset reached through _optimize_prefetch_queryset. Worth a line in RELEASE.md. I grepped the other info.path consumers -- optimizer.py:1443 walks path.typename only, and strawberry core's consumers only ever see the real resolve info -- so nothing in-tree breaks.
_get_model_hints (~lines 1591 and 1602) -- to_attr is threaded as hint_key into _get_hints_from_field (an annotation label) and as to_attr into _get_hints_from_django_field (a prefetch attribute). Same string, two subsystems. For a relation field with a single-expression callable annotate, _scoped_label returns exactly hint_key, so the annotation alias and the Prefetch(to_attr=...) would be the same name on the same instance and one clobbers the other. It's unreachable today -- that configuration already fails on the merge base for an unrelated reason -- so latent rather than live, but the two uses probably want distinct names.
Things I checked that came back clean: no assert_num_queries expectation was raised anywhere (the test diff is purely additive), no model changes so no migration needed, the snapshot changes are additive test-schema fields only, and aliased connections with equal arguments actually improve from 9 queries to 2. The existing suite passes on this branch (1242 passed).
One structural note in the PR's favour: the argument-coercion change in _get_field_arguments is a real fix, not a refactor. graphql.Node.__eq__ includes loc, so the old AST-tuple comparison returned False for any two selections carrying arguments, whatever their values -- aliased selections with arguments were never merged on main.
| continue | ||
|
|
||
| s_field = _find_strawberry_field(object_definition, schema, name) | ||
| if s_field is not None and _field_has_callable_hints(s_field): |
There was a problem hiding this comment.
Claude raised this concern to me:
Blocker. A field with a callable prefetch_related hint selected under two response keys now raises at query time. _field_has_callable_hints forces the per-response-key split even when the aliases are argument-free and identical, so the callable resolves once per alias into two Prefetch objects with the same prefetch_to and no _optimizer_sentinel -- PrefetchInspector.merge then refuses. _scoped_label below only rewrites annotate labels; prefetch_related passes through unchanged, so nothing disambiguates them.
Reproduced against this PR's own test schema:
{ milestoneList { a: myIssues { name } b: myIssues { name } } }
main: 2 queries, {'a': [], 'b': []}
PR: ValueError: Tried to prefetch 2 queries with different filters
to the same attribute. Use `to_attr` in this case...
Same for mixedPrefetchAnnotated.
Related: this branch runs before the same-args comparison, so identical-arg aliases are never merged at all. Confirmed on the annotate side too -- SELECT ..., 1 AS "_strawberry_alias_a", 1 AS "_strawberry_alias_b" for two aliases with the same arguments. Gating the split on the coerced arguments actually differing would fix both the ValueError and the duplicate work.
There was a problem hiding this comment.
Not fully convinced about it, but fixed it either way.
For me it's a natural way of thinking about callables in prefetch_related/annotate, that they are executed per alias.
| # Resolvers can read the values back with `get_hint_value`. | ||
| # Static expressions can't depend on the alias arguments, so they keep | ||
| # their shared label and are annotated only once. | ||
| def _scoped_label(label: str, value: AnnotateType) -> str: |
There was a problem hiding this comment.
Claude raised this concern to me:
Blocker. Dict-form callable annotate hints get renamed out from under existing resolvers. When the field is selected twice the label becomes _strawberry_alias_<key>__<label>, so a resolver reading the fixed attribute breaks.
Reproduced with this PR's own MilestoneType.my_bugs_count (return root._my_bugs_count):
{ milestoneList { id a: myBugsCount b: myBugsCount } }
main: {'a': 2, 'b': 2}
PR: AttributeError: 'Milestone' object has no attribute '_my_bugs_count'
This is the pattern docs/guide/optimizer.md currently teaches, and it breaks the moment a client aliases the field -- silently, from the user's point of view, since nothing in their code changed. tests/projects/schema.py::my_bugs_count isn't migrated to get_hint_value and no test aliases it.
There was a problem hiding this comment.
I think it currently has to be that way. It's related to this comment #930 (comment)
There is no option to provide a dynamic name based on the info object in the dict-form annotate. We would need to change the annotate so it also accepts Callable[[Info], dict] so we can use info. Looking back, I don’t really like it either that we change the name of the annotate attribute without user's knowledge :/
Ofc we can detect when the arguments are the same and merge it into a single annotate without changed the name -- it's been already fixed :)
| return value | ||
| ``` | ||
|
|
||
| A field can also annotate multiple values and combine them in the resolver. |
There was a problem hiding this comment.
Claude raised this concern to me:
This third example reteaches what the two above it already showed, with a different aggregate. The annotate example, the prefetch example and the static-annotation note carry the section on their own.
There was a problem hiding this comment.
Not sure about it.
I think a more complex example with usage of the default_attr argument is actually helpful. WDYT?
bellini666
left a comment
There was a problem hiding this comment.
Hey, sorry for taking long to review this
The PR is kinda big, so I asked claude to also take a look. It raised some concerns, so I asked it to post here
rcybulski1122012
left a comment
There was a problem hiding this comment.
I hope i fixed everything. Few things require a discussion, so let me know what you think.
Also, I used AI quite extensively to understand and implement this, but I still don't have a full understanding of how the optimizer works under the hood.
| return value | ||
| ``` | ||
|
|
||
| A field can also annotate multiple values and combine them in the resolver. |
There was a problem hiding this comment.
Not sure about it.
I think a more complex example with usage of the default_attr argument is actually helpful. WDYT?
| continue | ||
|
|
||
| s_field = _find_strawberry_field(object_definition, schema, name) | ||
| if s_field is not None and _field_has_callable_hints(s_field): |
There was a problem hiding this comment.
Not fully convinced about it, but fixed it either way.
For me it's a natural way of thinking about callables in prefetch_related/annotate, that they are executed per alias.
| # Resolvers can read the values back with `get_hint_value`. | ||
| # Static expressions can't depend on the alias arguments, so they keep | ||
| # their shared label and are annotated only once. | ||
| def _scoped_label(label: str, value: AnnotateType) -> str: |
There was a problem hiding this comment.
I think it currently has to be that way. It's related to this comment #930 (comment)
There is no option to provide a dynamic name based on the info object in the dict-form annotate. We would need to change the annotate so it also accepts Callable[[Info], dict] so we can use info. Looking back, I don’t really like it either that we change the name of the annotate attribute without user's knowledge :/
Ofc we can detect when the arguments are the same and merge it into a single annotate without changed the name -- it's been already fixed :)
Description
Builds on #882, which taught the optimizer to handle auto-generated relation fields selected multiple times via aliases with different arguments.
This PR extends that to custom fields with optimizer hint callables (
annotate=lambda info: ...,prefetch_related=lambda info: ...). Previously, aliasing such a field with different arguments broke: every alias resolved its callable into the same annotation label (or the same prefetch attribute), so the values clashed and one alias silently won.Now, when a field with callable hints is selected under multiple response keys, the optimizer processes each alias separately: the callable is resolved once per alias with an
infoscoped to that selection, and callable annotations are automatically stored under an alias-scoped label.Since annotation/
to_attrnames must be unique per alias, the hint callable and the resolver need a shared, deterministic way to derive that name. Three new public helpers provide it:optimizer_hint_key(info)— returns a unique attribute name for the current selection, derived from its response key (the alias, if aliased). Crucially, it returns the same value inside a hint callable and inside the resolver, so it can name aPrefetch(to_attr=...)in the hint and locate the value in the resolver. (Internally, the synthetic resolve info built during hint collection now uses the response key as its path key — that's what makes the two phases agree.)get_hint_value(source, info, default_attr=None, *, default=...)— reads the hint-produced value back: alias-scoped attribute first, thendefault_attr(the plain annotation label used when the field isn't aliased), thendefault(handy when the optimizer is turned off), otherwise raisesAttributeError.get_field_arguments(info)— resolves the current selection's argument values (variables included), usable inside hint callables where resolver kwargs aren't available.Example:
Notes:
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Enable the Django optimizer to safely handle aliased field selections with argument-dependent optimization hints.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: