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

Enhancements and Alignments in Errors #321

Open
PedroDiez opened this issue Oct 18, 2024 · 16 comments · May be fixed by #329
Open

Enhancements and Alignments in Errors #321

PedroDiez opened this issue Oct 18, 2024 · 16 comments · May be fixed by #329
Assignees
Labels
enhancement New feature or request Spring25

Comments

@PedroDiez
Copy link
Collaborator

PedroDiez commented Oct 18, 2024

Problem description
Two enhancements are proposed inder this issue:

  • Normalize mandatory errors that are common to every CAMARA API.
  • Define specific Errors for phoneNumber concept in the same fashion as done for device in MetaRelease Fall 24 for device error responses

Possible evolution

  1. Normalize Mandatory Error status
  • Add within section 6.1 Standardized use of CAMARA error responses the following information:

Mandatory Errors to be documented in CAMARA API Spec YAML are the following:

Error status 401
Error status 403
Error status 429 TOO_MANY_REQUESTS (For rate limit control)

NOTE: The rest of Error status defined in section 6.1. will apply depending on specific considerations within the given WG.

  1. Define specific Errors for phoneNumber concept

Add a new section 6.3 Error Responses - phoneNumber to detail error scenarios for phoneNumber

Case # Description Error status Error code Message example
0 The request body does not comply with the schema 400 INVALID_ARGUMENT Request body does not comply with the schema.
1 The phoneNumber provided is not supported by the implementation 422 UNSUPPORTED_PHONE_NUMBER phoneNumber is required.
2 phoneNumber is not found. 404 PHONE_NUMBER_NOT_FOUND phoneNumber not found.
3 Invalid access token context 403 INVALID_TOKEN_CONTEXT phoneNumber is not consistent with access token.
4 Service not applicable to the phoneNumber 422 PHONE_NUMBER_NOT_APPLICABLE The service is not available for the provided phoneNumber.
5 The phone number is not included in the request and the phone number information cannot be derived from the 3-legged access token 422 UNIDENTIFIABLE_PHONE_NUMBER The phoneNumber cannot be identified.
6 The phoneNumber is unnecesarily included in a 3-legged scenario where the subscription is identified from the 3-legged access token 422 UNNECESSARY_PHONE_NUMBER The phoneNumber is unnecesarily provided

NOTE: New exceptions would be added to CAMARA_common.yaml for reference

Alternative solution

Additional context

There are CAMARA APIs where phoneNumber represents the end's user subscription (e.g SIM-SWAP, Number Verification, Carrier Billing, KYC family,...). Therefore having specific errors related to the concept of phoneNumber is the natural way to address error scenarios in a fully semantic and user-friendly way

Proposed as a child issue of #306

@PedroDiez PedroDiez added enhancement New feature or request Spring25 labels Oct 18, 2024
@PedroDiez
Copy link
Collaborator Author

@eric-murray
Copy link
Collaborator

I don't like the proposal to maintain a parallel set of error codes dependent on whether the API used device or just phoneNumber as the optional "API subject" identifier to be used with 2-legged access tokens.

In all cases, the Device object could have been used, but for various reasons the simpler phoneNumber was used. And whilst someone may think that they do not want, for example, their API to support identifying mobile subscriptions from IP addresses, by accepting 3-legged access tokens they enable exactly that.

This is more than semantics. Having a parallel set of error codes complicates implementation logic for what is the same logical error, as the error to be returned is dependent solely on the options offered to identify the API subject. And I think application developers will also get confused if they are working with several APIs, as they end up implementing parallel but identical error traps for the "device" and "phone number" error codes.

Why, for example, should the error codes be different for:

POST /check
{ "device" : { "phoneNumber" : "+1234567890" } }

and

POST /check
{ "phoneNumber" : "+1234567890" }

I think it would be better if we could just agree on a common term that applies to both the "device" and the "subscription" (or "account" or whatever), both of which just identify the API subject. Given that application developers will have to get familiar with 3-legged tokens, and given that the ID token refers to device or subscription as the "subject", irrespective of how it was identified, I'd propose the term "API_SUBJECT" to replace both "DEVICE" and "PHONE_NUMBER".

So we could have (note that some of these are likely to become redundant shortly):
UNSUPPORTED_API_SUBJECT_IDENTIFIER
API_SUBJECT_NOT_FOUND
SERVICE_NOT_APPLICABLE_TO_API_SUBJECT
API_SUBJECT_CANNOT_BE_IDENTIFIED
UNNECESSARY_API_SUBJECT_IDENTIFIER
API_SUBJECT_IDENTIFIER_MISMATCH

In all cases, the exact error can be clarified in the message, which will only generally be read by a person and so can be used to resolve their confusion. Unlike the error code, which must be read and processed by a machine, which will not care about the exact name used.

@jlurien
Copy link
Contributor

jlurien commented Oct 22, 2024

After reviewing this with @PedroDiez, we think that certain standard HTTP codes would not need to be documented explicitly for every API operation. In particular those 4xx codes returned when the client sends something not specified as accepted by the API, as that is the standard expected behaviour:

  • 405 METHOD_NOT_ALLOWED: In OAS only the supported methods are documented, so any other method not explicitly indicated will get 405, but we do not need to include those not supported methods in the spec with a 405 response.
  • 406 NOT_ACCEPTABLE: OAS defines what it is accepted
  • 415 UNSUPPORTED_MEDIA_TYPE: OAS defines the accepted content types

5xx errors are unexpected errors and quite standard as well, so we may discuss also whether they need to be explicitly indicated.

Regarding distinguishing error codes for device vs phoneNumber. there is a good point in @eric-murray's comment, but the term "API_SUBJECT" I don't think is friendly enough. We suggest to simplify the codes removing the references to "device" as generally the term "identifier" is enough in this context, and we also have the message to add additional details if necessary, so we could consolidate the codes as:

  • UNSUPPORTED_API_SUBJECT_IDENTIFIER -> UNSUPPORTED_IDENTIFIER
  • API_SUBJECT_NOT_FOUND -> IDENTIFIER_NOT_FOUND
  • SERVICE_NOT_APPLICABLE_TO_API_SUBJECT -> SERVICE_NOT_APPLICABLE
  • API_SUBJECT_CANNOT_BE_IDENTIFIED -> MISSING_IDENTIFIER
  • UNNECESSARY_API_SUBJECT_IDENTIFIER -> UNNECESSARY_IDENTIFIER
  • API_SUBJECT_IDENTIFIER_MISMATCH -> IDENTIFIER_MISMATCH

If we decide to change the codes from Fall24 meta, for Spring25 one, that would imply a compatibility breaking change to released APIs. Probably, we should not release a new version just to adopt the new codes, but if a new version is released to include additional functionality, then the new codes must be adopted.

@eric-murray
Copy link
Collaborator

@jlurien
Yes, I'm fine with this proposal

@patrice-conil
Copy link
Collaborator

@jlurien
I'm also fine with this proposal.

@jlurien
Copy link
Contributor

jlurien commented Oct 24, 2024

Thanks @eric-murray @patrice-conil, Regarding the inclusion or not of 500, 503 and 504, what is your preference?

@eric-murray
Copy link
Collaborator

@jlurien
I don't think explicitly including 500, 503 and 504 in the YAML is particularly useful, so my preference is to have these in the list of common errors that any API might return, but not list them in the YAML.

@PedroDiez
Copy link
Collaborator Author

NOTE: This proposal has to be in line with #324

@eric-murray
Copy link
Collaborator

NOTE: This proposal has to be in line with #324

I can update that PR if these changes are agreed first. All that would change would be the error codes.

@PedroDiez
Copy link
Collaborator Author

NOTE: This proposal has to be in line with #324

I can update that PR if these changes are agreed first. All that would change would be the error codes.

There are two parts Eric in this issue. It is fine to unify the second one within #324 with error codes are agreed. The proposal is the one commented by Jose.

An I can generate PR for the first part (Add within section 6.1 Standardized use of CAMARA error responses the following information:...., based on the agreement reached here)

@rartych
Copy link
Collaborator

rartych commented Oct 25, 2024

@PedroDiez @eric-murray
Can we have the agreed changes included in #316 (marked as priority) or should we have another PR (also with priority label) to smoothly proceed the changes?

@patrice-conil
Copy link
Collaborator

@jlurien I don't think explicitly including 500, 503 and 504 in the YAML is particularly useful, so my preference is to have these in the list of common errors that any API might return, but not list them in the YAML.

@jlurien As @eric-murray I prefer to not include 50X errors in the YAML.

@PedroDiez
Copy link
Collaborator Author

@PedroDiez @eric-murray Can we have the agreed changes included in #316 (marked as priority) or should we have another PR (also with priority label) to smoothly proceed the changes?

I think that regarding errors with regards to identifiers we can align on #324, adding the new ones (Indeed Eric already covered some of them)

UNSUPPORTED_API_SUBJECT_IDENTIFIER -> UNSUPPORTED_IDENTIFIER
API_SUBJECT_NOT_FOUND -> IDENTIFIER_NOT_FOUND
SERVICE_NOT_APPLICABLE_TO_API_SUBJECT -> SERVICE_NOT_APPLICABLE
API_SUBJECT_CANNOT_BE_IDENTIFIED -> MISSING_IDENTIFIER
UNNECESSARY_API_SUBJECT_IDENTIFIER -> UNNECESSARY_IDENTIFIER
API_SUBJECT_IDENTIFIER_MISMATCH -> IDENTIFIER_MISMATCH

Within #316 enhance the proposal with the pending errors (so I take the part of updating CAMARA_Common.yaml)

And associated with this issue enhance the section 6.1 also from my side

cc @rartych, @eric-murray WDYT?

@bigludo7
Copy link
Collaborator

After reviewing this with @PedroDiez, we think that certain standard HTTP codes would not need to be documented explicitly for every API operation. In particular those 4xx codes returned when the client sends something not specified as accepted by the API, as that is the standard expected behaviour:

  • 405 METHOD_NOT_ALLOWED: In OAS only the supported methods are documented, so any other method not explicitly indicated will get 405, but we do not need to include those not supported methods in the spec with a 405 response.
  • 406 NOT_ACCEPTABLE: OAS defines what it is accepted
  • 415 UNSUPPORTED_MEDIA_TYPE: OAS defines the accepted content types

@jlurien @PedroDiez regarding this point, does it mean also that we have not to provide test definition against these codes?

There is one test for OTP API against 415 for example.
I tend to like consistency and no test what is not defined in the yaml but probably good to have a global alignement.

@PedroDiez
Copy link
Collaborator Author

After reviewing this with @PedroDiez, we think that certain standard HTTP codes would not need to be documented explicitly for every API operation. In particular those 4xx codes returned when the client sends something not specified as accepted by the API, as that is the standard expected behaviour:

  • 405 METHOD_NOT_ALLOWED: In OAS only the supported methods are documented, so any other method not explicitly indicated will get 405, but we do not need to include those not supported methods in the spec with a 405 response.
  • 406 NOT_ACCEPTABLE: OAS defines what it is accepted
  • 415 UNSUPPORTED_MEDIA_TYPE: OAS defines the accepted content types

@jlurien @PedroDiez regarding this point, does it mean also that we have not to provide test definition against these codes?

There is one test for OTP API against 415 for example. I tend to like consistency and no test what is not defined in the yaml but probably good to have a global alignement.

Hi Ludovic,
have talk internally and this is our view:

  • As general rule, as not mandatory "statuses" are not needed to be documented in API Specs, there should not be tests against these codes. This would be a general rule for 405, 406 and 415.

  • Tests have to be aligned with API Specs so as to have tests for a specific "status", such "status" MUST be documented in the API Spec. This is something to be managed and agreed at WG level because maybe one of non-mandatory "status" at Commonalities level is relevant in the context of a given API and deserves to have a specific test (e.g. 422 is not mandatory but relevant for APIs with the device concept in the request).

  • In the specific case for OTP API, we also think 415 "status" and related test are not needed to be documented.

@bigludo7
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Spring25
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants