[WIP] Fix bug in handling of graphql empty list defaults #2948
Conversation
📝 WalkthroughWalkthroughAdds two generated Pydantic model fixtures (Pydantic v1 and v2) showing list fields with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)tests/data/expected/main/graphql/pydantic_v2_empty_list_default.py (1)
⏰ 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). (12)
🔇 Additional comments (3)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @tests/main/graphql/test_main_graphql.py:
- Around line 124-125: Rename the test function
test_main_openapi_empty_list_default to test_main_graphql_empty_list_default and
update its docstring from "Test OpenAPI generation with empty list default
values." to "Test GraphQL generation with empty list default values." to reflect
that this is a GraphQL test (ensure any references to the old name in the test
module are updated accordingly).
- Line 129: The test passes a Path-prefixed value for expected_file which should
instead be the filename string; replace the argument
expected_file=GRAPHQL_DATA_PATH / expected_output with
expected_file=expected_output so expected_file uses the same filename string as
expected_output (refer to the expected_file and expected_output parameters in
the failing test function).
- Line 127: The test references GRAPHQL_DATA_PATH / "empty_list_default.graphql"
but the tests/data/graphql directory and the file empty_list_default.graphql are
missing; create the tests/data/graphql directory and add an
empty_list_default.graphql file matching the expected input schema (e.g., a
minimal GraphQL schema or the specific input used by the test) so the test can
load GRAPHQL_DATA_PATH / "empty_list_default.graphql" successfully.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/graphql/empty_list_default.graphqlis excluded by!tests/data/**/*.graphqland included by none
📒 Files selected for processing (3)
tests/data/expected/main/graphql/empty_list_default.pytests/data/expected/main/graphql/pydantic_v2_empty_list_default.pytests/main/graphql/test_main_graphql.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/data/expected/main/graphql/empty_list_default.py (3)
tests/data/expected/parser/openapi/openapi_parser_parse_modular/bar.py (1)
Field(6-7)tests/data/expected/main/graphql/pydantic_v2_empty_list_default.py (2)
Container(12-13)PodSpec(16-17)src/datamodel_code_generator/model/base.py (1)
name(839-841)
tests/data/expected/main/graphql/pydantic_v2_empty_list_default.py (1)
tests/data/expected/main/graphql/empty_list_default.py (2)
Container(12-13)PodSpec(16-17)
⏰ 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). (12)
- GitHub Check: 3.11 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.14 on Ubuntu
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.13 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
tests/data/expected/main/graphql/pydantic_v2_empty_list_default.py (1)
1-17: LGTM! Expected output correctly usesField(default_factory=list)for mutable defaults.The generated Pydantic models correctly use
Field(default_factory=list)for the optional list field, which is the proper pattern to avoid mutable default issues in Python.tests/data/expected/main/graphql/empty_list_default.py (1)
1-17: LGTM! Expected output uses correct Pydantic pattern.The expected output correctly demonstrates the
Field(default_factory=list)pattern for optional container fields with empty list defaults.
| def test_main_openapi_empty_list_default(output_model: str, expected_output: str, output_file: Path) -> None: | ||
| """Test OpenAPI generation with empty list default values.""" |
There was a problem hiding this comment.
Fix function name and docstring: should reference GraphQL, not OpenAPI.
The function name contains openapi and the docstring says "Test OpenAPI generation", but this test is for GraphQL code generation (as confirmed by input_file_type="graphql" on line 130).
🔎 Proposed fix
-def test_main_openapi_empty_list_default(output_model: str, expected_output: str, output_file: Path) -> None:
- """Test OpenAPI generation with empty list default values."""
+def test_main_graphql_empty_list_default(output_model: str, expected_output: str, output_file: Path) -> None:
+ """Test GraphQL generation with empty list default values."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_main_openapi_empty_list_default(output_model: str, expected_output: str, output_file: Path) -> None: | |
| """Test OpenAPI generation with empty list default values.""" | |
| def test_main_graphql_empty_list_default(output_model: str, expected_output: str, output_file: Path) -> None: | |
| """Test GraphQL generation with empty list default values.""" |
🤖 Prompt for AI Agents
In @tests/main/graphql/test_main_graphql.py around lines 124 - 125, Rename the
test function test_main_openapi_empty_list_default to
test_main_graphql_empty_list_default and update its docstring from "Test OpenAPI
generation with empty list default values." to "Test GraphQL generation with
empty list default values." to reflect that this is a GraphQL test (ensure any
references to the old name in the test module are updated accordingly).
| def test_main_openapi_empty_list_default(output_model: str, expected_output: str, output_file: Path) -> None: | ||
| """Test OpenAPI generation with empty list default values.""" | ||
| run_main_and_assert( | ||
| input_path=GRAPHQL_DATA_PATH / "empty_list_default.graphql", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the GraphQL input file exists anywhere in the repository
fd -t f "empty_list_default.graphql" .Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check the test file to understand the GRAPHQL_DATA_PATH
grep -n "GRAPHQL_DATA_PATH" tests/main/graphql/test_main_graphql.py | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 1491
🏁 Script executed:
#!/bin/bash
# List the test data directory to see what files are available
ls -la tests/data/ 2>/dev/null || ls -la tests/ 2>/dev/null | head -30Repository: koxudaxi/datamodel-code-generator
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Find conftest and check GRAPHQL_DATA_PATH definition
find . -name "conftest.py" -type f | grep -E "tests.*conftest"Repository: koxudaxi/datamodel-code-generator
Length of output: 250
🏁 Script executed:
#!/bin/bash
# Check tests/main/conftest.py for GRAPHQL_DATA_PATH
cat tests/main/conftest.py | grep -A 5 "GRAPHQL_DATA_PATH"Repository: koxudaxi/datamodel-code-generator
Length of output: 358
🏁 Script executed:
#!/bin/bash
# List GraphQL test data files
fd -t f "\.graphql$" tests/Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Find DATA_PATH definition in conftest
grep -n "^DATA_PATH" tests/main/conftest.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 135
🏁 Script executed:
#!/bin/bash
# List the graphql data directory
ls -la tests/data/graphql/ 2>/dev/null || echo "Directory does not exist"Repository: koxudaxi/datamodel-code-generator
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Check entire tests/data structure
find tests/data -type d | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Search for any graphql file in the entire repo with different extension handling
find . -iname "*graphql*" -type fRepository: koxudaxi/datamodel-code-generator
Length of output: 396
🏁 Script executed:
#!/bin/bash
# Check the test function for empty_list_default
sed -n '120,135p' tests/main/graphql/test_main_graphql.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 742
Add the missing GraphQL input file and directory.
The test references empty_list_default.graphql from tests/data/graphql/ directory, but this entire directory is missing from the repository. The test will fail without the input schema file. Create tests/data/graphql/ directory and add the required empty_list_default.graphql file.
🤖 Prompt for AI Agents
In @tests/main/graphql/test_main_graphql.py at line 127, The test references
GRAPHQL_DATA_PATH / "empty_list_default.graphql" but the tests/data/graphql
directory and the file empty_list_default.graphql are missing; create the
tests/data/graphql directory and add an empty_list_default.graphql file matching
the expected input schema (e.g., a minimal GraphQL schema or the specific input
used by the test) so the test can load GRAPHQL_DATA_PATH /
"empty_list_default.graphql" successfully.
| run_main_and_assert( | ||
| input_path=GRAPHQL_DATA_PATH / "empty_list_default.graphql", | ||
| output_path=output_file, | ||
| expected_file=GRAPHQL_DATA_PATH / expected_output, |
There was a problem hiding this comment.
Fix the expected_file path construction.
The expected_file parameter should use just the filename string (like the expected_output parameter value), not GRAPHQL_DATA_PATH / expected_output. Looking at other tests in this file (e.g., lines 58, 75, 102), they pass the expected filename directly without prefixing it with GRAPHQL_DATA_PATH.
🔎 Proposed fix
run_main_and_assert(
input_path=GRAPHQL_DATA_PATH / "empty_list_default.graphql",
output_path=output_file,
- expected_file=GRAPHQL_DATA_PATH / expected_output,
+ expected_file=expected_output,
input_file_type="graphql",
extra_args=[
"--output-model-type",
output_model,
"--target-python-version",
"3.10",
],
)🤖 Prompt for AI Agents
In @tests/main/graphql/test_main_graphql.py at line 129, The test passes a
Path-prefixed value for expected_file which should instead be the filename
string; replace the argument expected_file=GRAPHQL_DATA_PATH / expected_output
with expected_file=expected_output so expected_file uses the same filename
string as expected_output (refer to the expected_file and expected_output
parameters in the failing test function).
CodSpeed Performance ReportMerging this PR will degrade performance by 18.93%Comparing
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | test_perf_duplicate_names |
842.7 ms | 1,000.1 ms | -15.74% |
| ❌ | WallTime | test_perf_all_options_enabled |
5.8 s | 6.7 s | -13.52% |
| ❌ | WallTime | test_perf_deep_nested |
5.2 s | 6.3 s | -17.91% |
| ❌ | WallTime | test_perf_complex_refs |
1.8 s | 2.1 s | -15.02% |
| ❌ | WallTime | test_perf_large_models_pydantic_v2 |
3.1 s | 3.8 s | -18.93% |
| ❌ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.7 s | 1.9 s | -13.85% |
| ❌ | WallTime | test_perf_stripe_style_pydantic_v2 |
1.7 s | 2 s | -15.15% |
| ❌ | WallTime | test_perf_graphql_style_pydantic_v2 |
705 ms | 821.7 ms | -14.2% |
| ❌ | WallTime | test_perf_multiple_files_input |
3.2 s | 3.9 s | -18.05% |
| ❌ | WallTime | test_perf_openapi_large |
2.5 s | 3 s | -15.96% |
| ❌ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.2 s | 2.7 s | -16.34% |
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. ↩
rpmcginty
force-pushed
the
bugfix/graphql-empty-list-default-bug
branch
from
67079bb to
efbc5d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/data/expected/main/graphql/pydantic_v2_empty_list_default.py:
- Around line 16-17: The generated Pydantic model makes the GraphQL non-nullable
list field nullable by using Optional[List[Container]]; in the PodSpec class
remove Optional so the containers annotation is List[Container] and keep
Field(default_factory=list) to preserve the empty-list default, updating the
containers type hint in the PodSpec model accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/graphql/empty_list_default.graphqlis excluded by!tests/data/**/*.graphqland included by none
📒 Files selected for processing (3)
tests/data/expected/main/graphql/empty_list_default.pytests/data/expected/main/graphql/pydantic_v2_empty_list_default.pytests/main/graphql/test_main_graphql.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/main/graphql/test_main_graphql.py
- tests/data/expected/main/graphql/empty_list_default.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/data/expected/main/graphql/pydantic_v2_empty_list_default.py (1)
tests/data/expected/main/graphql/empty_list_default.py (2)
Container(12-13)PodSpec(16-17)
⏰ 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). (10)
- GitHub Check: 3.12 on macOS
- GitHub Check: 3.12 on Windows
- GitHub Check: 3.10 on macOS
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.13 on macOS
- GitHub Check: 3.13 on Windows
- GitHub Check: benchmarks
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
tests/data/expected/main/graphql/pydantic_v2_empty_list_default.py (2)
1-10: LGTM! Header and imports are correct for Pydantic v2.The generated file header, future annotations import, and Pydantic v2 imports are all appropriate for this test fixture.
12-13: LGTM! Container model is correctly defined.The Container model with an optional name field is properly structured for Pydantic v2.
| class PodSpec(BaseModel): | ||
| containers: Optional[List[Container]] = Field(default_factory=list) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for GraphQL schema files related to empty_list_default
fd -e graphql -e gql . | grep -i empty_list_default || echo "Not found with fd, trying broader search"
# Also try searching for any GraphQL files
fd -e graphql -e gql . | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Search for the test file directory and related GraphQL schemas
find . -type f -name "*empty_list_default*" 2>/dev/null | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 328
🏁 Script executed:
#!/bin/bash
# Look at the test data directory structure
ls -la tests/data/
ls -la tests/data/expected/ 2>/dev/null || ls -la tests/data/Repository: koxudaxi/datamodel-code-generator
Length of output: 941
🏁 Script executed:
#!/bin/bash
# Find the source GraphQL schema files in the tests/data directory
find tests/data -type f -name "*.graphql" | head -20
find tests/data -type f -name "*.gql" | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check the structure of tests/data to find input files
ls -la tests/data/graphql/ 2>/dev/null || find tests/data -type d -name "graphql" | head -5Repository: koxudaxi/datamodel-code-generator
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Search for files matching "empty_list_default" in all formats
find tests/data -type f -name "*empty_list_default*" | xargs ls -laRepository: koxudaxi/datamodel-code-generator
Length of output: 512
🏁 Script executed:
#!/bin/bash
# Search for input GraphQL files in tests/data (not just expected outputs)
find tests/data -name "*.graphql" -o -name "*.gql" | head -30Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check what's in tests/data/python directory structure
ls -la tests/data/python/ 2>/dev/null || echo "No python dir"
find tests/data -maxdepth 2 -type d | sortRepository: koxudaxi/datamodel-code-generator
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Search for references to "empty_list_default" in Python files to find test definitions
rg "empty_list_default" tests/ --type py -B 2 -A 2 | head -60Repository: koxudaxi/datamodel-code-generator
Length of output: 4161
🏁 Script executed:
#!/bin/bash
# Find GRAPHQL_DATA_PATH definition
rg "GRAPHQL_DATA_PATH" tests/main/graphql/test_main_graphql.py -B 2 -A 2Repository: koxudaxi/datamodel-code-generator
Length of output: 9292
🏁 Script executed:
#!/bin/bash
# Search for empty_list_default.graphql anywhere
find . -name "empty_list_default.graphql"Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check tests/data structure for graphql inputs
find tests/data -type f -name "*.graphql" 2>/dev/null | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Find where GRAPHQL_DATA_PATH is defined in conftest
find tests -name "conftest.py" -exec grep -l "GRAPHQL_DATA_PATH" {} \;Repository: koxudaxi/datamodel-code-generator
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Look at the conftest file in tests/main
cat tests/main/conftest.py | head -50Repository: koxudaxi/datamodel-code-generator
Length of output: 1672
🏁 Script executed:
#!/bin/bash
# Search for the definition of GRAPHQL_DATA_PATH
rg "GRAPHQL_DATA_PATH\s*=" tests/ -A 1Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Get more of conftest.py to find GRAPHQL_DATA_PATH
cat tests/main/conftest.py | grep -n "GRAPHQL_DATA_PATH" -B 2 -A 2Repository: koxudaxi/datamodel-code-generator
Length of output: 325
🏁 Script executed:
#!/bin/bash
# Check the entire conftest file size and read it fully
wc -l tests/main/conftest.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Read full conftest to find the definition
sed -n '1,100p' tests/main/conftest.py | tail -50Repository: koxudaxi/datamodel-code-generator
Length of output: 1969
🏁 Script executed:
#!/bin/bash
# Read the GraphQL schema source file
cat tests/data/graphql/empty_list_default.graphqlRepository: koxudaxi/datamodel-code-generator
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Read the expected output file
cat tests/data/expected/main/graphql/pydantic_v2_empty_list_default.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 464
🏁 Script executed:
#!/bin/bash
# Search for the graphql input files directory
find tests/data -type d -name "graphql"
ls -la tests/data/graphql/ 2>/dev/null || echo "Directory not found, searching for graphql files"Repository: koxudaxi/datamodel-code-generator
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Search for any graphql files in tests/data
find tests/data -name "*.graphql" -o -name "*.gql" 2>/dev/nullRepository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Look for python directory with graphql input
ls -la tests/data/python/ 2>/dev/null | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 396
🏁 Script executed:
#!/bin/bash
# Check the test file to understand where the input comes from
grep -A 10 "def test_main_graphql_empty_list_default" tests/main/graphql/test_main_graphql.pyRepository: koxudaxi/datamodel-code-generator
Length of output: 594
🏁 Script executed:
#!/bin/bash
# Search for any references to empty_list_default.graphql in the codebase
rg "empty_list_default" . --type py -B 5 -A 5 | grep -E "(graphql|\.graphql)" | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 1526
🏁 Script executed:
#!/bin/bash
# List all files in tests/data directory recursively to understand structure
find tests/data -type f | head -50Repository: koxudaxi/datamodel-code-generator
Length of output: 2884
🏁 Script executed:
#!/bin/bash
# Check if there's a conftest in tests/main/graphql
cat tests/main/graphql/conftest.py 2>/dev/null || echo "No conftest.py in graphql dir"Repository: koxudaxi/datamodel-code-generator
Length of output: 335
🏁 Script executed:
#!/bin/bash
# Search for where the graphql input files might be or if they're generated
find . -type f -name "*.graphql" 2>/dev/null | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check the git structure - maybe input files are in git but not in the sandbox
git ls-files | grep -i "graphql" | grep -v expected | head -20Repository: koxudaxi/datamodel-code-generator
Length of output: 972
🏁 Script executed:
#!/bin/bash
# Read the GraphQL schema file from git
git show HEAD:tests/data/graphql/empty_list_default.graphqlRepository: koxudaxi/datamodel-code-generator
Length of output: 213
The Optional type should be removed to match the GraphQL schema.
The source GraphQL schema defines containers: [Container!]! = [] (non-nullable list with empty default), but the generated Pydantic code uses Optional[List[Container]] which incorrectly makes the field nullable. The field can accept None at runtime even though the GraphQL schema does not allow null.
Change to:
class PodSpec(BaseModel):
containers: List[Container] = Field(default_factory=list)This correctly reflects that the GraphQL field is non-nullable.
🤖 Prompt for AI Agents
In @tests/data/expected/main/graphql/pydantic_v2_empty_list_default.py around
lines 16 - 17, The generated Pydantic model makes the GraphQL non-nullable list
field nullable by using Optional[List[Container]]; in the PodSpec class remove
Optional so the containers annotation is List[Container] and keep
Field(default_factory=list) to preserve the empty-list default, updating the
containers type hint in the PodSpec model accordingly.
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.