diff --git a/spring-ai-vector-store/src/main/java/org/springframework/ai/vectorstore/filter/FilterExpressionTextParser.java b/spring-ai-vector-store/src/main/java/org/springframework/ai/vectorstore/filter/FilterExpressionTextParser.java index 45d1bb6d70..9d5b793e8e 100644 --- a/spring-ai-vector-store/src/main/java/org/springframework/ai/vectorstore/filter/FilterExpressionTextParser.java +++ b/spring-ai-vector-store/src/main/java/org/springframework/ai/vectorstore/filter/FilterExpressionTextParser.java @@ -109,8 +109,6 @@ public class FilterExpressionTextParser { private static final String WHERE_PREFIX = "WHERE"; - private final DescriptiveErrorListener errorListener; - private final ANTLRErrorStrategy errorHandler; private final Map cache = new ConcurrentHashMap<>(); @@ -120,7 +118,6 @@ public FilterExpressionTextParser() { } public FilterExpressionTextParser(ANTLRErrorStrategy handler) { - this.errorListener = DescriptiveErrorListener.INSTANCE; this.errorHandler = handler; } @@ -141,9 +138,11 @@ public Filter.Expression parse(String textFilterExpression) { var tokens = new CommonTokenStream(lexer); var parser = new FiltersParser(tokens); + // Per-invocation error listener: each parse() call owns its own error state, + // so concurrent calls on the same instance do not share mutable error state. + var errorListener = new DescriptiveErrorListener(); parser.removeErrorListeners(); - this.errorListener.errorMessages.clear(); - parser.addErrorListener(this.errorListener); + parser.addErrorListener(errorListener); if (this.errorHandler != null) { parser.setErrorHandler(this.errorHandler); @@ -157,7 +156,7 @@ public Filter.Expression parse(String textFilterExpression) { return filterExpression; } catch (ParseCancellationException e) { - var msg = String.join("", this.errorListener.errorMessages); + var msg = String.join("", errorListener.errorMessages); var rootCause = NestedExceptionUtils.getRootCause(e); throw new FilterExpressionParseException(msg, rootCause); } @@ -329,8 +328,6 @@ else if (expression instanceof Filter.Expression exp) { public static class DescriptiveErrorListener extends BaseErrorListener { - public static final DescriptiveErrorListener INSTANCE = new DescriptiveErrorListener(); - public final List errorMessages = new CopyOnWriteArrayList<>(); @Override diff --git a/spring-ai-vector-store/src/test/java/org/springframework/ai/vectorstore/filter/FilterExpressionTextParserTests.java b/spring-ai-vector-store/src/test/java/org/springframework/ai/vectorstore/filter/FilterExpressionTextParserTests.java index bf227923a7..8d81eb5ca6 100644 --- a/spring-ai-vector-store/src/test/java/org/springframework/ai/vectorstore/filter/FilterExpressionTextParserTests.java +++ b/spring-ai-vector-store/src/test/java/org/springframework/ai/vectorstore/filter/FilterExpressionTextParserTests.java @@ -16,7 +16,14 @@ package org.springframework.ai.vectorstore.filter; +import java.util.ArrayList; import java.util.List; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Test; @@ -26,6 +33,7 @@ import org.springframework.ai.vectorstore.filter.Filter.Value; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.fail; import static org.springframework.ai.vectorstore.filter.Filter.ExpressionType.AND; import static org.springframework.ai.vectorstore.filter.Filter.ExpressionType.EQ; import static org.springframework.ai.vectorstore.filter.Filter.ExpressionType.GTE; @@ -228,4 +236,135 @@ public void testUnescapedIdentifierWithUnderscores() { assertThat(exp).isEqualTo(new Expression(EQ, new Key("file_name"), new Value("medicaid-wa-faqs.pdf"))); } + /** + * Regression test for #6807: {@code FilterExpressionTextParser} instances must not + * share mutable error state. Each instance owns its own + * {@link DescriptiveErrorListener}, so a parse failure on one instance must not leak + * its error messages into another instance. + */ + @Test + public void testErrorStateIsNotSharedAcrossInstances() { + FilterExpressionTextParser parserA = new FilterExpressionTextParser(); + FilterExpressionTextParser parserB = new FilterExpressionTextParser(); + + // parserB hits a syntax error; the exception must carry a non-empty message. + try { + parserB.parse("country =="); // missing right-hand side -> syntax error + fail("Expected FilterExpressionParseException"); + } + catch (FilterExpressionTextParser.FilterExpressionParseException expected) { + assertThat(expected.getMessage()).isNotEmpty(); + } + + // A fresh failed parse on parserA records its own (non-empty) error, independent + // of B's. With per-invocation listeners the two parses never share state. + try { + parserA.parse("city =="); + fail("Expected FilterExpressionParseException"); + } + catch (FilterExpressionTextParser.FilterExpressionParseException expected) { + assertThat(expected.getMessage()).isNotEmpty(); + } + } + + @Test + public void testParallelParsingDoesNotMixErrorState() throws Exception { + // Concurrent parses across distinct instances must not corrupt each other's error + // state (the listener used to be a shared singleton with a mutable message list). + int threads = 8; + var exceptions = new ConcurrentLinkedQueue(); + ExecutorService executor = Executors.newFixedThreadPool(threads); + try { + var futures = new ArrayList>(); + for (int i = 0; i < threads; i++) { + final int idx = i; + futures.add(executor.submit(() -> { + var p = new FilterExpressionTextParser(); + if (idx % 2 == 0) { + p.parse("k" + idx + " == 'v'"); + // A successful parse must not throw. + } + else { + try { + p.parse("k" + idx + " =="); + exceptions.add("thread " + idx + " should have failed"); + } + catch (FilterExpressionTextParser.FilterExpressionParseException e) { + // Each call owns its own error state; the message must be + // present. + if (e.getMessage() == null || e.getMessage().isBlank()) { + exceptions.add("thread " + idx + " got an empty error message"); + } + } + } + })); + } + for (var f : futures) { + f.get(10, TimeUnit.SECONDS); + } + } + finally { + executor.shutdown(); + } + assertThat(exceptions).isEmpty(); + } + + /** + * Regression test for #6807 (follow-up): per-invocation error state must also isolate + * concurrent {@code parse()} calls on the *same* parser instance. Even though the + * listener is now created inside {@code parse()}, two threads sharing one instance + * must not see each other's error messages. + */ + @Test + public void testConcurrentParseOnSameInstanceDoesNotMixErrorState() throws Exception { + var parser = new FilterExpressionTextParser(); + int threads = 8; + var exceptions = new ConcurrentLinkedQueue(); + // Count how many of the *expected-to-fail* threads (odd idx) actually threw, + // so a regression that silently drops errors under concurrency is caught + // (not just "message was empty"). + var failedCount = new AtomicInteger(0); + ExecutorService executor = Executors.newFixedThreadPool(threads); + try { + var futures = new ArrayList>(); + for (int i = 0; i < threads; i++) { + final int idx = i; + futures.add(executor.submit(() -> { + String expression = (idx % 2 == 0) ? "k" + idx + " == 'v'" : "k" + idx + " =="; + try { + parser.parse(expression); + if (idx % 2 != 0) { + exceptions.add("thread " + idx + " should have failed"); + } + } + catch (FilterExpressionTextParser.FilterExpressionParseException e) { + // Only the failing threads (odd idx) must have recorded an error. + if (idx % 2 == 0) { + exceptions.add("thread " + idx + " unexpectedly failed: " + e.getMessage()); + } + else { + failedCount.incrementAndGet(); + // The real regression guard: a concurrent parse() on the + // shared + // instance must not have cleared or lost this call's error + // message. + if (e.getMessage() == null || e.getMessage().isBlank()) { + exceptions.add("thread " + idx + " got an empty error message under concurrency"); + } + } + } + })); + } + for (var f : futures) { + f.get(10, TimeUnit.SECONDS); + } + } + finally { + executor.shutdown(); + } + // Exactly the 4 odd-indexed threads must have failed (no error lost/merged). + assertThat(failedCount.get()).isEqualTo(4); + assertThat(exceptions).isEmpty(); + } + }