CCM-22090: Recipient response message retrieval API - #1090
Conversation
|
This branch is work on a ticket in the NHS Digital APM JIRA Project. Here's a handy link to the ticket: CCM-22090 |
|
This branch is work on a ticket in the NHS Digital APM JIRA Project. Here's a handy link to the ticket: CCM-22090 |
|
This branch is work on a ticket in the NHS Digital APM JIRA Project. Here's a handy link to the ticket: CCM-22090 |
|
This branch is work on a ticket in the NHS Digital APM JIRA Project. Here's a handy link to the ticket: CCM-22090 |
| @@ -0,0 +1,24 @@ | |||
| description: |+ | |||
| There are too many responses associated with this message to return in a single response. This error occurs when more than 1000 responses exist for the given message ID. | |||
There was a problem hiding this comment.
The problem with documenting this scenario is we really need some guidance on what to do in this scenario. I don't know what that guidance is though! It's raise a support query with us / NHS App but I don't think we want to document that.
My first question as a dev writing code to implement response handling is what do I do with this response code.
Baking it into the sandbox makes you think its something you'll need to deal with and test but the reality is it shouldn't ever happen.
I think we are better changing the code to return a 500 so the generic 500 documentation catches it.
There was a problem hiding this comment.
My personal view is if it's > HTTP 300 then it's an error, as an integrator you'd log it and go through failure handling. I'd personally treat any 4xx other than a 401 / 403 as an explicit error.
Technically that would be the case in this situation, we should never realistically have > 1000 responses, but also you could argue that this wouldn't always be our failure, it could be upstream triggering it.
I'm leaning towards a 500 being the right call, but it does imply a critical failure with our system. Is that appropriate?
There was a problem hiding this comment.
It could be a failure with our system (e.g. rogue response event is processed multiple times) or more likely a failure with the NHS App. Either way it isn't the clients fault and we probably need to take responsibility for it.
What do you think @cgitim ?
There was a problem hiding this comment.
Yeah I think that's a good point - 4xx usually implies it's the fault of the caller of the API, whereas 5xx implies the inverse. This makes me lean much more towards a 5xx error code, although 500 still implies a crash or some critical fault in my view.
Maybe a 503 is more appropriate? It does indicate a bit more clearly that this is a temporary, resolvable situation.
There was a problem hiding this comment.
I don't think its that temporary or retryable. We'll need a fairly hard change at our end do something about it.
I think we did discuss it in refinement and the ticket does have "If >100 responses are received return a 500"
https://nhsd-jira.digital.nhs.uk/browse/CCM-22090
|
This branch is work on a ticket in the NHS Digital APM JIRA Project. Here's a handy link to the ticket: CCM-22090 |
mjewildnhs
left a comment
mjewildnhs
left a comment
There was a problem hiding this comment.
I've paused reviewing until we get an answer on whether we should be following the JSON:API convention as per my review comment.
There was a problem hiding this comment.
The error responses we are re-using all have CM_ prefix error codes.
I think we still need to take an decision on our deviation away from the JSON:API convention used by the other endpoints.
Does CM_ stand for comms-mgr?
I think its this list of error codes: https://github.com/NHSDigital/comms-mgr/blob/main/packages/libs/ingestion-lib/src/error-responses.ts#L95-L94
As it stands we are re-using the following error codes:
401 Access denied → CM_DENIED
403 Forbidden → CM_FORBIDDEN or CM_SERVICE_BAN
404 Not found → CM_NOT_FOUND
405 Method not allowed → CM_NOT_ALLOWED
408 Request timeout → CM_TIMEOUT
415 Unsupported media → CM_UNSUPPORTED_MEDIA
429 Too many requests → CM_QUOTA
500 Internal server error → CM_INTERNAL_SERVER_ERROR
503 Service unavailable → CM_SERVICE_UNAVAILABLE
504 Service timeout → CM_TIMEOUT
We'd need to check what error message and what API gateway does for some of these scenarios.
However I think for now we need to stop and have a conversation about our deviation from the JSON:API convention before making further change.
I suspect we don't want to return CM_ errors which would be a bit annoying as the CM_ errors have occupied specification/schemas/enums and might need moving/scoping.
If none of the JSON:API stuff has been discussed already then I think we need some input from @cgitim
3 participants
Summary
Adds proxy configuration for new recipient response message retrieval API.
Reviews Required
Checklist