-
-
Notifications
You must be signed in to change notification settings - Fork 424
Fix $ref handling in request-response mode for readOnly/writeOnly schemas #2942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@koxudaxi has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR fixes handling of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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-2942.datamodel-code-generator.pages.dev |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing
|
4755094 to
d825aec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/datamodel_code_generator/parser/openapi.py (2)
707-735: Consider logging exceptions instead of silently suppressing them.The broad
except Exception: passat lines 734-735 will silently swallow any errors during forced base model generation. While this prevents one problematic schema from breaking the entire generation, it could make debugging difficult.Consider adding a warning or debug log:
🔎 Proposed improvement for exception handling
except Exception: # noqa: BLE001, S110 # pragma: no cover - pass + # Schema couldn't be processed; continue with remaining refs + warn(f"Could not generate forced base model for {ref_path}", stacklevel=2)
737-737: Remove unusednoqadirective.Static analysis indicates the
# noqa: PLR0912directive is unnecessary since this rule is not enabled.🔎 Proposed fix
- def _parse_raw_impl(self) -> None: # noqa: PLR0912 + def _parse_raw_impl(self) -> None:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/openapi/read_only_write_only_ref_request_response.yamlis excluded by!tests/data/**/*.yamland included by none
📒 Files selected for processing (4)
src/datamodel_code_generator/parser/jsonschema.pysrc/datamodel_code_generator/parser/openapi.pytests/data/expected/main/openapi/read_only_write_only_ref_request_response.pytests/main/openapi/test_main_openapi.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/data/expected/main/openapi/read_only_write_only_ref_request_response.py (1)
src/datamodel_code_generator/model/base.py (1)
name(839-841)
src/datamodel_code_generator/parser/jsonschema.py (1)
src/datamodel_code_generator/enums.py (1)
ReadOnlyWriteOnlyModelType(181-189)
🪛 Ruff (0.14.10)
src/datamodel_code_generator/parser/openapi.py
737-737: Unused noqa directive (non-enabled: PLR0912)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.13 on macOS
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (10)
src/datamodel_code_generator/parser/jsonschema.py (5)
982-1016: LGTM - Well-structured variant detection logic.The method correctly determines if a referenced schema will generate a Request or Response variant by checking that it has the relevant read-only/write-only fields AND at least one field that would remain after filtering.
1018-1046: LGTM - Correctly identifies schemas that will produce models.The logic properly handles the edge cases where a schema with only readOnly or only writeOnly fields would not produce a base model in request-response mode.
1048-1065: LGTM - Recursive reference update with forced model tracking.The method correctly updates data type references to point to variant models and tracks schemas that need forced base model generation.
1067-1080: LGTM - Clean field reference update implementation.The method correctly guards against non-RequestResponse modes and updates field references to point to variant models.
1094-1095: LGTM - Proper integration of field reference updates.The call to
_update_field_refs_for_variantis correctly placed before variant model creation.src/datamodel_code_generator/parser/openapi.py (1)
702-705: LGTM - Clean refactoring to support forced base model generation.The separation of
_parse_raw_impland_generate_forced_base_modelsis well-structured.tests/main/openapi/test_main_openapi.py (1)
4424-4444: LGTM! Well-documented test for issue #2940.This test properly exercises the readOnly/writeOnly $ref handling in request-response mode. The docstring clearly explains the expected behavior:
- Variant references when the referenced schema generates Request/Response variants
- Forced base model generation when schemas contain only readOnly or writeOnly fields
The test structure follows established patterns and should effectively validate the fix.
tests/data/expected/main/openapi/read_only_write_only_ref_request_response.py (3)
10-19: LGTM! Correct variant references in parent models.The parent models (
ParentWithMixedChildRequest,ParentWithChildListRequest) correctly reference theChildMixedRequestvariant rather than a baseChildmodel. This demonstrates the fix working as intended—when a referenced schema generates Request/Response variants, parent schemas use the appropriate variant reference.
22-27: LGTM! Forced base models for single-direction schemas.These base models (
ChildOnlyReadOnly,ChildOnlyWriteOnly) correctly lack Request/Response suffixes. This aligns with the expected behavior described in the test: when a schema contains only readOnly or only writeOnly fields, the generator forces creation of a base model since no variants would otherwise be produced.
30-35: LGTM! Correct single-variant generation with base model references.The asymmetric variant generation is correct:
ParentWithOnlyReadOnlyChildRequest(only Request variant) referencesChildOnlyReadOnlybase modelParentWithOnlyWriteOnlyChildResponse(only Response variant) referencesChildOnlyWriteOnlybase modelThis demonstrates the fix properly handles the case where referenced schemas generate forced base models—the parent variants use those base model references instead of searching for non-existent variant types.
d825aec to
3aaf118
Compare
3aaf118 to
8358236
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2942 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 92 94 +2
Lines 16989 17686 +697
Branches 1979 2037 +58
==========================================
+ Hits 16989 17686 +697
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:
|
Breaking Change AnalysisResult: No breaking changes detected Reasoning: This PR is a bug fix for $ref handling in request-response mode with readOnly/writeOnly schemas. The changes improve correctness rather than breaking existing functionality:
Users who were affected by issue #2940 (broken $ref handling) will see improved, correct output. The change doesn't remove functionality or change existing valid behavior - it fixes broken behavior. No CLI options, Python API, or template interfaces changed. This analysis was performed by Claude Code Action |
Fixes: #2940
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.