Fix optional bodies #1365
Fix optional bodies #1365
Conversation
6c9c6dd to
a94f570
Compare
a94f570 to
fefc780
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes the handling of optional request bodies in the OpenAPI Python client generator. Previously, optional bodies were incorrectly treated as required, causing them to always be included in requests. The fix ensures that optional bodies (where requestBody.required is false or missing) are properly typed with Unset, default to UNSET, and are only included in requests when explicitly provided.
Key Changes:
- Modified body parsing to respect the
requiredfield from request body definitions - Updated all property transform templates to support a
skip_unsetparameter for conditional UNSET initialization - Added deduplication of union types to prevent duplicate type members
- Generated code now properly handles optional bodies across different content types (JSON, multipart, octet-stream, etc.)
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| openapi_python_client/parser/bodies.py | Passes the actual body.required value instead of hardcoding True |
| openapi_python_client/parser/properties/union.py | Adds deduplication logic to remove duplicate types from unions and uses Iterator for better memory efficiency |
| openapi_python_client/templates/property_templates/*.py.jinja | Adds skip_unset parameter to transform macros to conditionally skip UNSET initialization |
| openapi_python_client/templates/endpoint_macros.py.jinja | Updates body handling to check if bodies are optional and conditionally include them in requests |
| openapi_python_client/templates/endpoint_module.py.jinja | Uses no_optional=True for isinstance checks with multiple bodies |
| end_to_end_tests/golden-record/* | Updated golden records showing correct handling of optional bodies with Unset types |
| end_to_end_tests/baseline_openapi_. | Added test cases for optional bodies and duplicate union types |
| tests/test_parser/test_properties/test_union.py | Updated test to verify different property types in unions |
| .changeset/fix_optional_bodies.md | Documents the fix for optional bodies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{ destination }} = {{ source }}.to_tuple() | ||
| {% else %} | ||
| {{ destination }}{% if declare_type %}: {{ property.get_type_string(json=True) }}{% endif %} = UNSET | ||
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ property.get_type_string(json=True) }}{% endif %} = UNSET{% endif +%} |
There was a problem hiding this comment.
The {% endif +%} syntax is invalid in Jinja2. The + should be part of the closing tag as {%+ endif %} or simply removed as {% endif %}. The + is a whitespace control character that should precede the tag name.
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ property.get_type_string(json=True) }}{% endif %} = UNSET{% endif +%} | |
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ property.get_type_string(json=True) }}{% endif %} = UNSET{% endif %} |
| {{ destination }} = {{ transformed }} | ||
| {%- else %} | ||
| {{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET | ||
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif +%} |
There was a problem hiding this comment.
The {% endif +%} syntax is invalid in Jinja2. The + should be part of the closing tag as {%+ endif %} or simply removed as {% endif %}. The + is a whitespace control character that should precede the tag name.
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif +%} | |
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif %} |
| {% if endpoint.bodies | length == 1 %} | ||
| body: {{ endpoint.bodies[0].prop.get_type_string() }}, | ||
| body: {{ endpoint.bodies[0].prop.get_type_string() }}{% if not endpoint.bodies[0].prop.required %} = UNSET{% endif %}, | ||
| {% elif endpoint.bodies | length > 1 %} |
There was a problem hiding this comment.
The variable body_required is used without being initialized. This will cause a runtime error when there are multiple bodies. It should be initialized before the loop, e.g., {% set body_required = true %} before line 126.
| {% elif endpoint.bodies | length > 1 %} | |
| {% elif endpoint.bodies | length > 1 %} | |
| {% set body_required = true %} |
| {{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = {{ source }} | ||
| {%- else %} | ||
| {{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET | ||
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif +%} |
There was a problem hiding this comment.
The {% endif +%} syntax is invalid in Jinja2. The + should be part of the closing tag as {%+ endif %} or simply removed as {% endif %}. The + is a whitespace control character that should precede the tag name.
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif +%} | |
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif %} |
| {{ _transform(property, source, destination, "to_dict") }} | ||
| {% else %} | ||
| {{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET | ||
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif +%} |
There was a problem hiding this comment.
The {% endif +%} syntax is invalid in Jinja2. The + should be part of the closing tag as {%+ endif %} or simply removed as {% endif %}. The + is a whitespace control character that should precede the tag name.
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif +%} | |
| {% if not skip_unset %}{{ destination }}{% if declare_type %}: {{ type_string }}{% endif %} = UNSET{% endif %} |
| import types | ||
|
|
||
| from . import json_like, post_bodies_multiple, refs | ||
| from . import json_like, optional_body, post_bodies_multiple, refs |
There was a problem hiding this comment.
The module 'api.bodies' imports itself.
Reopening #1357