Merged
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughOverrides field-extras computation to remove generated Pydantic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 6/8 reviews remaining, refill in 10 minutes and 53 seconds.Comment |
Contributor
|
📚 Docs Preview: https://pr-566.fastapi-code-generator.pages.dev |
Merging this PR will not alter performance
|
koxudaxi
force-pushed
the
issue-480-discriminated-union-simple-types
branch
2 times, most recently
from
56bc4f6 to
69e8f95
Compare
![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 the current code and only fix it if needed.
Inline comments:
In `@fastapi_code_generator/parser.py`:
- Around line 584-598: The current heuristic in _is_object_discriminator_variant
treats any schema with allOf as object-shaped, which misclassifies primitives
wrapped in allOf; update _is_object_discriminator_variant (and by extension how
_has_non_object_discriminator_variant uses it) to inspect allOf contents rather
than assuming object-ness: when schema.allOf is present, recursively examine the
allOf member schemas (resolving refs via
get_ref_model/JsonSchemaObject.model_validate) and only treat the composed
schema as object-shaped if at least one member is object-like (schema.is_object
or schema.properties) or if recursive allOf members themselves contain
object-like subschemas; otherwise do not classify it as an object discriminator
variant. Ensure this logic is applied in _is_object_discriminator_variant and
retained by _has_non_object_discriminator_variant.
In `@tests/main/test_main.py`:
- Around line 230-241: The extra exec() call in
test_generate_discriminated_union_with_simple_type is redundant because
run_cli_and_assert already calls validate_generated_code which compiles the
generated files; remove the
exec(output_dir.joinpath("models.py").read_text(encoding="utf-8"), {}) line, or
if you specifically want to ensure module-level import semantics rather than
simple compilation, replace that exec usage with an explicit module load via
importlib.util.spec_from_file_location / module_from_spec to load the generated
models.py file; update the test to use either no additional check or the
importlib-based load and reference the symbols to ensure imports resolve.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff33acaf-feb2-4eb7-a829-d66f94271dbd
📒 Files selected for processing (5)
fastapi_code_generator/parser.pytests/data/expected/openapi/default_template/discriminated_union_simple_type/main.pytests/data/expected/openapi/default_template/discriminated_union_simple_type/models.pytests/data/openapi/default_template/discriminated_union_simple_type.yamltests/main/test_main.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/main/test_main.py (1)
241-241:⚠️ Potential issue | 🟠 MajorAvoid
execin test; it likely fails lint and is unnecessary.At Line 241,
exec(...)triggers RuffS102. Since this test already goes throughrun_cli_and_assert(...), dropexec(or switch to explicit importlib loading if you need module-import semantics).Suggested fix
def test_generate_discriminated_union_with_simple_type(output_dir: Path) -> None: run_cli_and_assert( input_path=DATA_PATH / OPEN_API_DEFAULT_TEMPLATE_DIR_NAME / "discriminated_union_simple_type.yaml", output_path=output_dir, expected_path=EXPECTED_OPENAPI_PATH / "default_template" / "discriminated_union_simple_type", ) - exec(output_dir.joinpath("models.py").read_text(encoding="utf-8"), {})#!/bin/bash set -euo pipefail # Verify whether run_cli_and_assert already validates generated code rg -n -C3 "def run_cli_and_assert|validate_generated_code" tests/main tests/conftest.py tests/main/conftest.py rg -n -C2 "exec\(" tests/main/test_main.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/main/test_main.py` at line 241, The test uses exec(output_dir.joinpath("models.py").read_text(...), {}) which triggers lint S102 and is unnecessary because run_cli_and_assert already exercises generated code; remove the exec call from the test (or replace it with safe import semantics using importlib.util.spec_from_file_location -> module = importlib.util.module_from_spec -> spec.loader.exec_module(module) only if you truly need module import behavior). Update the test to rely on run_cli_and_assert (and its validation helpers) and reference the exec removal around the exec(...) invocation and any assertions that depended on it.fastapi_code_generator/parser.py (1)
595-598:⚠️ Potential issue | 🟡 Minor
allOfis currently over-classified as object-shaped.At Line 598, any non-empty
allOfreturns truthy, so primitive-onlyallOfbranches can be treated as valid discriminator variants. That can leavediscriminatorattached in mixed unions and reintroduce the runtime Pydantic error this PR fixes.Suggested fix
def _is_object_discriminator_variant(self, schema: JsonSchemaObject) -> bool: if schema.ref: schema = JsonSchemaObject.model_validate(self.get_ref_model(schema.ref)) - return bool(schema.is_object or schema.properties or schema.allOf) + if schema.is_object or schema.properties: + return True + if not schema.allOf: + return False + return any(self._is_object_discriminator_variant(member) for member in schema.allOf)#!/bin/bash set -euo pipefail # Verify existing fixture coverage for discriminator + allOf + primitive variants fd -e yaml -e yml -e json tests/data/openapi | while read -r f; do if rg -n "discriminator" "$f" >/dev/null && rg -n "allOf" "$f" >/dev/null; then echo "---- $f" rg -n -C2 "discriminator|allOf|type:\s*(string|integer|number|boolean|null)" "$f" fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fastapi_code_generator/parser.py` around lines 595 - 598, The helper _is_object_discriminator_variant currently treats any non-empty schema.allOf as object-shaped; change it to inspect allOf's members and return true only if at least one constituent is object-shaped (i.e., has is_object, properties, or nested allOf, or a ref that resolves to an object). Update _is_object_discriminator_variant to, when schema.allOf is present, iterate over schema.allOf entries, resolve refs via self.get_ref_model and JsonSchemaObject.model_validate where needed, and only return True if any member meets the object-like checks; otherwise treat the allOf as non-object-shaped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fastapi_code_generator/parser.py`:
- Around line 595-598: The helper _is_object_discriminator_variant currently
treats any non-empty schema.allOf as object-shaped; change it to inspect allOf's
members and return true only if at least one constituent is object-shaped (i.e.,
has is_object, properties, or nested allOf, or a ref that resolves to an
object). Update _is_object_discriminator_variant to, when schema.allOf is
present, iterate over schema.allOf entries, resolve refs via self.get_ref_model
and JsonSchemaObject.model_validate where needed, and only return True if any
member meets the object-like checks; otherwise treat the allOf as
non-object-shaped.
In `@tests/main/test_main.py`:
- Line 241: The test uses exec(output_dir.joinpath("models.py").read_text(...),
{}) which triggers lint S102 and is unnecessary because run_cli_and_assert
already exercises generated code; remove the exec call from the test (or replace
it with safe import semantics using importlib.util.spec_from_file_location ->
module = importlib.util.module_from_spec -> spec.loader.exec_module(module) only
if you truly need module import behavior). Update the test to rely on
run_cli_and_assert (and its validation helpers) and reference the exec removal
around the exec(...) invocation and any assertions that depended on it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 784c2f67-bc49-48a4-816a-8ac1f4efc222
📒 Files selected for processing (8)
docs/llms-full.txtdocs/supported_formats.mdfastapi_code_generator/parser.pyfastapi_code_generator/prompt_data.pytests/data/expected/openapi/default_template/discriminated_union_simple_type/main.pytests/data/expected/openapi/default_template/discriminated_union_simple_type/models.pytests/data/openapi/default_template/discriminated_union_simple_type.yamltests/main/test_main.py
✅ Files skipped from review due to trivial changes (5)
- docs/supported_formats.md
- docs/llms-full.txt
- tests/data/expected/openapi/default_template/discriminated_union_simple_type/main.py
- fastapi_code_generator/prompt_data.py
- tests/data/expected/openapi/default_template/discriminated_union_simple_type/models.py
koxudaxi
force-pushed
the
issue-480-discriminated-union-simple-types
branch
2 times, most recently
from
6e3cdb0 to
efe19b0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 1285 1344 +59
Branches 131 139 +8
=========================================
+ Hits 1285 1344 +59
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:
|
koxudaxi
force-pushed
the
issue-480-discriminated-union-simple-types
branch
2 times, most recently
from
69acf2e to
f0c760c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fastapi_code_generator/parser.py (1)
587-593: Consider evaluating bothoneOfandanyOfwhen both are present.Line 587 currently picks
oneOffirst (obj.oneOf or obj.anyOf), which can skipanyOfvariants if both arrays are set.♻️ Proposed hardening
- combined_schemas = obj.oneOf or obj.anyOf + combined_schemas = [*(obj.oneOf or []), *(obj.anyOf or [])] if not combined_schemas: return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fastapi_code_generator/parser.py` around lines 587 - 593, The current logic uses combined_schemas = obj.oneOf or obj.anyOf which ignores anyOf when oneOf is present; change it to aggregate both arrays (e.g., treat missing as empty lists and concatenate oneOf and anyOf) before checking variants so _is_object_discriminator_variant runs against all combined schemas; update the variable combined_schemas and keep the subsequent any(... for schema in combined_schemas) and reference to _is_object_discriminator_variant unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fastapi_code_generator/parser.py`:
- Around line 595-604: The _is_object_discriminator_variant function can recurse
infinitely through cyclic allOf/$ref chains; modify it to track visited
schemas/refs (e.g., a set of ref strings or ids) and short-circuit when
encountering a previously seen schema to prevent unbounded recursion. Update
_is_object_discriminator_variant (and any internal recursive call) to accept an
optional visited parameter (or use a helper wrapper) and mark the current
schema.ref or id before recursing into get_ref_model() and schema.allOf members,
returning False (or safe default) when a cycle is detected; keep existing logic
for non-cyclic cases and continue using get_ref_model to resolve refs. Ensure
the visited check references unique identifiers (schema.ref or id(schema)) so
cycles are reliably detected.
---
Nitpick comments:
In `@fastapi_code_generator/parser.py`:
- Around line 587-593: The current logic uses combined_schemas = obj.oneOf or
obj.anyOf which ignores anyOf when oneOf is present; change it to aggregate both
arrays (e.g., treat missing as empty lists and concatenate oneOf and anyOf)
before checking variants so _is_object_discriminator_variant runs against all
combined schemas; update the variable combined_schemas and keep the subsequent
any(... for schema in combined_schemas) and reference to
_is_object_discriminator_variant unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01934ce9-e38b-4a33-90ff-dcd2f8c030e7
📒 Files selected for processing (9)
docs/llms-full.txtdocs/supported_formats.mdfastapi_code_generator/parser.pyfastapi_code_generator/prompt_data.pytests/data/expected/openapi/default_template/discriminated_union_simple_type/main.pytests/data/expected/openapi/default_template/discriminated_union_simple_type/models.pytests/data/openapi/default_template/discriminated_union_simple_type.yamltests/main/test_main.pytests/test_parser.py
✅ Files skipped from review due to trivial changes (5)
- docs/supported_formats.md
- docs/llms-full.txt
- fastapi_code_generator/prompt_data.py
- tests/data/expected/openapi/default_template/discriminated_union_simple_type/main.py
- tests/data/expected/openapi/default_template/discriminated_union_simple_type/models.py
koxudaxi
force-pushed
the
issue-480-discriminated-union-simple-types
branch
from
f0c760c to
93ca45d
Compare
…criminated-union-simple-types
github-actions
Bot
added
the
breaking-change-analyzed
Contributor
Breaking Change AnalysisResult: No breaking changes detected Reasoning: The This analysis was performed by repository automation using PR labels and the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #480
Summary
oneOforanyOfunion includes non-object variants such asstring.allOfvariants.allOfdiscriminator inspection against cyclic refs.Checks
tox -e typetox -e py314-parallel -- tests/test_parser.py::test_get_field_extras_preserves_non_union_discriminator tests/test_parser.py::test_get_field_extras_removes_discriminator_for_all_of_simple_variant tests/test_parser.py::test_get_field_extras_preserves_discriminator_for_all_of_object_variant tests/test_parser.py::test_get_field_extras_removes_discriminator_for_cyclic_all_of_ref tests/main/test_main.py::test_generate_discriminated_union_with_simple_typetox -e schema-docs -- --checktox -e cli-docs -- --checktox -e llms-txt -- --checktox -e fixSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation