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

Distinguish LIST-only paths in OpenAPI #13643

Merged
merged 3 commits into from
Jan 18, 2022

Conversation

digivava
Copy link
Collaborator

Fixes #12283

Differentiates between paths that only support LIST (including GET w/ list=true query params), and those that support both LIST and plain GET.

According to my testing, at this point in time there are 77 List-only endpoints and 10 that support both.

@digivava digivava requested review from kalafut and ncabatoff January 13, 2022 00:40
@vercel vercel bot temporarily deployed to Preview – vault-storybook January 13, 2022 00:47 Inactive
@vercel vercel bot temporarily deployed to Preview – vault January 13, 2022 00:47 Inactive
Copy link
Contributor

@kalafut kalafut left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll let one of the devs approve. You might want to generate our full openapi spec using this branch and pop it into a validator (e.g. I used to use https://editor.swagger.io/) just to make sure we're spec-compliant.

@digivava
Copy link
Collaborator Author

digivava commented Jan 13, 2022

You might want to generate our full openapi spec using this branch and pop it into a validator (e.g. I used to use https://editor.swagger.io/) just to make sure we're spec-compliant.

Good catch there. It looked fine to me since it was valid json but now that I paste it in there, I see that it's giving me this for every List-only path:

Structural error at paths./ad/library.get.parameters.0
should NOT have additional properties
additionalProperty: enum

The output json looks like this:

      parameters:
        - name: list
          description: Must be set to `true`
          in: query
          schema:
            type: string
          required: true
          enum:
            - 'true'

Should I perhaps remove the newly added enum field and just keep the required: true, since that's still programmatically useful enough to differentiate from a list+read endpoint and makes the validator happy?

@kalafut
Copy link
Contributor

kalafut commented Jan 13, 2022

Are you validating as OpenAPI 3 (I don't recall how that is set in the swagger tool)? Because I think the enum you added looks ok, per https://swagger.io/docs/specification/data-models/enums/ ?

@kalafut
Copy link
Contributor

kalafut commented Jan 13, 2022

Scratch that. I think the enum needs to move under schema.

@vercel vercel bot temporarily deployed to Preview – vault January 13, 2022 21:51 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook January 13, 2022 21:51 Inactive
@digivava digivava merged commit 1bc8fb0 into main Jan 18, 2022
@digivava digivava deleted the digivava/differentiate-list-only-and-list-able branch January 18, 2022 17:21
@peaceofthepai peaceofthepai added this to the 1.10 milestone Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants