Skip to content

#127 Improve Error Handling for Failed DefectDojo API Requests #128

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

Merged
merged 3 commits into from
Jul 27, 2024
Merged

#127 Improve Error Handling for Failed DefectDojo API Requests #128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fae09a2
#127 Improve Error Handling for Failed DefectDojo API Requests
Weltraumschaf Jul 12, 2024
ba2db23
#127 Use Parameterized Log to Prevent Performance Penalties
Weltraumschaf Jul 26, 2024
0ff32f3
#127 Also Handle HTTP Client Exception for All Other HTTP Methods
Weltraumschaf Jul 26, 2024
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
4 changes: 2 additions & 2 deletions src/main/java/io/securecodebox/persistence/defectdojo/service/DefaultImportScanService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;
import org.springframework.web.client.HttpClientErrorException;
import org.springframework.web.client.RestClientException;
import org.springframework.web.client.RestTemplate;

import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -124,7 +124,7 @@ public String getFilename() {

final var payload = new HttpEntity<MultiValueMap<String, Object>>(body, headers);
return exchangeRequest(endpoint, payload);
} catch (HttpClientErrorException e) {
} catch (RestClientException e) {
log.error("Exception while attaching findings to engagement: {}", e.getMessage());
throw new PersistenceException("Failed to attach findings to engagement.", e);
}
Expand Down
23 changes: 15 additions & 8 deletions src/main/java/io/securecodebox/persistence/defectdojo/service/GenericDefectDojoService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.fasterxml.jackson.databind.cfg.CoercionInputShape;
import io.securecodebox.persistence.defectdojo.config.Config;
import io.securecodebox.persistence.defectdojo.exception.LoopException;
import io.securecodebox.persistence.defectdojo.exception.PersistenceException;
import io.securecodebox.persistence.defectdojo.http.Foo;
import io.securecodebox.persistence.defectdojo.http.ProxyConfigFactory;
import io.securecodebox.persistence.defectdojo.model.Engagement;
Expand All @@ -29,6 +30,7 @@
import org.springframework.http.converter.StringHttpMessageConverter;
import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.web.client.RestClientException;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.util.UriComponentsBuilder;

Expand Down Expand Up @@ -73,14 +75,19 @@ public final T get(long id) {

final var url = this.config.getUrl() + API_PREFIX + this.getUrlPath() + "/" + id;
log.debug("Requesting URL: {}", url);
ResponseEntity<T> response = restTemplate.exchange(
url,
HttpMethod.GET,
payload,
getModelClass()
);

return response.getBody();
try {
ResponseEntity<T> response = restTemplate.exchange(
Copy link
Member

Choose a reason for hiding this comment

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

Did you intentionally only add this for the GET requests?
The error handling for the other http methods seems to be the same

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔No. This means that any error handling is missing on non-GET bc I simply grepped HttpClientErrorException 😬

url,
HttpMethod.GET,
payload,
getModelClass()
);

return response.getBody();
} catch (RestClientException e) {
log.error("Exception while getting data: {}", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Whats in a message for a request client exception?
Is that enough for users to understand what request failed? Does it include the http method and url of the request?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a common pattern in Java that you simply log the exception message, if the exception is rethrown with the origin exception as cause because the full exception handling is done where the rethrown exception is catched. It's quite debatable if this log message is really necessary.

throw new PersistenceException("Failed to get data.", e);
}
}

@Override
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/io/securecodebox/persistence/defectdojo/service/ImportScanService2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;
import org.springframework.web.client.HttpClientErrorException;
import org.springframework.web.client.RestClientException;
import org.springframework.web.client.RestTemplate;

import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -94,8 +94,8 @@ public String getFilename() {
var payload = new HttpEntity<>(mvn, headers);

return restTemplate.exchange(config.getUrl() + "/api/v2/" + endpoint + "/", HttpMethod.POST, payload, ImportScanResponse.class).getBody();
} catch (HttpClientErrorException e) {
throw new PersistenceException("Failed to attach findings to engagement.");
} catch (RestClientException e) {
throw new PersistenceException("Failed to attach findings to engagement.", e);
}
}

Expand Down