diff --git a/CHANGELOG.md b/CHANGELOG.md index c69c928a7ab56..cf886bf8bc13e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix NPE of ScriptScoreQuery ([#19650](https://github.com/opensearch-project/OpenSearch/pull/19650)) - Fix ClassCastException in FlightClientChannel for requests larger than 16KB ([#20010](https://github.com/opensearch-project/OpenSearch/pull/20010)) - Fix GRPC Bulk ([#19937](https://github.com/opensearch-project/OpenSearch/pull/19937)) +- Fix bug in Assertion framework(Yaml Rest test): numeric comparison fails when comparing Integer vs Long (or Float vs Double) ([#19376](https://github.com/opensearch-project/OpenSearch/pull/19376)) - Fix node bootstrap error when enable stream transport and remote cluster state ([#19948](https://github.com/opensearch-project/OpenSearch/pull/19948)) - Keep track and release Reactor Netty 4 Transport accepted Http Channels during the Node shutdown ([#20106](https://github.com/opensearch-project/OpenSearch/pull/20106)) - Fix deletion failure/error of unused index template; case when an index template matches a data stream but has a lower priority. ([#20102](https://github.com/opensearch-project/OpenSearch/pull/20102)) diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/Assertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/Assertion.java index 732d4291ae670..4bbc2c16abf01 100644 --- a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/Assertion.java +++ b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/Assertion.java @@ -91,7 +91,11 @@ static Object convertActualValue(Object actualValue, Object expectedValue) { } else if (expectedValue instanceof Double) { return Double.parseDouble(actualValue.toString()); } else if (expectedValue instanceof Integer) { - return Integer.parseInt(actualValue.toString()); + try { + return Long.parseLong(actualValue.toString()); + } catch (NumberFormatException e) { + return Integer.parseInt(actualValue.toString()); + } } else if (expectedValue instanceof Long) { return Long.parseLong(actualValue.toString()); } diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanAssertion.java index 0d20dc7c326b0..7534f185bfd1b 100644 --- a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanAssertion.java +++ b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanAssertion.java @@ -31,65 +31,32 @@ package org.opensearch.test.rest.yaml.section; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.opensearch.common.collect.Tuple; import org.opensearch.core.xcontent.XContentLocation; import org.opensearch.core.xcontent.XContentParser; import java.io.IOException; -import static org.hamcrest.Matchers.greaterThan; -import static org.hamcrest.Matchers.instanceOf; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.fail; - /** * Represents a gt assert section: *

* - gt: { fields._ttl: 0} */ -public class GreaterThanAssertion extends Assertion { +public class GreaterThanAssertion extends OrderingAssertion { public static GreaterThanAssertion parse(XContentParser parser) throws IOException { - XContentLocation location = parser.getTokenLocation(); - Tuple stringObjectTuple = ParserUtils.parseTuple(parser); - if (!(stringObjectTuple.v2() instanceof Comparable)) { - throw new IllegalArgumentException( - "gt section can only be used with objects that support natural ordering, found " - + stringObjectTuple.v2().getClass().getSimpleName() - ); - } - return new GreaterThanAssertion(location, stringObjectTuple.v1(), stringObjectTuple.v2()); + return parseOrderingAssertion(parser, Relation.GT, GreaterThanAssertion::new); } - private static final Logger logger = LogManager.getLogger(GreaterThanAssertion.class); - - public GreaterThanAssertion(XContentLocation location, String field, Object expectedValue) { - super(location, field, expectedValue); + public GreaterThanAssertion(XContentLocation loc, String field, Object expected) { + super(loc, field, expected); } @Override - protected void doAssert(Object actualValue, Object expectedValue) { - logger.trace("assert that [{}] is greater than [{}] (field: [{}])", actualValue, expectedValue, getField()); - actualValue = convertActualValue(actualValue, expectedValue); - assertThat( - "value of [" + getField() + "] is not comparable (got [" + safeClass(actualValue) + "])", - actualValue, - instanceOf(Comparable.class) - ); - assertThat( - "expected value of [" + getField() + "] is not comparable (got [" + expectedValue.getClass() + "])", - expectedValue, - instanceOf(Comparable.class) - ); - try { - assertThat(errorMessage(), (Comparable) actualValue, greaterThan((Comparable) expectedValue)); - } catch (ClassCastException e) { - fail("cast error while checking (" + errorMessage() + "): " + e); - } + protected Relation relation() { + return Relation.GT; } - private String errorMessage() { + @Override + protected String errorMessage() { return "field [" + getField() + "] is not greater than [" + getExpectedValue() + "]"; } } diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanEqualToAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanEqualToAssertion.java index a6435c1303489..82670f77314a0 100644 --- a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanEqualToAssertion.java +++ b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanEqualToAssertion.java @@ -32,65 +32,32 @@ package org.opensearch.test.rest.yaml.section; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.opensearch.common.collect.Tuple; import org.opensearch.core.xcontent.XContentLocation; import org.opensearch.core.xcontent.XContentParser; import java.io.IOException; -import static org.hamcrest.Matchers.greaterThanOrEqualTo; -import static org.hamcrest.Matchers.instanceOf; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.fail; - /** * Represents a gte assert section: *

* - gte: { fields._ttl: 0 } */ -public class GreaterThanEqualToAssertion extends Assertion { +public class GreaterThanEqualToAssertion extends OrderingAssertion { public static GreaterThanEqualToAssertion parse(XContentParser parser) throws IOException { - XContentLocation location = parser.getTokenLocation(); - Tuple stringObjectTuple = ParserUtils.parseTuple(parser); - if (!(stringObjectTuple.v2() instanceof Comparable)) { - throw new IllegalArgumentException( - "gte section can only be used with objects that support natural ordering, found " - + stringObjectTuple.v2().getClass().getSimpleName() - ); - } - return new GreaterThanEqualToAssertion(location, stringObjectTuple.v1(), stringObjectTuple.v2()); + return parseOrderingAssertion(parser, Relation.GTE, GreaterThanEqualToAssertion::new); } - private static final Logger logger = LogManager.getLogger(GreaterThanEqualToAssertion.class); - - public GreaterThanEqualToAssertion(XContentLocation location, String field, Object expectedValue) { - super(location, field, expectedValue); + public GreaterThanEqualToAssertion(XContentLocation loc, String field, Object expected) { + super(loc, field, expected); } @Override - protected void doAssert(Object actualValue, Object expectedValue) { - logger.trace("assert that [{}] is greater than or equal to [{}] (field: [{}])", actualValue, expectedValue, getField()); - actualValue = convertActualValue(actualValue, expectedValue); - assertThat( - "value of [" + getField() + "] is not comparable (got [" + safeClass(actualValue) + "])", - actualValue, - instanceOf(Comparable.class) - ); - assertThat( - "expected value of [" + getField() + "] is not comparable (got [" + expectedValue.getClass() + "])", - expectedValue, - instanceOf(Comparable.class) - ); - try { - assertThat(errorMessage(), (Comparable) actualValue, greaterThanOrEqualTo((Comparable) expectedValue)); - } catch (ClassCastException e) { - fail("cast error while checking (" + errorMessage() + "): " + e); - } + protected Relation relation() { + return Relation.GTE; } - private String errorMessage() { + @Override + protected String errorMessage() { return "field [" + getField() + "] is not greater than or equal to [" + getExpectedValue() + "]"; } } diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanAssertion.java index acffe03d34439..e14ec1b169104 100644 --- a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanAssertion.java +++ b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanAssertion.java @@ -31,66 +31,33 @@ package org.opensearch.test.rest.yaml.section; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.opensearch.common.collect.Tuple; import org.opensearch.core.xcontent.XContentLocation; import org.opensearch.core.xcontent.XContentParser; import java.io.IOException; -import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.lessThan; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.fail; - /** * Represents a lt assert section: *

* - lt: { fields._ttl: 20000} * */ -public class LessThanAssertion extends Assertion { +public class LessThanAssertion extends OrderingAssertion { public static LessThanAssertion parse(XContentParser parser) throws IOException { - XContentLocation location = parser.getTokenLocation(); - Tuple stringObjectTuple = ParserUtils.parseTuple(parser); - if (false == stringObjectTuple.v2() instanceof Comparable) { - throw new IllegalArgumentException( - "lt section can only be used with objects that support natural ordering, found " - + stringObjectTuple.v2().getClass().getSimpleName() - ); - } - return new LessThanAssertion(location, stringObjectTuple.v1(), stringObjectTuple.v2()); + return parseOrderingAssertion(parser, Relation.LT, LessThanAssertion::new); } - private static final Logger logger = LogManager.getLogger(LessThanAssertion.class); - - public LessThanAssertion(XContentLocation location, String field, Object expectedValue) { - super(location, field, expectedValue); + public LessThanAssertion(XContentLocation loc, String field, Object expected) { + super(loc, field, expected); } @Override - protected void doAssert(Object actualValue, Object expectedValue) { - logger.trace("assert that [{}] is less than [{}] (field: [{}])", actualValue, expectedValue, getField()); - actualValue = convertActualValue(actualValue, expectedValue); - assertThat( - "value of [" + getField() + "] is not comparable (got [" + safeClass(actualValue) + "])", - actualValue, - instanceOf(Comparable.class) - ); - assertThat( - "expected value of [" + getField() + "] is not comparable (got [" + expectedValue.getClass() + "])", - expectedValue, - instanceOf(Comparable.class) - ); - try { - assertThat(errorMessage(), (Comparable) actualValue, lessThan((Comparable) expectedValue)); - } catch (ClassCastException e) { - fail("cast error while checking (" + errorMessage() + "): " + e); - } + protected Relation relation() { + return Relation.LT; } - private String errorMessage() { + @Override + protected String errorMessage() { return "field [" + getField() + "] is not less than [" + getExpectedValue() + "]"; } } diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanOrEqualToAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanOrEqualToAssertion.java index d685d3e46a543..b45d987adcae5 100644 --- a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanOrEqualToAssertion.java +++ b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanOrEqualToAssertion.java @@ -32,65 +32,32 @@ package org.opensearch.test.rest.yaml.section; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.opensearch.common.collect.Tuple; import org.opensearch.core.xcontent.XContentLocation; import org.opensearch.core.xcontent.XContentParser; import java.io.IOException; -import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.lessThanOrEqualTo; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.fail; - /** * Represents a lte assert section: *

* - lte: { fields._ttl: 0 } */ -public class LessThanOrEqualToAssertion extends Assertion { +public class LessThanOrEqualToAssertion extends OrderingAssertion { public static LessThanOrEqualToAssertion parse(XContentParser parser) throws IOException { - XContentLocation location = parser.getTokenLocation(); - Tuple stringObjectTuple = ParserUtils.parseTuple(parser); - if (false == stringObjectTuple.v2() instanceof Comparable) { - throw new IllegalArgumentException( - "lte section can only be used with objects that support natural ordering, found " - + stringObjectTuple.v2().getClass().getSimpleName() - ); - } - return new LessThanOrEqualToAssertion(location, stringObjectTuple.v1(), stringObjectTuple.v2()); + return parseOrderingAssertion(parser, Relation.LTE, LessThanOrEqualToAssertion::new); } - private static final Logger logger = LogManager.getLogger(LessThanOrEqualToAssertion.class); - - public LessThanOrEqualToAssertion(XContentLocation location, String field, Object expectedValue) { - super(location, field, expectedValue); + public LessThanOrEqualToAssertion(XContentLocation loc, String field, Object expected) { + super(loc, field, expected); } @Override - protected void doAssert(Object actualValue, Object expectedValue) { - logger.trace("assert that [{}] is less than or equal to [{}] (field: [{}])", actualValue, expectedValue, getField()); - actualValue = convertActualValue(actualValue, expectedValue); - assertThat( - "value of [" + getField() + "] is not comparable (got [" + safeClass(actualValue) + "])", - actualValue, - instanceOf(Comparable.class) - ); - assertThat( - "expected value of [" + getField() + "] is not comparable (got [" + expectedValue.getClass() + "])", - expectedValue, - instanceOf(Comparable.class) - ); - try { - assertThat(errorMessage(), (Comparable) actualValue, lessThanOrEqualTo((Comparable) expectedValue)); - } catch (ClassCastException e) { - fail("cast error while checking (" + errorMessage() + "): " + e); - } + protected Relation relation() { + return Relation.LTE; } - private String errorMessage() { + @Override + protected String errorMessage() { return "field [" + getField() + "] is not less than or equal to [" + getExpectedValue() + "]"; } } diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/OrderingAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/OrderingAssertion.java new file mode 100644 index 0000000000000..dbab2b4738038 --- /dev/null +++ b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/OrderingAssertion.java @@ -0,0 +1,138 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.test.rest.yaml.section; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.opensearch.common.collect.Tuple; +import org.opensearch.core.xcontent.XContentLocation; +import org.opensearch.core.xcontent.XContentParser; + +import java.io.IOException; + +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +/** + * Base class for all ordering assertions (gt/gte/lt/lte). + * It provides: + * - shared YAML parse boilerplate (via {@link #parseOrderingAssertion}) + * - numeric normalization so both sides are Comparable of the same class + * - common comparison logic based on a {@link Relation} + */ +public abstract class OrderingAssertion extends Assertion { + protected OrderingAssertion(XContentLocation location, String field, Object expectedValue) { + super(location, field, expectedValue); + } + + private static final Logger logger = LogManager.getLogger(OrderingAssertion.class); + + protected enum Relation { + GT, + GTE, + LT, + LTE + } + + protected abstract Relation relation(); + + protected abstract String errorMessage(); + + @FunctionalInterface + protected interface OrderingAssertionFactory { + T create(XContentLocation location, String field, Object expectedValue); + } + + /** + * Common parser for {gt|gte|lt|lte}: { field: expectedValue } + */ + protected static T parseOrderingAssertion( + XContentParser parser, + Relation relation, + OrderingAssertionFactory factory + ) throws IOException { + XContentLocation location = parser.getTokenLocation(); + Tuple t = ParserUtils.parseTuple(parser); + Object expected = t.v2(); + if (!(expected instanceof Comparable)) { + throw new IllegalArgumentException( + relation + + " section can only be used with objects that support natural ordering, found " + + expected.getClass().getSimpleName() + ); + } + return factory.create(location, t.v1(), expected); + } + + @Override + @SuppressWarnings({ "rawtypes", "unchecked" }) + protected final void doAssert(Object actualValue, Object expectedValue) { + logger.trace("assert that [{}] {} [{}] (field: [{}])", actualValue, relationToString(relation()), expectedValue, getField()); + + Object castActual = convertActualValue(actualValue, expectedValue); + Tuple normalized = normalizePair(castActual, expectedValue); + Object actualNormalized = normalized.v1(); + Object expectedNormalized = normalized.v2(); + + assertThat( + "value of [" + getField() + "] is not comparable (got [" + safeClass(actualNormalized) + "])", + actualNormalized, + instanceOf(Comparable.class) + ); + assertThat( + "expected value of [" + getField() + "] is not comparable (got [" + expectedNormalized.getClass() + "])", + expectedNormalized, + instanceOf(Comparable.class) + ); + + try { + int cmp = ((Comparable) actualNormalized).compareTo(expectedNormalized); + boolean ok = switch (relation()) { + case GT -> cmp > 0; + case GTE -> cmp >= 0; + case LT -> cmp < 0; + case LTE -> cmp <= 0; + }; + if (!ok) { + fail(errorMessage()); + } + } catch (ClassCastException e) { + fail("cast error while checking (" + errorMessage() + "): " + e); + } + } + + private static String relationToString(Relation r) { + return switch (r) { + case GT -> "is greater than"; + case GTE -> "is greater than or equal to"; + case LT -> "is less than"; + case LTE -> "is less than or equal to"; + }; + } + + /** + * Normalize a pair to comparable types to avoid Integer vs Long / Double vs Integer ClassCastExceptions. + * Rules: + * - If both are Numbers and any is floating (Float/Double), coerce both to Double. + * - Else if both are Numbers, coerce both to Long (widening smaller integral types). + * - Else return as-is (let Comparable semantics handle it). + */ + private static Tuple normalizePair(Object a, Object b) { + if (a instanceof Number aNum && b instanceof Number bNum) { + boolean isFloating = aNum instanceof Float || aNum instanceof Double || bNum instanceof Float || bNum instanceof Double; + if (isFloating) { + return Tuple.tuple(aNum.doubleValue(), bNum.doubleValue()); + } else { + return Tuple.tuple(aNum.longValue(), bNum.longValue()); + } + } + return Tuple.tuple(a, b); + } +} diff --git a/test/framework/src/test/java/org/opensearch/test/rest/yaml/section/AssertionTests.java b/test/framework/src/test/java/org/opensearch/test/rest/yaml/section/AssertionTests.java index 625b3cde104e2..07d40a7b5a494 100644 --- a/test/framework/src/test/java/org/opensearch/test/rest/yaml/section/AssertionTests.java +++ b/test/framework/src/test/java/org/opensearch/test/rest/yaml/section/AssertionTests.java @@ -67,6 +67,23 @@ public void testParseGreaterThan() throws Exception { assertThat(greaterThanAssertion.getField(), equalTo("field")); assertThat(greaterThanAssertion.getExpectedValue(), instanceOf(Integer.class)); assertThat((Integer) greaterThanAssertion.getExpectedValue(), equalTo(3)); + + // test mixed types + + // long vs int + greaterThanAssertion.doAssert(3000000000L, 2); + + // int vs long + greaterThanAssertion.doAssert(3, 2L); + + // Double vs Float + greaterThanAssertion.doAssert(3.0d, 2.0f); + + // Float vs Double + greaterThanAssertion.doAssert(3.0f, 2.0d); + + // sanity: both as floating-point but different concrete classes + greaterThanAssertion.doAssert(Double.valueOf(3.0), Float.valueOf(2.0f)); } public void testParseLessThan() throws Exception { @@ -77,6 +94,23 @@ public void testParseLessThan() throws Exception { assertThat(lessThanAssertion.getField(), equalTo("field")); assertThat(lessThanAssertion.getExpectedValue(), instanceOf(Integer.class)); assertThat((Integer) lessThanAssertion.getExpectedValue(), equalTo(3)); + + // test mixed types + + // long vs int. Should throw AssertionError instead of ClassCastException + expectThrows(AssertionError.class, "Expected AssertionError", () -> lessThanAssertion.doAssert(3000000000L, 2)); + + // int vs long + lessThanAssertion.doAssert(2, 3L); + + // Double vs Float + lessThanAssertion.doAssert(2.0d, 3.0f); + + // Float vs Double + lessThanAssertion.doAssert(2.0f, 3.0d); + + // Should throw AssertionError instead of ClassCastException + expectThrows(AssertionError.class, "Expected AssertionError", () -> lessThanAssertion.doAssert(3.0f, 2.0d)); } public void testParseLength() throws Exception {