refactor: Address SonarQube findings on TypeMapper/RequestHandler change

thced · thced · commit ad6ec3f51859 · 2026-05-14T09:34:47.000+02:00
diff --git a/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java b/src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java
@@ -23,6 +23,8 @@
 
 public final class RequestPreparationFilter extends Filter {
 
+  private static final String BODY_POINTER = "/body";
+
   private final Spec spec;
   private final Router router;
   private final Validator validator;
@@ -126,7 +128,7 @@ private Object validateAndParseBody(HttpExchange exchange, Operation op, byte[]
     if (body.length == 0) {
       if (rb.get().required()) {
         throw new ValidationException(
-            new ValidationError("/body", "required", "request body is required", null));
+            new ValidationError(BODY_POINTER, "required", "request body is required", null));
       }
       return null;
     }
@@ -136,13 +138,13 @@ private Object validateAndParseBody(HttpExchange exchange, Operation op, byte[]
     if (mt == null) {
       throw new ValidationException(
           new ValidationError(
-              "/body", "content-type", "unsupported content type: " + mediaType, null));
+              BODY_POINTER, "content-type", "unsupported content type: " + mediaType, null));
     }
     TypeMapper mapper = bodyMappers.get(mediaType);
     if (mapper == null) {
       throw new ValidationException(
           new ValidationError(
-              "/body", "content-type", "unsupported content type: " + mediaType, null));
+              BODY_POINTER, "content-type", "unsupported content type: " + mediaType, null));
     }
     Object parsed = mapper.readFrom(body, header);
     if (mediaType.equals("application/x-www-form-urlencoded") && parsed instanceof Map<?, ?> map) {
diff --git a/src/main/java/com/retailsvc/http/internal/gson/GsonJsonMapper.java b/src/main/java/com/retailsvc/http/internal/gson/GsonJsonMapper.java
@@ -70,39 +70,56 @@ public byte[] writeTo(Object value) {
   private static Object toJavaObject(JsonElement element) {
     if (element == null || element instanceof JsonNull) {
       return null;
-    } else if (element instanceof JsonObject obj) {
-      Map<String, Object> map = new LinkedHashMap<>();
-      for (Map.Entry<String, JsonElement> entry : obj.entrySet()) {
-        map.put(entry.getKey(), toJavaObject(entry.getValue()));
-      }
-      return map;
-    } else if (element instanceof JsonArray arr) {
-      List<Object> list = new ArrayList<>(arr.size());
-      for (JsonElement item : arr) {
-        list.add(toJavaObject(item));
-      }
-      return list;
-    } else if (element instanceof JsonPrimitive prim) {
-      if (prim.isBoolean()) {
-        return prim.getAsBoolean();
-      } else if (prim.isString()) {
-        return prim.getAsString();
-      } else {
-        // Number
-        String raw = prim.getAsString();
-        if (raw.indexOf('.') < 0 && raw.indexOf('e') < 0 && raw.indexOf('E') < 0) {
-          try {
-            return Long.parseLong(raw);
-          } catch (NumberFormatException _) {
-            // falls through to Double for out-of-range integers
-          }
-        }
-        return Double.parseDouble(raw);
-      }
+    }
+    if (element instanceof JsonObject obj) {
+      return toMap(obj);
+    }
+    if (element instanceof JsonArray arr) {
+      return toList(arr);
+    }
+    if (element instanceof JsonPrimitive prim) {
+      return toPrimitive(prim);
     }
     throw new IllegalStateException("Unexpected JsonElement type: " + element.getClass());
   }
 
+  private static Map<String, Object> toMap(JsonObject obj) {
+    Map<String, Object> map = new LinkedHashMap<>();
+    for (Map.Entry<String, JsonElement> entry : obj.entrySet()) {
+      map.put(entry.getKey(), toJavaObject(entry.getValue()));
+    }
+    return map;
+  }
+
+  private static List<Object> toList(JsonArray arr) {
+    List<Object> list = new ArrayList<>(arr.size());
+    for (JsonElement item : arr) {
+      list.add(toJavaObject(item));
+    }
+    return list;
+  }
+
+  private static Object toPrimitive(JsonPrimitive prim) {
+    if (prim.isBoolean()) {
+      return prim.getAsBoolean();
+    }
+    if (prim.isString()) {
+      return prim.getAsString();
+    }
+    return toNumber(prim.getAsString());
+  }
+
+  private static Object toNumber(String raw) {
+    if (raw.indexOf('.') < 0 && raw.indexOf('e') < 0 && raw.indexOf('E') < 0) {
+      try {
+        return Long.parseLong(raw);
+      } catch (NumberFormatException _) {
+        // Falls through to Double for out-of-Long-range integers.
+      }
+    }
+    return Double.parseDouble(raw);
+  }
+
   private static <T> TypeAdapter<T> isoStringWriter(Function<T, String> toIso) {
     return new TypeAdapter<T>() {
       @Override
diff --git a/src/test/java/com/retailsvc/http/OpenApiServerTest.java b/src/test/java/com/retailsvc/http/OpenApiServerTest.java
@@ -29,27 +29,28 @@ void shouldStartHttpServerWithValidConfiguration() {
 
   @Test
   void shouldThrowExceptionWhenSpecIsNull() {
-    assertThatThrownBy(() -> OpenApiServer.builder().handlers(emptyMap()).port(0).build())
+    OpenApiServer.Builder builder = OpenApiServer.builder().handlers(emptyMap()).port(0);
+
+    assertThatThrownBy(builder::build)
         .isInstanceOf(NullPointerException.class)
         .hasMessageContaining("Spec must not be null");
   }
 
   @Test
   void shouldThrowExceptionWhenHandlersMapIsNull() {
-    Spec validSpec = testSpec();
+    OpenApiServer.Builder builder = OpenApiServer.builder().spec(testSpec()).port(0);
 
-    assertThatThrownBy(() -> OpenApiServer.builder().spec(validSpec).port(0).build())
+    assertThatThrownBy(builder::build)
         .isInstanceOf(NullPointerException.class)
         .hasMessageContaining("handlers must not be null");
   }
 
   @Test
   void testExceptionIsThrownOnInvalidHttpPort() {
-    Spec validSpec = testSpec();
+    OpenApiServer.Builder builder =
+        OpenApiServer.builder().spec(testSpec()).handlers(emptyMap()).port(-1);
 
-    assertThatThrownBy(
-            () -> OpenApiServer.builder().spec(validSpec).handlers(emptyMap()).port(-1).build())
-        .isInstanceOf(IllegalArgumentException.class);
+    assertThatThrownBy(builder::build).isInstanceOf(IllegalArgumentException.class);
   }
 
   private Spec testSpec() {
diff --git a/src/test/java/com/retailsvc/http/TypeMapperRegistrationTest.java b/src/test/java/com/retailsvc/http/TypeMapperRegistrationTest.java
@@ -12,6 +12,7 @@
 import java.net.http.HttpRequest.BodyPublishers;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.junit.jupiter.api.Test;
 
 class TypeMapperRegistrationTest extends ServerBaseTest {
@@ -52,10 +53,12 @@ void gsonFallbackIsAutoRegisteredWhenNoJsonMapperConfigured() throws Exception {
 
   @Test
   void userSuppliedMapperOverridesDefault() throws Exception {
+    AtomicBoolean readFromCalled = new AtomicBoolean();
     TypeMapper marker =
         new TypeMapper() {
           @Override
           public Object readFrom(byte[] b, String h) {
+            readFromCalled.set(true);
             return Map.of("aList", java.util.List.of("x"), "feelingGood", true);
           }
 
@@ -65,20 +68,31 @@ public byte[] writeTo(Object v) {
           }
         };
     RequestHandler echo = req -> req.respond(200).empty();
-    OpenApiServer s =
+    server =
         OpenApiServer.builder()
             .spec(spec)
             .bodyMapper("application/json", marker)
             .handlers(Map.of("get-data", echo, "post-data", echo))
             .port(0)
             .build();
-    s.close();
+    HttpClient.newHttpClient()
+        .send(
+            HttpRequest.newBuilder()
+                .uri(URI.create("http://localhost:%d/api/v1/data".formatted(server.listenPort())))
+                .header("Content-Type", "application/json")
+                .POST(BodyPublishers.ofString("{\"anything\":\"goes\"}"))
+                .build(),
+            ofString());
+
+    assertThat(readFromCalled).isTrue();
   }
 
   @Test
   void bodyMapperRejectsNullArgs() {
-    var b = OpenApiServer.builder();
-    assertThatThrownBy(() -> b.bodyMapper(null, new GsonOnlyMapper()))
+    OpenApiServer.Builder b = OpenApiServer.builder();
+    TypeMapper anyMapper = new GsonOnlyMapper();
+
+    assertThatThrownBy(() -> b.bodyMapper(null, anyMapper))
         .isInstanceOf(NullPointerException.class);
     assertThatThrownBy(() -> b.bodyMapper("application/json", null))
         .isInstanceOf(NullPointerException.class);
diff --git a/src/test/java/com/retailsvc/http/internal/FormTypeMapperTest.java b/src/test/java/com/retailsvc/http/internal/FormTypeMapperTest.java
@@ -23,7 +23,9 @@ void readsKeyValuePairs() {
 
   @Test
   void writeToIsUnsupported() {
-    assertThatThrownBy(() -> mapper.writeTo(Map.of("k", "v")))
+    Map<String, String> value = Map.of("k", "v");
+
+    assertThatThrownBy(() -> mapper.writeTo(value))
         .isInstanceOf(UnsupportedOperationException.class);
   }
 }
diff --git a/src/test/java/com/retailsvc/http/internal/gson/GsonJsonMapperTest.java b/src/test/java/com/retailsvc/http/internal/gson/GsonJsonMapperTest.java
@@ -41,10 +41,11 @@ void readBasicTypes() {
         (Map<String, Object>)
             mapper.readFrom(
                 bytes("{\"s\":\"hi\",\"b\":true,\"n\":null,\"a\":[1,2]}"), "application/json");
-    assertThat(parsed.get("s")).isEqualTo("hi");
-    assertThat(parsed.get("b")).isEqualTo(Boolean.TRUE);
-    assertThat(parsed.get("n")).isNull();
-    assertThat(parsed.get("a")).isEqualTo(List.of(1L, 2L));
+    assertThat(parsed)
+        .containsEntry("s", "hi")
+        .containsEntry("b", Boolean.TRUE)
+        .containsEntry("n", null)
+        .containsEntry("a", List.of(1L, 2L));
   }
 
   @Test

Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,9 @@ void readsKeyValuePairs() {`
`23`	`23`
`24`	`24`	`@Test`
`25`	`25`	`void writeToIsUnsupported() {`
`26`		`- assertThatThrownBy(() -> mapper.writeTo(Map.of("k", "v")))`
	`26`	`+ Map<String, String> value = Map.of("k", "v");`
	`27`	`+`
	`28`	`+ assertThatThrownBy(() -> mapper.writeTo(value))`
`27`	`29`	`.isInstanceOf(UnsupportedOperationException.class);`
`28`	`30`	`}`
`29`	`31`	`}`