feat(connect): add http connector response error handling #4447
Conversation
…error handling enhancement for 4XX and 5XX errors Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors -GIT Issue 4241 Signed-off-by: Shenoy, Nandan <[email protected]>
…ror handling enhancement for 4XX and 5XX errors Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors -GIT Issue 4241 Signed-off-by: Shenoy, Nandan <[email protected]>
…handling enhancement for 4XX and 5XX errors Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors -GIT Issue 4241 Signed-off-by: Shenoy, Nandan <[email protected]>
…handling enhancement for 4XX and 5XX errors Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors -GIT Issue 4241 Signed-off-by: Shenoy, Nandan <[email protected]>
|
@danielkelemen , Thanks and Regards, |
There was a problem hiding this comment.
Hi @Nandanrshenoy, Thanks for the PR!
👍 It looks pretty nice and thank you for adding tests, too.
🔧 I added a few suggestions to separate the http error response from communication errors.
🔧 Code formatting is off at several places. Could you reformat the code according to our style guidelines? Thank you!
| @@ -59,4 +59,9 @@ public void ignoreConfig(String field, Object value) { | |||
| logInfo("009", "Ignoring request configuration option with name '{}' and value '{}'", field, value); | |||
| } | |||
|
|
|||
| public ConnectorRequestException unableToExecuteRequest(int statusCode , String connectorResponse) { | |||
| return new ConnectorRequestException(exceptionMessage("010", "Unable to execute HTTP request with Status Code : " | |||
There was a problem hiding this comment.
🔧 I'd rename this to httpRequestError or something similar and the message could also say something along HTTP request failed with status code: 500...
🔧 Let's also use the message template with parameters instead of string concatenation, like: "HTTP request failed with Status Code: {} , Response: {}", statusCode, connectorResponse)
| throw LOG.unableToExecuteRequest(e); | ||
| } | ||
|
|
||
| R invocationResult = createResponse((CloseableHttpResponse) invocation.proceed()); |
There was a problem hiding this comment.
🔧 Let's restructure this because this way the http error exception is caught and rethrown as unableToExecuteRequest: Move the new logic out from the try catch so it's not caught:
R invocationResult;
try {
invocationResult = createResponse((CloseableHttpResponse) invocation.proceed());
} catch (Exception e) {
throw LOG.unableToExecuteRequest(e);
}
handleErrorResponse(request, invocationResult);
return invocationResult;| Optional<Object> handleHttpError = Optional.ofNullable(configOptions.get("throw-http-error")); | ||
|
|
||
| if (handleHttpError.isPresent() && handleHttpError.get().toString() | ||
| .equalsIgnoreCase("TRUE") && statusCode >= 400 && statusCode <= 599) { |
There was a problem hiding this comment.
🔧 Maybe a bit more simple with orElse() and Boolean.parseBoolean:
| Optional<Object> handleHttpError = Optional.ofNullable(configOptions.get("throw-http-error")); | |
| if (handleHttpError.isPresent() && handleHttpError.get().toString() | |
| .equalsIgnoreCase("TRUE") && statusCode >= 400 && statusCode <= 599) { | |
| Object handleHttpError = Optional.ofNullable(configOptions.get("throw-http-error")).orElse("FALSE"); | |
| if (Boolean.parseBoolean(handleHttpError.toString()) && statusCode >= 400 && statusCode <= 599) { |
danielkelemen
changed the title
| connector.createRequest().configOption("throw-http-error", "TRUE") | ||
| .url("http://camunda.com").get() | ||
| .execute(); | ||
| } catch (ConnectorRequestException e) { |
There was a problem hiding this comment.
🔧 Let's add a fail in these tests so we notice if the logic is skipped for some reason. Otherwise, the test would still succeed.
| connector.createRequest().configOption("throw-http-error", "TRUE") | |
| .url("http://camunda.com").get() | |
| .execute(); | |
| } catch (ConnectorRequestException e) { | |
| connector.createRequest().configOption("throw-http-error", "TRUE") | |
| .url("http://camunda.com").get() | |
| .execute(); | |
| Assertions.fail("Should have thrown an exception"); | |
| } catch (ConnectorRequestException e) { |
| // then | ||
| assertThat(response.getStatusCode()).isEqualTo(200); | ||
| } | ||
|
|
There was a problem hiding this comment.
🔧 And please also add one more test case where throw-http-error is false and statusCode is an error (400-500). Thank you!
…rror handling enhancement for 4XX and 5XX errors Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors -GIT Issue 4241 Signed-off-by: Shenoy, Nandan <[email protected]>
…or handling enhancement for 4XX and 5XX errors Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors -GIT Issue 4241 Signed-off-by: Shenoy, Nandan <[email protected]>
…andling enhancement for 4XX and 5XX errors Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors -GIT Issue 4241 Signed-off-by: Shenoy, Nandan <[email protected]>
…rror handling enhancement for 4XX and 5XX errors Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors -GIT Issue 4241 Signed-off-by: Shenoy, Nandan <[email protected]>
|
@danielkelemen , Thanks and Regards, |
|
@Nandanrshenoy, Thank you! It looks really good. I'll run the CI in a local PR and then it'll be merged. |
|
@danielkelemen,Thanks for all your support. |
|
Superseded with 42c77fc. |
Contribution towards Camunda HTTP Connector enhancement for 4XX and 5XX errors
related to #4241