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

error_model_alignment #213

Merged
merged 19 commits into from
Jun 11, 2024
Merged

error_model_alignment #213

merged 19 commits into from
Jun 11, 2024

Conversation

PedroDiez
Copy link
Collaborator

@PedroDiez PedroDiez commented May 23, 2024

What type of PR is this?

Add one of the following kinds:

  • correction
  • enhancement/feature
  • documentation

What this PR does / why we need it:

This PR covers mainly 3 topics:

Documentation Impacted:

  • API Design Guidelines (sections 3.2 and section 6, new sections 6.1 and 6.2)
  • CAMARA_common.yaml (aligment of message examples and ordering responses model by status)

Which issue(s) this PR fixes:

Fixes #127
Fixes #128
Fixes #162
Fixes #172
Fixes #180

Special notes for reviewers:

Regarding API design guidelines, section 6.2 could also be documented in a separate file (.md for instance and refer from that section). I preferred to explicitly reflect to be easy to get the aim of the section.

@rartych regarding Issue #180 in case you were thinking in additional points I can just take out (Fixes #180) and only reference in the (#### What this PR does / why we need it:) as partial fixing

Changelog input

 - Clarify Error properties are required
 - Error model for device Identifier
 - Added clarification about AuthN/AuthZ standardized flows follow their own Error Response format
 - Error model alignment

Additional documentation

Not apply

Copy link
Collaborator

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work ... one typo to fix but LGTM

documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
@rartych
Copy link
Collaborator

rartych commented May 27, 2024

To tackle #128 and #172 and similar questions I suggest to add paragraph like:

When standardized authentication flows are used - cf. 10.2 Security Implementation the format and content of Error Response should follow relevant standards.

It can be in error format definition or particularly for 401 Status.

@jlurien
Copy link
Contributor

jlurien commented May 30, 2024

I have linked another related discussion in QoD (camaraproject/QualityOnDemand#297) to define the status and code error for situations when a request is rejected due to exceeding a business quota limit. A new code QUOTA_EXCEEDED makes sense, but status could be either 429 or 403. The first being more related to rate limiting and the second a more generic "forbidden due to business logic".

I will align the PR in QoD with the decision here. cc @PedroDiez

@PedroDiez PedroDiez requested a review from jlurien June 1, 2024 09:39
@PedroDiez
Copy link
Collaborator Author

Actions covered in commit 4611515ce7eedb10c961d14b1ccb3966dcdc447b

  • Suggestion by Patrice
  • Suggestions by Rafal to unify info and add clarification for Issues#128 and Issue#172
  • Added QUOTA_EXCEEDED code, based on topic raised by Jose. Proposed to be 429 status in the sense it is focused in a business quota limit that has been overpassed (e.g. number of N QoD minutes or sessions per period). Also fine to be 403 in case seen more suitable.

@PedroDiez
Copy link
Collaborator Author

Actions covered in commit 082f5e52ddec8920432fe12d97cbce62e4cb5fb3

  • Grouping in responses section all the ones that own the same status due to an API that responds to a status must reference a single responses object (it cannot indicate that it is an anyOf and show N $refs).

| 410 | `GONE` | Access to the target resource is no longer available. | Use in notifications flow to allow API Consumer to indicate that its callback is no longer available |
| 412 | `FAILED_PRECONDITION` | Request cannot be executed in the current system state. | Indication by the API Server that the request cannot be processed in current system state |
| 415 | `UNSUPPORTED_MEDIA_TYPE` | The server refuses to accept the request because the payload format is in an unsupported format. | Payload format of the request is in an unsupported format by the Server. Should not happen |
| 500 | `DATA_LOSS` | Unrecoverable data loss or data corruption. | Some data is lost or corrupted when the server is processing the request so as it cannot manage it properly. It may point to a potential problem at DD.BB. level |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this error specified?
If so, then DD.BB. level is not clear for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the description i was referring that when this happens one of the reasons can be at DD.BB. level (problem in DD.BB. system providing support to API BE). It is fine to me to rephrase to:

Some data is lost or corrupted when the server is processing the request so as it cannot manage it properly

On the other hand, Currently it is not used in any API. So, we could take out so far
WDYT? @rartych, @patrice-conil

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed DATA_LOSS error code

@PedroDiez
Copy link
Collaborator Author

05/JUN: Addressed first round of reviews

Pending point:

Copy link
Collaborator

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support removing DATA_LOSS because I don't see any use cases using it

@PedroDiez PedroDiez requested a review from patrice-conil June 11, 2024 08:59
Copy link
Collaborator

@shilpa-padgaonkar shilpa-padgaonkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM

Copy link
Collaborator

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants