Fix RecursionError in _merge_ref_with_schema for circular $ref#2983
Fix RecursionError in _merge_ref_with_schema for circular $ref#2983
Conversation
📝 WalkthroughWalkthroughAdded per-instance circular-$ref detection and caching to the JSON Schema parser; new private helpers traverse resolved $ref targets to detect cycles and short-circuit deep‑merges, deferring emit/parse to reference handling when cycles are found. Changes
Sequence DiagramsequenceDiagram
participant Parser as JSON Schema Parser
participant Resolver as Ref Resolver
participant Cache as Circular Ref Cache
participant Merger as Schema Merger
Parser->>Resolver: resolve($ref)
Resolver-->>Parser: resolved_ref
Parser->>Cache: _is_ref_circular(resolved_ref)?
Cache-->>Parser: cached result / miss
alt cache miss
Parser->>Parser: _has_ref_cycle(resolved_ref, target, visited)
Parser->>Parser: _walk_for_ref(schema, target, visited)
Parser->>Cache: store circular? true/false
end
alt Circular Detected
Parser->>Merger: skip deep-merge → treat as plain $ref
Merger-->>Parser: unmerged attribute
Parser->>Parser: parse_ref() / parse_root_type() if named-definition path
else No Cycle
Parser->>Merger: deep-merge schemas
Merger-->>Parser: merged schema
Parser->>Parser: continue parse_obj flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
|
📚 Docs Preview: https://pr-2983.datamodel-code-generator.pages.dev |
Merging this PR will not alter performance
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2983 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 94 94
Lines 17961 18051 +90
Branches 2077 2091 +14
=========================================
+ Hits 17961 18051 +90
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
koxudaxi
force-pushed
the
fix/circular-ref-merge-recursion-error
branch
2 times, most recently
from
2adc6be to
4d447f9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/datamodel_code_generator/parser/jsonschema.py`:
- Around line 1588-1623: The cycle detection fails for refs inside external
files because _has_ref_cycle loads external raw_doc via _get_ref_body(file_part)
but never scopes model_resolver to that file before resolving nested $ref values
in _walk_for_ref; update _has_ref_cycle to enter the resolver context for the
external file (use the ModelResolver context managers current_base_path_context
and/or base_url_context with the external file_part or its derived base) before
calling get_model_by_path/_walk_for_ref so any relative refs are resolved
against the external file, and similarly wrap the resolve_ref call in
_walk_for_ref (or ensure _has_ref_cycle calls _walk_for_ref inside the resolver
context) so resolve_ref uses the correct base when checking visited/resolved
refs; reference symbols: _has_ref_cycle, _walk_for_ref, _get_ref_body,
model_resolver.current_base_path_context, model_resolver.base_url_context,
model_resolver.resolve_ref.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/datamodel_code_generator/parser/jsonschema.py`:
- Around line 1611-1631: The call to model_resolver.resolve_ref inside
_walk_for_ref can raise custom exceptions beyond KeyError/ValueError and should
be caught so a broken $id doesn't escape the cycle-check; update the try/except
around model_resolver.resolve_ref(ref_value) in _walk_for_ref to catch broader
exceptions (e.g., Exception or the resolver's custom Error) and fall back to
using the raw ref_value (as currently done for KeyError/ValueError), preserving
the existing logic that then compares resolved to target and calls
_has_ref_cycle when appropriate.
4d447f9 to
13485ab
Compare
koxudaxi
force-pushed
the
fix/circular-ref-merge-recursion-error
branch
from
13485ab to
26f1041
Compare
…e-recursion-error # Conflicts: # tests/main/jsonschema/test_main_jsonschema.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/datamodel_code_generator/parser/jsonschema.py`:
- Around line 4064-4067: The current early return when encountering obj.ref in
the obj.ref check causes named schemas that are "$ref + keywords" (circular
refs) to skip generating their alias/root model; remove the immediate return
after calling self.parse_ref(obj, path) in the same conditional so parsing still
handles the ref but execution continues to generate the alias/root schema for
the named model (i.e., call self.parse_ref(obj, path) and then proceed to the
subsequent model-generation logic for the current schema instead of returning),
ensuring referenced class names are created; update any tests to cover a schema
with "$ref + keywords" to verify an alias/root model is emitted.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/datamodel_code_generator/parser/jsonschema.py`:
- Around line 1683-1700: _has_ref_cycle currently treats URL-style refs as
filesystem paths causing incorrect context; update _has_ref_cycle to detect URL
refs (use the same is_url check used by resolve_ref) and when file_part is a URL
set base_path to None and root_path to [file_part] (and base_url to file_part)
before entering the model_resolver context managers; keep the existing logic for
non-URL file_part values. This mirrors resolve_ref behavior and ensures cycle
detection for external URL refs uses the correct resolution context (refer to
function _has_ref_cycle and helper _get_ref_body/_walk_for_ref).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/datamodel_code_generator/parser/jsonschema.py`:
- Around line 1672-1681: The current _is_ref_circular defaults to False on any
exception from _has_ref_cycle which can cause malformed or unreachable refs to
be treated as non-circular and re-processed; change the fallback to True so
unknown/error cases are conservatively treated as circular, cache that result in
self._circular_ref_cache[resolved_ref] and optionally log the exception for
debugging; update the exception branch in _is_ref_circular (which calls
_has_ref_cycle) to set result = True (and store it) so _merge_ref_with_schema
will avoid re-entering problematic refs.
Breaking Change AnalysisResult: No breaking changes detected Reasoning: This PR is a pure bug fix that resolves RecursionError when processing circular $ref schemas with additional schema keywords. The changes only affect schemas that previously crashed - no existing working schemas have their output changed. All expected test output files in the PR are new test cases, not modifications to existing outputs. The PR enables more JSON Schema patterns to work correctly without changing the behavior for previously working schemas. This analysis was performed by Claude Code Action |
|
🎉 Released in 0.54.0 This PR is now available in the latest release. See the release notes for details. |
1 participant
Fixes #2971
Summary by CodeRabbit
Bug Fixes
New Features
Tests