Skip to content
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

Refine 'RefTo' and 'ContentTypeFormat' #21

Merged
merged 1 commit into from Mar 30, 2021
Merged

Conversation

wy-z
Copy link
Contributor

@wy-z wy-z commented Mar 30, 2021

Signed-off-by: weiyang weiyang.ones@gmail.com

Hi friends, first of all, thank you for your excellent work.

I encountered two problems while using openapi-type:

  1. Unrecognised reference location "requestBodies"
  2. Unsupported Media Type format "multipart/form-data"

To solve these problems I have made the following changes:

  1. Any content type that conforms to RFC6838 is valid, enumerating all content types is too expensive, so I changed the type of ContentTypeFormat to str.
    Related documents: Media Types.
  2. Added ["examples", "requestBodies", "securitySchemes", "callbacks"] to 'RefTo'.
    All fixed fields can be found at Fixed Fields.

Signed-off-by: weiyang <weiyang.ones@gmail.com>
@wy-z
Copy link
Contributor Author

wy-z commented Mar 30, 2021

Thank you for your excellent work again.
I'd really appreciate it if you can spare the time to review this PR.
@avanov

@coveralls
Copy link

Pull Request Test Coverage Report for Build 701801266

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 78.73%

Totals Coverage Status
Change from base Build 579270847: 0.2%
Covered Lines: 248
Relevant Lines: 315

💛 - Coveralls

@avanov
Copy link
Owner

avanov commented Mar 30, 2021

Thanks for the PR!

RefTo change LGTM. And I'm totally with you regarding the current restrictive nature of ContentTypeFormat and I can merge the newtype variant of it. However I cannot guarantee you that I will keep it around for long afterwards.

A free-form text representation of media types is problematic in downstream code generators that utilise the python type to match unique traits of the media types (like https://github.com/avanov/openapi-client-generator/blob/a058af4ec28a1e53809273a662fb8cba0157695e/openapi_client_generator/transformers/__init__.py#L659), like them being markers of a plain JSON response or a binary stream. As there's a whole range of prefixes and suffixes that still belong to the same common traits like JSON responses and streamed data, the media type representation should carry this additional information in the type to facilitate downstream libraries that rely on exact matching of the mentioned traits, and therefore I cannot keep it as a free-form string for long.

RFC6838 suggests it too - the parser should be aware that the media type string is not exactly a free-form string - it is formatted according to strict rules, and hence all ill-formated representations of a media type should be rejected.

I'm happy to merge your PR now to unblock your use cases, but the free-form string representation of it will be switched for a more precise parser at some point.

@wy-z
Copy link
Contributor Author

wy-z commented Mar 30, 2021

Thanks for the PR!

RefTo change LGTM. And I'm totally with you regarding the current restrictive nature of ContentTypeFormat and I can merge the newtype variant of it. However I cannot guarantee you that I will keep it around for long afterwards.

A free-form text representation of media types is problematic in downstream code generators that utilise the python type to match unique traits of the media types (like https://github.com/avanov/openapi-client-generator/blob/a058af4ec28a1e53809273a662fb8cba0157695e/openapi_client_generator/transformers/__init__.py#L659), like them being markers of a plain JSON response or a binary stream. As there's a whole range of prefixes and suffixes that still belong to the same common traits like JSON responses and streamed data, the media type representation should carry this additional information in the type to facilitate downstream libraries that rely on exact matching of the mentioned traits, and therefore I cannot keep it as a free-form string for long.

RFC6838 suggests it too - the parser should be aware that the media type string is not exactly a free-form string - it is formatted according to strict rules, and hence all ill-formated representations of a media type should be rejected.

I'm happy to merge your PR now to unblock your use cases, but the free-form string representation of it will be switched for a more precise parser at some point.

That's great, thank you again for your excellent work.
Best wishes.

@avanov avanov merged commit db07535 into avanov:develop Mar 30, 2021
@avanov
Copy link
Owner

avanov commented Mar 30, 2021

No problem! I will publish a new version of the package a bit later this week.

avanov pushed a commit that referenced this pull request Apr 29, 2021
avanov added a commit that referenced this pull request Apr 29, 2021
* Refine 'RefTo' and 'ContentTypeFormat' (#21)

* Release 0.1.0

Co-authored-by: weiyang <weiyang.ones@gmail.com>
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.

None yet

3 participants