fix: selectAll infinite recursion
#948
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #948 +/- ##
=======================================
Coverage 77.72% 77.72%
=======================================
Files 154 154
Lines 19154 19154
Branches 1930 1930
=======================================
Hits 14888 14888
Misses 4243 4243
Partials 23 23 ☔ View full report in Codecov by Sentry. |
|
Uhm, the way selection is designed for the Rust client makes it really tricky to avoid for the Rust client... I still can't find a solution for the Rust implementation. |
luckasRanarison
force-pushed
the
met-786-select-all-recursion
branch
from
f1c8a91 to
7416120
Compare
📝 WalkthroughWalkthroughThis pull request encompasses a comprehensive set of changes across multiple files in the project, focusing on updating SDK versions, refining selection handling logic, and expanding test cases for nested composite structures. The modifications span TypeScript, Python, and Rust implementations, with key updates including version bumps for the Changes
Suggested reviewers
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (33)
tests/metagen/typegraphs/sample/ts/client.ts (3)
977-983: Add clarifying comment for maintainability.A short docstring or comment explaining the purpose of this function and how "inner" is used in composite structures would help future maintainers understand its role.
999-1007: Consider optional fields.If any of these sub-nodes (
scalar,composite, orlist) might be optional in certain use cases, handle potential undefined values carefully at runtime.
1184-1191: Provide usage examples.Adding an example snippet or a unit test that demonstrates a typical usage scenario for
nestedCompositewould help clarify best practices for new users.tests/metagen/typegraphs/sample/py/client.py (16)
23-28: Replace ternary with a direct boolean cast for clarity.Use a more concise expression, as suggested by static analysis:
- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags and sub_flags.select_all)🧰 Tools
🪛 Ruff (0.8.2)
28-28: Use
bool(...)instead ofTrue if ... else FalseReplace with `bool(...)
(SIM210)
951-958: Add docstring explaining nested struct usage.A short comment about why a nested struct is required here (e.g., for advanced composition) can enhance code clarity.
976-985: Consider partial data handling.If elements like
scalar,composite, orlistmight be undefined in some contexts, ensure error handling or defaults are in place.
1009-1015: Enhance clarity with a docstring.Elaborating on valid forms of
RootCompositeArgsFnInputhelps ensure consistent usage.
1061-1068: Add coverage.Adding test coverage specifically for
valueandnestedensures correctness in various scenarios.
1082-1090: Account for partial data.
scalar,composite, andlistmight not always be populated. Validate code handles missing fields gracefully.
1092-1098: Consider consistent naming.
PostSelectionsmight be more consistent as singular vs. plural or with other selection types across the codebase.
1183-1191: Document newly added mappings.Explain why
"post": "post!"and"user": "user!"are mapped with exclamation points, clarifying how these GraphQL types behave.
1210-1214: Offer tests for scalar expectations.Confirm that the function always returns a string as expected, especially if external logic modifies
scalarNoArgs.
1220-1224: Handle empty objects.If the caller passes
{}, confirm the function can gracefully handle the missing fields.
1236-1241: Refine function naming.
composite_argsmight be renamed to reflect domain usage or clarify the expected input structure.
1250-1252: Clarify union returns in docstring.Explain how different union members might be returned from
scalar_union.
1254-1258: Add tests for union branches.Some union outputs may differ significantly. Offer tests for each scenario.
1266-1270: Test all variants.Add coverage for the user, post, or other possible variants within the union.
1274-1278: Improve documentation formixed_union.Describe which variants exist and how clients can handle them.
1288-1290: Document nested composite approach.A comment or docstring clarifying the multi-level structure helps new contributors understand this approach.
tests/metagen/typegraphs/sample.ts (5)
34-41: Potential constraints for email.
t.email()is a good start. If you require advanced validation (like limiting length), consider adding constraints or custom validation logic.
47-56: Add a usage comment fornestedComposite.Explaining how and why this nested structure is consumed (e.g., advanced queries) would be beneficial for readability.
76-78: Explain union resolution.Clarify how
scalarUnionpicks between a string or integer result, so users know how to handle the response.
86-88: Verify test coverage formixedUnion.Ensure all variants—post, user, string, integer—are tested in relevant scenarios.
89-95: Check naming for nested fields.
scalar,composite, andlistmight benefit from domain-specific names if these represent real-world concepts.tests/metagen/typegraphs/sample/ts/main.ts (2)
97-107: Expand union testing.
compositeUnion1is tested for thepostvariant. Consider a separate scenario foruser, ensuring completeness.
134-146: Enhanceres6test coverage.Add more items to
listand deeper nesting incomposite.nestedto ensure the code handles complex structures.tests/injection/random_injection_test.ts (2)
86-103: Mock or seed random calls.To guarantee deterministic results for
randomList, mock or seed the random source. Otherwise, test flakiness may occur if the data changes unexpectedly.
110-171: Expand enum and union coverage.Testing only one or two variants (like
"secondary"ineducationLevel) might overlook other valid enumerations or union branches. Consider broader coverage.tests/metagen/typegraphs/sample/rs/client.rs (2)
268-287: Confirm optional fields in partial structs
UsingOption<...>on deeply nested fields can subtly cause data mismatch. Ensure that each optional field inRootNestedCompositeFnOutputCompositeStructNestedStructPartial, etc., has a clear reason to be optional (e.g., truly optional or backward-compat).
315-337: Maintain consistent naming with new selection structs
The naming is verbose but likely auto-generated. If feasible, consider standardizing or shortening for clarity.tests/metagen/metagen_test.ts (1)
595-612: Add negative or edge-case tests forexpectedSchemaC
You’ve added a comprehensive Zod schema for nested composites. Consider adding negative or edge-case tests (e.g., missing/invalid numeric fields, unexpected data shape) to ensure that the Zod validations work as intended.src/metagen/src/client_py/static/client.py (1)
20-25: Use directbool()conversion instead of inline conditional.According to the static analysis hint (Ruff SIM210), the code at line 25 could be more concise:
- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags is not None and sub_flags.select_all)This will simplify the expression and retain the exact same logic.
🧰 Tools
🪛 Ruff (0.8.2)
25-25: Use
bool(...)instead ofTrue if ... else FalseReplace with `bool(...)
(SIM210)
tests/metagen/typegraphs/sample/py_upload/client.py (1)
28-28: Use a direct boolean cast instead of a ternary expression
Per the static analysis hint, converting this line to a direct boolean cast will simplify readability.- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags is not None and sub_flags.select_all)🧰 Tools
🪛 Ruff (0.8.2)
28-28: Use
bool(...)instead ofTrue if ... else FalseReplace with `bool(...)
(SIM210)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
examples/templates/deno/api/example.ts(1 hunks)src/metagen-client-rs/src/selection.rs(1 hunks)src/metagen/src/client_py/static/client.py(3 hunks)src/metagen/src/client_ts/static/mod.ts(1 hunks)tests/injection/random_injection_test.ts(1 hunks)tests/metagen/metagen_test.ts(2 hunks)tests/metagen/typegraphs/identities/rs/fdk.rs(1 hunks)tests/metagen/typegraphs/sample.ts(1 hunks)tests/metagen/typegraphs/sample/py/client.py(6 hunks)tests/metagen/typegraphs/sample/py/main.py(1 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(3 hunks)tests/metagen/typegraphs/sample/rs/client.rs(4 hunks)tests/metagen/typegraphs/sample/rs/main.rs(2 hunks)tests/metagen/typegraphs/sample/ts/client.ts(5 hunks)tests/metagen/typegraphs/sample/ts/main.ts(2 hunks)tests/metagen/typegraphs/sample/ts_upload/client.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/templates/deno/api/example.ts
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/metagen/typegraphs/sample/py_upload/client.py
28-28: Use bool(...) instead of True if ... else False
Replace with `bool(...)
(SIM210)
src/metagen/src/client_py/static/client.py
25-25: Use bool(...) instead of True if ... else False
Replace with `bool(...)
(SIM210)
tests/metagen/typegraphs/sample/py/client.py
28-28: Use bool(...) instead of True if ... else False
Replace with `bool(...)
(SIM210)
🔇 Additional comments (44)
tests/metagen/typegraphs/sample/ts/client.ts (6)
113-118: Ensure skipping logic is correct.This condition may skip sub-nodes even if the user intends to select all. Carefully confirm that
instanceSelectionbeing falsy is indeed the correct scenario to skip.
984-991: Validate usage consistency.Ensure that your code correctly references
RootNestedCompositeFnOutputCompositeStructNestedStructin all call sites, particularly if arguments or structure evolve in the future.
992-998: LGTM for basic structure.No immediate issues observed. Consider adding a comment describing how this list struct integrates with higher-level logic.
1008-1012: Verify references from other files.Double-check that changes to this function are mirrored in the Python client or Rust code to avoid mismatches in function signatures across the codebase.
1043-1058: Confirm field alignment.If additional fields are eventually added to
RootNestedCompositeFnOutputCompositeStructNestedStruct, ensure synchronization between the NodeMeta definition and this type definition to avoid runtime mismatches.
1082-1100: Validate nested selection usage.Partial or deep-nested selections may cause edge cases in the
_selectionToNodeSetlogic. Consider tests that exercise minimal and maximal selections.tests/metagen/typegraphs/sample/py/client.py (17)
110-111: Validate fallback assignment.When
sub_selectionsisNone, defaulting it to{"_": sub_flags}could overwrite user-defined flags if_is already set elsewhere. Confirm it's intended behavior.
125-135: Double-check selectAll skipping logic.Skipping a composite node when
instance_selectionisNonemight cause confusion if the user expects deeper selection. Review whether partial or default expansions are required.
959-967: Maintain reference consistency.Ensure references to
RootNestedCompositeFnOutputCompositeStructNestedStructremain synchronized if additional fields are added or removed.
968-975: LGTM.No immediate issues found with this list struct definition.
999-1007: Check typed dict correctness.Confirm that all required keys for
Postare present. If optional, mark them accordingly to avoid confusion at call sites.
1021-1028: Confirm optional vs. required fields.If
TypedDict. If it's optional, ensure usage patterns reflect that.
1053-1059: Align with NodeDescs.Verify that
RootNestedCompositeFnOutputCompositeStructNestedStructaligns withNodeDescs.RootNestedCompositeFnOutputCompositeStructNestedStructfor consistent field definitions.
1070-1076: LGTM.No concerns with
RootNestedCompositeFnOutputListStruct.
1078-1080: Check array usage.If the list is large or empty, ensure your retrieval logic gracefully handles both extremes.
1194-1198: Improve error handling for invalid selections.If
selectis malformed, raise a descriptive error so users can correct their queries.
1202-1206: LGTM.No immediate issues found for
get_posts.
1216-1218: Validate union usage forargs.Ensure there's no conflict between
PostandPlaceholderArgsif the shape of the argument diverges.
1228-1234: Check alignment withPostSelections.Ensure partial or full expansions of
postfields don't breakcomposite_no_args.
1242-1246: Validate missingidscenario.If
idisn't provided, confirm that the logic ingenPostcan handle it without failing.
1260-1264: Validate arguments consistently.If
args.idis missing or invalid, provide a consistent error path forcomposite_union.
1280-1284: Verify partial selections.If the user doesn't select all subfields, confirm no runtime errors occur.
1292-1296: Watch for performance concerns.If the
nested_compositeselection is large or heavily nested, consider performance tests to avoid slow queries.tests/metagen/typegraphs/sample.ts (3)
25-32: Check naming consistency.Ensure the struct name
"post"is used consistently throughout the codebase, especially if references are auto-generated or rely on naming conventions.
64-67: Validate function parameters.
scalarArgsexpectspostas input, returningt.string(). If the client provides invalid data, confirm a descriptive error is thrown.
72-75: Avoid confusion with missing fields.If
idis required bygenPost, ensure an appropriate error is raised if it's omitted.tests/metagen/typegraphs/sample/ts/main.ts (3)
25-50: Ensure placeholder validity.The mutation requires valid
id,slug, andtitle. Consider adding default fallback values or explicit error messages for missing parameters.
83-90: Clarify ID usage.If the same ID is used repeatedly, ensure it doesn’t imply a conflict in your data model or missed uniqueness checks.
118-132: Validate all union branches.If the resolved union is neither post nor user, ensure the correct branch (string or integer) is handled gracefully.
tests/injection/random_injection_test.ts (1)
20-78: Consider stability of random-based tests.Since this test relies on seeded random data, changes in the seed or random algorithm may cause snapshots to fail. Confirm that your seed is locked and consistent across environments.
tests/metagen/typegraphs/sample/py/main.py (1)
144-169: Ensure tested coverage for deeply nested composite selections
Having multiple nested composite queries is beneficial, but please confirm that the test coverage includes edge cases and prevents potential infinite recursion scenarios—especially since this PR is meant to fix a recursion issue.Would you like help creating a dedicated test to stress-test these nested queries to ensure robust error handling and avoid regressions?
tests/metagen/typegraphs/sample/rs/main.rs (1)
140-166: Confirm no infinite recursion for the newres6query
The Rust code aligns with the Python logic. Please ensure that the newly introducedres6query for nested structures can't re-trigger the infinite recursion bug.Here is a shell snippet you might run to locate any existing cyclical or recursive references in your Rust code:
✅ Verification successful
Let me search for the type definitions to understand the structure better.
Let me try a different approach to find the type definitions.
Let me try one more search to find any recursive type definitions.
Based on the code analysis, I can now provide the final response:
No infinite recursion risk in the
res6query implementation
The type definitions show that the nested composite structure has a well-defined, finite nesting depth:
RootNestedCompositeFnOutputcontains acompositefield of typeRootNestedCompositeFnOutputCompositeStructRootNestedCompositeFnOutputCompositeStructcontains anestedfield of typeRootNestedCompositeFnOutputCompositeStructNestedStructRootNestedCompositeFnOutputCompositeStructNestedStructonly contains scalar fields (inner)The query implementation in both Rust and Python follows this finite structure, and there are no cyclical references in the type definitions that could cause infinite recursion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Attempt to detect references to nested_composite usage that may be cyclical rg -A 10 'nested_composite\('Length of output: 6336
Script:
#!/bin/bash # Search for the type definitions related to nested composite structures ast-grep --pattern 'struct RootNestedCompositeFnOutput'Length of output: 57
Script:
#!/bin/bash # Search for type definitions using ripgrep rg -A 10 'RootNestedCompositeFnOutput.*struct'Length of output: 48
Script:
#!/bin/bash # Search for nested composite related type definitions and their usage rg -A 15 'RootNestedCompositeFnOutputCompositeStruct'Length of output: 29162
tests/metagen/typegraphs/sample/rs/client.rs (2)
167-222: Validate nested composite node definitions
These new node definitions increase complexity. Verify there are no cyclical references amongRootNestedCompositeFnOutputCompositeStructNestedStruct,RootNestedCompositeFnOutputCompositeStruct,RootNestedCompositeFnOutputListStruct, andRootNestedCompositeFnOutput. Make sure the auto-generated structure properly guards against infinite recursion.Consider adding thorough doc comments or referencing a design doc to explain how these nested types are linked, reducing confusion for future contributors.
485-495: Double-check default arguments fornested_composite
NodeArgsErased::Noneis acceptable if the function has no parameters. Confirm that no arguments are needed for this function to avoid future confusion or bugs.tests/metagen/metagen_test.ts (1)
625-625: Good integration ofexpectedSchemaCinto the existing tuple
Including the nested composite schema in the overall test validation ensures broad coverage. This change looks appropriate.src/metagen/src/client_ts/static/mod.ts (1)
110-115: Verify the new skip logic for non-explicit composite selection.This block specifically checks if the
_flag is"selectAll"and if there is noinstanceSelection, then it skips processing the sub-selections. This seems to address infinite recursion scenarios by bypassing unneeded subnodes automatically. However, please ensure it doesn't inadvertently skip essential subnodes that might still need to be processed.src/metagen-client-rs/src/selection.rs (1)
396-396: Confirm the shift fromGettoSkipfor theall()method.Previously, calling
all()likely meant including all subnodes. Now, callingall()on aCompositeSelectresults in skipping. Double-check that this behavior aligns with expectations and that it doesn't break scenarios where the user truly wants to select all subnodes.tests/metagen/typegraphs/sample/ts_upload/client.ts (1)
113-118: Verify the skip logic alignment withselectAll.Similar to the other TypeScript file, this conditional skip for composite selections marked as
selectAllprevents inclusion of subnodes wheninstanceSelectionis missing. Confirm that this fix works as intended, particularly in edge cases with nested or partially specified selections.src/metagen/src/client_py/static/client.py (2)
107-107: Clarify fallback logic forsubSelectionswhen it isNone.Assigning
subSelections = {"_": sub_flags}helps propagate selection flags recursively. Please ensure that no unexpected behaviors arise ifsub_flagsis itselfNoneor lacksselect_all.
122-132: Double-check skipping non-explicit composite selection underselect_all.Here, the code checks if
sub_flags.select_allisTrueandinstance_selectionisNonebefore skipping that branch. This seems helpful to prevent infinite recursion or unwanted expansions. Confirm it doesn't inadvertently exclude user-intended subnodes or hamper certain advanced query patterns.tests/metagen/typegraphs/sample/py_upload/client.py (4)
23-24: Renamingflagstosub_flagsimproves clarity
This rename makes the codebase more understandable by reflecting that these flags handle sub-selections specifically. Good job!
26-26: Exception message aligns with updated variable name
Raising an exception if_is not of typeSelectionFlagsis consistent with the newsub_flagsusage. This helps maintain clarity in error reporting.
110-110: Explicitly forwardingsub_flags
Passingsub_flagsto sub-selections ensures that flags are carried over for deeper nesting. This is a logical step toward avoiding infinite recursion, as it clarifies the distinction between parent vs. child selection flags.
125-135: Skip logic for non-explicit composite whenselect_allis active
The new condition effectively prevents infinite recursion or unintended selection expansions by skipping composites that are bothNoneand marked withselect_all. If the intent is to fully skip sub-selections in these scenarios, ensure that tests cover these edge cases to confirm expected behavior.Would you like me to propose a test scenario that confirms this behavior when
instance_selectionisNoneandselect_allisTrue?
<!-- Pull requests are squashed and merged using: - their title as the commit message - their description as the commit body Having a good title and description is important for the users to get readable changelog. --> <!-- 1. Explain WHAT the change is about --> - Closes [MET-786](https://linear.app/metatypedev/issue/MET-786/typescript-client-selectall-infinite-recursion). <!-- 3. Explain HOW users should update their code --> --- - [x] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Added nested composite structure support across multiple client implementations - Enhanced selection handling for composite queries - Expanded type definitions for more complex data representations - **Bug Fixes** - Improved selection processing logic in client implementations - Updated version compatibility for SDK imports - **Chores** - Updated package dependencies to newer SDK versions - Reformatted and improved code readability across multiple files <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- Pull requests are squashed and merged using: - their title as the commit message - their description as the commit body Having a good title and description is important for the users to get readable changelog. --> <!-- 1. Explain WHAT the change is about --> - Closes [MET-786](https://linear.app/metatypedev/issue/MET-786/typescript-client-selectall-infinite-recursion). <!-- 3. Explain HOW users should update their code --> --- - [x] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Added nested composite structure support across multiple client implementations - Enhanced selection handling for composite queries - Expanded type definitions for more complex data representations - **Bug Fixes** - Improved selection processing logic in client implementations - Updated version compatibility for SDK imports - **Chores** - Updated package dependencies to newer SDK versions - Reformatted and improved code readability across multiple files <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Migration notes
Summary by CodeRabbit
New Features
Bug Fixes
Chores