diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cbb2519..a6058418 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,17 @@ CHANGELOG ========= +4.0.2 +------------------ + +* Fixed a bug where enums with `@MaxMindDbCreator` would throw + `ConstructorNotFoundException` when the data was stored via a pointer + in the database. This commonly occurred with deduplicated data in larger + databases. Reported by Fabrice Bacchella. GitHub #644 in GeoIP2-java. +* Improved error messages when constructor invocation fails. The error now + correctly identifies null values being passed to primitive parameters + instead of reporting misleading boxed/primitive type mismatches. + 4.0.1 (2025-12-02) ------------------ diff --git a/src/main/java/com/maxmind/db/Decoder.java b/src/main/java/com/maxmind/db/Decoder.java index 608fac57..f76b5851 100644 --- a/src/main/java/com/maxmind/db/Decoder.java +++ b/src/main/java/com/maxmind/db/Decoder.java @@ -30,6 +30,9 @@ class Decoder { private static final int[] POINTER_VALUE_OFFSETS = {0, 0, 1 << 11, (1 << 19) + (1 << 11), 0}; + // Sentinel to cache "no creator method exists" to avoid repeated method scanning + private static final CachedCreator NO_CREATOR = new CachedCreator(null, null); + private final NodeCache cache; private final long pointerBase; @@ -184,10 +187,17 @@ private boolean requiresLookupContext(Class cls) { || cls.equals(Object.class) || Map.class.isAssignableFrom(cls) || List.class.isAssignableFrom(cls) + || cls.isEnum() || isSimpleType(cls)) { return false; } + // Non-enum classes with @MaxMindDbCreator don't require lookup context + // since they just convert simple values (strings, booleans, etc.) + if (getCachedCreator(cls) != null) { + return false; + } + var cached = getCachedConstructor(cls); if (cached == null) { cached = loadConstructorMetadata(cls); @@ -696,16 +706,66 @@ private Object decodeMapIntoObject(int size, Class cls) var sbErrors = new StringBuilder(); for (var key : parameterIndexes.keySet()) { var index = parameterIndexes.get(key); - if (parameters[index] != null - && !parameters[index].getClass().isAssignableFrom(parameterTypes[index])) { - sbErrors.append(" argument type mismatch in " + key + " MMDB Type: " - + parameters[index].getClass().getCanonicalName() - + " Java Type: " + parameterTypes[index].getCanonicalName()); + if (parameters[index] == null && parameterTypes[index].isPrimitive()) { + sbErrors.append(" null value for primitive ") + .append(parameterTypes[index].getName()) + .append(" parameter '").append(key).append("'."); + } else if (parameters[index] != null + && !isAssignableType(parameters[index].getClass(), parameterTypes[index])) { + sbErrors.append(" argument type mismatch in ").append(key) + .append(" MMDB Type: ") + .append(parameters[index].getClass().getCanonicalName()) + .append(" Java Type: ") + .append(parameterTypes[index].getCanonicalName()) + .append("."); } } throw new DeserializationException( - "Error creating object of type: " + cls.getSimpleName() + " - " + sbErrors, e); + "Error creating object of type: " + cls.getSimpleName() + " -" + sbErrors, e); + } + } + + /** + * Checks if an actual type can be assigned to an expected type, + * accounting for primitive/boxed equivalents (e.g., Boolean to boolean). + */ + private static boolean isAssignableType(Class actual, Class expected) { + if (expected.isAssignableFrom(actual)) { + return true; + } + // Check primitive/boxed equivalents for auto-unboxing + if (expected.isPrimitive()) { + return actual.equals(boxedType(expected)); } + return false; + } + + private static Class boxedType(Class primitive) { + if (primitive == boolean.class) { + return Boolean.class; + } + if (primitive == byte.class) { + return Byte.class; + } + if (primitive == short.class) { + return Short.class; + } + if (primitive == int.class) { + return Integer.class; + } + if (primitive == long.class) { + return Long.class; + } + if (primitive == float.class) { + return Float.class; + } + if (primitive == double.class) { + return Double.class; + } + if (primitive == char.class) { + return Character.class; + } + return primitive; } private boolean shouldInstantiateFromContext(Class parameterType) { @@ -960,14 +1020,15 @@ private Object convertValue(Object value, Class targetType) { private CachedCreator getCachedCreator(Class cls) { CachedCreator cached = this.creators.get(cls); + if (cached == NO_CREATOR) { + return null; // Known to have no creator + } if (cached != null) { return cached; } CachedCreator creator = findCreatorMethod(cls); - if (creator != null) { - this.creators.putIfAbsent(cls, creator); - } + this.creators.putIfAbsent(cls, creator != null ? creator : NO_CREATOR); return creator; } diff --git a/src/test/java/com/maxmind/db/ReaderTest.java b/src/test/java/com/maxmind/db/ReaderTest.java index 890a95ac..adfe2f3d 100644 --- a/src/test/java/com/maxmind/db/ReaderTest.java +++ b/src/test/java/com/maxmind/db/ReaderTest.java @@ -493,8 +493,8 @@ public void testNoIpV4SearchTreeStream(int chunkSizes) throws IOException { private void testNoIpV4SearchTree(Reader reader) throws IOException { - assertEquals("::0/64", reader.get(InetAddress.getByName("1.1.1.1"), String.class)); - assertEquals("::0/64", reader.get(InetAddress.getByName("192.1.1.1"), String.class)); + assertEquals("::/64", reader.get(InetAddress.getByName("1.1.1.1"), String.class)); + assertEquals("::/64", reader.get(InetAddress.getByName("192.1.1.1"), String.class)); } @ParameterizedTest @@ -2042,6 +2042,148 @@ private void testIpV6(Reader reader, File file) throws IOException { } } + // ========================================================================= + // Tests for enum with @MaxMindDbCreator when data is stored via pointer + // See: https://github.com/maxmind/GeoIP2-java/issues/644 + // ========================================================================= + + /** + * Enum with @MaxMindDbCreator for converting string values. + * This simulates how ConnectionType enum works in geoip2. + */ + enum ConnectionTypeEnum { + DIALUP("Dialup"), + CABLE_DSL("Cable/DSL"), + CORPORATE("Corporate"), + CELLULAR("Cellular"), + SATELLITE("Satellite"), + UNKNOWN("Unknown"); + + private final String name; + + ConnectionTypeEnum(String name) { + this.name = name; + } + + @MaxMindDbCreator + public static ConnectionTypeEnum fromString(String s) { + if (s == null) { + return UNKNOWN; + } + return switch (s) { + case "Dialup" -> DIALUP; + case "Cable/DSL" -> CABLE_DSL; + case "Corporate" -> CORPORATE; + case "Cellular" -> CELLULAR; + case "Satellite" -> SATELLITE; + default -> UNKNOWN; + }; + } + } + + /** + * Model class that uses the ConnectionTypeEnum for the connection_type field. + */ + static class TraitsModel { + ConnectionTypeEnum connectionType; + + @MaxMindDbConstructor + public TraitsModel( + @MaxMindDbParameter(name = "connection_type") + ConnectionTypeEnum connectionType + ) { + this.connectionType = connectionType; + } + } + + /** + * Top-level model for Enterprise database records. + */ + static class EnterpriseModel { + TraitsModel traits; + + @MaxMindDbConstructor + public EnterpriseModel( + @MaxMindDbParameter(name = "traits") + TraitsModel traits + ) { + this.traits = traits; + } + } + + /** + * This test passes because IP 74.209.24.0 has connection_type stored inline. + */ + @ParameterizedTest + @MethodSource("chunkSizes") + public void testEnumCreatorWithInlineData(int chunkSize) throws IOException { + try (var reader = new Reader(getFile("GeoIP2-Enterprise-Test.mmdb"), chunkSize)) { + var ip = InetAddress.getByName("74.209.24.0"); + var result = reader.get(ip, EnterpriseModel.class); + assertNotNull(result); + assertNotNull(result.traits); + assertEquals(ConnectionTypeEnum.CABLE_DSL, result.traits.connectionType); + } + } + + /** + * This test verifies that enums with @MaxMindDbCreator work correctly when + * the data is stored via a pointer (common for deduplication in databases). + * + *

Previously, this would throw ConstructorNotFoundException because + * requiresLookupContext() called loadConstructorMetadata() before checking + * for creator methods. + */ + @ParameterizedTest + @MethodSource("chunkSizes") + public void testEnumCreatorWithPointerData(int chunkSize) throws IOException { + try (var reader = new Reader(getFile("GeoIP2-Enterprise-Test.mmdb"), chunkSize)) { + var ip = InetAddress.getByName("89.160.20.112"); + var result = reader.get(ip, EnterpriseModel.class); + assertNotNull(result); + assertNotNull(result.traits); + assertEquals(ConnectionTypeEnum.CORPORATE, result.traits.connectionType); + } + } + + // Model class with primitive double field for testing null-to-primitive error messages + static class IpRiskModelWithPrimitive { + public final double ipRisk; + + @MaxMindDbConstructor + public IpRiskModelWithPrimitive( + @MaxMindDbParameter(name = "ip_risk") double ipRisk + ) { + this.ipRisk = ipRisk; + } + } + + /** + * Tests that error messages correctly report null-to-primitive issues instead of + * misleading boxed/primitive type mismatch messages. + * + *

IP 11.1.2.3 in the IP Risk test database doesn't have an ip_risk field. + * When deserializing to a model with a primitive double, the error message should + * correctly identify "null value for primitive double" rather than reporting + * misleading Boolean/boolean mismatches. + */ + @ParameterizedTest + @MethodSource("chunkSizes") + public void testNullToPrimitiveErrorMessage(int chunkSize) throws IOException { + try (var reader = new Reader(getFile("GeoIP2-IP-Risk-Test.mmdb"), chunkSize)) { + var ip = InetAddress.getByName("11.1.2.3"); + + var exception = assertThrows(DeserializationException.class, + () -> reader.get(ip, IpRiskModelWithPrimitive.class)); + + // Error message should mention null value for primitive, not type mismatch + assertTrue(exception.getMessage().contains("null value for primitive double"), + "Error message should identify null-to-primitive issue: " + exception.getMessage()); + assertTrue(exception.getMessage().contains("ip_risk"), + "Error message should name the problematic parameter: " + exception.getMessage()); + } + } + static File getFile(String name) { return new File(ReaderTest.class.getResource("/maxmind-db/test-data/" + name).getFile()); } diff --git a/src/test/resources/maxmind-db b/src/test/resources/maxmind-db index 86095bd9..b6eb21df 160000 --- a/src/test/resources/maxmind-db +++ b/src/test/resources/maxmind-db @@ -1 +1 @@ -Subproject commit 86095bd9855d6313c501fe0097891a3c6734ae90 +Subproject commit b6eb21df882cc939d6b31bc05811a101f17fe148