fix: using strawberry.lazy(...) break circular refs - #169
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 58 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (22)
📝 WalkthroughWalkthroughCore utilities now defer Strawberry LazyType resolution during decoration and refactor registry reference replacement to correctly handle containers and union members. A suite of test schema modules was added covering lazy and forward-ref circular patterns (default/global scope) and union-override cases. ChangesLazy Type Resolution and Circular Reference Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
==========================================
+ Coverage 92.18% 92.26% +0.07%
==========================================
Files 70 70
Lines 6170 6206 +36
Branches 816 825 +9
==========================================
+ Hits 5688 5726 +38
+ Misses 333 332 -1
+ Partials 149 148 -1 ☔ View full report in Codecov by Harness. |
|
Wow! @gazorby killing it! Just curious - is there any value of me submitting PRs? |
Owner
Author
|
Of course if you can spend some more time to spawn PRs that would be greatly appreciated, But your issues are really valuable! |
gazorby
force-pushed
the
worktree-fix+163-lazytype-eager-resolution
branch
from
June 6, 2026 17:23
713a5d5 to
942832c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/utils/strawberry.py`:
- Around line 47-51: The function strawberry_contained_types currently resolves
LazyType instances correctly via LazyType.resolve_type(), but the reviewer
suggests making the resolve_lazy parameter keyword-only for clarity; update the
function signature for strawberry_contained_types to accept resolve_lazy as a
keyword-only argument (use a bare * before resolve_lazy) while keeping the
default True and preserve the existing LazyType resolution logic that calls
type_.resolve_type() when resolve_lazy is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02c647c4-a2d8-4393-be35-548afd8b6eec
⛔ Files ignored due to path filters (4)
tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[forwardref_circular_default_scope].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[forwardref_circular_global_scope].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[lazy_circular_default_scope].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[lazy_circular_global_scope].gqlis excluded by!**/__snapshots__/**
📒 Files selected for processing (19)
src/strawchemy/utils/registry.pysrc/strawchemy/utils/strawberry.pytests/unit/mapping/test_schemas.pytests/unit/schemas/forwardref/__init__.pytests/unit/schemas/forwardref/a.pytests/unit/schemas/forwardref/b.pytests/unit/schemas/forwardref/query.pytests/unit/schemas/forwardref_global/__init__.pytests/unit/schemas/forwardref_global/a.pytests/unit/schemas/forwardref_global/b.pytests/unit/schemas/forwardref_global/query.pytests/unit/schemas/lazy/__init__.pytests/unit/schemas/lazy/a.pytests/unit/schemas/lazy/b.pytests/unit/schemas/lazy/query.pytests/unit/schemas/lazy_global/__init__.pytests/unit/schemas/lazy_global/a.pytests/unit/schemas/lazy_global/b.pytests/unit/schemas/lazy_global/query.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/strawchemy/utils/registry.py (1)
265-269:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLazyType arguments can bypass reference tracking after lazy deferral
At Line 265, argument tracking is gated only by
get_object_definition(...). Withresolve_lazy=False,LazyTypewon’t satisfy that condition, so_update_referencesis skipped for lazy-only argument types, and override/forward-ref updates won’t run for those arguments.Suggested fix
for field in object_definition.fields: for argument in field.arguments: if any( - get_object_definition(inner_type) is not None + isinstance(inner_type, LazyType) or get_object_definition(inner_type) is not None for inner_type in strawberry_contained_types(argument.type, resolve_lazy=False) ): self._update_references(argument, "input") self._update_references(field, graphql_type)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/utils/registry.py` around lines 265 - 269, The argument reference-tracking skips LazyType-only arguments because get_object_definition is called with resolve_lazy=False; change the gating so lazy deferred types are resolved or explicitly detected before calling self._update_references. Locate the expression using get_object_definition(...) inside the generator over strawberry_contained_types(argument.type, resolve_lazy=False) and either (a) call strawberry_contained_types with resolve_lazy=True or (b) add an additional check for lazy types (e.g., detect LazyType or use the library's is_lazy helper) and treat them as matchable so that self._update_references(argument, "input") runs for lazy arguments as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/strawchemy/utils/registry.py`:
- Around line 265-269: The argument reference-tracking skips LazyType-only
arguments because get_object_definition is called with resolve_lazy=False;
change the gating so lazy deferred types are resolved or explicitly detected
before calling self._update_references. Locate the expression using
get_object_definition(...) inside the generator over
strawberry_contained_types(argument.type, resolve_lazy=False) and either (a)
call strawberry_contained_types with resolve_lazy=True or (b) add an additional
check for lazy types (e.g., detect LazyType or use the library's is_lazy helper)
and treat them as matchable so that self._update_references(argument, "input")
runs for lazy arguments as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99958f68-fddf-4989-807a-caa954b7a888
⛔ Files ignored due to path filters (2)
tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[union_override_lazy].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[union_override_plain].gqlis excluded by!**/__snapshots__/**
📒 Files selected for processing (4)
src/strawchemy/utils/registry.pytests/unit/mapping/test_schemas.pytests/unit/schemas/union_override_lazy.pytests/unit/schemas/union_override_plain.py
gazorby
force-pushed
the
worktree-fix+163-lazytype-eager-resolution
branch
from
June 7, 2026 09:22
45f1ef6 to
27f2744
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/schemas/union_override_lazy.py`:
- Line 11: The import statement includes Union which is no longer needed when
using PEP 604 syntax. Remove Union from the import statement on the first line.
Then locate the ChildUnion type definition (referenced in the comment as also
applying to lines 26-29) and replace the Union syntax with the pipe operator
syntax, changing from Union[GroupNode, Annotated[...]] to GroupNode |
Annotated[...] to satisfy the UP007 linting rule.
In `@tests/unit/schemas/union_override_plain.py`:
- Line 10: Replace the use of typing.Union for the ChildUnion type with Python
3.10+ union operator; change any occurrences of Union[GroupNode, TagNode] to
GroupNode | TagNode and remove the now-unused Union import from the top of
tests/unit/schemas/union_override_plain.py (also update the other occurrences
mentioned around lines 30–33 where ChildUnion is declared or referenced).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cfeeaeff-5a2e-4364-944b-82d586582278
⛔ Files ignored due to path filters (6)
tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[forwardref_circular_default_scope].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[forwardref_circular_global_scope].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[lazy_circular_default_scope].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[lazy_circular_global_scope].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[union_override_lazy].gqlis excluded by!**/__snapshots__/**tests/unit/mapping/__snapshots__/test_schemas/test_query_schemas[union_override_plain].gqlis excluded by!**/__snapshots__/**
📒 Files selected for processing (21)
src/strawchemy/utils/registry.pysrc/strawchemy/utils/strawberry.pytests/unit/mapping/test_schemas.pytests/unit/schemas/forwardref/__init__.pytests/unit/schemas/forwardref/a.pytests/unit/schemas/forwardref/b.pytests/unit/schemas/forwardref/query.pytests/unit/schemas/forwardref_global/__init__.pytests/unit/schemas/forwardref_global/a.pytests/unit/schemas/forwardref_global/b.pytests/unit/schemas/forwardref_global/query.pytests/unit/schemas/lazy/__init__.pytests/unit/schemas/lazy/a.pytests/unit/schemas/lazy/b.pytests/unit/schemas/lazy/query.pytests/unit/schemas/lazy_global/__init__.pytests/unit/schemas/lazy_global/a.pytests/unit/schemas/lazy_global/b.pytests/unit/schemas/lazy_global/query.pytests/unit/schemas/union_override_lazy.pytests/unit/schemas/union_override_plain.py
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Annotated, Union |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Prefer | syntax for ChildUnion to satisfy UP007.
Use PEP 604 union syntax here (GroupNode | Annotated[...]) and remove Union from imports; this keeps the test module lint-clean.
Suggested diff
-from typing import Annotated, Union
+from typing import Annotated
@@
ChildUnion = Annotated[
- Union[GroupNode, Annotated["TagNode", strawberry.lazy("tests.unit.schemas.union_override_lazy")]],
+ GroupNode | Annotated["TagNode", strawberry.lazy("tests.unit.schemas.union_override_lazy")],
strawberry.union("ChildUnion"),
]Also applies to: 26-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/schemas/union_override_lazy.py` at line 11, The import statement
includes Union which is no longer needed when using PEP 604 syntax. Remove Union
from the import statement on the first line. Then locate the ChildUnion type
definition (referenced in the comment as also applying to lines 26-29) and
replace the Union syntax with the pipe operator syntax, changing from
Union[GroupNode, Annotated[...]] to GroupNode | Annotated[...] to satisfy the
UP007 linting rule.
Source: Linters/SAST tools
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Annotated, Union |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use | unions for ChildUnion (UP007).
Switch Union[GroupNode, TagNode] to GroupNode | TagNode and drop the Union import to avoid lint churn.
Suggested diff
-from typing import Annotated, Union
+from typing import Annotated
@@
ChildUnion = Annotated[
- Union[GroupNode, TagNode],
+ GroupNode | TagNode,
strawberry.union("ChildUnion"),
]Also applies to: 30-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/schemas/union_override_plain.py` at line 10, Replace the use of
typing.Union for the ChildUnion type with Python 3.10+ union operator; change
any occurrences of Union[GroupNode, TagNode] to GroupNode | TagNode and remove
the now-unused Union import from the top of
tests/unit/schemas/union_override_plain.py (also update the other occurrences
mentioned around lines 30–33 where ChildUnion is declared or referenced).
Source: Linters/SAST tools
gazorby
force-pushed
the
worktree-fix+163-lazytype-eager-resolution
branch
from
June 8, 2026 06:28
27f2744 to
b4583e3
Compare
gazorby
force-pushed
the
worktree-fix+163-lazytype-eager-resolution
branch
from
June 9, 2026 18:55
27d2959 to
fe0f8ad
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.
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by CodeRabbit
New Features
Tests