Update device error model #232

fernandopradocabrillo · 2024-12-05T16:10:21Z

What type of PR is this?

Add one of the following kinds:

correction
enhancement/feature
documentation

What this PR does / why we need it:

Align error model with Commonalities for meta Spring25

Which issue(s) this PR fixes:

Fixes #230

Special notes for reviewers:

In the subscriptions APIs there is one error that I'm not sure if we have to update or not.

TokenMismatch:
   value:
    status: 403
    code: SUBSCRIPTION_MISMATCH
    message: Inconsistent access token for requested events subscription

It looks like it is the equivalent to 403 INVALID_TOKEN_CONTEXT. It may make sense to keep it since the device object is required when creating the subscription, regardless of the token type. I tend to think we should keep it.

bigludo7 · 2024-12-06T19:59:09Z

@fernandopradocabrillo Agree with you to keep it but in order to be explicit the code should not be: INVALID_TOKEN_CONTEXT ?

bigludo7 · 2024-12-08T15:23:13Z

Hi @fernandopradocabrillo
Additional thinking:

The callback must have 429 in device-roaming-status-subscriptions.yaml (in lines 140/155)

Following points are open:

Do we keep 500, 501, 502, 503 & 504 in the yamls ? they are not anymore mandatory to be defined
429 is not mandatory for the GET /subscriptions/{subscriptionId} & GET /subscriptions.
same for DELETE

fernandopradocabrillo · 2024-12-10T09:40:21Z

@fernandopradocabrillo Agree with you to keep it but in order to be explicit the code should not be: INVALID_TOKEN_CONTEXT ?

I'm not sure, I see that in geofencing it is also SUBSCRIPTION_MISMATCH: https://github.com/camaraproject/DeviceLocation/blob/main/code/API_definitions/geofencing-subscriptions.yaml#L1166

do you know if there is any conversation right now in commonalities regarding the errors in subscriptions?

akoshunyadi · 2024-12-18T13:48:02Z

@fernandopradocabrillo would it be possible to cover the 2 new APIs here as well? 🙂 (connected-network-type-subscriptions is still to be merged)

…into update-device-error-model

fernandopradocabrillo · 2024-12-19T12:10:56Z

@fernandopradocabrillo would it be possible to cover the 2 new APIs here as well? 🙂 (connected-network-type-subscriptions is still to be merged)

yeah and I won't charge extra

fernandopradocabrillo · 2024-12-19T17:32:37Z

@camaraproject/device-status_maintainers shouldn't the POST /subscription endpoint also have the 404 IDENTIFIER_NOT_FOUND error?

…into update-device-error-model

fernandopradocabrillo · 2025-01-21T13:06:42Z

@camaraproject/device-status_maintainers connected network subscriptions aligned.

What strategy should we follow for non-mandatory errors? What errors do you think we should keep/remove?

akoshunyadi · 2025-01-22T15:42:32Z

@camaraproject/device-status_maintainers shouldn't the POST /subscription endpoint also have the 404 IDENTIFIER_NOT_FOUND error?

You mean for missing phone number? For that case I think 422 - MISSING_IDENTIFIER should be returned

akoshunyadi · 2025-01-26T19:56:30Z

For the subscription APIs there is quite much overlapping with #250. Maybe we should work on the subscription APIs there and only on the retrieve APIs here. @fernandopradocabrillo what do you think?

fernandopradocabrillo · 2025-01-27T09:41:04Z

For the subscription APIs there is quite much overlapping with #250. Maybe we should work on the subscription APIs there and only on the retrieve APIs here. @fernandopradocabrillo what do you think?

@akoshunyadi agree, it made sense earlier but now there are too many conflicts, I'll remove the subscription changes and leave only the retrieve yamls

bigludo7

@fernandopradocabrillo - look good for me except that we do not have the 'new' 429 message. Let me know if you want to handle in this one or in other PR.

Should be (as defined in Defined in Commonalities camaraproject/Commonalities#390 )

          examples:
            GENERIC_429_QUOTA_EXCEEDED:
              description: Request is rejected due to exceeding a business quota limit
              value:
                status: 429
                code: QUOTA_EXCEEDED
                message: Out of resource quota.
            GENERIC_429_TOO_MANY_REQUESTS:
              description: Access to the API has been temporarily blocked due to rate or spike arrest limits being reached
              value:
                status: 429
                code: TOO_MANY_REQUESTS
                message: Rate limit reached.

bigludo7 · 2025-01-28T12:38:07Z

code/API_definitions/connected-network-type.yaml

+                    enum:
+                      - QUOTA_EXCEEDED
+                      - TOO_MANY_REQUESTS
+          examples:


We have to align message fieldd and description for Error 429
Defined in Commonalities camaraproject/Commonalities#390

examples: GENERIC_429_QUOTA_EXCEEDED: description: Request is rejected due to exceeding a business quota limit value: status: 429 code: QUOTA_EXCEEDED message: Out of resource quota. GENERIC_429_TOO_MANY_REQUESTS: description: Access to the API has been temporarily blocked due to rate or spike arrest limits being reached value: status: 429 code: TOO_MANY_REQUESTS message: Rate limit reached.

done, thanks!

bigludo7 · 2025-01-28T12:39:01Z

code/API_definitions/device-reachability-status.yaml

+                  code:
+                    enum:
+                      - QUOTA_EXCEEDED
+                      - TOO_MANY_REQUESTS


Same - probably easy to align for new 429 message.

bigludo7 · 2025-01-28T12:39:36Z

code/API_definitions/device-roaming-status.yaml

+                      - 429
+                  code:
+                    enum:
+                      - QUOTA_EXCEEDED


eric-murray · 2025-01-28T16:03:49Z

code/API_definitions/connected-network-type.yaml

+                      - 401
+                  code:
+                    enum:
+                      - UNAUTHENTICATED


There is an open issue in Commonalities to discuss the future of UNAUTHENTICATED / AUTHENTICATION_REQUIRED. I'm happy just to have UNAUTHENTICATED for now, but let's not forget to review again after Commonalities have made a decision.

I'm also happy with UNAUTHENTICATED

code/API_definitions/device-reachability-status-subscriptions.yaml

eric-murray · 2025-01-28T16:32:26Z

@fernandopradocabrillo
Whilst reviewing, I noted that your fork does not appear to be synchronised to the latest version of the repository. You have clobbered all the recent changes to device-reachability-status-subscriptions.yaml, for example.

Can you sync your fork and resolve any outstanding conflicts first before I continue my review? Thanks.

fernandopradocabrillo · 2025-01-28T17:09:21Z

@fernandopradocabrillo Whilst reviewing, I noted that your fork does not appear to be synchronised to the latest version of the repository. You have clobbered all the recent changes to device-reachability-status-subscriptions.yaml, for example.

Can you sync your fork and resolve any outstanding conflicts first before I continue my review? Thanks.

I was trying to remove all the changes I included for the subscriptions since a new PR was raised duplicating them and including new ones, but it has been a mess so yeah, I'll simply pull the changes from main and try to sync

…into update-device-error-model

fernandopradocabrillo · 2025-01-28T17:43:24Z

@akoshunyadi

@camaraproject/device-status_maintainers shouldn't the POST /subscription endpoint also have the 404 IDENTIFIER_NOT_FOUND error?

You mean for missing phone number? For that case I think 422 - MISSING_IDENTIFIER should be returned

IDENTIFIER_NOT_FOUND is the error to return when the API consumer specifies a valid identifier (e.g. MSISDN), but that MSISDN is not managed by the API provider (i.e. the MSISDN is managed by another network operator). In an aggregated world, hopefully this will never happen, but it might if the API consumer is directly consuming the API from the network operator.

maybe we can move this discussion to PR #250 and include the error there if it is needed. In theory it could happen that in a 2-legged scenario, the client specifies a valid msisdn that doesn't belong to the operator

akoshunyadi · 2025-02-02T15:29:53Z

code/API_definitions/device-reachability-status.yaml

              value:
                status: 422
                code: UNABLE_TO_PROVIDE_REACHABILITY_STATUS
                message: Network issue - Unable to provide reachability status


Is this case really a 422 Unprocessable Content ? If the request is incorrect, we should tell which parameter is wrong. But this one looks like a 5xx error, maybe 503, to me.

isn't this error for when it is not possible to return the real-time status of the device? I'm not sure it should be a 5XX, that would be telling the client to not retry the request with the same data, but this might be a temporary error

if only the referenced device is affected, not generally, then I would add the "device" to the case. e.g.
GENERIC_422_UNABLE_TO_PROVIDE_REACHABILITY_STATUS_FOR_DEVICE:
code: UNABLE_TO_PROVIDE_REACHABILITY_STATUS_FOR_DEVICE

@bigludo7 @eric-murray @sachinvodafone Do we need this service specific code at all? Is there a use case for that?

I agree this should be a 503. That assumes that the error is temporary and that normally a status can be returned for the device.

If there are devices for which a reachability status can never be returned, that is 422 SERVICE_NOT_APPLICABLE.

I'm fine with @eric-murray proposal and not introduce a specific code.
Perhaps worth to ad a note in commonalities as 503 definition is "the server is currently unable to handle the request due to a temporary overload or scheduled maintenance" - here we have one additional case - we're not able to provide the service for a device we're supposed to be able to provide.

@bigludo7
Yes, agree. We can explicitly include 503 in the YAML with the "network issue" message as an example.

Updated accordingly

akoshunyadi · 2025-02-02T15:33:03Z

code/API_definitions/device-roaming-status.yaml

              value:
                status: 422
                code: UNABLE_TO_PROVIDE_ROAMING_STATUS
                message: Network issue - Unable to provide roaming status


Same as above: Is this case really a 422 Unprocessable Content ? If the request is incorrect, we should tell which parameter is wrong. But this one looks like a 5xx error, maybe 503, to me.

Same comment as above:

Temporary problem: 503

Permanent problem: 422 SERVICE_NOT_APPLICABLE

akoshunyadi · 2025-02-02T15:36:53Z

code/API_definitions/connected-network-type.yaml

-          $ref: "#/components/responses/Generic503"
+          $ref: "#/components/responses/Generic422"
+        "429":
+          $ref: "#/components/responses/Generic429"


This API doesn't have 406 and 415 as the other 2. I think for consistency reason the responses should be same for all 3 APIs. I'm not sure if we need those though...

I think that the tendency is to remove unused errors, maybe what we have to do is delete them from the other two apis

I agree. We should remove 415 and 406 from all APIs. Neither is required to be documented, and their use in these APIs is nothing out of the ordinary for those status codes, and not related to the use case.

Agree as well - we should remove 415 & 406.

maxl2287

LGTM

maxl2287 · 2025-02-07T21:46:56Z

@eric-murray @sachinvodafone @bigludo7 as this PR is blocking the test-PRs, could you be so kind and review this again please?

Many thanks in advance.

eric-murray

I don't see any scenario that would result in 404 NOT_FOUND, but we can revisit that at a later date

bigludo7

LGTM

maxl2287 · 2025-02-10T08:30:06Z

@akoshunyadi @eric-murray can we merge this now?
I will update everything regarding the error models after this PR and #251 is merged during my #255.

Wdyt?

eric-murray · 2025-02-10T09:21:59Z

Yes, we should merge this now, and then review other PRs.

I can merge if @akoshunyadi is unavailable

maxl2287 · 2025-02-10T09:39:48Z

Thanks! :)

maxl2287 · 2025-02-10T12:23:42Z

Addiotional remark

This PR also fixes #254
Fixes #254

fernandopradocabrillo added 3 commits December 5, 2024 16:43

update device reachability errors

d2504bc

update device roaming errors

7578a1b

update subscriptions error model

61ec76a

fernandopradocabrillo requested review from akoshunyadi, bigludo7, eric-murray and sachinvodafone as code owners December 5, 2024 16:10

akoshunyadi added the Spring25 label Dec 12, 2024

akoshunyadi mentioned this pull request Dec 18, 2024

Scope for Spring25 release #224

Closed

Merge branch 'main' of https://github.com/camaraproject/DeviceStatus …

7112ce4

…into update-device-error-model

fernandopradocabrillo added 5 commits December 20, 2024 12:55

adapt to latest error model from commonalities

1b9f17b

fix linter

5bcb4f2

remove trailing spaces

a822248

Merge branch 'main' of https://github.com/camaraproject/DeviceStatus …

4eb54d1

…into update-device-error-model

align errors subscriptions

f2de84d

akoshunyadi added the RC1 label Jan 25, 2025

remove changes from subscriptions

dc1525c

bigludo7 reviewed Jan 28, 2025

View reviewed changes

eric-murray reviewed Jan 28, 2025

View reviewed changes

revert subscripotion changes

f089e13

eric-murray reviewed Jan 28, 2025

View reviewed changes

code/API_definitions/device-reachability-status-subscriptions.yaml Show resolved Hide resolved

fernandopradocabrillo added 4 commits January 28, 2025 18:09

update error 429

e62d505

Merge branch 'main' of https://github.com/camaraproject/DeviceStatus …

f5fa5e0

…into update-device-error-model

pull subscriptions from main

5091776

eof

661d45a

fernandopradocabrillo added 2 commits January 29, 2025 10:20

remove 5XX errors

6c36277

include x-correlator pattern

dfd4562

akoshunyadi reviewed Feb 2, 2025

View reviewed changes

remove non-mandatory errors

dc7bc1c

akoshunyadi approved these changes Feb 6, 2025

View reviewed changes

maxl2287 requested review from bigludo7 and eric-murray February 7, 2025 09:40

maxl2287 approved these changes Feb 7, 2025

View reviewed changes

eric-murray mentioned this pull request Feb 7, 2025

Add basic testcases for the new APIs conncted-network-type and connected-network-type-subscriptions #251

Merged

maxl2287 mentioned this pull request Feb 7, 2025

Add extended test cases for device-roaming-status and device-reachability-status + refactor *.feature-files #255

Merged

sachinvodafone approved these changes Feb 7, 2025

View reviewed changes

eric-murray approved these changes Feb 8, 2025

View reviewed changes

bigludo7 approved these changes Feb 9, 2025

View reviewed changes

akoshunyadi merged commit b94f50c into camaraproject:main Feb 10, 2025
2 checks passed

maxl2287 mentioned this pull request Feb 10, 2025

Allow zero-length strings in x-correlator header #254

Closed

Update device error model #232

Update device error model #232

Uh oh!

Conversation

fernandopradocabrillo commented Dec 5, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewers:

Uh oh!

bigludo7 commented Dec 6, 2024

Uh oh!

bigludo7 commented Dec 8, 2024

Uh oh!

fernandopradocabrillo commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akoshunyadi commented Dec 18, 2024

Uh oh!

fernandopradocabrillo commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fernandopradocabrillo commented Dec 19, 2024

Uh oh!

fernandopradocabrillo commented Jan 21, 2025

Uh oh!

akoshunyadi commented Jan 22, 2025

Uh oh!

akoshunyadi commented Jan 26, 2025

Uh oh!

fernandopradocabrillo commented Jan 27, 2025

Uh oh!

bigludo7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eric-murray commented Jan 28, 2025

Uh oh!

fernandopradocabrillo commented Jan 28, 2025

Uh oh!

fernandopradocabrillo commented Jan 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fernandopradocabrillo commented Dec 10, 2024 •

edited

Loading

fernandopradocabrillo commented Dec 19, 2024 •

edited

Loading