Skip to content

error_model_alignment#213

Merged
rartych merged 19 commits intomainfrom
error_model_alignment
Jun 11, 2024
Merged

error_model_alignment#213
rartych merged 19 commits intomainfrom
error_model_alignment

Conversation

@PedroDiez
Copy link
Contributor

@PedroDiez PedroDiez commented May 23, 2024 •
edited
Loading

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

@PedroDiez PedroDiez requested review from bigludo7 and eric-murray May 23, 2024 15:54
patrice-conil
patrice-conil reviewed May 24, 2024
View reviewed changes
Copy link
Contributor

@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

This was referenced May 27, 2024
Closed
Closed
Closed
@rartych
Copy link
Contributor

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 jlurien mentioned this pull request May 30, 2024
Closed
@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
Contributor Author

PedroDiez commented Jun 1, 2024

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
Contributor Author

PedroDiez commented Jun 3, 2024

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).

rartych
rartych reviewed Jun 3, 2024
View reviewed changes
documentation/API-design-guidelines.md Outdated
| 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
Contributor

@rartych rartych Jun 3, 2024

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
Contributor Author

@PedroDiez PedroDiez Jun 3, 2024

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
Contributor Author

@PedroDiez PedroDiez Jun 11, 2024

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

rartych
rartych reviewed Jun 5, 2024
View reviewed changes
rartych
rartych reviewed Jun 5, 2024
View reviewed changes
rartych
rartych reviewed Jun 5, 2024
View reviewed changes
rartych
rartych reviewed Jun 5, 2024
View reviewed changes
rartych
rartych reviewed Jun 5, 2024
View reviewed changes
rartych
rartych reviewed Jun 5, 2024
View reviewed changes
rartych
rartych reviewed Jun 5, 2024
View reviewed changes
rartych
rartych reviewed Jun 5, 2024
View reviewed changes
rartych
rartych reviewed Jun 5, 2024
View reviewed changes
PedroDiez and others added 12 commits June 5, 2024 16:19
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
0e44cee 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
bb9e418 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
6f9c2b5 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
dc068e5 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez
merge_main
6cb5bf7
@PedroDiez
fix_2nd_review_comments
ec93be9
@PedroDiez
align_with_remote_branch
7985722
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
36ab9a4 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
76dbe74 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
b1e2500 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
5fde739 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez @rartych
Update documentation/API-design-guidelines.md
3dc3fc0 
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@PedroDiez
Copy link
Contributor Author

PedroDiez commented Jun 5, 2024

05/JUN: Addressed first round of reviews

Pending point:

patrice-conil
patrice-conil reviewed Jun 11, 2024
View reviewed changes
Copy link
Contributor

@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
shilpa-padgaonkar
shilpa-padgaonkar approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@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

patrice-conil
patrice-conil approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@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

@rartych rartych merged commit 2f5d6a2 into main Jun 11, 2024
@bigludo7 bigludo7 mentioned this pull request Jun 18, 2024
Merged
@Kevsy Kevsy mentioned this pull request Jul 12, 2024
Closed
@rartych rartych deleted the error_model_alignment branch October 9, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patrice-conil patrice-conil patrice-conil approved these changes

@bigludo7 bigludo7 bigludo7 approved these changes

@shilpa-padgaonkar shilpa-padgaonkar shilpa-padgaonkar approved these changes

@rartych rartych rartych approved these changes

@RubenBG7 RubenBG7 Awaiting requested review from RubenBG7

@eric-murray eric-murray Awaiting requested review from eric-murray

@jlurien jlurien Awaiting requested review from jlurien jlurien is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

6 participants

@PedroDiez @rartych @jlurien @patrice-conil @bigludo7 @shilpa-padgaonkar