diff --git a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java index 79ea58b8608..1e19050f6e3 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java @@ -162,7 +162,7 @@ private Optional 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(); } @@ -175,8 +175,24 @@ private Optional resolve( * this case, wrap F and return equal(BOOL, cast_to_bool(STRING)). */ private FunctionBuilder castArguments( - List sourceTypes, List targetTypes, FunctionBuilder funcBuilder) { + FunctionName funcName, + List sourceTypes, + List 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()) { + throw new ExpressionEvaluationException( + String.format( + "Expected %d arguments for function %s but received %d", + targetTypes.size(), funcName.getFunctionName(), arguments.size())); + } List argsCasted = new ArrayList<>(); for (int i = 0; i < arguments.size(); i++) { Expression arg = arguments.get(i); diff --git a/core/src/test/java/org/opensearch/sql/expression/function/BuiltinFunctionRepositoryTest.java b/core/src/test/java/org/opensearch/sql/expression/function/BuiltinFunctionRepositoryTest.java index 237477050dc..7d8f2d2ac61 100644 --- a/core/src/test/java/org/opensearch/sql/expression/function/BuiltinFunctionRepositoryTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/function/BuiltinFunctionRepositoryTest.java @@ -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() { diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/VectorSearchIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/VectorSearchIT.java index 8ae3167b40b..4e4295ec440 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/VectorSearchIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/VectorSearchIT.java @@ -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 = diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionResolverTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionResolverTest.java index c6fece7bf32..afa4e9e725e 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionResolverTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/VectorSearchTableFunctionResolverTest.java @@ -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; @@ -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 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 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 =