Skip to content

Support multiple bodies #900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Dec 31, 2023
Merged

Support multiple bodies #900

merged 17 commits into from
Dec 31, 2023

Conversation

dbanty
Copy link
Collaborator

@dbanty dbanty commented Dec 7, 2023

It's possible for an endpoint to allow multiple content-types. In this case, the caller should select one at runtime, rather than one being picked arbitrarily.

This is a breaking change because instead of separate named params per type of body, there is only one body parameter (to better reflect that a choice is required).

Closes #822

@kgutwin
Copy link
Contributor

kgutwin commented Dec 7, 2023

Thanks! I think this approach makes a lot of sense. I actually ran into this problem just yesterday; my API has a method that accepts both application/json and multipart/form-data in the same endpoint, and I needed to do an ugly workaround to make it work.

One thought is that you could still support the old parameters with a signature like:

from typing import override

@override
def _get_kwargs(
    other_param: str,
    *,
    multipart_data: FormStyleBody,
) -> Dict[str, Any]:
    ...
@override
def _get_kwargs(
    other_param: str,
    *,
    json_body: JsonStyleBody,
) -> Dict[str, Any]:
    ...
@override
def _get_kwargs(
    other_param: str,
    *,
    body: Union[FormStyleBody, JsonStyleBody],
) -> Dict[str, Any]:
    ....

def _get_kwargs(
    other_param: str,
    *,
    multipart_data: Optional[FormStyleBody] = None,
    json_body: Optional[JsonStyleBody] = None,
    body: Optional[Union[FormStyleBody, JsonStyleBody]] = None,
) -> Dict[str, Any]:
    # code
    pass

This would allow the older parameters to be deprecated without needing to have it be a breaking change.

# Conflicts:
#	end_to_end_tests/golden-record/my_test_api_client/api/tests/octet_stream_tests_octet_stream_post.py
#	openapi_python_client/parser/openapi.py
#	openapi_python_client/templates/endpoint_macros.py.jinja
#	openapi_python_client/templates/endpoint_module.py.jinja
#	tests/test_parser/test_openapi.py
Copy link

codecov bot commented Dec 16, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (f0cf8d2) 100.00% compared to head (15b2c5d) 99.38%.
Report is 4 commits behind head on main.

❗ Current head 15b2c5d differs from pull request most recent head f17b699. Consider uploading reports for the commit f17b699 to get more accurate results

Files Patch % Lines
openapi_python_client/parser/bodies.py 86.66% 8 Missing ⚠️
openapi_python_client/parser/openapi.py 81.81% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #900      +/-   ##
===========================================
- Coverage   100.00%   99.38%   -0.62%     
===========================================
  Files           49       50       +1     
  Lines         1940     1948       +8     
===========================================
- Hits          1940     1936       -4     
- Misses           0       12      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbanty
Copy link
Collaborator Author

dbanty commented Dec 16, 2023

This would allow the older parameters to be deprecated without needing to have it be a breaking change.

I'd also have to keep the old class names, which were inconsistent. Or generate two copies of the classes. Maybe if the deprecation story in Python was better (i.e., provided in-IDE help or could give a warning through type checkers) it'd feel more worth it, but I'm not sure it's worth the hassle.

I'm going to bundle this with OpenAPI 3.1 support I think which will make for a big "use care when updating" release anyway, since things could start breaking in unexpected ways.

@dbanty dbanty marked this pull request as ready for review December 31, 2023 22:52
@dbanty dbanty merged commit 86f96dd into main Dec 31, 2023
@dbanty dbanty deleted the support-multiple-bodies branch December 31, 2023 23:29
dbanty added a commit that referenced this pull request Dec 31, 2023
This PR was created by Knope. Merging it will create a new release

### Breaking Changes

#### Removed query parameter nullable/required special case

In previous versions, setting _either_ `nullable: true` or `required:
false` on a query parameter would act like both were set, resulting in a
type signature like `Union[None, Unset, YourType]`. This special case
has been removed, query parameters will now act like all other types of
parameters.

#### Renamed body types and parameters

PR #900 addresses #822.

Where previously there would be one body parameter per supported content
type, now there is a single `body` parameter which takes a union of all
the possible inputs. This correctly models the fact that only one body
can be sent (and ever would be sent) in a request.

For example, when calling a generated endpoint, code which used to look
like this:

```python
post_body_multipart.sync_detailed(
    client=client,
    multipart_data=PostBodyMultipartMultipartData(),
)
```

Will now look like this:

```python
post_body_multipart.sync_detailed(
    client=client,
    body=PostBodyMultipartBody(),
)
```

Note that both the input parameter name _and_ the class name have
changed. This should result in simpler code when there is only a single
body type and now produces correct code when there are multiple body
types.

### Features

#### OpenAPI 3.1 support

The generator will now attempt to generate code for OpenAPI documents
with versions 3.1.x (previously, it would exit immediately on seeing a
version other than 3.0.x). The following specific OpenAPI 3.1 features
are now supported:

- `null` as a type
- Arrays of types (e.g., `type: [string, null]`)
- `const` (defines `Literal` types)

The generator does not currently validate that the OpenAPI document is
valid for a specific version of OpenAPI, so it may be possible to
generate code for documents that include both removed 3.0 syntax (e.g.,
`nullable`) and new 3.1 syntax (e.g., `null` as a type).

Thanks to everyone who helped make this possible with discussions and
testing, including:

- @frco9
- @vogre
- @naddeoa
- @staticdev
- @philsturgeon
- @johnthagen

#### Support multiple possible `requestBody`

PR #900 addresses #822.

It is now possible in some circumstances to generate valid code for
OpenAPI documents which have multiple possible `requestBody` values.
Previously, invalid code could have been generated with no warning (only
one body could actually be sent).

Only one content type per "category" is currently supported at a time.
The categories are:

- JSON, like `application/json`
- Binary data, like `application/octet-stream`
- Encoded form data, like `application/x-www-form-urlencoded`
- Files, like `multipart/form-data`

### Fixes

#### Always use correct content type for requests

In previous versions, a request body that was similar to a known content
type would use that content type in the request. For example
`application/json` would be used for `application/vnd.api+json`. This
was incorrect and could result in invalid requests being sent.

Now, the content type defined in the OpenAPI document will always be
used.

Co-authored-by: GitHub <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants