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

fix: update http exception's schema construction to include items key… #2999

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nathanielarking
Copy link

An Openapi schema is generated for every HttpException passed into the raises kwargs of a route handler. The schema for the extra attribute of the HttpException currently generates this schema:

extra:
  additionalProperties: {}
  type:
    - 'null'
    - object
    - array

This schema fails to comply with two rules of the IBM openapi validator, ibm-avoid-multiple-types and ibm-array-attributes. This causes sdk generation with Orval to fail.

This PR changes the schema generation for the HttpException to generate this:

extra:
  additionalProperties: {}
  anyOf:
    - type: 'null'
    - type: object
    - items: {}
      type: array

@provinzkraut provinzkraut force-pushed the http-exception-extra-types branch 2 times, most recently from a574f20 to bf04119 Compare February 3, 2024 18:52
@provinzkraut
Copy link
Member

@nathanielarking this looks good to me but it's still in draft mode. Is there any more work you want to do here?

@nathanielarking nathanielarking marked this pull request as ready for review February 4, 2024 00:21
@nathanielarking nathanielarking requested review from a team as code owners February 4, 2024 00:21
@provinzkraut
Copy link
Member

@nathanielarking It seems like our TypeScript generation assumes the faulty schema this PR fixes. Do you feel comfortable fixing this as well?

Additionally, after consulting the OA 3 spec, it seems that a list of types is actually disallowed in general:

type takes a single value. type as a list is not valid in OpenAPI

https://swagger.io/docs/specification/data-models/data-types/

Maybe we should disallow passing a list there in the first place. @guacs?

@guacs
Copy link
Member

guacs commented Feb 5, 2024

This constraint specified in the provided link is for 3.0, but we are using 3.1 which does allow multiple types to be provided for type.

type takes a single value. type as a list is not valid in OpenAPI

The ibm-avoid-multiple-types states that following:

The OpenAPI 3.1 Specification allows multiple types to be used within a schema's type field, but this can create ambiguity in an API definition. Therefore, multiple types should be avoided.

That is, it's part of their linting rules for the spec, but it's not in violation of the spec. However, I feel this is a reasonable rule though and we could default to not using multiple types and instead only provide one type per type field and using anyOf instead.

@nathanielarking
Copy link
Author

Heyo @provinzkraut @guacs, thanks for taking a look at this. I had made the initial commits but didn't have time to look into why the CI was failing and if changes needed to be made to any tests. Is that related to the TS changes mentioned? I'll have time later this week to check things out some more.

@guacs
Copy link
Member

guacs commented Feb 6, 2024

Is that related to the TS changes mentioned?

Yes, those are the tests that are failing.

@github-actions github-actions bot added area/openapi This PR involves changes to the OpenAPI schema area/private-api This PR involves changes to the privatized API size: small type/bug pr/internal labels Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/openapi This PR involves changes to the OpenAPI schema area/private-api This PR involves changes to the privatized API pr/internal size: small type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants