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

Need a way to mark response as error #236

Closed
stankovski opened this issue Dec 31, 2014 · 14 comments
Closed

Need a way to mark response as error #236

stankovski opened this issue Dec 31, 2014 · 14 comments
Labels
error desc Describing error responses beyond error codes and response schemas

Comments

@stankovski
Copy link

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
Copy link
Member

webron commented Dec 31, 2014

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
Copy link
Member

webron commented Mar 6, 2015

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

@jharmn
Copy link
Contributor

jharmn commented Feb 5, 2016

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
Copy link
Member

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
Copy link
Member

webron commented Feb 5, 2016

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
Copy link
Member

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

@jharmn
Copy link
Contributor

jharmn commented Feb 5, 2016

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
Copy link
Contributor

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
Copy link
Member

webron commented Feb 13, 2016

@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
Copy link
Contributor

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

  • consistent
  • predictable
  • following standards if needed
  • make it easy to do the right think and harder to do it in a sub-optimal way and yet support the requirement.

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

@webron
Copy link
Member

webron commented Feb 13, 2016

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

@jharmn
Copy link
Contributor

jharmn commented Feb 16, 2016

Link to meta #566

@handrews
Copy link
Member

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 handrews added http Supporting HTTP features and interactions error desc Describing error responses beyond error codes and response schemas and removed http Supporting HTTP features and interactions labels Jan 29, 2024
@handrews
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error desc Describing error responses beyond error codes and response schemas
Projects
None yet
Development

No branches or pull requests

6 participants