Skip to content

Clarification on non-mandatory error statuses, 429 made non-madatory#374

Merged
rartych merged 5 commits intocamaraproject:mainfrom
rartych:mandatory-errors
Jan 24, 2025
Merged

Clarification on non-mandatory error statuses, 429 made non-madatory#374
rartych merged 5 commits intocamaraproject:mainfrom
rartych:mandatory-errors

Conversation

@rartych
Copy link
Copy Markdown
Contributor

@rartych rartych commented Jan 13, 2025

What type of PR is this?

  • correction

What this PR does / why we need it:

Clarification proposed in the issue discussion #355.
429 made non-madatory - #365
501 mandatory, if and only if an endpoint is optional for implementations

Which issue(s) this PR fixes:

Fixes #355 #365 #246

Does this PR introduce a breaking change?

  • Yes
  • No

Special notes for reviewers:

The discussion in #355 concludes to keep the error definitions in CAMARA_common.yaml.
Adding comments to each error schema (saying if it is mandatory) can be considered.

Changelog input

Added clarification on non-mandatory error statuses

Additional documentation

This section can be blank.

docs

PedroDiez
PedroDiez previously approved these changes Jan 14, 2025
Copy link
Copy Markdown
Contributor

@PedroDiez PedroDiez 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 previously approved these changes Jan 15, 2025
Copy link
Copy Markdown
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

429 made non-mandatory
@rartych rartych dismissed stale reviews from patrice-conil and PedroDiez via 4216b5a January 15, 2025 15:49
@rartych rartych changed the title Clarification on non-mandatory error statuses Clarification on non-mandatory error statuses, 429 made non-madatory Jan 15, 2025
PedroDiez
PedroDiez previously approved these changes Jan 15, 2025
Copy link
Copy Markdown
Contributor

@PedroDiez PedroDiez 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 previously approved these changes Jan 16, 2025
Copy link
Copy Markdown
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

@PedroDiez
Copy link
Copy Markdown
Contributor

I like to propose a wording within the API design guideline in line with this comment

Not include line 886 (indeed 501 is not a mandatory code in the way like 401, and 403 are) and after line 811 include specific section, in this way:

Special Consideration for 501 code:
• This code can be only documented in an API endpoint under decision and agreement within a given API Subproject.
• Following guidelines to be considered:
o The use of optional endpoints is discouraged in order to have aligned implementations and deployments
o When the API Subproyect concludes the endpoint can be optional and after the evaluation of the approach of API splitting in the case of being and independent resource.

I think it respects the spirit targeted for this code and also refelcts the relevant guidelines derived from TSC.
@rartych, @eric-murray, @hdamker please if you can take a look

@rartych
Copy link
Copy Markdown
Contributor Author

rartych commented Jan 23, 2025

@PedroDiez I am fine with not mixing 501 with 401 and 403.

I propose to extend the point starting in line 806 with the special considerations.
Then it would look like this:

  • Error statuses 405, 406, 410, 412, 415, and 5xx: These error statuses are not documented by default in the API specification. However, they should be included if there is a relevant use case that justifies their documentation
    • Special Consideration for error 501 NOT IMPLEMENTED to indicate optional endpoint:
      • The use of optional endpoints is discouraged in order to have aligned implementations
      • Only for exceptions where an optional endpoint can not be avoided and defining it in separate, atomic API is not possible - error status 501 should be documented as a valid response

@PedroDiez
Copy link
Copy Markdown
Contributor

@PedroDiez I am fine with not mixing 501 with 401 and 403.

I propose to extend the point starting in line 806 with the special considerations. Then it would look like this:

  • Error statuses 405, 406, 410, 412, 415, and 5xx: These error statuses are not documented by default in the API specification. However, they should be included if there is a relevant use case that justifies their documentation

    • Special Consideration for error 501 NOT IMPLEMENTED to indicate optional endpoint:

      • The use of optional endpoints is discouraged in order to have aligned implementations
      • Only for exceptions where an optional endpoint can not be avoided and defining it in separate, atomic API is not possible - error status 501 should be documented as a valid response

Perfect Rafal!

@PedroDiez PedroDiez mentioned this pull request Jan 23, 2025
2 tasks
@rartych
Copy link
Copy Markdown
Contributor Author

rartych commented Jan 24, 2025

I propose to extend the point starting in line 806 with the special considerations. Then it would look like this:

Proposed changes have been applied to the PR

Copy link
Copy Markdown
Contributor

@PedroDiez PedroDiez 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

Projects

None yet

4 participants