zalando / restful-api-guidelines

A model set of guidelines for RESTful APIs and Events, created by Zalando
https://opensource.zalando.com/restful-api-guidelines/
Creative Commons Attribution 4.0 International
2.62k stars 384 forks source link

Section "Use Common Error Return Objects" is difficult to understand #120

Closed akauppi closed 7 years ago

akauppi commented 8 years ago

https://github.com/zalando/restful-api-guidelines/blob/master/common-data-objects/CommonDataObjects.md#-bookmust--use-common-error-return-objects

I know the background of how this came to be. But currently, the text is unnecessarily hard for a developer to get right. We should aim at better.

Some concerns:

I would advocate placing the mention about extensions (if we really need them, what's the use case?) as a short mention. It's JSON anyways, so any extension would be a valid RFC 7807 error object, anyways. The definition of Swagger format is unnecessary - it takes space and suggests making extensions is a common thing.

Also, the Swagger format is based on an earlier error object than 7807. It differs from 7807 in that:

I would recommend removing the whole Swagger declaration from the Guidelines. It's unnecessary cruft and a source of inconsistencies. If it remains, it should match RFC 7807 exactly.

Here is the definition in Open API 2.0 YAML format in the case you don't need any extensions:

Shouldn't we try to point people to not use extensions, for simplicity and general conformance. I'd reorganize the content so that it:

  1. clearly states the "must" of using RFC 7807 when an error status code itself is not enough
  2. the RFC 7807 compliant Swagger definition
  3. short mention that the format may be extended, if needed
  4. mention about the earlier version, and how a client should deal with both

I'm in Berlin 15.-26.8. and could discuss these in person. Also willing to provide a PR once there would be enough comments.

akauppi commented 8 years ago

Also type is optional in 7807, but defaults to about:blank and suggests the title "SHOULD be the same as the recommended HTTP status phrase for that code".

All of that gets rather complex, so I would suggest keeping type a required field, within Zalando APIs, and using just error status codes without an error object for plain cases.

We should aim at text that does not require one to read the RFC 7807, but only treats it as a reference.

ePaul commented 8 years ago

I think having an example Swagger definition there is useful, so people can copy it in their APIs instead of copying it from another API, or having to invent it themselves from the description. Of course, it should be updated to match the text (and the RFC).

ePaul commented 8 years ago

About the type, RFC 7807 was changed compared to the earlier RFC so now you only should include a type if it adds more information compared to the HTTP status code itself. This means there is no point in using http://httpstatus.es/..., instead leave it blank (don't include the item). I think this is a good idea in general.

akauppi commented 8 years ago

Would someone like to team with me to create a PR about these things, during this or next week?

ePaul commented 8 years ago

I'm in BMO tomorrow after the API guild meeting, we could meet then.

ePaul commented 7 years ago

@akauppi Do you still intend to create another pull request for this?

ePaul commented 7 years ago

@akauppi Please reopen if you still want to change anything (or simply open a pull request).