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 @@ -162,7 +162,7 @@ private Optional<FunctionBuilder> resolve(
|| sourceTypes.equals(targetTypes)) {
return Optional.of(funcBuilder);
}
return Optional.of(castArguments(sourceTypes, targetTypes, funcBuilder));
return Optional.of(castArguments(functionName, sourceTypes, targetTypes, funcBuilder));
} else {
return Optional.empty();
}
Expand All @@ -175,8 +175,24 @@ private Optional<FunctionBuilder> resolve(
* this case, wrap F and return equal(BOOL, cast_to_bool(STRING)).
*/
private FunctionBuilder castArguments(
List<ExprType> sourceTypes, List<ExprType> targetTypes, FunctionBuilder funcBuilder) {
FunctionName funcName,
List<ExprType> sourceTypes,
List<ExprType> targetTypes,
FunctionBuilder funcBuilder) {
return (fp, arguments) -> {
// A resolver may return a fixed-arity resolved signature regardless of how many arguments it
// was called with (e.g. a table function called with a duplicate named argument). When more
// arguments are supplied than the resolved signature declares, the cast loop would index past
// the end of the resolved type list and throw an unchecked IndexOutOfBoundsException that
// surfaces as HTTP 500. Reject that overflow with a clean ExpressionEvaluationException.
// Under-arity is left to the resolver's own validation, which produces a more specific
// message; it cannot overflow the resolved type list.
if (arguments.size() > targetTypes.size()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

> and not !=?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is method is very generic. So the changes here will fix all such mismatch argument case for similar functions?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's intended, though the real scope is narrower than it looks. Normal functions never reach this path, since DefaultFunctionResolver rejects an arity mismatch at match() first. It only matters for custom resolvers that hand roll resolve() to return a fixed arity signature without size matching the input, like the table function resolvers (vectorSearch, and the Prometheus ones follow the same shape). So it covers all fixed arity custom resolvers, which is why I put the guard here rather than only in vectorSearch.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, > is on purpose. The crash only happens on overflow, when there are more args than the resolved signature, so the cast loop runs off the end of targetTypes. Fewer args can't overflow, and under arity is already caught upstream anyway: normal functions reject it at the resolver via match() returning NOT_MATCH, and vectorSearch catches it in validateArguments with the specific "requires 4 arguments (table, field, vector, option)" message. So != would just shadow those two better messages with this generic one. One side effect worth noting: over vs under arity now produce slightly different wording, both 400s, so no functional difference.

throw new ExpressionEvaluationException(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be IllegalArgumentException or Syntax exception?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExpressionEvaluationException is the right one here. In JdbcResponseFormatter the status comes out as (SyntaxCheckException || QueryEngineException) ? 400 : 503, and ExpressionEvaluationException extends QueryEngineException, so it maps to 400. IllegalArgumentException is neither, so it would land on the 503 branch and bring back a 5xx for bad input. It's also what the vectorSearch resolver already throws for its other validations, so this keeps it consistent.

String.format(
"Expected %d arguments for function %s but received %d",
targetTypes.size(), funcName.getFunctionName(), arguments.size()));
}
List<Expression> argsCasted = new ArrayList<>();
for (int i = 0; i < arguments.size(); i++) {
Expression arg = arguments.get(i);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,63 @@ void resolve_should_throw_exception_for_unsupported_conversion() {
assertEquals(error.getMessage(), "Type conversion to type STRUCT is not supported");
}

@Test
@DisplayName("resolve should not crash when resolved signature is shorter than the arguments")
void resolve_should_not_index_past_resolved_signature() {
// A custom FunctionResolver (e.g. vectorSearch) may return a fixed-arity resolved signature
// regardless of the number of arguments it was called with. When more arguments are supplied
// than the resolved signature declares (e.g. a duplicate named argument), castArguments must
// not index past the end of the resolved type list. Before the fix this threw an unchecked
// IndexOutOfBoundsException, surfacing as an HTTP 500 instead of the resolver's clean 400.
when(mockFunctionName.getFunctionName()).thenReturn("mock");

FunctionName funcName = mockFunctionName;
FunctionSignature sourceSignature =
new FunctionSignature(funcName, ImmutableList.of(STRING, STRING));
FunctionSignature resolvedSignature = new FunctionSignature(funcName, ImmutableList.of(STRING));

DefaultFunctionResolver funcResolver = mock(DefaultFunctionResolver.class);
FunctionBuilder funcBuilder = mock(FunctionBuilder.class);
when(mockMap.containsKey(eq(funcName))).thenReturn(true);
when(mockMap.get(eq(funcName))).thenReturn(funcResolver);
when(funcResolver.resolve(any())).thenReturn(Pair.of(resolvedSignature, funcBuilder));

ExpressionEvaluationException error =
assertThrows(
ExpressionEvaluationException.class,
() ->
repo.resolve(Collections.emptyList(), sourceSignature)
.apply(functionProperties, ImmutableList.of(mockExpression, mockExpression)));
assertEquals("Expected 1 arguments for function mock but received 2", error.getMessage());
}

@Test
@DisplayName("resolve should defer under-arity to the builder rather than the cast guard")
void resolve_should_not_intercept_under_arity() {
// When fewer arguments are supplied than the resolved signature declares, the cast loop cannot
// index past the resolved type list, so there is no crash to guard against. The overflow guard
// must not fire here; the call must reach the resolved builder so a resolver's own (more
// specific) arity validation still runs.
FunctionName funcName = mockFunctionName;
FunctionSignature sourceSignature = new FunctionSignature(funcName, ImmutableList.of(STRING));
FunctionSignature resolvedSignature =
new FunctionSignature(funcName, ImmutableList.of(STRING, STRING));

DefaultFunctionResolver funcResolver = mock(DefaultFunctionResolver.class);
FunctionBuilder funcBuilder = mock(FunctionBuilder.class);
when(mockMap.containsKey(eq(funcName))).thenReturn(true);
when(mockMap.get(eq(funcName))).thenReturn(funcResolver);
when(funcResolver.resolve(any())).thenReturn(Pair.of(resolvedSignature, funcBuilder));
when(funcBuilder.apply(eq(functionProperties), any()))
.thenReturn(new FakeFunctionExpression(funcName, ImmutableList.of(mockExpression)));

repo.resolve(Collections.emptyList(), sourceSignature)
.apply(functionProperties, ImmutableList.of(mockExpression));

// The builder is reached (overflow guard did not short-circuit under-arity).
verify(funcBuilder).apply(eq(functionProperties), any());
}

@Test
@DisplayName("resolve unregistered function should throw exception")
void resolve_unregistered() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,26 @@ public void testEmptyVectorRejects() throws IOException {
assertThat(ex.getMessage(), containsString("must not be empty"));
}

@Test
public void testDuplicateNamedArgumentRejectsWith400() throws IOException {
// A duplicate named argument on top of all four distinct names sends five arguments to the
// resolver, exceeding its fixed four-arg signature. This previously crashed with an unchecked
// IndexOutOfBoundsException surfaced as HTTP 500; it must now be a clean 400 with a
// user-facing message.
ResponseException ex =
expectThrows(
ResponseException.class,
() ->
executeQuery(
"SELECT v._id FROM vectorSearch(table='t', table='t', field='f', "
+ "vector='[1.0]', option='k=5') AS v"));

assertEquals(400, ex.getResponse().getStatusLine().getStatusCode());
// The five-argument case is caught by the repository arity guard before the resolver's
// duplicate-name check, so the message reports the argument-count mismatch.
assertThat(ex.getMessage(), containsString("Expected 4 arguments"));
}

@Test
public void testInvalidFieldNameRejects() throws IOException {
ResponseException ex =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.opensearch.sql.exception.ExpressionEvaluationException;
import org.opensearch.sql.expression.DSL;
import org.opensearch.sql.expression.Expression;
import org.opensearch.sql.expression.function.BuiltinFunctionRepository;
import org.opensearch.sql.expression.function.FunctionBuilder;
import org.opensearch.sql.expression.function.FunctionName;
import org.opensearch.sql.expression.function.FunctionProperties;
Expand Down Expand Up @@ -182,6 +183,64 @@ void resolve_rejectsDuplicateNamedArgument() {
assertTrue(ex.getMessage().contains("table"));
}

@Test
void resolverArityGuardRejectsExtraNamedArgument() {
// The resolver's own builder runs validateArguments() first, so applying it directly to a
// duplicate named argument plus all four required arguments (five total) throws a clean
// ExpressionEvaluationException. This covers the resolver's arity guard only; the
// repository-level cast path that originally crashed is covered by
// resolveThroughRepositoryRejectsExtraNamedArgument and BuiltinFunctionRepositoryTest.
VectorSearchTableFunctionResolver resolver =
new VectorSearchTableFunctionResolver(client, settings);
FunctionName functionName = FunctionName.of("vectorsearch");
List<Expression> expressions =
List.of(
DSL.namedArgument("table", DSL.literal("a")),
DSL.namedArgument("table", DSL.literal("b")),
DSL.namedArgument("field", DSL.literal("embedding")),
DSL.namedArgument("vector", DSL.literal("[1.0]")),
DSL.namedArgument("option", DSL.literal("k=5")));
FunctionSignature functionSignature =
new FunctionSignature(
functionName, expressions.stream().map(Expression::type).collect(Collectors.toList()));
FunctionBuilder builder = resolver.resolve(functionSignature).getValue();

ExpressionEvaluationException ex =
assertThrows(
ExpressionEvaluationException.class,
() -> builder.apply(functionProperties, expressions));
assertTrue(ex.getMessage().contains("requires 4 arguments"));
}

@Test
void resolveThroughRepositoryRejectsExtraNamedArgument() {
// Routes the vectorSearch resolver through BuiltinFunctionRepository the way production does
// (storage-engine resolver registered as a datasource function). A duplicate named argument
// plus all four required arguments gives five arguments against the resolver's fixed four-arg
// signature, which exercises the cast-wrapper path that originally crashed with an unchecked
// IndexOutOfBoundsException (surfacing as HTTP 500). It must now fail with a clean
// ExpressionEvaluationException instead.
VectorSearchTableFunctionResolver resolver =
new VectorSearchTableFunctionResolver(client, settings);
BuiltinFunctionRepository repository = BuiltinFunctionRepository.getInstance();
List<Expression> expressions =
List.of(
DSL.namedArgument("table", DSL.literal("a")),
DSL.namedArgument("table", DSL.literal("b")),
DSL.namedArgument("field", DSL.literal("embedding")),
DSL.namedArgument("vector", DSL.literal("[1.0]")),
DSL.namedArgument("option", DSL.literal("k=5")));

assertThrows(
ExpressionEvaluationException.class,
() ->
repository.compile(
functionProperties,
List.of(resolver),
FunctionName.of("vectorsearch"),
expressions));
}

@Test
void resolve_rejectsUnknownArgumentName() {
VectorSearchTableFunctionResolver resolver =
Expand Down
Loading