Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<Integer> {

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,
Expand Down Expand Up @@ -220,9 +252,9 @@ public static class HybridLimitsDeserializer extends StdDeserializer<HybridLimit
// {"options": {"hybridLimits" : 100}}
// or, both fields are required
// {"options": {"hybridLimits" : {"$vector" : 100, "$lexical" : 10}}
private static final JsonFieldMatcher<NumericNode> MATCH_LIMIT_FIELDS =
private static final JsonFieldMatcher<JsonNode> 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);
Expand All @@ -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));
}
Expand All @@ -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));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Arguments> 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);
}
}
Original file line number Diff line number Diff line change
@@ -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<Arguments> 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"));
}
}
Loading