-
Notifications
You must be signed in to change notification settings - Fork 28
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
Added 501 Not Supported error response #145
base: main
Are you sure you want to change the base?
Conversation
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NotSupported: | |
PhoneNumberNotSupported: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be aligned with Issue#127 output (so far 422)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code: NOT_SUPPORTED | |
code: PHONE_NUMBER_NOT_SUPPORTED | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.