Align error format with Commonalities guidelines #104

jlurien · 2023-01-25T12:46:50Z

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

As discussed in issue [camaraproject#97](camaraproject#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

code/API_definitions/qod-api.yaml

hdamker · 2023-01-25T13:53:06Z

@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 · 2023-01-25T16:43:18Z

@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

Schemas for GenericXXX responses must restrict the ErrorInfo model to the allowed values at `status` and `code` for every specific response

hdamker · 2023-01-28T07:30:06Z

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

patrice-conil · 2023-01-30T08:42:57Z

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 · 2023-01-30T12:15:17Z

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 · 2023-01-30T13:52:48Z

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.

patrice-conil · 2023-01-30T08:30:42Z

code/API_definitions/qod-api.yaml

          example:
+            status: "403"
            code: FORBIDDEN
            message: "Operation not allowed: ..."
    SessionNotFound404:


Why not Generic404 as Generic403, Generic401 ?
All these errors are specialization of ErrorInfo ... usage is to define a type or @type field as discriminator in ErrorInfo to help deserializer to manage subtype deserialization.

I could have named it Generic404 as well, but as the only case in this API for 404 was Session Not Found, I chose a more specific name and added an explicit message in the example. Problem with Generic responses is that example has to be generic as well. For automatic API publishing in a web portal it is convenient to embed good examples in the spec, but for client generation purposes it may complicate things.

I can change the name to Generic404 if that is more coherent with the rest of responses

jlurien · 2023-01-30T16:49:03Z

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"
}

code/API_definitions/qod-api.yaml

maxl2287 · 2023-01-31T12:31:06Z

code/API_definitions/qod-api.yaml

            code: NOT_FOUND
            message: "Session Id does not exist"
+    Generic500:
+      description: Server error


A better naming would be "Internal Server Error" for the description of a 500 - Internal Server Error.

maxl2287 · 2023-01-31T12:32:23Z

code/API_definitions/qod-api.yaml

+                    enum: ["500"]
+                  code:
+                    type: string
+                    enum: ["INTERNAL"]


It's not clear what "INTERNAL" means.
Better use the following naming:

enum: ["INTERNAL_SERVER_ERROR"]

"Internal Server Error" is also the description for the status code "500" within Hypertext Transfer Protocol (HTTP) Status Code Registry. Therefore +1 .

I agree on aligning as much as possible to standard HTTP terms.

I have aligned the codes with the ones in Commonalities, and currently they define INTERNAL for 500 and UNAVAILABLE for 503. code has not to be necessarily the description of the HTTP code, but in any case, the discussion for this should be moved to Commonalities.

Ok for now ... we just should get a conclusion on that before we close v0.9.0.

- `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: <camaraproject/WorkingGroups#151>. When a common approach is decided we should align this to it.

jlurien · 2023-02-08T12:22:13Z

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: Schemas in error responses WorkingGroups#151. When a common approach is decided we should align this to it.

hdamker · 2023-02-09T07:05:43Z

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

Resolving conflicts with updated main

eric-murray · 2023-02-09T16:12:08Z

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

jlurien · 2023-02-09T16:30:39Z

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.

Align error format with Commonalities guidelines

0a15130

As discussed in issue [camaraproject#97](camaraproject#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

jlurien requested review from hdamker and shilpa-padgaonkar as code owners January 25, 2023 12:46

jlurien mentioned this pull request Jan 25, 2023

Add-501-error-code-[NOT_IMPLEMENTED]-main (#4) #94

Closed

sfnuser reviewed Jan 25, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

jlurien commented Jan 25, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

Update code/API_definitions/qod-api.yaml

4050c21

sfnuser previously approved these changes Jan 25, 2023

View reviewed changes

Update qod-api.yaml

c43b46b

Schemas for GenericXXX responses must restrict the ErrorInfo model to the allowed values at `status` and `code` for every specific response

jlurien dismissed sfnuser’s stale review via c43b46b January 26, 2023 09:56

hdamker requested review from eric-murray, patrice-conil and sfnuser January 28, 2023 07:28

patrice-conil reviewed Jan 30, 2023

View reviewed changes

eric-murray reviewed Jan 30, 2023

View reviewed changes

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved

maxl2287 reviewed Jan 31, 2023

View reviewed changes

hdamker mentioned this pull request Jan 31, 2023

Design Guidelines: HTTP Status Code type should be Integer, not String camaraproject/WorkingGroups#143

Closed

jlurien added 2 commits February 2, 2023 13:38

Merge branch 'main' into chore/errors-alignment-with-guidelines

c27657b

hdamker requested review from patrice-conil and maxl2287 February 9, 2023 07:05

hdamker requested review from eric-murray and maxl2287 and removed request for maxl2287 February 9, 2023 07:05

jlurien added 2 commits February 9, 2023 16:25

Merge branch 'main' into chore/errors-alignment-with-guidelines

c4416ba

Update qod-api.yaml

1e58c7f

Resolving conflicts with updated main

eric-murray approved these changes Feb 9, 2023

View reviewed changes

hdamker approved these changes Feb 10, 2023

View reviewed changes

sfnuser approved these changes Feb 10, 2023

View reviewed changes

hdamker merged commit 39f9c9b into camaraproject:main Feb 10, 2023

jlurien deleted the chore/errors-alignment-with-guidelines branch February 13, 2023 08:58

dfischer-tech mentioned this pull request Mar 1, 2023

Introducing error code 501 "Not Implemented" #124

Merged

hdamker mentioned this pull request Apr 19, 2023

Alignment with API design guidelines #97

Closed

hdamker mentioned this pull request Jun 17, 2023

Pre release v0.9.0 rc #169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align error format with Commonalities guidelines #104

Align error format with Commonalities guidelines #104

jlurien commented Jan 25, 2023

hdamker commented Jan 25, 2023

jlurien commented Jan 25, 2023

hdamker commented Jan 28, 2023

patrice-conil commented Jan 30, 2023

jlurien commented Jan 30, 2023

patrice-conil commented Jan 30, 2023

patrice-conil Jan 30, 2023

jlurien Jan 30, 2023

jlurien commented Jan 30, 2023

maxl2287 Jan 31, 2023

maxl2287 Jan 31, 2023

hdamker Jan 31, 2023

jlurien Jan 31, 2023

jlurien Feb 8, 2023

hdamker Feb 9, 2023

jlurien commented Feb 8, 2023

hdamker commented Feb 9, 2023

eric-murray commented Feb 9, 2023

jlurien commented Feb 9, 2023

Align error format with Commonalities guidelines #104

Align error format with Commonalities guidelines #104

Conversation

jlurien commented Jan 25, 2023

hdamker commented Jan 25, 2023

jlurien commented Jan 25, 2023

hdamker commented Jan 28, 2023

patrice-conil commented Jan 30, 2023

jlurien commented Jan 30, 2023

patrice-conil commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlurien commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlurien commented Feb 8, 2023

hdamker commented Feb 9, 2023

eric-murray commented Feb 9, 2023

jlurien commented Feb 9, 2023