OAI / OpenAPI-Specification

The OpenAPI Specification Repository
https://openapis.org
Apache License 2.0
28.91k stars 9.07k forks source link

Need a way to mark response as error #236

Closed stankovski closed 4 months ago

stankovski commented 9 years ago

This may be similar to #166. Currently there is no way to indicate whether a ResponseObject is an error or not. In the pet store example a "default" response is an error, however there is no way for the tooling to understand whether the model being returned is an error or not unless the assumption is made that all "default" responses are errors (which is not always correct). Please consider adding something like "isError" property to a ResponseObject.

webron commented 9 years ago

default is not an error. It's a general-purpose response which can be used as an error or not.

There's no real need to mark responses as error. By standard, 2XX responses are 'ok', 4XX responses are errors due to the client request and 5XX responses are errors due to server issues.

webron commented 9 years ago

I'll mark is as a proposal for the next version and we'll see how people respond to the suggestion.

jharmn commented 8 years ago

I could see the following scenario: An API only responds with 200 for happy path and errors (which produces all the niceness of the aforementioned 4xx, 5xx statuses). These things exist, sadly. In order to express an error, there would be a need to designate a response as error (vs. default).

As much as I hate to encourage the use of 200-only APIs, here's a potential solution (and one that perhaps makes default more accurate in it's intent):

        200:
          description: Expected response to a valid request
          schema:
            $ref: '#/definitions/Pets'
        default:
          error: true  # Note addition of `error`
          description: unexpected error
          schema:
            $ref: '#/definitions/Error'
whitlockjc commented 8 years ago

As an API author/consumer, I am not sure that I need extra information to indicate if a response is an error or not. The 4xx and 5xx range of status code are pretty clear about their intent. I realize that the default response could be used for an error response and that guessing that it is an error is likely impossible from a Swagger perspective alone programmatically. _(I say programmatically because a well crafted description and/or a reference to an _Error definition should be pretty clear from a human consumption perspective.)* But API clients, generated from Swagger or not, will have access to and should make use of the API's response status code for these things.

I'm not saying we shouldn't do it but adding an extra error property to the response object just seems unnecessary. I surely don't want to get to the point where I have to attach that property to response objects that are obviously errors like the 4xx and 5xx range.

webron commented 8 years ago

Not sure this is something we should support because "these things exist". Haven't seen many people ask for it, and it feels like that's one of the few things about REST APIs that people commonly agree on. Even if it's easy to support, don't know if we should. However, if that error addition applies to the default response only, I can live with that (and we need to decide on a default value).

whitlockjc commented 8 years ago

Agreed, if it had to be only for the default value, I could hate that less. ;)

jharmn commented 8 years ago

I'd agree @webron that it's probably an issue that's trending down in public APIs. However, I have concern that as internal APIs are being built more and more (often with much lower standards/education), this could be a bigger issue. The other reasoning behind indicating default means error is that it can be unclear without looking at the description. From a programmatic standpoint, it would be nice for test validation to ensure that you're using the right element. Overall, this seems lower priority, and since it seems we could address it with a backward compatible change, might be worth of reviewing after we've addressed some of the bigger structural issues (and also see if the issue gains any momentum).

dilipkrish commented 8 years ago

Just thought I'd chime in here. Since this is a in vNext (read breaking changes allowed) I think this is a great opportunity to describe these things as we'd like the spec to handle them ideally and not try to jam these things in existing constructs.

IMHO, as a spec we'd be better served guiding the spec in a direction such that it drives the consumers, implementors and tool vendors into the "pit of success". I'd rather make it hard to do things sub-optimally. Littering the spec with these exception cases, just to be everything for everybody will make the job for spec implementers that much harder. Also will make evolution of the spec harder.

Having said that, It seems like this could benefit by using the oneOf construct.

       200:
          description: Expected response to a valid request
          schema:
            oneOf: [
               { $ref: '#/definitions/Pets' },
               { $ref: '#/definitions/Error' }
             ],
webron commented 8 years ago

@dilipkrish That's just something I don't get. Your example shows a 200 response sending either Pets or Error - but how does that help the consumer? How would they know which is the successful response case and which is the error (ignoring the fact that we both have some level of knowledge in English of course). That definitely doesn't help automatic processing, whether it's for validation or code generation. I'd rather not support the construct at all then support it like that.

As a side note, the oneOf is a completely unnecessary construct there that just bloats the spec, but that's a matter for discussion elsewhere ;)

dilipkrish commented 8 years ago

May be I wasnt being clear. I'm NOT for doing this the suggested way. My example was in response to this use case 👇🏻, and wasn't particularly fond of the "default" approach 😋

An API only responds with 200 for happy path and errors (which produces all the niceness of the aforementioned 4xx, 5xx statuses). These things exist, sadly.

My example is no different than the one with the "default" from the clients perspective; but it introduces a new construct that implementers need to care about just for the response object alone.

IMO as a spec there needs to be standard patterns when we're introducing new constructs. It doesn't have to use oneOf, but it needs to be a consistent construct. My comment was to reiterate that when we're making decisions about adding new constructs to parts of the spec let's be

Ironic that I suggested the support for this one-off use case with oneOf 😜

webron commented 8 years ago

Heh, fair enough, it was my misunderstanding. Generally, I believe we have a similar view re your last comment.

jharmn commented 8 years ago

Link to meta #566

handrews commented 4 years ago

I'd argue that the right way to handle this is really to use a media type for error responses - application/problem+json being the most obvious, although a vendor-specific one would also work.

handrews commented 4 months ago

My suggested solution from 2020 got some upvotes, and was the first comment since 2016. It does not require a spec change.

We're working on a new version over in the OAI/sig-moonwalk repository, so larger philosophical concerns around error modeling should join the discussion there.

Also, if there is truly interest in an error flag field of some sort, I'd suggest trying an extension which could go in the registry. If it gets enough use it might get adopted.

Given that no one else has commented since 2016 and there are several paths forward that do not involve an immediate spec change, I'm going to close this.