Added 501 Not Supported error response #145
Conversation
mhfoo
requested review from
shilpa-padgaonkar,
rartych,
patrice-conil,
RubenBG7 and
PedroDiez
as code owners
|
@rartych @shilpa-padgaonkar @patrice-conil @RubenBG7 @PedroDiez Kindly review this pull request for merging please. |
| @@ -265,6 +265,16 @@ components: | |||
| status: 501 | |||
| code: NOT_IMPLEMENTED | |||
| message: This functionality is not implemented yet | |||
| NotSupported: | |||
There was a problem hiding this comment.
| NotSupported: | |
| PhoneNumberNotSupported: | |
There was a problem hiding this comment.
@PedroDiez
Suggest not to be specific to "phoneNumber", so this error can be reused.
| schema: | ||
| $ref: "#/components/schemas/ErrorInfo" | ||
| example: | ||
| status: 501 |
There was a problem hiding this comment.
To be aligned with Issue#127 output (so far 422)
There was a problem hiding this comment.
@PedroDiez
Understand there is a difference between 4xx and 5xx HTTP error codes.
The 4xx codes are intended for cases in which the client has erred, and the 5xx codes for the cases in which the server is aware that the server has erred.
In this context, the client cannot "correct" the request as the server is not supporting this specific request.
See 400 vs 500
There was a problem hiding this comment.
@mhfoo please see my comment #127 (comment) regarding your same statement there. 422 with appropriate description is definitely more helpful for the client then 501.
There was a problem hiding this comment.
I think 501 is appropriate for functionality that has not yet been implemented, but will be (or, at least, could be) implemented at some point in the future, whereas a 4XX error should be returned for a request that will never make sense and will never be supported (such as requesting SIM Swap status for a landline).
However desirable it is that all implementations support all possible functionality from the start, I do not think that will be the case.
So I support using a 501 error in some circumstances where a phone number is not supported, but only if it will be supported at a later date.
| $ref: "#/components/schemas/ErrorInfo" | ||
| example: | ||
| status: 501 | ||
| code: NOT_SUPPORTED |
There was a problem hiding this comment.
| code: NOT_SUPPORTED | |
| code: PHONE_NUMBER_NOT_SUPPORTED | |
There was a problem hiding this comment.
@PedroDiez
Suggest not to be specific to "Phone_Number", so this error can be reused.
|
@mhfoo As we agreed on the last Commonalities meeting could you modify your proposal to: Further discussion is possible, but for now we have consensus to use 422. |
In last Commonalities meeting, my statement are as follows: I do not want my PR #145 to block this coming meta-release (Fall24) of Commonalities. Please continue this PR #145 discussion for the next release (Spring25). |
|
@mhfoo Thank you for clarification, I got it wrong. |
|
@rartych @shilpa-padgaonkar propose to "convert to draft" if this is not an active review |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Enhancement as per #138 Proposal to add 501 Not Supported error response
Which issue(s) this PR fixes:
Fixes #138
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.