From 5a8970edefda0ceba6714fc9676d8f220ed2fd71 Mon Sep 17 00:00:00 2001 From: currantw Date: Mon, 2 Dec 2024 15:37:59 -0800 Subject: [PATCH] Assert that comparison only valid if same type, update tests accordingly Signed-off-by: currantw --- .../sql/data/model/ExprIpValue.java | 30 ++++++++----------- .../sql/data/model/ExprIpValueTest.java | 6 ++-- .../convert/TypeCastOperatorTest.java | 3 +- .../data/type/OpenSearchDataType.java | 2 ++ .../value/OpenSearchExprValueFactoryTest.java | 3 +- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java index bbf4bd8c59..6207796fcb 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java @@ -30,7 +30,17 @@ public class ExprIpValue extends AbstractExprValue { .toParams(); public ExprIpValue(String s) { - value = stringToIpAddress(s); + try { + IPAddress address = new IPAddressString(s, validationOptions).toAddress(); + + // Convert IPv6 mapped IPv4 addresses to IPv4 + if (address.isIPv4Convertible()) address = address.toIPv4(); + + value = address; + } catch (AddressStringException e) { + final String errorFormatString = "IP address string '%s' is not valid. Error details: %s"; + throw new SemanticCheckException(String.format(errorFormatString, s, e.getMessage())); + } } @Override @@ -45,14 +55,11 @@ public ExprType type() { @Override public int compare(ExprValue other) { - IPAddress otherValue = - other instanceof ExprIpValue exprIpValue - ? exprIpValue.value - : stringToIpAddress(other.stringValue()); + assert (other instanceof ExprIpValue); // Map IPv4 addresses to IPv6 for comparison IPv6Address ipv6Value = toIPv6Address(value); - IPv6Address otherIpv6Value = toIPv6Address(otherValue); + IPv6Address otherIpv6Value = toIPv6Address(((ExprIpValue) other).value); return ipv6Value.compareTo(otherIpv6Value); } @@ -67,17 +74,6 @@ public String toString() { return String.format("IP %s", value()); } - /** Returns the {@link IPAddress} corresponding to the given {@link String}. */ - private static IPAddress stringToIpAddress(String s) { - try { - IPAddress address = new IPAddressString(s, validationOptions).toAddress(); - return address.isIPv4Convertible() ? address.toIPv4() : address; - } catch (AddressStringException e) { - final String errorFormatString = "IP address '%s' is not valid. Error details: %s"; - throw new SemanticCheckException(String.format(errorFormatString, s, e.getMessage())); - } - } - /** Returns the {@link IPv6Address} corresponding to the given {@link IPAddress}. */ private static IPv6Address toIPv6Address(IPAddress ipAddress) { return ipAddress instanceof IPv4Address iPv4Address diff --git a/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java b/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java index e985937df5..0e248cf05f 100644 --- a/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java +++ b/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java @@ -9,7 +9,6 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import inet.ipaddr.AddressStringException; import java.util.List; import org.junit.jupiter.api.Test; import org.opensearch.sql.data.type.ExprCoreType; @@ -57,11 +56,12 @@ public void testInvalid() { assertTrue( ex.getMessage() .matches( - String.format("IP address '%s' is not valid. Error details: .*", ipInvalidString))); + String.format( + "IP address string '%s' is not valid. Error details: .*", ipInvalidString))); } @Test - public void testValue() throws AddressStringException { + public void testValue() { ipv4EqualStrings.forEach((s) -> assertEquals(ipv4String, ExprValueUtils.ipValue(s).value())); ipv6EqualStrings.forEach((s) -> assertEquals(ipv6String, ExprValueUtils.ipValue(s).value())); } diff --git a/core/src/test/java/org/opensearch/sql/expression/operator/convert/TypeCastOperatorTest.java b/core/src/test/java/org/opensearch/sql/expression/operator/convert/TypeCastOperatorTest.java index 80df91b6f5..c8c365b5d1 100644 --- a/core/src/test/java/org/opensearch/sql/expression/operator/convert/TypeCastOperatorTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/operator/convert/TypeCastOperatorTest.java @@ -364,7 +364,8 @@ void castToIp() { exp = DSL.castIp(DSL.literal(ipInvalidString)); actualMsg = assertThrows(SemanticCheckException.class, exp::valueOf).getMessage(); - expectedMsg = String.format("IP address '%s' is not valid. Error details: .*", ipInvalidString); + expectedMsg = + String.format("IP address string '%s' is not valid. Error details: .*", ipInvalidString); assertTrue(actualMsg.matches(expectedMsg)); // From IP address diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java index 6c8912be86..2bcad9576a 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java @@ -160,6 +160,8 @@ public static OpenSearchDataType of(MappingType mappingType, Map return OpenSearchGeoPointType.of(); case Binary: return OpenSearchBinaryType.of(); + case Ip: + return OpenSearchIpType.of(); case Date: case DateNanos: // Default date formatter is used when "" is passed as the second parameter diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index 90efba3a64..bbbdfee684 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java @@ -59,6 +59,7 @@ import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.opensearch.data.type.OpenSearchDataType; import org.opensearch.sql.opensearch.data.type.OpenSearchDateType; +import org.opensearch.sql.opensearch.data.type.OpenSearchIpType; import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; import org.opensearch.sql.opensearch.data.utils.OpenSearchJsonContent; @@ -116,7 +117,7 @@ class OpenSearchExprValueFactoryTest { "textKeywordV", OpenSearchTextType.of( Map.of("words", OpenSearchDataType.of(OpenSearchDataType.MappingType.Keyword)))) - .put(fieldIp, OpenSearchDataType.of(OpenSearchDataType.MappingType.Ip)) + .put(fieldIp, OpenSearchIpType.of(OpenSearchDataType.MappingType.Ip)) .put("geoV", OpenSearchDataType.of(OpenSearchDataType.MappingType.GeoPoint)) .put("binaryV", OpenSearchDataType.of(OpenSearchDataType.MappingType.Binary)) .build();