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.

Sign up for GitHub

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

Jump to bottom

Added 501 Not Supported error response #145

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −0
Draft

Added 501 Not Supported error response #145

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions artifacts/CAMARA_common.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

description: Not Supported
content:
application/json:
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 •
edited
Loading

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 •
edited
Loading

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.

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.

message: Service not supported for this phone number
AuthenticationRequired:
description: Authentication Required
content:
Expand Down