Align error format with Commonalities guidelines

[jlurien]

As discussed in issue #97, we have to align current error model and examples to the guidelines:

• Add new status property
• Created new Generic500 response and $ref all previous instances

[hdamker]

@jlurien Short question: is this PR intended for the minor fix release (which I want to close "now") or already for v0.9? As status is a required field, it might fit better into v0.9.0?

[jlurien]

@jlurien Short question: is this PR intended for the minor fix release (which I want to close "now") or already for v0.9? As status is a required field, it might fit better into v0.9.0?

0.9 definitely

[hdamker]

Added rewievers @eric-murray @patrice-conil as discussed within our call. @SfnUser please renew your review as well. Thanks!

[patrice-conil]

Hi @jlurien, Just a sample to illustrate my review ErrorInfo definition may look like this:

ErrorInfo: discriminator: type type: object required:

• type
• status
• code
• message properties: type: description: The error class name type: "string" status: type: string pattern: "^[4-5][0-9]{2}$" description: HTTP status code returned along with this error response code: type: string description: Code given to this error message: type: string description: Detailed error description

[jlurien]

Hi @jlurien, Just a sample to illustrate my review ErrorInfo definition may look like this:

ErrorInfo: discriminator: type type: object required: - type - status - code - message properties: type: description: The error class name type: "string" status: type: string pattern: "^[4-5][0-9]{2}$" description: HTTP status code returned along with this error response code: type: string description: Code given to this error message: type: string description: Detailed error description

Hi @patrice-conil, have you commented in the file? I guess this is about the problem you mentioned in the meeting with allOf for some openapi generator. ErrorModel is defined in the commonalities so any change should be aligned there. But in case we decide to define a discriminator to the base class, we could use the existent status or code for this, along with a mapping. Adding a new required property type is would repeat the same information.

Another alternative, as we are not reusing that much of the base ErrorModel, it is just to add the message property to all GenericXXXResponses and remove the allOf. I would not be surprised if some tool has problems as well with OAS3 mapping as that is not supported in legacy Swagger 2.

[patrice-conil]

Hi @jlurien, yes I commented in the file. and it is indeed linked to the problem that I mentioned last Friday. I agree with you, this mapping should do the trick... but it's quite new and probably not so well supported by generators. I think simplifying the generation of client side is one way to have a usable API. Perhaps we should spend more time documenting our validation rules than documenting sample responses to describe the behaviour of our proposal.

[jlurien]

Hi @jlurien, yes I commented in the file. and it is indeed linked to the problem that I mentioned last Friday. I agree with you, this mapping should do the trick... but it's quite new and probably not so well supported by generators. I think simplifying the generation of client side is one way to have a usable API. Perhaps we should spend more time documenting our validation rules than documenting sample responses to describe the behaviour of our proposal.

I only see the latest comment about Generic404. If mapping is not well supported, I prefer to remove the allOf that adding an additional property @type in the API spec for all errors to overcome the limitation, as that would add too much redundancy in the response:

HTTP 404
{ 
  "@type": "Generic404",
  "status": "404"
  "code": "NOT_FOUND",
  "message": "Session not found"
}

[jlurien]

Please review the latest version. Some comments:

• status is now an integer
• code values aligned with Commonalities (INVALID_ARGUMENT, UNAUTHENTICATED, PERMISSION_DENIED...)
• Specific schemas defined for all errors, so ErrorInfo schema is not used. I have created a new issue in Commonalities to decide whether to reference specific model per error or the common model: https://github.com/camaraproject/WorkingGroups/issues/151. When a common approach is decided we should align this to it.

[hdamker]

LGTM, just asking @patrice-conil @maxl2287 @eric-murray for a final review (and closing remarks for the open conversations).

[eric-murray]

Is it intended to approve and merge this PR before Commonalities Issue #151 is resolved?

[jlurien]

Is it intended to approve and merge this PR before Commonalities Issue #151 is resolved?

We could merge it as it is now and later update the $refs to the right schemas, and remove the unused ones. Indeed I have intentionally left the ErrorInfo schema even if it is currently unused.