@W-21053358 Handle 429 response from svc-api gracefully by adding NPE checks and relevant messages#40
Conversation
…sage to the response Added a map to associate HTTP status codes with error messages and updated error handling to utilize this map.
|
Thanks for the contribution! Before we can merge this, we need @SaranshSinha to sign the Salesforce Inc. Contributor License Agreement. |
Add relevant message handling for associated error codes.
Removed the status code to error message mapping and simplified error handling in the buildErrorFromClientResponseBodyString method.
| if (contentType.contains("application/json")) { | ||
| HttpHeaders headers = inputMessage.getHeaders(); | ||
| MediaType contentType = headers != null ? headers.getContentType() : null; | ||
| if (contentType != null && contentType.toString().contains("application/json")) { |
There was a problem hiding this comment.
This check is case sensitive, check if you need case insensitive check (recommended).
There was a problem hiding this comment.
Also check if you need to support formats like application/problem+json and application/vnd.api+json
| BodyExtractor<Mono<Error>, ReactiveHttpInputMessage> extractor = (inputMessage, context) -> { | ||
| String contentType = inputMessage.getHeaders().getContentType().toString(); | ||
| if (contentType.contains("application/json")) { | ||
| HttpHeaders headers = inputMessage.getHeaders(); | ||
| MediaType contentType = headers != null ? headers.getContentType() : null; | ||
| if (contentType != null && contentType.toString().contains("application/json")) { | ||
| return BodyExtractors.toMono(Error.class) | ||
| .extract(inputMessage, context); | ||
| } else { | ||
| return buildErrorFromClientResponseBodyString(inputMessage, context); | ||
| } | ||
| }; | ||
| return extractor; |
There was a problem hiding this comment.
A simpler suggestion:
| return (inputMessage, context) -> { | |
| final MediaType contentType = inputMessage.getHeaders().getContentType(); | |
| if (MediaType.APPLICATION_JSON.includes(contentType)) { | |
| return BodyExtractors | |
| .toMono(Error.class) | |
| .extract(inputMessage, context); | |
| } | |
| return buildErrorFromClientResponseBodyString(inputMessage, context); | |
| }; |
There was a problem hiding this comment.
As per the spec /einstein-bot-sdk-java/src/main/resources/v5_3_0_api_specs.yml, only application/json is returned for all requests. So I think the current case sensitive check should be more than sufficient.
There was a problem hiding this comment.
Yes, the suggested code does the same
| } | ||
|
|
||
| } No newline at end of file | ||
| @Test |
|
|
||
| ChatbotResponseException chatbotResponseException = validateAndGetCause(exceptionThrown, | ||
| ChatbotResponseException.class); | ||
| assertEquals(HttpStatus.TOO_MANY_REQUESTS.value(), chatbotResponseException.getStatus()); |
There was a problem hiding this comment.
We are stubbing this response in line 416 along with a null contentType. This basically mocks the current flow where the sdk breaks due to NPE. Now the test verifies that the NPE issue is fixed for such responses
There was a problem hiding this comment.
Hmm... I'm not convinced on the 429 part. If the issue is with content type, it should have been a 415.
There was a problem hiding this comment.
The issue is with a "null" content-type. GRLS metrics confirmed that it was throwing 429 and we faced this NPE during that time interval
Removed unnecessary blank line and fixed formatting in the WebClientUtil class.
3 participants
https://gus.lightning.force.com/lightning/r/ADM_Work__c/a07EE00002TdTMgYAN/view