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

Add precision for Error message #44

Closed
bigludo7 opened this issue Jun 8, 2023 · 8 comments
Closed

Add precision for Error message #44

bigludo7 opened this issue Jun 8, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@bigludo7
Copy link
Collaborator

bigludo7 commented Jun 8, 2023

Problem description
We are a bit vague on some error message - we probably need to be more explicit - like for OTP.

Possible evolution
Add more error definition:

          PhoneNumberNotAllowed:
              value:
                status: 403
                code: SIMSWAP.PHONE_NUMBER_NOT_IN_OPERATOR_SCOPE
                message: SI Swap can't be checked because this number is not managed by operator.

@monamok @DT-DawidWroblewski WDYT ?

@bigludo7 bigludo7 added the enhancement New feature or request label Jun 8, 2023
@gregory1g
Copy link
Collaborator

403 is "Forbidden", this better suites a case when an API client does not have a consent to request SimSwap data.
If we would use a resource centric API, for numbers which are not managed by the CSP I would suggest 404 - not found.
But since we use procedure style here, this is not fully correct - the "resource" (which is a function in our case) is found. So, probably we should use 400 or a custom 4xx error code.

@bigludo7
Copy link
Collaborator Author

bigludo7 commented Jun 9, 2023

@gregory1g Thanks - it helps !
I'm fine with 400 as long as we provided a correct message

@monamok
Copy link
Collaborator

monamok commented Jun 13, 2023

Isn't it a scenario that could happen in all CAMARA APIs?
I'm not sure about the status code. 400 is Invalid argument. I think 404 fits better here.

@gregory1g
Copy link
Collaborator

yes, 404 is semantically better. the only issue is - 404 is designed for restful APIs, where the resource is represented by the URI,
From the REST point of view the resource is ".../sim-swap/v0/check". 404 could mislead developer - either they used incorrect URI or provided unknown MSISDN in the input.

404 could have some technical consequences as well, for example according to RFC (https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.4) it is cacheable by default - exactly this one is not relevant for POST, but still.

@bigludo7
Copy link
Collaborator Author

@gregory1g if we send the specific message with 404 : SI Swap can't be checked because this number is not managed by operator does it resolve the point for misleading developer?

The argument for cacheable data is not relevant there, correct ? but I guess this relevant in other discussion we have abour POST or GET.

@gregory1g
Copy link
Collaborator

agree, this looks like is a good-enough option.

bigludo7 added a commit to monamok/SimSwap that referenced this issue Jun 13, 2023
@bigludo7
Copy link
Collaborator Author

Thanks @gregory1g

@monamok I have update #40 to accomodate this.

@bigludo7
Copy link
Collaborator Author

Team meeting decision June 15/06
Change code to UNKNOWN_PHONE_NUMBER and message to Unknown phone number.

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

No branches or pull requests

4 participants