Fix generation reliability edge cases#3561
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds string-path coercion in ChangesString Path Input Handling
CLI Command Header Redaction and Cleanup
Ruff Subprocess Execution Centralization
Pydantic v2 Config Propagation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
|
📚 Docs Preview: https://pr-3561.datamodel-code-generator.pages.dev |
Merging this PR will improve performance by 17.85%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_perf_complex_refs |
2 s | 1.7 s | +21.9% |
| ⚡ | WallTime | test_perf_duplicate_names |
1,048.6 ms | 877.6 ms | +19.48% |
| ⚡ | WallTime | test_perf_multiple_files_input |
3.6 s | 3 s | +19.41% |
| ⚡ | WallTime | test_perf_deep_nested |
5.2 s | 4.3 s | +19.06% |
| ⚡ | WallTime | test_perf_openapi_large |
3 s | 2.5 s | +18.85% |
| ⚡ | WallTime | test_perf_large_models_pydantic_v2 |
3.5 s | 3 s | +17.81% |
| ⚡ | WallTime | test_perf_all_options_enabled |
5.4 s | 4.6 s | +17.67% |
| ⚡ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.9 s | 1.6 s | +16.76% |
| ⚡ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.6 s | 2.2 s | +16.3% |
| ⚡ | WallTime | test_perf_graphql_style_pydantic_v2 |
782.2 ms | 674.4 ms | +15.99% |
| ⚡ | WallTime | test_perf_stripe_style_pydantic_v2 |
1.9 s | 1.7 s | +13.39% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix-reliability-edge-cases (a46d57b) with main (b5d167a)
Footnotes
-
98 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3561 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 163
Lines ? 34928
Branches ? 4044
========================================
Hits ? 34928
Misses ? 0
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/datamodel_code_generator/__main__.py (1)
191-227: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact
--http-query-parametersin command headers.
SENSITIVE_COMMAND_OPTIONSonly includes--http-headers, but the CLI also accepts--http-query-parameters, which can carry tokens or API keys. When--enable-command-headeris enabled, those values will be written verbatim; add that option to the sensitive set.🤖 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/datamodel_code_generator/__main__.py` around lines 191 - 227, Add --http-query-parameters to SENSITIVE_COMMAND_OPTIONS so _redact_command_args in __main__.py treats it like --http-headers and redacts its values when command headers are enabled. Update the sensitive option set near the redaction helpers and ensure both the standalone option and the =value form are covered by the existing redaction logic.
🧹 Nitpick comments (2)
tests/main/openapi/test_main_openapi.py (1)
1458-1469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider simplifying the assertion pattern.
Two minor points on this test's tail:
- Line 1459's
next(...)has no default; if the# command:line is ever missing, this raises a bareStopIterationinstead of a clear assertion failure.- Building a synthetic
redacted=/secret_absent=/following_option_preserved=string and diffing it against a golden fixture viaassert_outputis more indirect than plainassertstatements, and requires maintaining an extra fixture file for what is effectively three booleans.♻️ Suggested simplification
- content = output_file.read_text(encoding="utf-8") - command_line = next(line for line in content.splitlines() if line.startswith("# command:")) - following_option_preserved = expected_visible is None or expected_visible in command_line - assert_output( - "\n".join([ - f"redacted={'yes' if '<redacted>' in command_line else 'no'}", - f"secret_absent={'yes' if 'secret-token' not in command_line else 'no'}", - f"following_option_preserved={'yes' if following_option_preserved else 'no'}", - ]) - + "\n", - EXPECTED_OPENAPI_PATH / "enable_command_header_redacts_http_headers.txt", - ) + content = output_file.read_text(encoding="utf-8") + command_line = next( + (line for line in content.splitlines() if line.startswith("# command:")), None + ) + assert command_line is not None, "command header line not found in output" + assert "<redacted>" in command_line + assert "secret-token" not in command_line + if expected_visible is not None: + assert expected_visible in command_lineThis is purely a test-clarity nit; the current logic is correct.
🤖 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/main/openapi/test_main_openapi.py` around lines 1458 - 1469, The test tail in test_main_openapi.py is overly indirect: update the command-line lookup in the openapi header redaction test so a missing “# command:” line fails with a clear assertion instead of bare StopIteration, and replace the synthetic status-string plus assert_output fixture check with direct assert statements for redaction, secret absence, and following-option preservation. Keep the existing intent in the same test function, but simplify the logic around content, command_line, and following_option_preserved so the assertions are explicit and no extra golden file is needed.src/datamodel_code_generator/format.py (1)
537-563: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: parametrize the return type and reuse
errors="replace"on success decode.The helper is correct and the error-selection logic matches the accompanying tests. Two small polish items:
- The return annotation
subprocess.CompletedProcessis unparametrized; sincecapture_output=Trueyields bytes,subprocess.CompletedProcess[bytes]documents intent and satisfies stricter type-checking configs.- The failure path decodes with
errors="replace", but the callers decoderesult.stdoutwithout it; harmless for generated code, just noting the inconsistency.The static-analysis S603 flag on Line 547 is a false positive here — the command tuple is constructed internally from resolved paths and fixed Ruff flags, not from request input, and is already suppressed via
# noqa: S603.♻️ Proposed annotation tweak
- ) -> subprocess.CompletedProcess: + ) -> subprocess.CompletedProcess[bytes]:🤖 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/datamodel_code_generator/format.py` around lines 537 - 563, The `_run_ruff_command` helper should use a parametrized return type and consistent decoding behavior. Update the return annotation on `_run_ruff_command` to reflect bytes output from `subprocess.run`, and reuse the same `errors="replace"` decoding approach for any success-path stdout handling so the typing and decode behavior stay aligned. Use the existing `_run_ruff_command` symbol and keep the current error-selection logic unchanged.Source: Linters/SAST tools
🤖 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/datamodel_code_generator/__init__.py`:
- Around line 764-778: The _coerce_existing_path_input helper can still fail on
unresolved user-home paths because Path.expanduser() may raise RuntimeError;
update the exception handling in _coerce_existing_path_input to also catch
RuntimeError alongside OSError and ValueError so it safely falls back to
returning the original input_ instead of crashing.
---
Outside diff comments:
In `@src/datamodel_code_generator/__main__.py`:
- Around line 191-227: Add --http-query-parameters to SENSITIVE_COMMAND_OPTIONS
so _redact_command_args in __main__.py treats it like --http-headers and redacts
its values when command headers are enabled. Update the sensitive option set
near the redaction helpers and ensure both the standalone option and the =value
form are covered by the existing redaction logic.
---
Nitpick comments:
In `@src/datamodel_code_generator/format.py`:
- Around line 537-563: The `_run_ruff_command` helper should use a parametrized
return type and consistent decoding behavior. Update the return annotation on
`_run_ruff_command` to reflect bytes output from `subprocess.run`, and reuse the
same `errors="replace"` decoding approach for any success-path stdout handling
so the typing and decode behavior stay aligned. Use the existing
`_run_ruff_command` symbol and keep the current error-selection logic unchanged.
In `@tests/main/openapi/test_main_openapi.py`:
- Around line 1458-1469: The test tail in test_main_openapi.py is overly
indirect: update the command-line lookup in the openapi header redaction test so
a missing “# command:” line fails with a clear assertion instead of bare
StopIteration, and replace the synthetic status-string plus assert_output
fixture check with direct assert statements for redaction, secret absence, and
following-option preservation. Keep the existing intent in the same test
function, but simplify the logic around content, command_line, and
following_option_preserved so the assertions are explicit and no extra golden
file is needed.
🪄 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: CHILL
Plan: Pro
Run ID: 1627d071-32da-4217-a606-1af23ae8690e
⛔ Files ignored due to path filters (1)
tests/data/expected/main/openapi/enable_command_header_redacts_http_headers.txtis excluded by!tests/data/**/*.txtand included by none
📒 Files selected for processing (8)
src/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/format.pytests/data/expected/main/generate_accepts_string_path.pytests/data/expected/main/generate_keeps_non_path_string_input.pytests/main/openapi/test_main_openapi.pytests/main/test_main_general.pytests/test_format.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/main/test_main_general.py (1)
2899-2917: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
# pragma: no coveron the exercised except branch.This test now covers the
except (OSError, RuntimeError, ValueError)branch in_coerce_existing_path_input(src/datamodel_code_generator/__init__.py), which is currently marked# pragma: no cover. Consider removing that pragma so coverage reporting stays accurate now that the branch is tested.🤖 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/main/test_main_general.py` around lines 2899 - 2917, The new test covers the exception path in _coerce_existing_path_input, so the stale # pragma: no cover on the except (OSError, RuntimeError, ValueError) branch should be removed. Update that branch in datamodel_code_generator.__init__ to no longer exclude it from coverage, keeping the behavior unchanged while allowing the exercised path to count in coverage reports.
🤖 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.
Nitpick comments:
In `@tests/main/test_main_general.py`:
- Around line 2899-2917: The new test covers the exception path in
_coerce_existing_path_input, so the stale # pragma: no cover on the except
(OSError, RuntimeError, ValueError) branch should be removed. Update that branch
in datamodel_code_generator.__init__ to no longer exclude it from coverage,
keeping the behavior unchanged while allowing the exercised path to count in
coverage reports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee6b0359-7858-40fa-841c-af5a44e9d453
📒 Files selected for processing (5)
src/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/format.pytests/main/openapi/test_main_openapi.pytests/main/test_main_general.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/datamodel_code_generator/init.py
- tests/main/openapi/test_main_openapi.py
- src/datamodel_code_generator/format.py
- src/datamodel_code_generator/main.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/datamodel_code_generator/model/pydantic_v2/root_model.py (1)
106-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented side-effect: forcing
fields[0].type_hintevaluation and discarding the result.fields = self.fields if use_custom_template else self.rendered_fields if fields: _ = fields[0].type_hint self._sync_config_items()Per the commit message this is meant to avoid "path monkeypatch leakage," but nothing here explains why evaluating (and discarding) the first field's
type_hintbefore syncing config is necessary. Without a comment, a future refactor could easily drop this line and silently reintroduce the exact regression being fixed here.📝 Suggested comment
fields = self.fields if use_custom_template else self.rendered_fields if fields: + # Force type_hint resolution before any config sync so the underlying + # data-type resolution runs while `self.path` is still authoritative, + # avoiding stale/leaked path state from monkeypatching during rendering. _ = fields[0].type_hint self._sync_config_items()🤖 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/datamodel_code_generator/model/pydantic_v2/root_model.py` around lines 106 - 109, The RootModel sync flow in the method that selects `self.fields` or `self.rendered_fields` and then calls `_sync_config_items()` relies on forcing `fields[0].type_hint` evaluation for its side effect, but this is undocumented. Add a concise comment next to that access in the `RootModel` logic explaining that the first field’s `type_hint` must be realized before syncing config to avoid path monkeypatch leakage, so future refactors don’t remove it.
🤖 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/datamodel_code_generator/model/pydantic_v2/root_model.py`:
- Around line 76-88: The `_sync_config_items()` logic in `RootModel` is
overwriting `extra_template_data["config"]` with a plain mapping, which breaks
templates that expect the original `ConfigDict` object. Update
`_sync_config_items()` so `config` remains the model/config object, and only
store the rendered key-value mapping in `_CONFIG_ITEMS_TEMPLATE_DATA_KEY`
(exposed as `config_items`). Keep the import/cache handling in sync with this
behavior inside `root_model.py`.
---
Nitpick comments:
In `@src/datamodel_code_generator/model/pydantic_v2/root_model.py`:
- Around line 106-109: The RootModel sync flow in the method that selects
`self.fields` or `self.rendered_fields` and then calls `_sync_config_items()`
relies on forcing `fields[0].type_hint` evaluation for its side effect, but this
is undocumented. Add a concise comment next to that access in the `RootModel`
logic explaining that the first field’s `type_hint` must be realized before
syncing config to avoid path monkeypatch leakage, so future refactors don’t
remove it.
🪄 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: CHILL
Plan: Pro
Run ID: c1d7ba6f-c570-4a90-8fb7-acda26e5665c
📒 Files selected for processing (5)
src/datamodel_code_generator/model/pydantic_v2/base_model.pysrc/datamodel_code_generator/model/pydantic_v2/root_model.pytests/data/expected/main/generate_keeps_unresolved_user_path_string_input.pytests/main/test_main_general.pytests/model/pydantic_v2/test_root_model.py
✅ Files skipped from review due to trivial changes (1)
- tests/data/expected/main/generate_keeps_unresolved_user_path_string_input.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/main/test_main_general.py
koxudaxi
force-pushed
the
fix-reliability-edge-cases
branch
from
e34ecfa to
712b980
Compare
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/datamodel_code_generator/format.py`:
- Around line 554-555: The stdout fallback in the Ruff invocation path is too
broad because it accepts any non-zero exit with stdout as success. Update the
return handling in the formatting flow around the result check so stdout
fallback is only allowed when Ruff exits with code 1, and keep all other
non-zero codes as failures even if stdout is present. Use the existing result
handling in the formatting function to gate the fallback by the Ruff returncode.
🪄 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: CHILL
Plan: Pro
Run ID: 3d91423a-e8d8-44d0-80d1-011b108f1883
⛔ Files ignored due to path filters (1)
tests/data/expected/main/openapi/enable_command_header_redacts_http_headers.txtis excluded by!tests/data/**/*.txtand included by none
📒 Files selected for processing (12)
src/datamodel_code_generator/__init__.pysrc/datamodel_code_generator/__main__.pysrc/datamodel_code_generator/format.pysrc/datamodel_code_generator/model/pydantic_v2/base_model.pysrc/datamodel_code_generator/model/pydantic_v2/root_model.pytests/data/expected/main/generate_accepts_string_path.pytests/data/expected/main/generate_keeps_non_path_string_input.pytests/data/expected/main/generate_keeps_unresolved_user_path_string_input.pytests/main/openapi/test_main_openapi.pytests/main/test_main_general.pytests/model/pydantic_v2/test_root_model.pytests/test_format.py
✅ Files skipped from review due to trivial changes (2)
- tests/data/expected/main/generate_keeps_non_path_string_input.py
- tests/data/expected/main/generate_keeps_unresolved_user_path_string_input.py
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/data/expected/main/generate_accepts_string_path.py
- tests/model/pydantic_v2/test_root_model.py
- tests/main/test_main_general.py
- src/datamodel_code_generator/init.py
- tests/test_format.py
- src/datamodel_code_generator/model/pydantic_v2/base_model.py
- tests/main/openapi/test_main_openapi.py
- src/datamodel_code_generator/model/pydantic_v2/root_model.py
- src/datamodel_code_generator/main.py
Breaking Change AnalysisResult: Breaking changes detected Reasoning: The diff introduces four observable behavior/output changes beyond internal refactoring. (1) Ruff subprocess failures and missing-ruff now raise RuntimeError where the removed lines show failures were previously ignored (check=False, stdout used unconditionally) — an Error Handling change. (2) The removed Content for Release NotesError Handling Changes
Code Generation Changes
Default Behavior Changes
This analysis was performed by Claude Code Action |
|
🎉 Released in 0.68.0 This PR is now available in the latest release. See the release notes for details. |
2 participants
Fixes: #2381
Summary by CodeRabbit
generate()string handling to better distinguish inline schema text from existing local paths (including unresolved~cases).--enable-command-headerto redact sensitive HTTP header/query-parameter values in the emitted command.--check/diff and--watchexit paths.ruffdiscovery.