Fix legacy pydantic extra templates#3602
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:
📝 WalkthroughWalkthroughAdds compatibility for legacy Pydantic v2 custom templates that assign typed ChangesLegacy typed-extra rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant JsonSchemaTest
participant Generator
participant LegacyTemplateDetector
participant GeneratedModel
JsonSchemaTest->>Generator: generate model with custom template directory
Generator->>LegacyTemplateDetector: inspect BaseModel.jinja2
LegacyTemplateDetector-->>Generator: identify legacy template
Generator->>GeneratedModel: render typed __pydantic_extra__ annotation
GeneratedModel-->>JsonSchemaTest: return runtime validation details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
📚 Docs Preview: https://pr-3602.datamodel-code-generator.pages.dev |
Merging this PR will improve performance by 61.88%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_perf_large_models_pydantic_v2_noformat |
842.7 ms | 498.2 ms | +69.16% |
| ⚡ | WallTime | test_perf_large_models_pydantic_v2_builtin |
897.6 ms | 535 ms | +67.76% |
| ⚡ | WallTime | test_perf_deep_nested |
5.1 s | 3.1 s | +63.85% |
| ⚡ | WallTime | test_perf_complex_refs |
2 s | 1.2 s | +63.2% |
| ⚡ | WallTime | test_perf_all_options_enabled |
5.3 s | 3.2 s | +63.01% |
| ⚡ | WallTime | test_perf_multiple_files_input |
3.4 s | 2.1 s | +61.75% |
| ⚡ | WallTime | test_perf_large_models_pydantic_v2 |
3.5 s | 2.2 s | +61.62% |
| ⚡ | WallTime | test_perf_openapi_large |
2.9 s | 1.8 s | +61.58% |
| ⚡ | WallTime | test_perf_duplicate_names |
1,031.6 ms | 642.1 ms | +60.66% |
| ⚡ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.6 s | 1.6 s | +60.58% |
| ⚡ | WallTime | test_perf_graphql_style_pydantic_v2 |
778.3 ms | 491.7 ms | +58.29% |
| ⚡ | WallTime | test_perf_stripe_style_pydantic_v2 |
1.9 s | 1.2 s | +57.05% |
| ⚡ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.9 s | 1.2 s | +56.47% |
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/legacy-pydantic-extra-template (bf40653) with main (a425913)
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. ↩
![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
🤖 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/main/jsonschema/test_main_jsonschema.py`:
- Around line 6059-6161: Update
test_main_jsonschema_legacy_pydantic_extra_custom_template so the old-style
BaseModel.jinja2 copy is placed at tmp_path/pydantic_v2/BaseModel.jinja2,
creating the pydantic_v2 directory as needed, while keeping
--custom-template-dir pointed at tmp_path so the custom legacy template is
actually loaded.
In `@tox.ini`:
- Line 375: Add
test_main_jsonschema_legacy_pydantic_extra_custom_template_rejects_partial_migration
to the tox test selection alongside
test_main_jsonschema_legacy_pydantic_extra_custom_template, preferably in the
pydantic{20,25,213} environment, so it runs in CI.
🪄 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: fb9cf51b-81c4-4976-abad-4dc220ef7b30
⛔ Files ignored due to path filters (3)
tests/data/templates_extensions/pydantic_v2/BaseModel.jinja2is excluded by none and included by nonetests/data/templates_pydantic_extra_pre_3593/pydantic_v2/BaseModel.jinja2is excluded by none and included by nonetests/data/templates_pydantic_extra_unsupported/pydantic_v2/BaseModel.jinja2is excluded by none and included by none
📒 Files selected for processing (5)
src/datamodel_code_generator/model/_custom_template.pysrc/datamodel_code_generator/model/base.pytests/main/jsonschema/test_main_jsonschema.pytests/model/test_base.pytox.ini
koxudaxi
force-pushed
the
fix/legacy-pydantic-extra-template
branch
2 times, most recently
from
8f69733 to
0466057
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
koxudaxi
force-pushed
the
fix/legacy-pydantic-extra-template
branch
from
0466057 to
4b2eecb
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3602 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 35526 35580 +54
Branches 4094 4100 +6
=========================================
+ Hits 35526 35580 +54
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:
|
koxudaxi
force-pushed
the
fix/legacy-pydantic-extra-template
branch
2 times, most recently
from
24c5276 to
975b93c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/main/jsonschema/test_main_jsonschema.py (1)
6111-6113: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winOld-style template copied to wrong path — custom template not loaded.
The
scalar-future-old-stylecase copiesBaseModel.jinja2totmp_path/BaseModel.jinja2(root), but the template loader resolves custom templates as{custom_template_dir}/pydantic_v2/BaseModel.jinja2. The file won't be found attmp_path/pydantic_v2/BaseModel.jinja2, so the built-in template is used instead — this case doesn't actually test the old-style template path. A previous review flagged this same issue and it was marked as addressed, but the current code doesn't reflect the fix.🐛 Proposed fix: copy template into pydantic_v2/ subdirectory
if old_style_template: - copy_files = [(template_dir / "pydantic_v2/BaseModel.jinja2", tmp_path / "BaseModel.jinja2")] + (tmp_path / "pydantic_v2").mkdir(parents=True, exist_ok=True) + copy_files = [(template_dir / "pydantic_v2/BaseModel.jinja2", tmp_path / "pydantic_v2" / "BaseModel.jinja2")] template_dir = tmp_path#!/bin/bash set -euo pipefail # 1. Verify the template file path constant for pydantic_v2 BaseModel. rg -n "TEMPLATE_FILE_PATH.*pydantic_v2" src/datamodel_code_generator/model/pydantic_v2/base_model.py # 2. Check how copy_files is resolved in _copy_files. rg -n "def _copy_files" --type py tests/main/conftest.py sed -n "$(rg -n 'def _copy_files' tests/main/conftest.py | head -1 | cut -d: -f1),+30p" tests/main/conftest.py # 3. Verify the template dir structure. ls -la tests/main/jsonschema/data/templates_pydantic_extra_pre_3593/pydantic_v2/ 2>/dev/null || echo "pydantic_v2 subdir not found" # 4. Check how custom_template_dir is resolved in the template loading path. rg -n "custom_template_dir.*TEMPLATE_FILE_PATH|TEMPLATE_FILE_PATH.*custom_template_dir" --type py src/ | head -10🤖 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/jsonschema/test_main_jsonschema.py` around lines 6111 - 6113, The old-style template setup in the scalar-future-old-style test copies BaseModel.jinja2 to the wrong location. Update the old_style_template branch and its copy_files entry to create/use the pydantic_v2 subdirectory under tmp_path, so the file is available at the custom template loader’s expected path and the test exercises the custom old-style template.
🤖 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.
Duplicate comments:
In `@tests/main/jsonschema/test_main_jsonschema.py`:
- Around line 6111-6113: The old-style template setup in the
scalar-future-old-style test copies BaseModel.jinja2 to the wrong location.
Update the old_style_template branch and its copy_files entry to create/use the
pydantic_v2 subdirectory under tmp_path, so the file is available at the custom
template loader’s expected path and the test exercises the custom old-style
template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15378826-9be5-429f-a863-48199b28db9d
⛔ Files ignored due to path filters (1)
tests/data/templates_pydantic_extra_pre_3593/pydantic_v2/BaseModel.jinja2is excluded by none and included by none
📒 Files selected for processing (6)
src/datamodel_code_generator/model/pydantic_v2/base_model.pytests/data/expected/main/jsonschema/additional_properties_schema_with_properties_legacy_custom_template.pytests/data/expected/main/jsonschema/additional_properties_self_ref_legacy_custom_template.pytests/main/jsonschema/test_main_jsonschema.pytests/model/test_base.pytox.ini
✅ Files skipped from review due to trivial changes (1)
- tests/data/expected/main/jsonschema/additional_properties_self_ref_legacy_custom_template.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/data/expected/main/jsonschema/additional_properties_schema_with_properties_legacy_custom_template.py
koxudaxi
force-pushed
the
fix/legacy-pydantic-extra-template
branch
from
975b93c to
f18d508
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model/test_base.py (1)
289-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer explicit empty-string assertion for clarity.
assert not _strip(...)relies on the falsiness of an empty string to confirm the entire input was stripped. While correct, this is easily misread as "the function did not strip anything." An explicit== ""comparison makes the intent unambiguous.♻️ Suggested clarity improvement
- assert not _strip_legacy_pydantic_extra_post_class_assignment( - "℘Model.__annotations__['__pydantic_extra__'] = Dict[str, int]\n℘Model.model_rebuild(force=True)\n", - "℘Model", - ) + assert _strip_legacy_pydantic_extra_post_class_assignment( + "℘Model.__annotations__['__pydantic_extra__'] = Dict[str, int]\n℘Model.model_rebuild(force=True)\n", + "℘Model", + ) == ""🤖 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/model/test_base.py` around lines 289 - 292, Update the assertion around _strip_legacy_pydantic_extra_post_class_assignment to explicitly compare its result with an empty string, preserving the existing input and model arguments.
🤖 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/model/test_base.py`:
- Around line 289-292: Update the assertion around
_strip_legacy_pydantic_extra_post_class_assignment to explicitly compare its
result with an empty string, preserving the existing input and model arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc8d013e-d531-40d2-b2dd-30c7dfdd3959
📒 Files selected for processing (4)
src/datamodel_code_generator/model/pydantic_v2/base_model.pytests/data/expected/main/jsonschema/additional_properties_schema_with_properties_legacy_custom_template.pytests/data/expected/main/jsonschema/additional_properties_self_ref_legacy_custom_template.pytests/model/test_base.py
💤 Files with no reviewable changes (2)
- tests/data/expected/main/jsonschema/additional_properties_self_ref_legacy_custom_template.py
- tests/data/expected/main/jsonschema/additional_properties_schema_with_properties_legacy_custom_template.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/datamodel_code_generator/model/pydantic_v2/base_model.py
|
Keeping |
2 participants
Summary by CodeRabbit
__pydantic_extra__typing (including self-references).