Skip to content

fix: Parsing endpoint content types with semicolon separator #727

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

Conversation

expobrain
Copy link
Collaborator

This PR allows to extract the content type safely removing any extra parameter like charset or boundary.

@codecov
Copy link

codecov bot commented Mar 12, 2023

Codecov Report

Merging #727 (aaffff3) into main (f79bccb) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #727   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           49        49           
  Lines         1971      1978    +7     
=========================================
+ Hits          1971      1978    +7     
Impacted Files Coverage Δ
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)
openapi_python_client/utils.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@emann emann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix, but IMO implementation is a bit heavy handed. I could in theory see us wanting to use the parsing in email to get type/subtype/parameters separately in the future, but better to keep this as simple as possible for now IMO. Open to discussing though!

@expobrain
Copy link
Collaborator Author

Great fix, but IMO implementation is a bit heavy handed. I could in theory see us wanting to use the parsing in email to get type/subtype/parameters separately in the future, but better to keep this as simple as possible for now IMO. Open to discussing though!

The rationale is that there's already a function in the standard library that performs the operation we need and provides the confidence that it does it in the safest and most tested way than us.

@dbanty
Copy link
Collaborator

dbanty commented Mar 28, 2023

It definitely feels weird to use the email module to handle headers.. but the builtin http code is pretty unusable. The RFC space for header parsing is... dizzying, so it's unclear to me whether mail semantics are the same as HTTP in every scenario—however, I took a peek at how httpx deals with this and they also use the email.Message class.

So, given that we trust httpx as our interface to the internet, I think we follow their lead here and keep the implementation that @expobrain wrote.

dbanty
dbanty previously approved these changes Mar 28, 2023
@dbanty dbanty changed the title Fixed parsing endpoint content type with semicolon separator fix: Parsing endpoint content types with semicolon separator Mar 28, 2023
@dbanty dbanty merged commit 4fb9775 into openapi-generators:main Mar 28, 2023
@expobrain expobrain deleted the fix_content_type_with_semicolon_separator branch March 28, 2023 23:18
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.

3 participants