[REFACTOR] simplify JsonInjector and NetworkDeserializer with safer parsing, validation, and created test classes as well

AKakkar28 · AKakkar28 · commit abe97a2eb3a1 · 2025-09-25T22:50:24.000-04:00
diff --git a/src/main/java/com/maxmind/geoip2/JsonInjector.java b/src/main/java/com/maxmind/geoip2/JsonInjector.java
@@ -5,34 +5,31 @@
 import com.fasterxml.jackson.databind.InjectableValues;
 import com.maxmind.db.Network;
 import com.maxmind.geoip2.record.Traits;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
-class JsonInjector extends InjectableValues {
-    private final List<String> locales;
-    private final String ip;
-    private final Network network;
+final class JsonInjector extends InjectableValues {
+    private final Map<String, Object> injectables;
 
-    public JsonInjector(List<String> locales, String ip, Network network) {
-        this.locales = locales;
-        this.ip = ip;
-        this.network = network;
+    JsonInjector(List<String> locales, String ip, Network network) {
+        var map = new HashMap<String, Object>(4, 1f);
+        if (locales != null) {
+            map.put("locales", locales);
+        }
+        if (ip != null) {
+            map.put("ip_address", ip);
+        }
+        if (network != null) {
+            map.put("network", network);
+        }
+        map.put("traits", new Traits(ip, network));
+        this.injectables = Map.copyOf(map);
     }
 
     @Override
     public Object findInjectableValue(Object valueId, DeserializationContext ctxt,
                                       BeanProperty forProperty, Object beanInstance) {
-        if ("locales".equals(valueId)) {
-            return locales;
-        }
-        if ("ip_address".equals(valueId)) {
-            return ip;
-        }
-        if ("network".equals(valueId)) {
-            return network;
-        }
-        if ("traits".equals(valueId)) {
-            return new Traits(ip, network);
-        }
-        return null;
+        return (valueId instanceof String k) ? injectables.get(k) : null;
     }
-}
+}
diff --git a/src/main/java/com/maxmind/geoip2/NetworkDeserializer.java b/src/main/java/com/maxmind/geoip2/NetworkDeserializer.java
@@ -11,10 +11,10 @@
 /**
  * This class provides a deserializer for the Network class.
  */
-public class NetworkDeserializer extends StdDeserializer<Network> {
+public final class NetworkDeserializer extends StdDeserializer<Network> {
 
     /**
-     * Constructs a @{code NetworkDeserializer} object.
+     * Constructs a {@code NetworkDeserializer} with no type specified.
      */
     public NetworkDeserializer() {
         this(null);
@@ -30,23 +30,46 @@ public NetworkDeserializer(Class<?> vc) {
     }
 
     @Override
-    public Network deserialize(
-        JsonParser jsonparser, DeserializationContext context)
-        throws IOException {
+    public Network deserialize(JsonParser jsonparser, DeserializationContext context)
+            throws IOException {
 
-        String cidr = jsonparser.getText();
-        if (cidr == null) {
+        final String cidr = jsonparser.getValueAsString();
+        if (cidr == null || cidr.isBlank()) {
             return null;
         }
-        String[] parts = cidr.split("/", 2);
+        return parseCidr(cidr.trim());
+    }
+
+    private static Network parseCidr(String cidr) throws IOException {
+        final String[] parts = cidr.split("/", 2);
         if (parts.length != 2) {
-            throw new RuntimeException("Invalid cidr format: " + cidr);
+            throw new IllegalArgumentException("Invalid CIDR format: " + cidr);
         }
-        int prefixLength = Integer.parseInt(parts[1]);
+
+        final String addrPart = parts[0].trim();
+        final String prefixPart = parts[1].trim();
+
+        final InetAddress address;
         try {
-            return new Network(InetAddress.getByName(parts[0]), prefixLength);
+            address = InetAddress.getByName(addrPart);
         } catch (UnknownHostException e) {
-            throw new RuntimeException(e);
+            throw new IOException("Unknown host in CIDR: " + cidr, e);
         }
+
+        final int prefixLength;
+        try {
+            prefixLength = Integer.parseInt(prefixPart);
+        } catch (NumberFormatException e) {
+            throw new IllegalArgumentException(
+                    "Invalid prefix length in CIDR: " + cidr, e);
+        }
+
+        final int maxPrefix = (address.getAddress().length == 4) ? 32 : 128;
+        if (prefixLength < 0 || prefixLength > maxPrefix) {
+            throw new IllegalArgumentException(
+                    "Prefix length out of range (0-" + maxPrefix + ") for CIDR: " + cidr);
+        }
+
+        return new Network(address, prefixLength);
     }
-}
+}
diff --git a/src/test/java/com/maxmind/geoip2/JsonInjectorTest.java b/src/test/java/com/maxmind/geoip2/JsonInjectorTest.java
@@ -0,0 +1,40 @@
+package com.maxmind.geoip2;
+
+import com.maxmind.db.Network;
+import com.maxmind.geoip2.record.Traits;
+import org.junit.jupiter.api.Test;
+
+import java.net.InetAddress;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+final class JsonInjectorTest {
+
+    @Test
+    void injectsValuesAndReusesTraits() throws Exception {
+        var locales = List.of("en", "fr");
+        var ip = "1.2.3.4";
+        var network = new Network(InetAddress.getByName("1.2.3.0"), 24);
+
+        var injector = new JsonInjector(locales, ip, network);
+
+        assertEquals(locales, injector.findInjectableValue("locales", null, null, null));
+        assertEquals(ip, injector.findInjectableValue("ip_address", null, null, null));
+        assertEquals(network, injector.findInjectableValue("network", null, null, null));
+
+        var traits1 = injector.findInjectableValue("traits", null, null, null);
+        var traits2 = injector.findInjectableValue("traits", null, null, null);
+        assertNotNull(traits1);
+        assertSame(traits1, traits2);
+        assertEquals(ip, ((Traits) traits1).getIpAddress());
+        assertEquals(network, ((Traits) traits1).getNetwork());
+    }
+
+    @Test
+    void unknownOrNonStringKeyReturnsNull() {
+        var injector = new JsonInjector(List.of(), null, null);
+        assertNull(injector.findInjectableValue("unknown", null, null, null));
+        assertNull(injector.findInjectableValue(123, null, null, null));
+    }
+}
diff --git a/src/test/java/com/maxmind/geoip2/NetworkDeserializerTest.java b/src/test/java/com/maxmind/geoip2/NetworkDeserializerTest.java
@@ -0,0 +1,88 @@
+package com.maxmind.geoip2;
+
+import com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonParser;
+import com.maxmind.db.Network;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.net.InetAddress;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+final class NetworkDeserializerTest {
+
+
+    private static Network parse(String jsonString) throws IOException {
+        var deserializer = new NetworkDeserializer();
+        JsonFactory jf = new JsonFactory();
+        try (JsonParser p = jf.createParser(jsonString)) {
+            p.nextToken();
+            return deserializer.deserialize(p, null);
+        }
+    }
+    private static void assertNetwork(Network n, String addr, int prefix) throws Exception {
+        assertNotNull(n);
+        assertEquals(InetAddress.getByName(addr), n.getNetworkAddress());
+        assertEquals(prefix, n.getPrefixLength());
+    }
+
+    @Test
+    void parsesValidIPv4Cidr() throws Exception {
+        Network actual = parse("\"1.2.3.0/24\"");
+        assertNetwork(actual, "1.2.3.0", 24);
+    }
+
+    @Test
+    void parsesValidIPv6Cidr() throws Exception {
+        Network actual = parse("\"2001:db8::/32\"");
+        assertNetwork(actual, "2001:db8::", 32);
+    }
+
+    @Test
+    void trimsWhitespace() throws Exception {
+        Network actual = parse("\"  10.0.0.0/8  \"");
+        assertNetwork(actual, "10.0.0.0", 8);
+    }
+
+
+    @Test
+    void returnsNullOnJsonNull() throws Exception {
+        Network actual = parse("null");
+        assertNull(actual);
+    }
+
+    @Test
+    void returnsNullOnBlankString() throws Exception {
+        Network actual = parse("\"   \"");
+        assertNull(actual);
+    }
+
+    @Test
+    void throwsOnMissingSlash() {
+        assertThrows(IllegalArgumentException.class, () -> parse("\"1.2.3.0\""));
+    }
+
+    @Test
+    void throwsOnNonNumericPrefix() {
+        assertThrows(IllegalArgumentException.class, () -> parse("\"1.2.3.0/xx\""));
+    }
+
+    @Test
+    void throwsOnOutOfRangePrefixIpv4() {
+        assertThrows(IllegalArgumentException.class, () -> parse("\"1.2.3.0/64\""));
+        assertThrows(IllegalArgumentException.class, () -> parse("\"1.2.3.0/-1\""));
+    }
+
+    @Test
+    void throwsOnOutOfRangePrefixIpv6() {
+        assertThrows(IllegalArgumentException.class, () -> parse("\"::/129\""));
+        assertThrows(IllegalArgumentException.class, () -> parse("\"::/-1\""));
+    }
+
+    @Test
+    void wrapsUnknownHostInIOException() {
+        IOException ex = assertThrows(IOException.class, () -> parse("\"999.999.999.999/24\""));
+        assertNotNull(ex.getCause());
+    }
+}
