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(petstore): Update petstore to conform with OpenAPI 3.0.3 spec #4637

Merged
merged 4 commits into from
Jan 12, 2024

Conversation

rstular
Copy link
Contributor

@rstular rstular commented Dec 4, 2023

The petstore YAML is not consistent with the OpenAPI schema 3.0.2 and 3.0.3 as it uses HTTP response code 405 incorrectly (OAI/OpenAPI-Specification#1486)

Description

This changes the Pet store example schema to use HTTP return codes 422 and 400 to indicate an invalid entity instead of using 405. 400 is used to indicate that the syntax of the body is incorrect, and 422 is used to indicate that the syntax is valid, but some other aspect of the body (e.g. its structure) makes it invalid.

Motivation and Context

OpenAPI 3.0.2 has changed the description of the HTTP response code 405 from Invalid input to Method not allowed to conform with RFC7231 (see here).

The pet store example schema has the version set to 3.0.3, but it does not reflect the changes that were made to the response code description. This renders the example schema misleading, as it uses a non-standard description for the code.

How Has This Been Tested?

Ran the tests using npm run test, as well as manually inspected the results by running a local instance (see screenshots below).

Screenshots (if appropriate):

image

image

image

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@rstular rstular marked this pull request as ready for review December 4, 2023 00:50
@char0n char0n self-assigned this Dec 6, 2023
@char0n
Copy link
Member

char0n commented Dec 6, 2023

Hi @rstular,

Thanks for opening this up! You're absolutely correct, the petstore fixture should use appropriate status codes.

Copy link
Member

@char0n char0n left a comment

Choose a reason for hiding this comment

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

LGTM

@frankkilcommins
Copy link
Contributor

@rstular thanks for this. Indeed the representation is not leveraging the HTTP status codes appropriately. I'd prefer to have consistent descriptions while we're at it. Could you modify such that:

  • 400 has description Invalid input
  • 422 has description Validation exception

rstular and others added 2 commits December 22, 2023 03:18
The petstore YAML is not consistent with the OpenAPI schema 3.0.2 and 3.0.3 as it uses HTTP response code 405 incorrectly (OAI/OpenAPI-Specification#1486)
@rstular rstular force-pushed the fix-validation-error-http-response branch from ec9c166 to b5b0491 Compare December 22, 2023 02:19
@rstular
Copy link
Contributor Author

rstular commented Dec 22, 2023

@rstular thanks for this. Indeed the representation is not leveraging the HTTP status codes appropriately. I'd prefer to have consistent descriptions while we're at it. Could you modify such that:

* 400 has description `Invalid input`

* 422 has description `Validation exception`

I've done that for all occurrences where both 400 and 422 status codes were present, but didn't touch the ones without a 422 response. Is that fine?

Copy link
Contributor

@frankkilcommins frankkilcommins left a comment

Choose a reason for hiding this comment

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

LGTM. @char0n this will likely have to be scheduled with updates to dependent components.

@char0n
Copy link
Member

char0n commented Jan 11, 2024

@frantuma can you please confirm that this change is aligned with https://github.com/swagger-api/swagger-petstore/ ? I've inspected the PetController.java and I could tell that Response.Status.BAD_REQUEST === 400 is returned.

Regarding to 422 status code - does the petstore webapp returns 422 in any situation?

@frantuma
Copy link
Member

@char0n

@frantuma can you please confirm that this change is aligned with swagger-api/swagger-petstore ? I've inspected the PetController.java and I could tell that Response.Status.BAD_REQUEST === 400 is returned.

Regarding to 422 status code - does the petstore webapp returns 422 in any situation?

Swagger Petstore v3.0 current APIs implementation only return 400 or 404, no 422 or 405 are ever returned

however this version is based on spec first inflector, and its OpenAPI spec is currently using 405 in some responses. We have a note to update that, not sure if it's so important to have editor and petstore aligned..

@char0n
Copy link
Member

char0n commented Jan 12, 2024

As @frantuma confirmed the changes in this PR are already closer to Petstore java implementation than the old definitions without these changes. I'm going for a merge and will follow up on applying these changes to the next branch as well.

@char0n char0n merged commit 8d82288 into swagger-api:master Jan 12, 2024
2 checks passed
swagger-bot pushed a commit that referenced this pull request Jan 23, 2024
# [5.0.0-alpha.87](v5.0.0-alpha.86...v5.0.0-alpha.87) (2024-01-23)

### Bug Fixes

* **content-fixtures:** update to conform with OpenAPI 3.0.3 spec ([#4728](#4728)) ([24b1c84](24b1c84)), closes [#4637](#4637)
* **editor-content-fixtures:** fix typo in OpenAPI 2.0 ([#4708](#4708)) ([fca1f8b](fca1f8b)), closes [#4684](#4684)

### Features

* add complete editing experience for OpenAPI 2.0 ([#4777](#4777)) ([b0c603b](b0c603b))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants