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:
WalkthroughRequest-body parsing now treats Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
Merging this PR will improve performance by 16.31%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_generate_default_template_benchmark |
53.1 ms | 45.7 ms | +16.31% |
Comparing fix-multiple-file-upload (a00be0e) with main (88b72cd)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
📚 Docs Preview: https://pr-553.fastapi-code-generator.pages.dev |
![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
🤖 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 535-559: The helpers _get_upload_file_type_hint,
_has_upload_file_array and _is_upload_file_schema currently hardcode the form
parameter name to "file" and mix Pydantic v1/v2 APIs; update
_get_upload_file_type_hint to derive the parameter name from the schema when the
schema is an object with properties (e.g., use the property key such as "images"
when schema.properties exists, falling back to "file" only if no property name
is available), update any code that converts ReferenceObject to JsonSchemaObject
to consistently use JsonSchemaObject.model_validate(...) (replace parse_obj
usages to align with model_validate), and ensure _has_upload_file_array inspects
property keys so the generator uses the actual property name when producing
List[UploadFile] vs UploadFile; reference functions: _get_upload_file_type_hint,
_has_upload_file_array, _is_upload_file_schema, and the place where
ReferenceObject is converted to JsonSchemaObject.
🪄 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: 67375519-9a2b-483a-ad41-444ea7970c7b
📒 Files selected for processing (3)
fastapi_code_generator/parser.pytests/data/expected/openapi/default_template/upload/main.pytests/data/openapi/default_template/upload.yaml
koxudaxi
force-pushed
the
fix-multiple-file-upload
branch
3 times, most recently
from
2da9d10 to
d7053b6
Compare
koxudaxi
force-pushed
the
fix-multiple-file-upload
branch
4 times, most recently
from
2044806 to
96662fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 577-584: _update _get_upload_file_name currently only returns a
property when _is_upload_file_array(property_schema) is true, which skips single
binary properties; modify _get_upload_file_name to also detect plain binary
properties (e.g., property_schema.type == "string" and property_schema.format ==
"binary" or via a small helper _is_binary_property) and return
stringcase.snakecase(self.model_resolver.get_valid_field_name(property_name))
for those as well; also update _get_upload_file_type so when the selected
property_schema represents a single binary (not an array) it yields UploadFile
(matching the array case) and ensure you pick a consistent strategy for multiple
file properties (e.g., return the first matching file property but avoid
silently dropping others by documenting or adding a separate handling path).
- Around line 497-501: The current check only skips parsing when the entire
content set equals {'multipart/form-data'}, so mixed content types still allow
generation of multipart fields; update the parse_request_body handling in
parser.parse_request_body so that if 'multipart/form-data' is present you strip
it out before delegating: if removing 'multipart/form-data' leaves no media
types return {} (same as pure-multipart case), otherwise call
super().parse_request_body with a temporary request_body that has
request_body.content filtered to exclude 'multipart/form-data'. Ensure you
reference the existing request_body_fields assignment and use
request_body.content and parse_request_body when implementing the change.
In `@fastapi_code_generator/visitors/imports.py`:
- Around line 25-38: The code calls a non-existent Imports.remove_unused which
will raise AttributeError; instead compute the used identifier set via
_collect_used_names (using IDENTIFIER_PATTERN against operation.arguments,
return_type, response, additional_responses, callbacks) and then prune the
Imports instance by iterating its tracked imports and calling Imports.remove for
any import name not in that used set (or use Imports.extract_future separately
for from __future__ entries); update the call site to use this explicit removal
loop on the Imports object rather than remove_unused.
🪄 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: 328c7d6e-7162-4166-8659-4f374127bf27
📒 Files selected for processing (7)
fastapi_code_generator/parser.pyfastapi_code_generator/visitors/imports.pytests/data/expected/openapi/coverage/callbacks/main.pytests/data/expected/openapi/coverage/callbacks_with_operation_id/main.pytests/data/expected/openapi/default_template/upload/main.pytests/data/openapi/default_template/upload.yamltests/test_parser.py
✅ Files skipped from review due to trivial changes (2)
- tests/data/expected/openapi/coverage/callbacks/main.py
- tests/data/expected/openapi/coverage/callbacks_with_operation_id/main.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #553 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 17 +1
Lines 1117 1199 +82
Branches 111 126 +15
=========================================
+ Hits 1117 1199 +82
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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_parser.py (2)
42-47: Consider broadening the non-schema rejection coverage.The test only exercises the boolean
True. Since_is_upload_file_arrayis supposed to returnFalsefor any non-JsonSchemaObjectinput, parametrizing across a representative set (e.g.,None,"str",123,{},[]) would more robustly guard the contract.♻️ Optional parametrization
-def test_is_upload_file_array_rejects_non_schema() -> None: - parser = OpenAPIParser( - "openapi: 3.0.0\ninfo: {title: Test, version: '1.0'}\npaths: {}\n" - ) - - assert parser._is_upload_file_array(True) is False +@pytest.mark.parametrize("value", [True, None, "string", 123, {}, []]) +def test_is_upload_file_array_rejects_non_schema(value) -> None: + parser = OpenAPIParser( + "openapi: 3.0.0\ninfo: {title: Test, version: '1.0'}\npaths: {}\n" + ) + + assert parser._is_upload_file_array(value) is False(Requires
import pytest.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_parser.py` around lines 42 - 47, Update the test_is_upload_file_array_rejects_non_schema to cover a broader set of non-JsonSchemaObject inputs by parametrizing it (e.g., values None, "str", 123, {}, []); call parser._is_upload_file_array with each value and assert it returns False. Use pytest.mark.parametrize to iterate the inputs so OpenAPIParser._is_upload_file_array is validated against multiple non-schema types rather than only True.
9-39: LGTM on the reference-resolution test.The test cleanly exercises the new
multipart/form-datapath: aReferenceObjectis resolved throughget_ref_model, the array-of-binary property is detected, and both the derived field name ("images") andList[UploadFile]type hint are asserted. Good coverage of the regression that motivated the PR.One small optional addition: you could also assert that an
Import(from_='typing', import_='List')was appended toparser.imports_for_fastapi, since that side effect is part of_get_upload_file_type's contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_parser.py` around lines 9 - 39, Add an assertion to the test_get_upload_file_type_resolves_reference test to verify the side-effect that _get_upload_file_type appends the typing import; after calling parser._get_upload_file_type (and before final asserts), check that parser.imports_for_fastapi contains an Import object for List (e.g., Import(from_='typing', import_='List')) so the test validates both the returned type_hint and the required import being recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_parser.py`:
- Around line 42-47: Update the test_is_upload_file_array_rejects_non_schema to
cover a broader set of non-JsonSchemaObject inputs by parametrizing it (e.g.,
values None, "str", 123, {}, []); call parser._is_upload_file_array with each
value and assert it returns False. Use pytest.mark.parametrize to iterate the
inputs so OpenAPIParser._is_upload_file_array is validated against multiple
non-schema types rather than only True.
- Around line 9-39: Add an assertion to the
test_get_upload_file_type_resolves_reference test to verify the side-effect that
_get_upload_file_type appends the typing import; after calling
parser._get_upload_file_type (and before final asserts), check that
parser.imports_for_fastapi contains an Import object for List (e.g.,
Import(from_='typing', import_='List')) so the test validates both the returned
type_hint and the required import being recorded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3bc065f-fef2-415b-9e6f-aa37135f0029
📒 Files selected for processing (7)
fastapi_code_generator/parser.pyfastapi_code_generator/visitors/imports.pytests/data/expected/openapi/coverage/callbacks/main.pytests/data/expected/openapi/coverage/callbacks_with_operation_id/main.pytests/data/expected/openapi/default_template/upload/main.pytests/data/openapi/default_template/upload.yamltests/test_parser.py
✅ Files skipped from review due to trivial changes (2)
- tests/data/expected/openapi/coverage/callbacks/main.py
- tests/data/expected/openapi/coverage/callbacks_with_operation_id/main.py
🚧 Files skipped from review as they are similar to previous changes (2)
- fastapi_code_generator/visitors/imports.py
- fastapi_code_generator/parser.py
koxudaxi
force-pushed
the
fix-multiple-file-upload
branch
2 times, most recently
from
c5c914d to
09d0eb7
Compare
koxudaxi
force-pushed
the
fix-multiple-file-upload
branch
from
09d0eb7 to
a00be0e
Compare
github-actions
Bot
added
the
breaking-change-analyzed
Breaking Change AnalysisResult: No breaking changes detected Reasoning: The This analysis was performed by repository automation using PR labels and the |
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores
Tests