camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
37 stars 60 forks source link

Add 400 - Bad Request for an invalid sessionId GET + DELETE /sessions/{sessionId}: #209

Closed maxl2287 closed 9 months ago

maxl2287 commented 10 months ago

Problem description When attempting to access a session using the GET /sessions/{sessionId} endpoint, if you provide an invalid UUID as the sessionId parameter, the API will respond with an appropriate HTTP status code to indicate the error. However, it appears that there is currently no specific HTTP status code mentioned in the documentation for this scenario.

Expected behavior The expected behavior for the scenario where an invalid UUID is provided as the sessionId parameter is to return an HTTP status code 400 - Bad Request. This status code signifies that the client's request was malformed or invalid, specifically in the context of an incorrect session ID format.

Alternative solution

  /sessions/{sessionId}:
    get:
      responses:
         "400":
            $ref: "#/components/responses/Generic400"

Additionally

Same should be done for DELETE /sessios/{sessionId}

patrice-conil commented 10 months ago

Hi @maxl2287, I'm not a big fan of manually writing field-specific error messages because I prefer to delegate surface controls to my favorite JSON serializer/deserializer.

If we have so many clients submitting invalid requests... why don't we provide them with a sandbox exposing a swagger UI to help them forge valid requests? And this sandbox would be a good starting point to help telecom operators validate implementation of their APIs. Another option should be to explain how to stop using curl and to import OAS spec in friendly tools that generate valid requests. 😄

What do you think about?

maxl2287 commented 10 months ago

Hi @maxl2287, I'm not a big fan of manually writing field-specific error messages because I prefer to delegate surface controls to my favorite JSON serializer/deserializer.

I only used this "field-specific error message" just for this endpoint, because here it is only one possible parameter as a pathVariable, called "sessionId". That's why I used here this specific field error message.

patrice-conil commented 10 months ago

Yes @maxl2287, I understand the idea. But Jackson/JsonB/Gson centralizes error handling for all endpoints of an API. It is then quite simple to personalize the error messages detected by our code after the surface check, i.e. those which are not linked to constraints or the data type but not those generated by the serialization framework.

For example, if you send a sessionId 12345678 the default error message sent by Jackson will be

{
    "status":400,
    "code":"INVALID_ARGUMENT",
    "message":"Invalid UUID string: 12345678"
}

How to differentiate whether this comes from an invalid sessionId or another UUID field to specialize the message? Doesn’t Jackson’s message make enough sense? You could argue that we could specialize an error mapper for a single interface/endpoint, but is it really worth it?

In other words, I am in favor of strictly defining the status and code and giving the implementer some freedom for personalizing messages.

maxl2287 commented 10 months ago

@patrice-conil understand, I guess the easiest solution would be to stay with:

  responses:
       "400":
          $ref: "#/components/responses/Generic400"
RandyLevensalor commented 10 months ago

Should we add the 400 response as an incremental 0.9.x release? It shouldn't break anything and adds better error handling.

patrice-conil commented 10 months ago

Hi @maxl2287, I think we also need to add the 400 response in the delete.

maxl2287 commented 10 months ago

@patrice-conil yes you are right. I have updated the issue and PR.

eric-murray commented 10 months ago

Hi @maxl2287

I opened an issue in Commonalities proposing the adoption of RFC 7807 error responses to allow linking to more specific error documentation when the cause of the error may not be obvious. The proposal would require each API to define a full set of error responses within the API definition as "model error responses" which (taking into account the concerns of @patrice-conil) would be optional for an API provider to use.

Now in this case, where the error is due to straightforward non-compliance with the API OAS definition, then I agree with @patrice-conil that additional explanation of the error is not really required. But other errors may result from OAS compliant inputs and yet still be errors.

As you have raised this issue, and so are thinking about how API consumers use error responses, it would be useful to have your input in the issue in Commonalities.