Fix empty list default for GraphQL list fields#2957
Conversation
|
Warning Rate limit exceeded@koxudaxi has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 48 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 (4)
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 |
When GraphQL input fields have empty list defaults (e.g., `requiredList: [String!]! = []`), the generated code was incorrectly producing: - `Field(...)` for required lists (ignoring the default) - `Field([])` for nullable lists (using mutable default) This fix ensures all list fields with empty defaults generate `Field(default_factory=list)`, which is the correct way to handle mutable defaults in Pydantic. Changes: - Add early check in `_get_default_as_pydantic_model()` for any list type with empty default - Update `__str__()` to not skip default handling when `required=True` but `has_default=True` - Update field argument generation to not add `...` when there's already a `default_factory` Fixes issue where GraphQL schemas with default empty arrays would generate incorrect Pydantic code.
fe65bfc to
0476ab3
Compare
|
📚 Docs Preview: https://pr-2957.datamodel-code-generator.pages.dev |
|
Closing in favor of PR #2948 |
Merging this PR will degrade performance by 23.72%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | test_perf_large_models_pydantic_v2 |
3.1 s | 4.1 s | -23.72% |
| ❌ | WallTime | test_perf_multiple_files_input |
3.2 s | 4.1 s | -21.9% |
| ❌ | WallTime | test_perf_duplicate_names |
849.3 ms | 1,017.8 ms | -16.56% |
| ❌ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.7 s | 2.1 s | -21.08% |
| ❌ | WallTime | test_perf_deep_nested |
5.1 s | 6.2 s | -16.94% |
| ❌ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.2 s | 2.7 s | -17.38% |
| ❌ | WallTime | test_perf_stripe_style_pydantic_v2 |
1.7 s | 2.1 s | -21.18% |
| ❌ | WallTime | test_perf_openapi_large |
2.5 s | 3.1 s | -21.44% |
| ❌ | WallTime | test_perf_complex_refs |
1.7 s | 2.2 s | -22.55% |
| ❌ | WallTime | test_perf_graphql_style_pydantic_v2 |
703.2 ms | 902.6 ms | -22.09% |
| ❌ | WallTime | test_perf_all_options_enabled |
5.7 s | 6.8 s | -16.08% |
Comparing fix/required-list-empty-default (0476ab3) with main (aa088d6)
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. ↩
Summary
[String!]! = []) now correctly generateField(default_factory=list)instead ofField(...)[String!] = []) now correctly generateField(default_factory=list)instead ofField([])Problem
When GraphQL input types have fields with empty list defaults like:
The generated Pydantic code was incorrect:
Field(...)- completely ignored the defaultField([])- used mutable default directly (anti-pattern)Solution
_get_default_as_pydantic_model()for any list type with empty default__str__()to not skip default handling whenrequired=Truebuthas_default=True...when there's already adefault_factoryTest plan
test_main_graphql_empty_list_defaultwith both Pydantic v1 and v2 variants🤖 Generated with Claude Code