From 6cdff461240979d94c865780c8615d323cdf5fbc Mon Sep 17 00:00:00 2001 From: Tom Roman Date: Sat, 9 Nov 2024 15:15:01 +0000 Subject: [PATCH 1/2] chore: refactor JacksonBodyHandler to remove unchecked operation --- .../hcloud/http/HetznerCloudHttpClient.java | 21 ++++--- .../dev/tomr/hcloud/http/HetznerResult.java | 31 ++++++++++ .../tomr/hcloud/http/JacksonBodyHandler.java | 57 +++++++++++-------- .../hcloud/service/server/ServerService.java | 2 +- .../http/HttpClientComponentTest.java | 37 +++++++++++- .../tomr/hcloud/http/HetznerResultTest.java | 56 ++++++++++++++++++ .../service/server/ServerServiceTest.java | 10 ++-- 7 files changed, 175 insertions(+), 39 deletions(-) create mode 100644 src/main/java/dev/tomr/hcloud/http/HetznerResult.java create mode 100644 src/test/java/dev/tomr/hcloud/http/HetznerResultTest.java diff --git a/src/main/java/dev/tomr/hcloud/http/HetznerCloudHttpClient.java b/src/main/java/dev/tomr/hcloud/http/HetznerCloudHttpClient.java index 8eb7f24..254208c 100644 --- a/src/main/java/dev/tomr/hcloud/http/HetznerCloudHttpClient.java +++ b/src/main/java/dev/tomr/hcloud/http/HetznerCloudHttpClient.java @@ -3,6 +3,8 @@ import dev.tomr.hcloud.HetznerCloud; import dev.tomr.hcloud.http.exception.HetznerApiException; import dev.tomr.hcloud.http.model.HetznerErrorResponse; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.net.URI; @@ -15,6 +17,7 @@ public class HetznerCloudHttpClient { + private static final Logger log = LoggerFactory.getLogger(HetznerCloudHttpClient.class); private static HetznerCloudHttpClient instance; private final HttpClient httpClient; @@ -49,7 +52,7 @@ public static HetznerCloudHttpClient getInstance() { * @throws IOException Exception passed from HTTP client * @throws InterruptedException Exception passed from HTTP client */ - public T sendHttpRequest(Class clazz, String endpoint, RequestVerb requestVerb, String apiKey) throws IOException, InterruptedException { + public T sendHttpRequest(Class clazz, String endpoint, RequestVerb requestVerb, String apiKey) throws IOException, InterruptedException, IllegalAccessException { return sendHttpRequest(clazz, endpoint, requestVerb, apiKey, null); } @@ -65,16 +68,20 @@ public T sendHttpRequest(Class clazz, String en * @throws IOException Exception passed from HTTP client * @throws InterruptedException Exception passed from HTTP client */ - public T sendHttpRequest(Class clazz, String endpoint, RequestVerb requestVerb, String apiKey, String body) throws IOException, InterruptedException { + public T sendHttpRequest(Class clazz, String endpoint, RequestVerb requestVerb, String apiKey, String body) throws IOException, InterruptedException, IllegalAccessException { HttpRequest request = createHttpRequest(endpoint, requestVerb, apiKey, body); - HttpResponse response = httpClient.send(request, new JacksonBodyHandler<>(clazz)); + HttpResponse> response = httpClient.send(request, new JacksonBodyHandler<>(clazz, HetznerErrorResponse.class)); - switch (response.statusCode()) { - case 200, 201, 204 -> { - return (T) response.body(); + switch (Integer.parseInt(Integer.toString(response.statusCode()).substring(0, 1))) { + case 2 -> { + if (response.statusCode() == 204) { + log.info("Response was 204, continuing..."); + return null; + } + return response.body().getSuccessObject(); } default -> { - HetznerErrorResponse errorResponse = (HetznerErrorResponse) response.body(); + HetznerErrorResponse errorResponse = response.body().getFailureObject(); throw new HetznerApiException(errorResponse); } } diff --git a/src/main/java/dev/tomr/hcloud/http/HetznerResult.java b/src/main/java/dev/tomr/hcloud/http/HetznerResult.java new file mode 100644 index 0000000..9a44551 --- /dev/null +++ b/src/main/java/dev/tomr/hcloud/http/HetznerResult.java @@ -0,0 +1,31 @@ +package dev.tomr.hcloud.http; + +public class HetznerResult { + private final T successObject; + private final U failureObject; + + public HetznerResult(T successObject, U failureObject) { + this.successObject = successObject; + this.failureObject = failureObject; + } + + public boolean isSuccess() { + return successObject != null; + } + + public T getSuccessObject() throws IllegalAccessException { + if (isSuccess()) { + return successObject; + } else { + throw new IllegalAccessException("No success object!"); + } + } + + public U getFailureObject() throws IllegalAccessException { + if (!isSuccess()) { + return failureObject; + } else { + throw new IllegalAccessException("No failure object!"); + } + } +} diff --git a/src/main/java/dev/tomr/hcloud/http/JacksonBodyHandler.java b/src/main/java/dev/tomr/hcloud/http/JacksonBodyHandler.java index 3aa93bb..eaad21d 100644 --- a/src/main/java/dev/tomr/hcloud/http/JacksonBodyHandler.java +++ b/src/main/java/dev/tomr/hcloud/http/JacksonBodyHandler.java @@ -1,56 +1,67 @@ package dev.tomr.hcloud.http; import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonMappingException; import dev.tomr.hcloud.HetznerCloud; import dev.tomr.hcloud.http.model.HetznerErrorResponse; +import java.io.IOException; +import java.io.InputStream; import java.net.http.HttpResponse; import java.nio.charset.StandardCharsets; +import java.util.function.Function; /** * Internal helper to handle the converting of a returned HTTP response's body, to an Object, with Jackson ObjectMapper * @param */ -public class JacksonBodyHandler implements HttpResponse.BodyHandler { +public class JacksonBodyHandler implements HttpResponse.BodyHandler> { private final Class clazz; + private final Class errorClass; - public JacksonBodyHandler(Class clazz) { + private int statusCode; + + public JacksonBodyHandler(Class clazz, Class errorClass) { this.clazz = clazz; + this.errorClass = errorClass; } @Override - public HttpResponse.BodySubscriber apply(HttpResponse.ResponseInfo responseInfo) { - switch (responseInfo.statusCode()) { - case 200, 201 -> { - return jsonPayload(clazz); + public HttpResponse.BodySubscriber> apply(HttpResponse.ResponseInfo responseInfo) { + Function> mapper; + statusCode = responseInfo.statusCode(); + switch (Integer.parseInt(Integer.toString(statusCode).substring(0, 1))) { + case 2 -> { + mapper = this::parseSuccess; } default -> { - return apiErrorJson(); + mapper = this::parseError; } } + return HttpResponse.BodySubscribers.mapping(HttpResponse.BodySubscribers.ofInputStream(), mapper); } - public static HttpResponse.BodySubscriber jsonPayload(Class targetClass) { - HttpResponse.BodySubscriber bodyString = HttpResponse.BodySubscribers.ofString(StandardCharsets.UTF_8); - return HttpResponse.BodySubscribers.mapping(bodyString, (String b) -> { - try { - return HetznerCloud.getObjectMapper().readValue(b, targetClass); - } catch (JsonProcessingException e) { - throw new RuntimeException(e); - } - }); + private HetznerResult parseSuccess(InputStream input) { + return new HetznerResult<>(parseJson(input, clazz), null); } - public static HttpResponse.BodySubscriber apiErrorJson() { - HttpResponse.BodySubscriber bodyString = HttpResponse.BodySubscribers.ofString(StandardCharsets.UTF_8); - return HttpResponse.BodySubscribers.mapping(bodyString, (String b) -> { - try { - return HetznerCloud.getObjectMapper().readValue(b, HetznerErrorResponse.class); - } catch (JsonProcessingException e) { + private HetznerResult parseError(InputStream input) { + return new HetznerResult<>(null, parseJson(input, errorClass)); + } + + + private R parseJson(InputStream input, Class clazz) { + try { + return HetznerCloud.getObjectMapper().readValue(input, clazz); + } catch (IOException e) { + // this is actually just bad, but handles 204 properly. I do **not** like this though, need to revisit + if (statusCode == 204) { + return null; + } else { throw new RuntimeException(e); } - }); + } } } diff --git a/src/main/java/dev/tomr/hcloud/service/server/ServerService.java b/src/main/java/dev/tomr/hcloud/service/server/ServerService.java index e465245..1bda750 100644 --- a/src/main/java/dev/tomr/hcloud/service/server/ServerService.java +++ b/src/main/java/dev/tomr/hcloud/service/server/ServerService.java @@ -99,7 +99,7 @@ private CompletableFuture scheduleHttpRequest(String host, String apiKey) } else { throw new RuntimeException("No updated values??"); } - } catch (InterruptedException | IOException e) { + } catch (InterruptedException | IOException | IllegalAccessException e) { throw new RuntimeException(e); } }, serviceManager.getExecutor()); diff --git a/src/test/java/dev/tomr/hcloud/component/http/HttpClientComponentTest.java b/src/test/java/dev/tomr/hcloud/component/http/HttpClientComponentTest.java index 122bfa2..dae8937 100644 --- a/src/test/java/dev/tomr/hcloud/component/http/HttpClientComponentTest.java +++ b/src/test/java/dev/tomr/hcloud/component/http/HttpClientComponentTest.java @@ -12,6 +12,8 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import java.io.IOException; @@ -32,7 +34,7 @@ public class HttpClientComponentTest { @Test @DisplayName("HTTP Client can make a successful GET request and map to class") - public void testHttpClientAndMapping() throws IOException, InterruptedException { + public void testHttpClientAndMapping() throws IOException, InterruptedException, IllegalAccessException { wireMockExtension.stubFor(get("/test").willReturn(ok(objectMapper.writeValueAsString(new TestModel(1, 1, "sunt aut facere repellat provident occaecati excepturi optio reprehenderit", "quia et suscipit\nsuscipit recusandae consequuntur expedita et cum\nreprehenderit molestiae ut ut quas totam\nnostrum rerum est autem sunt rem eveniet architecto"))))); HetznerCloudHttpClient client = new HetznerCloudHttpClient(); @@ -47,7 +49,7 @@ public void testHttpClientAndMapping() throws IOException, InterruptedException @Test @DisplayName("HTTP Client can make a successful POST request with Body and map to response class") - void testHttpClientAndMappingWithBody() throws IOException, InterruptedException { + void testHttpClientAndMappingWithBody() throws IOException, InterruptedException, IllegalAccessException { wireMockExtension.stubFor(post("/test").willReturn(ok(objectMapper.writeValueAsString(new TestModel(2, 1, "some title", "some body"))))); HetznerCloudHttpClient client = new HetznerCloudHttpClient(); @@ -64,7 +66,7 @@ void testHttpClientAndMappingWithBody() throws IOException, InterruptedException @Test @DisplayName("HTTP Client can make a successful PUT request with Body and map to response class") - void testHttpClientAndMappingWithBodyPUT() throws IOException, InterruptedException { + void testHttpClientAndMappingWithBodyPUT() throws IOException, InterruptedException, IllegalAccessException { wireMockExtension.stubFor(put("/test").willReturn(ok(objectMapper.writeValueAsString(new TestModel(2, 1, "some title", "some body"))))); HetznerCloudHttpClient client = new HetznerCloudHttpClient(); @@ -111,4 +113,33 @@ void testHttpClientThrowsHetznerApiExceptionWhenBadJsonResponse() { assertThrows(IOException.class, () -> client.sendHttpRequest(TestModel.class, HOST + "badResponseWithXml", RequestVerb.GET, "")); } + + @ParameterizedTest + @ValueSource(ints = {201, 200}) + @DisplayName("HTTP Client can make a successful GET request with Body and map to response class with status") + void testHttpClientAndMappingWithBodyGetStatus(int statusCode) throws IOException, InterruptedException, IllegalAccessException { + wireMockExtension.stubFor(get("/test").willReturn(aResponse().withStatus(statusCode).withBody(objectMapper.writeValueAsString(new TestModel(2, 1, "some title", "some body"))))); + HetznerCloudHttpClient client = new HetznerCloudHttpClient(); + + TestModel testModel = new TestModel(); + testModel.setUserId(2); + testModel.setTitle("some title"); + testModel.setBody("some body"); + + TestModel response = client.sendHttpRequest(TestModel.class, HOST + "test", RequestVerb.GET, ""); + assertEquals(testModel.getBody(), response.getBody()); + assertEquals(testModel.getTitle(), response.getTitle()); + assertEquals(testModel.getUserId(), response.getUserId()); + } + + @Test + @DisplayName("HTTP Client handles 204 no content correctly") + void httpClientHandles204NoContent() throws IOException, InterruptedException, IllegalAccessException { + // this test is needed because 204 does not support handling a body + // todo refactor how we handle HTTP Status codes we **know** will never supply a body, i.e. 204 - this implementation leaves a lot to be desired + wireMockExtension.stubFor(get("/test").willReturn(aResponse().withStatus(204))); + HetznerCloudHttpClient client = new HetznerCloudHttpClient(); + + assertDoesNotThrow(() -> client.sendHttpRequest(TestModel.class, HOST + "test", RequestVerb.GET, "")); + } } diff --git a/src/test/java/dev/tomr/hcloud/http/HetznerResultTest.java b/src/test/java/dev/tomr/hcloud/http/HetznerResultTest.java new file mode 100644 index 0000000..3a1b7b7 --- /dev/null +++ b/src/test/java/dev/tomr/hcloud/http/HetznerResultTest.java @@ -0,0 +1,56 @@ +package dev.tomr.hcloud.http; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +public class HetznerResultTest { + + @Test + @DisplayName("HetznerResult will disallow access to the failure object, when a success object exists") + void hetznerResultWillDisallowAccessToFailureWhenSuccessExists() { + HetznerResult hetznerResult = new HetznerResult<>("success", "failure"); + assertThrows(IllegalAccessException.class, hetznerResult::getFailureObject); + } + + @Test + @DisplayName("Returns given success object") + void returnsGivenSuccessObject() throws IllegalAccessException { + HetznerResult hetznerResult = new HetznerResult<>("success", null); + assertEquals(hetznerResult.getSuccessObject(), "success"); + } + + @Test + @DisplayName("Returns given failure object") + void returnsGivenFailureObject() throws IllegalAccessException { + HetznerResult hetznerResult = new HetznerResult<>(null, "failure"); + assertEquals(hetznerResult.getFailureObject(), "failure"); + } + + @Test + @DisplayName("Returns whether it has a success object correctly") + void returnsWhetherItHasASuccessObject() { + HetznerResult hetznerResult = new HetznerResult<>("success", null); + assertTrue(hetznerResult.isSuccess()); + + HetznerResult hetznerResult2 = new HetznerResult<>(null, "failure"); + assertFalse(hetznerResult2.isSuccess()); + } + + @Test + @DisplayName("Throws IllegalAccessException when trying to access a non existent success object") + void throwsIllegalAccessExceptionWhenTryingToAccessNonExistentSuccessObject() { + HetznerResult hetznerResult = new HetznerResult<>(null, new Object()); + + assertThrows(IllegalAccessException.class, hetznerResult::getSuccessObject); + } + + @Test + @DisplayName("Throws IllegalAccessException when trying to access a non existent failure object") + void throwsIllegalAccessExceptionWhenTryingToAccessNonExistentFailureObject() { + HetznerResult hetznerResult = new HetznerResult<>(new Object(), null); + + assertThrows(IllegalAccessException.class, hetznerResult::getFailureObject); + } +} diff --git a/src/test/java/dev/tomr/hcloud/service/server/ServerServiceTest.java b/src/test/java/dev/tomr/hcloud/service/server/ServerServiceTest.java index 5e0343d..a5668b2 100644 --- a/src/test/java/dev/tomr/hcloud/service/server/ServerServiceTest.java +++ b/src/test/java/dev/tomr/hcloud/service/server/ServerServiceTest.java @@ -110,7 +110,7 @@ void taskIsScheduledWhenServerNameOrLabelUpdateCalledForFirstTime() { serverService.serverNameOrLabelUpdate("name", "name", server); verify(hetznerCloudHttpClient, timeout(2000).times(1)).sendHttpRequest(eq(ServerDTO.class), eq("http://host/server/1"), any(RequestVerb.class), eq("key1234"), eq("{\"name\":\"name\"}")); - } catch (IOException | InterruptedException e) { + } catch (IOException | InterruptedException | IllegalAccessException e) { throw new RuntimeException(e); } } @@ -165,7 +165,7 @@ void taskIsScheduledWhenServerNameOrLabelUpdateCalledForFirstTimeWithLabels() { serverService.serverNameOrLabelUpdate("labels", Map.of("label", "value"), server); verify(hetznerCloudHttpClient, timeout(2000).times(1)).sendHttpRequest(eq(ServerDTO.class), eq("http://host/server/1"), any(RequestVerb.class), eq("key1234"), eq("{\"labels\":{\"label\":\"value\"}}")); - } catch (IOException | InterruptedException e) { + } catch (IOException | InterruptedException | IllegalAccessException e) { throw new RuntimeException(e); } } @@ -221,7 +221,7 @@ void taskUsesExtraFieldsChangedAfterFirstInvocation() { serverService.serverNameOrLabelUpdate("labels", Map.of("l", "v"), server); verify(hetznerCloudHttpClient, timeout(2000).times(1)).sendHttpRequest(eq(ServerDTO.class), eq("http://host/server/1"), any(RequestVerb.class), eq("key1234"), eq("{\"labels\":{\"label\":\"value\"},\"name\":\"name\"}")); - } catch (IOException | InterruptedException e) { + } catch (IOException | InterruptedException | IllegalAccessException e) { throw new RuntimeException(e); } } @@ -279,7 +279,7 @@ void whenHttpClientThrowsGracefully() { verify(hetznerCloudHttpClient, timeout(2000).times(0)).sendHttpRequest(eq(ServerDTO.class), eq("http://host/server/1"), any(RequestVerb.class), eq("key1234"), eq("{\"labels\":{\"label\":\"value\"},\"name\":\"name\"}")); verify(serviceManager, timeout(2000).times(1)).closeExecutor(); - } catch (IOException | InterruptedException e) { + } catch (IOException | InterruptedException | IllegalAccessException e) { throw new RuntimeException(e); } } @@ -337,7 +337,7 @@ void cancelMethodPreventsTheRequestBeingSent() { verify(hetznerCloudHttpClient, timeout(2000).times(0)).sendHttpRequest(any(), any(), any(), any(), any()); verify(serviceManager, timeout(2000).times(1)).closeExecutor(); - } catch (IOException | InterruptedException e) { + } catch (IOException | InterruptedException | IllegalAccessException e) { throw new RuntimeException(e); } } From f575894cfd4de749b70ac3d245d6422a725d3bce Mon Sep 17 00:00:00 2001 From: Tom Roman Date: Sat, 9 Nov 2024 15:17:47 +0000 Subject: [PATCH 2/2] fix: remove redundant test --- .../http/HttpClientComponentTest.java | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/test/java/dev/tomr/hcloud/component/http/HttpClientComponentTest.java b/src/test/java/dev/tomr/hcloud/component/http/HttpClientComponentTest.java index dae8937..fde7094 100644 --- a/src/test/java/dev/tomr/hcloud/component/http/HttpClientComponentTest.java +++ b/src/test/java/dev/tomr/hcloud/component/http/HttpClientComponentTest.java @@ -114,24 +114,6 @@ void testHttpClientThrowsHetznerApiExceptionWhenBadJsonResponse() { assertThrows(IOException.class, () -> client.sendHttpRequest(TestModel.class, HOST + "badResponseWithXml", RequestVerb.GET, "")); } - @ParameterizedTest - @ValueSource(ints = {201, 200}) - @DisplayName("HTTP Client can make a successful GET request with Body and map to response class with status") - void testHttpClientAndMappingWithBodyGetStatus(int statusCode) throws IOException, InterruptedException, IllegalAccessException { - wireMockExtension.stubFor(get("/test").willReturn(aResponse().withStatus(statusCode).withBody(objectMapper.writeValueAsString(new TestModel(2, 1, "some title", "some body"))))); - HetznerCloudHttpClient client = new HetznerCloudHttpClient(); - - TestModel testModel = new TestModel(); - testModel.setUserId(2); - testModel.setTitle("some title"); - testModel.setBody("some body"); - - TestModel response = client.sendHttpRequest(TestModel.class, HOST + "test", RequestVerb.GET, ""); - assertEquals(testModel.getBody(), response.getBody()); - assertEquals(testModel.getTitle(), response.getTitle()); - assertEquals(testModel.getUserId(), response.getUserId()); - } - @Test @DisplayName("HTTP Client handles 204 no content correctly") void httpClientHandles204NoContent() throws IOException, InterruptedException, IllegalAccessException {