diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/FindAndRerankCommand.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/FindAndRerankCommand.java index d5a8887fc6..a86ff5f9f5 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/FindAndRerankCommand.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/FindAndRerankCommand.java @@ -69,6 +69,7 @@ public void addCommandFeatures(CommandFeatures commandFeatures) { public record Options( @Positive(message = "limit should be greater than `0`") + @JsonDeserialize(using = LimitDeserializer.class) @Schema( description = "The maximum number of documents to return after reranking.", type = SchemaType.INTEGER, @@ -150,6 +151,37 @@ public static class RerankOverrideSchema { public static final String REF = SCHEMA_REF_PREFIX + NAME; } + /** Deserializer for the outer limit, without Jackson's numeric coercions. */ + public static class LimitDeserializer extends StdDeserializer { + + protected LimitDeserializer() { + super(Integer.class); + } + + @Override + public Integer deserialize(JsonParser jsonParser, DeserializationContext deserializationContext) + throws IOException { + return integerLimit(jsonParser, deserializationContext.readTree(jsonParser), "options.limit"); + } + } + + // Validate the JSON integer before converting it. Positivity and configured hybrid bounds + // are checked later by Bean Validation and FindAndRerankPlanner, respectively. + private static int integerLimit(JsonParser jsonParser, JsonNode value, String path) + throws JsonMappingException { + if (!value.isIntegralNumber() || !value.canConvertToInt()) { + String valueDescription = + value.isFloatingPointNumber() + ? "%s (floating-point number)".formatted(value) + : value.toString(); + throw new JsonMappingException( + jsonParser, + "%s must be an integer between %d and %d (inclusive), got %s" + .formatted(path, Integer.MIN_VALUE, Integer.MAX_VALUE, valueDescription)); + } + return value.intValue(); + } + @JsonDeserialize(using = HybridLimitsDeserializer.class) @Schema( name = HybridLimits.NAME, @@ -220,9 +252,9 @@ public static class HybridLimitsDeserializer extends StdDeserializer MATCH_LIMIT_FIELDS = + private static final JsonFieldMatcher MATCH_LIMIT_FIELDS = new JsonFieldMatcher<>( - NumericNode.class, List.of(LEXICAL_CONTENT_FIELD, VECTOR_EMBEDDING_FIELD), List.of()); + JsonNode.class, List.of(LEXICAL_CONTENT_FIELD, VECTOR_EMBEDDING_FIELD), List.of()); protected HybridLimitsDeserializer() { super(HybridLimits.class); @@ -238,15 +270,16 @@ public HybridLimits deserialize( case JsonNode node -> throw new JsonMappingException( jsonParser, - "hybridLimits must be an integer or an object, got %s" + "options.hybridLimits must be an integer or an object, got %s" .formatted(node.getNodeType())); }; } // Range validation against dynamic OperationsConfig bounds happens later in - // FindAndRerankPlanner.plan(); the deserializer only handles JSON shape. - private HybridLimits deserialize(JsonParser jsonParser, NumericNode limitsNumber) { - var limit = limitsNumber.asInt(); + // FindAndRerankPlanner.plan(); only integer representation is checked here. + private HybridLimits deserialize(JsonParser jsonParser, NumericNode limitsNumber) + throws JsonMappingException { + var limit = integerLimit(jsonParser, limitsNumber, "options.hybridLimits"); return new HybridLimits( limit, limit, CommandFeatures.of(CommandFeature.HYBRID_LIMITS_NUMBER)); } @@ -257,8 +290,14 @@ private HybridLimits deserialize(JsonParser jsonParser, ObjectNode limitsObject) var limitMatch = MATCH_LIMIT_FIELDS.matchAndThrow(limitsObject, jsonParser, ERROR_CONTEXT); return new HybridLimits( - limitMatch.matched().get(VECTOR_EMBEDDING_FIELD).asInt(), - limitMatch.matched().get(LEXICAL_CONTENT_FIELD).asInt(), + integerLimit( + jsonParser, + limitMatch.matched().get(VECTOR_EMBEDDING_FIELD), + "options.hybridLimits." + VECTOR_EMBEDDING_FIELD), + integerLimit( + jsonParser, + limitMatch.matched().get(LEXICAL_CONTENT_FIELD), + "options.hybridLimits." + LEXICAL_CONTENT_FIELD), CommandFeatures.of( CommandFeature.HYBRID_LIMITS_VECTOR, CommandFeature.HYBRID_LIMITS_LEXICAL)); } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/FindAndRerankCommandLimitsTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/FindAndRerankCommandLimitsTest.java new file mode 100644 index 0000000000..b1570913d4 --- /dev/null +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/model/command/impl/FindAndRerankCommandLimitsTest.java @@ -0,0 +1,114 @@ +package io.stargate.sgv2.jsonapi.api.model.command.impl; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; + +/** Tests that reranking limits retain integer values without truncation or overflow. */ +class FindAndRerankCommandLimitsTest { + + private final ObjectMapper objectMapper = new ObjectMapper(); + + @ParameterizedTest + @MethodSource("invalidNumericLimits") + void rejectsInvalidNumericLimits(String options, String path, String value) { + var error = + assertThrows( + JsonMappingException.class, + () -> objectMapper.readValue(commandWithOptions(options), FindAndRerankCommand.class)); + + assertThat(error) + .hasMessageContaining(path) + .hasMessageContaining("must be an integer") + .hasMessageContaining(value); + } + + private static Stream invalidNumericLimits() { + return Stream.of( + "0.5", + "5.9", + "50.9", + "5.0", + "3000000000", + "-3000000000", + "4294967306", + "-4294967286", + "18446744073709551626") + .flatMap( + value -> + Stream.of( + Arguments.of("{\"limit\": %s}".formatted(value), "options.limit", value), + Arguments.of( + "{\"hybridLimits\": %s}".formatted(value), "options.hybridLimits", value), + Arguments.of( + "{\"hybridLimits\": {\"$vector\": %s, \"$lexical\": 10}}".formatted(value), + "options.hybridLimits.$vector", + value), + Arguments.of( + "{\"hybridLimits\": {\"$vector\": 10, \"$lexical\": %s}}".formatted(value), + "options.hybridLimits.$lexical", + value))); + } + + @ParameterizedTest + @ValueSource(strings = {"5e0", "\"5\"", "true", "[]"}) + void rejectsNonIntegerValues(String value) { + for (String options : + new String[] { + "{\"limit\": %s}".formatted(value), + "{\"hybridLimits\": %s}".formatted(value), + "{\"hybridLimits\": {\"$vector\": %s, \"$lexical\": 10}}".formatted(value), + "{\"hybridLimits\": {\"$vector\": 10, \"$lexical\": %s}}".formatted(value) + }) { + var error = + assertThrows( + JsonMappingException.class, + () -> + objectMapper.readValue(commandWithOptions(options), FindAndRerankCommand.class)); + + assertThat(error).hasMessageContaining("must be an integer"); + } + } + + @ParameterizedTest + @ValueSource(ints = {Integer.MIN_VALUE, -1, 0, 1, 50, Integer.MAX_VALUE}) + void preservesIntegerValuesForLaterRangeValidation(int value) throws Exception { + var command = + objectMapper.readValue( + commandWithOptions("{\"limit\": %d, \"hybridLimits\": %d}".formatted(value, value)), + FindAndRerankCommand.class); + + assertThat(command.options().limit()).isEqualTo(value); + assertThat(command.options().hybridLimits().vectorLimit()).isEqualTo(value); + assertThat(command.options().hybridLimits().lexicalLimit()).isEqualTo(value); + + command = + objectMapper.readValue( + commandWithOptions( + "{\"hybridLimits\": {\"$vector\": %d, \"$lexical\": %d}}".formatted(value, value)), + FindAndRerankCommand.class); + + assertThat(command.options().hybridLimits().vectorLimit()).isEqualTo(value); + assertThat(command.options().hybridLimits().lexicalLimit()).isEqualTo(value); + } + + @ParameterizedTest + @ValueSource(strings = {"{}", "{\"limit\": null, \"hybridLimits\": null}"}) + void preservesDefaultLimits(String options) throws Exception { + var command = objectMapper.readValue(commandWithOptions(options), FindAndRerankCommand.class); + + assertThat(command.options().limit()).isNull(); + assertThat(command.options().hybridLimits()).isNull(); + } + + private static String commandWithOptions(String options) { + return "{\"findAndRerank\": {\"options\": %s}}".formatted(options); + } +} diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindAndRerankLimitsTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindAndRerankLimitsTest.java new file mode 100644 index 0000000000..8a11c2e49a --- /dev/null +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindAndRerankLimitsTest.java @@ -0,0 +1,115 @@ +package io.stargate.sgv2.jsonapi.api.v1; + +import static io.restassured.RestAssured.given; +import static io.stargate.sgv2.jsonapi.api.v1.ResponseAssertions.responseIsError; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; + +import io.quarkus.test.junit.QuarkusTest; +import io.quarkus.test.junit.TestProfile; +import io.restassured.http.ContentType; +import io.stargate.sgv2.jsonapi.config.constants.HttpConstants; +import io.stargate.sgv2.jsonapi.exception.RequestException; +import io.stargate.sgv2.jsonapi.testresource.NoGlobalResourcesTestProfile; +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; + +/** Tests limit validation and HTTP error mapping without a database or reranking provider. */ +@QuarkusTest +@TestProfile(NoGlobalResourcesTestProfile.Impl.class) +class FindAndRerankLimitsTest { + + @ParameterizedTest + @MethodSource("invalidLimits") + void invalidLimitReturnsRequestError(String options, String path) { + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, "test-token") + .contentType(ContentType.JSON) + .body( + """ + {"findAndRerank": { + "sort": {"$hybrid": "abc"}, + "options": { + "rerankQuery": "I like cheese!", "rerankOn": "content", %s + } + }} + """ + .formatted(options)) + .when() + .post(CollectionResource.BASE_PATH, "test_keyspace", "test_collection") + .then() + .statusCode(200) + .body("$", responseIsError()) + .body("errors.size()", is(1)) + .body("errors[0].family", is("REQUEST")) + .body("errors[0].errorCode", is(RequestException.Code.REQUEST_STRUCTURE_MISMATCH.name())) + .body("errors[0].message", containsString(path)) + .body("errors[0].message", containsString("must be an integer")); + } + + private static Stream invalidLimits() { + return Stream.of( + "0.5", + "5.9", + "50.9", + "5.0", + "5e0", + "\"5\"", + "3000000000", + "-3000000000", + "4294967306", + "-4294967286", + "18446744073709551626") + .flatMap( + value -> + Stream.of( + Arguments.of( + "\"limit\": %s, \"hybridLimits\": 10".formatted(value), "options.limit"), + Arguments.of( + "\"limit\": 10, \"hybridLimits\": %s".formatted(value), + "options.hybridLimits"), + Arguments.of( + "\"hybridLimits\": {\"$vector\": %s, \"$lexical\": 10}".formatted(value), + "options.hybridLimits.$vector"), + Arguments.of( + "\"hybridLimits\": {\"$vector\": 10, \"$lexical\": %s}".formatted(value), + "options.hybridLimits.$lexical"))); + } + + @ParameterizedTest + @ValueSource(ints = {0, -1}) + void integerOuterLimitStillRequiresPositiveValue(int limit) { + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, "test-token") + .contentType(ContentType.JSON) + .body("{\"findAndRerank\": {\"options\": {\"limit\": %d}}}".formatted(limit)) + .when() + .post(CollectionResource.BASE_PATH, "test_keyspace", "test_collection") + .then() + .statusCode(200) + .body("$", responseIsError()) + .body("errors[0].family", is("REQUEST")) + .body("errors[0].errorCode", is(RequestException.Code.COMMAND_FIELD_VALUE_INVALID.name())) + .body("errors[0].message", containsString("limit should be greater than `0`")); + } + + @ParameterizedTest + @ValueSource(strings = {"5.0", "5e0"}) + void explainsRejectionOfWholeValuedFloatingPointNumbers(String value) { + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, "test-token") + .contentType(ContentType.JSON) + .body("{\"findAndRerank\": {\"options\": {\"limit\": %s}}}".formatted(value)) + .when() + .post(CollectionResource.BASE_PATH, "test_keyspace", "test_collection") + .then() + .statusCode(200) + .body("$", responseIsError()) + .body("errors[0].family", is("REQUEST")) + .body("errors[0].message", containsString("options.limit")) + .body("errors[0].message", containsString("floating-point number")); + } +}