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

Custom HTTP status codes should not be marked as issues #153

Open
Herrick19 opened this issue May 28, 2022 · 2 comments
Open

Custom HTTP status codes should not be marked as issues #153

Herrick19 opened this issue May 28, 2022 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@Herrick19
Copy link

Hi,

I have defined a status code 352 as a response in a path like this

description: ''
get:
    tags:
        - Object_Activesession
    responses:
        '200':
            content:
                application/json:
                    schema:
                        $ref: '#/components/schemas/activesession-getCurrent-v1-Response'
            description: OK
        '352':
            $ref: '#/components/responses/Response-Redirect-352'
    security:
        -
            Authorization: []
    operationId: Activesession_GetCurrent_V1
    summary: Get Current Activesession
    description: Retrieve the details about the current activesession

Apicurito marks the 352 as an issue with this message:

"352" is not a valid HTTP response status code.
All Responses declared for an Operation must correspond to a valid HTTP response status code. Valid status codes are things like 200, 404, 500. A full list of status codes can be found here: https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml

I've read these pages:
https://swagger.io/specification/
https://datatracker.ietf.org/doc/html/rfc7231#section-6
https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml

And nowhere I can see that we MUST NOT use a custom code.

If you read this section:
https://datatracker.ietf.org/doc/html/rfc7231#section-6

We can read:

HTTP status codes are extensible.  HTTP clients are not required to
   understand the meaning of all registered status codes, though such
   understanding is obviously desirable.  However, a client MUST
   understand the class of any status code, as indicated by the first
   digit, and treat an unrecognized status code as being equivalent to
   the x00 status code of that class, with the exception that a
   recipient MUST NOT cache a response with an unrecognized status code.

   For example, if an unrecognized status code of 471 is received by a
   client, the client can assume that there was something wrong with its
   request and treat the response as if it had received a 400 (Bad
   Request) status code.  The response message will usually contain a
   representation that explains the status.

So my opinion is that Apicurito is flagging this an an issue when it should not since the codes are extensibles so we can put anything we want in there.

It's an easy fix it you are accepting this change since it's just a matter of removing a test for you.

By the way, redoc will show the 352 redirect response correctly without any complaint.

Thanks for considering this fix.

Best Regards

@EricWittmann
Copy link
Member

Hi @Herrick19 - I went back to review the OpenAPI specification and I can't find any reason why I implemented the rule in the strict way that I did. Probably just a lack of fully understanding the RFCs you have linked. The relevant section in the OpenAPI spec is, I think, here:

https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.2.md#httpCodes

The HTTP Status Codes are used to indicate the status of the executed operation. The available status codes are defined by [RFC7231](https://tools.ietf.org/html/rfc7231#section-6) and registered status codes are listed in the [IANA Status Code Registry](https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml).

I think that implies that the only allowed HTTP status codes are the ones defined in the list of registered status codes linked, but it certainly doesn't state that.

The fix is probably to modify the existing rule to allow custom HTTP status codes but only if those custom codes are in the 1-5 range. That is explicitly stated in the OpenAPI spec here:

https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.2.md#responsesObject

Only the following range definitions are allowed: 1XX, 2XX, 3XX, 4XX, and 5XX. If a response is defined using an explicit code, the explicit code definition takes precedence over the range definition for that code.

I think we should also create a new validation rule that would preserve the current check - strictly validating against the set of registered HTTP response codes as we do today. However I think we should set the default severity level for that new rule to "disabled".

Does that all make sense to you?

@Herrick19
Copy link
Author

Hi @EricWittmann,

You are right, you should at least verify the code is an integer between 100 and 599 instead of completely removing the validation rule.

For the second part to have a stricter validation rule checking for only registered response: personally, I don't see the need for this since custom codes are allowed by the standard. I would leave this kind of "preference checks" to linter projects that will validate other preferences like variable naming, description filling, etc. But it doesn't cause any harm to leave it in apicurito if you feel it should be there.

Best Regards

@EricWittmann EricWittmann added the bug Something isn't working label May 31, 2022
@EricWittmann EricWittmann self-assigned this May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants