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.

Sign up for GitHub

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

fix: reworked HTTP status code handling for 2xx and 3xx ranges (MWTELE-68, MWTELE-69) #83

Merged
merged 1 commit into from
May 11, 2023
Merged

fix: reworked HTTP status code handling for 2xx and 3xx ranges (MWTELE-68, MWTELE-69) #83

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions runtime/src/main/java/com/redhat/insights/core/httpclient/InsightsJdkHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ HttpClient getHttpClient() {
ProxySelector.of(new InetSocketAddress(conf.getHost(), conf.getPort())));
}

return clientBuilder.build();
return clientBuilder.followRedirects(HttpClient.Redirect.NORMAL).build();
}

@Override
Expand Down Expand Up @@ -138,35 +138,40 @@ protected void sendInsightsReportWithClient(
configuration,
() -> {
var response = client.send(request, HttpResponse.BodyHandlers.ofString());
int statusCode = response.statusCode();
logger.debug(
"Red Hat Insights HTTP Client: status="
+ response.statusCode()
+ statusCode
+ ", body="
+ response.body());
switch (response.statusCode()) {
case 201:
logger.debug(
"Red Hat Insights - Advisor content type with no metadata accepted for"
+ " processing");
switch (statusCode / 100) {
case 2:
if (statusCode == 201) {
logger.debug(
"Red Hat Insights - Advisor content type with no metadata accepted for"
+ " processing");
} else {
logger.debug("Red Hat Insights - Payload was accepted for processing");
}
break;
case 202:
logger.debug("Red Hat Insights - Payload was accepted for processing");
break;
case 401:
throw new InsightsException(
ERROR_HTTP_SEND_AUTH_ERROR, "Authentication missing from request");
case 413:
throw new InsightsException(ERROR_HTTP_SEND_PAYLOAD, "Payload too large");
case 415:
throw new InsightsException(
ERROR_HTTP_SEND_INVALID_CONTENT_TYPE,
"Content type of payload is unsupported");
case 500:
case 503:
case 4:
switch (statusCode) {
case 401:
throw new InsightsException(
ERROR_HTTP_SEND_AUTH_ERROR, "Authentication missing from request");
case 413:
throw new InsightsException(ERROR_HTTP_SEND_PAYLOAD, "Payload too large");
case 415:
default:
throw new InsightsException(
ERROR_HTTP_SEND_INVALID_CONTENT_TYPE,
"Content type of payload is unsupported");
}
case 5:
Copy link
Collaborator Author

@jponge jponge May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I have deliberately kept multiple case label statements based on the original OpenAPI spec of the Go insights backend. These are not very clean switch constructs, but I wanted to keep track of the expected status codes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not defining an enum for each 'category': could be more readable WDYT ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we deal with 403 as maybe the subscription is no longer active or something like this ?

default:
throw new InsightsException(
ERROR_HTTP_SEND_SERVER_ERROR,
"Request failed on the server with code: " + response.statusCode());
"Request failed on the server with code: " + statusCode);
}
});
try {
Expand Down