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

Added 501 Not Supported error response #145

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mhfoo
Copy link
Collaborator

@mhfoo mhfoo commented Feb 21, 2024

What type of PR is this?

Add one of the following kinds:

  • enhancement

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

release-note
+ Add 501 Not Supported error response

Additional documentation

This section can be blank.

docs

@mhfoo
Copy link
Collaborator Author

mhfoo commented Mar 25, 2024

@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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
NotSupported:
PhoneNumberNotSupported:

Copy link
Collaborator Author

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
Copy link
Collaborator

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)

Copy link
Collaborator Author

@mhfoo mhfoo Apr 16, 2024

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

Copy link
Collaborator

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.

Copy link
Collaborator

@eric-murray eric-murray May 13, 2024

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
code: NOT_SUPPORTED
code: PHONE_NUMBER_NOT_SUPPORTED

Copy link
Collaborator Author

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.

@rartych
Copy link
Collaborator

rartych commented May 23, 2024

@mhfoo As we agreed on the last Commonalities meeting could you modify your proposal to:

  NotSupported:
   description: Not Supported
   content:
    application/json:
     schema:
      $ref: "#/components/schemas/ErrorInfo"
     example:
      status:  422
      code: NOT_SUPPORTED
      message: Service not supported for this device

Further discussion is possible, but for now we have consensus to use 422.

@mhfoo
Copy link
Collaborator Author

mhfoo commented May 24, 2024

@mhfoo As we agreed on the last Commonalities meeting could you modify your proposal to:

  NotSupported:
   description: Not Supported
   content:
    application/json:
     schema:
      $ref: "#/components/schemas/ErrorInfo"
     example:
      status:  422
      code: NOT_SUPPORTED
      message: Service not supported for this device

Further discussion is possible, but for now we have consensus to use 422.

@rartych

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

@rartych
Copy link
Collaborator

rartych commented May 24, 2024

@mhfoo Thank you for clarification, I got it wrong.
Let's postpone this PR as you suggest.

@hdamker
Copy link
Collaborator

hdamker commented Jun 3, 2024

@rartych @shilpa-padgaonkar propose to "convert to draft" if this is not an active review

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

Successfully merging this pull request may close these issues.

Proposal to add error response for not supported method for phone number
6 participants