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.

Sign up for GitHub

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

Jump to bottom

Swagger fixes for MultiVerb and ErrorDescription #1707

Merged
merged 6 commits into from
Aug 19, 2021
Merged

Swagger fixes for MultiVerb and ErrorDescription #1707

merged 6 commits into from
Aug 19, 2021

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Aug 13, 2021 •
edited

This PR fixes a few issues with Swagger generation for MultiVerb endpoints containing ErrorDescription responses. Namely:

  • Responses with the same status code were overwriting each other, and only one was visible in Swagger.
  • ErrorDescription schemas were referenced, resulting in only one possible schema for all errors.
  • There was some code duplication between the Swagger instances for CanThrow and MultiVerb.

These should all be fixed now. Here is the diff of the swagger JSON before and after the changes.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • Section Unreleased of CHANGELOG.md contains the following bits of information:
    • A line with the title and number of the PR in one or more suitable sub-sections.

@pcapriotti pcapriotti force-pushed the pcapriotti/multiverb-combine-v2 branch from 835aa11 to 94df354 Compare August 13, 2021 12:18
@pcapriotti pcapriotti marked this pull request as ready for review August 13, 2021 12:23
@fisx
Copy link
Contributor

fisx commented Aug 13, 2021

should this go into this release?

@pcapriotti
Copy link
Contributor Author

should this go into this release?

I don't know. It's nothing particularly critical, just Swagger. It probably won't break anything else either.

@mdimjasevic
Copy link
Contributor

mdimjasevic commented Aug 13, 2021 •
edited

Seems to work for me! Thanks for this PR!

multiple-error-description-responses

@mdimjasevic mdimjasevic mentioned this pull request Aug 13, 2021
Merged
10 tasks
@pcapriotti
Combine MultiVerb swaggers into a map
b320471
@pcapriotti
Make ErrorDescription schemas inline
5e85099 
This fixes an issue with MultiVerb error responses all having the same
schema (the first one being declared).
@pcapriotti
Merge MultiVerb schemas for error descriptions
b5732c0
@pcapriotti
Reuse combineResponseSwagger in ErrorDescription
d7f9cc3 
The combineResponseSwagger function, introduced for `MultiVerb`, can
also be used to implement the functionality of `CanThrow`. This PR
reimplements the `HasSwagger` instance of `CanThrow` on top of
`combineResponseSwagger`, leaving the functionality the same modulo the
order of responses.
@pcapriotti
Update CHANGELOG
5588ccc
@pcapriotti pcapriotti force-pushed the pcapriotti/multiverb-combine-v2 branch from 94df354 to 5588ccc Compare August 13, 2021 15:39
@akshaymankar akshaymankar merged commit 8f8093e into develop Aug 19, 2021
@akshaymankar akshaymankar deleted the pcapriotti/multiverb-combine-v2 branch August 19, 2021 14:08
@akshaymankar akshaymankar mentioned this pull request Sep 8, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fisx fisx fisx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pcapriotti @fisx @mdimjasevic @akshaymankar