From d8245668a0b1c477307189ad47f33bb9bfdd0686 Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Fri, 11 Sep 2026 14:49:33 -0400 Subject: [PATCH 1/4] fix(search): tolerate blank optional arguments and return safe errors Signed-off-by: Aditya Parikh Co-authored-by: Junie --- .../solr/mcp/server/search/SearchService.java | 105 +++++++++----- .../server/McpClientStdioIntegrationTest.java | 34 +++++ .../search/SearchServiceIntegrationTest.java | 64 ++++++++- .../mcp/server/search/SearchServiceTest.java | 129 +++++++++++++++++- 4 files changed, 289 insertions(+), 43 deletions(-) diff --git a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java index ac1b2f37..37ef3928 100644 --- a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java +++ b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java @@ -142,6 +142,12 @@ public class SearchService { /** Remediation hint naming the {@code list-collections} tool. */ static final String LIST_COLLECTIONS_HINT = ". Hint: call list-collections to see available collections."; + private static final String CONNECTION_ERROR = "Search failed: unable to communicate with Solr. Retry later;" + + " if it persists, ask the operator to check Solr availability and connection settings."; + private static final String RESPONSE_ERROR = "Search failed: unable to process the Solr response." + + " If facetFields were supplied, call get-schema to check those fields or retry without facetFields;" + + " if it persists, ask the operator to check server logs and Solr compatibility."; + private final SolrClient solrClient; /** @@ -192,12 +198,13 @@ private static Map> getFacets(QueryResponse queryRespo * The Solr query string (q parameter). Defaults to "*:*" if not * specified * @param filterQueries - * List of filter queries (fq parameter) + * List of filter queries (fq parameter); blank entries are ignored * @param facetFields - * List of fields to facet on + * List of fields to facet on; blank entries are ignored * @param sortClauses * List of sort clauses for ordering results; each names a field and - * an optional {@code asc}/{@code desc} order (default {@code asc}) + * an optional {@code asc}/{@code desc} order (default {@code asc}); + * entries with blank fields are ignored * @param start * Starting offset for pagination * @param rows @@ -250,12 +257,14 @@ public SearchResponse search(@McpToolParam(description = "Solr collection to que + " {!edismax qf='name author'}. If none specified defaults to \"*:*\"", required = false) @Nullable String query, @McpToolParam( - description = "Solr fq parameter: list of filter queries, one filter per entry", + description = "Solr fq parameter: list of filter queries, one filter per entry. Blank entries are ignored", required = false) @Nullable List filterQueries, - @McpToolParam(description = "Solr facet fields", required = false) @Nullable List facetFields, + @McpToolParam( + description = "Solr facet fields. Blank entries are ignored", + required = false) @Nullable List facetFields, @McpToolParam( description = "Sort clauses applied in order. Each has 'field' (field name to sort" - + " on) and 'order' ('asc' or 'desc', default 'asc')", + + " on) and 'order' ('asc' or 'desc', default 'asc'). Entries with blank fields are ignored", required = false) @Nullable List sortClauses, @McpToolParam(description = "Starting offset for pagination", required = false) @Nullable Integer start, @McpToolParam(description = "Number of rows to return", required = false) @Nullable Integer rows) @@ -272,20 +281,22 @@ public SearchResponse search(@McpToolParam(description = "Solr collection to que // filter queries if (!CollectionUtils.isEmpty(filterQueries)) { - solrQuery.setFilterQueries(filterQueries.toArray(new String[0])); + filterQueries.stream().filter(StringUtils::hasText).forEach(solrQuery::addFilterQuery); } // facets if (!CollectionUtils.isEmpty(facetFields)) { - solrQuery.setFacet(true); - solrQuery.addFacetField(facetFields.toArray(new String[0])); + facetFields.stream().filter(StringUtils::hasText).forEach(solrQuery::addFacetField); + } + if (solrQuery.getFacetFields() != null) { solrQuery.setFacetMinCount(1); solrQuery.setFacetSort(FacetParams.FACET_SORT_COUNT); } // sorting if (!CollectionUtils.isEmpty(sortClauses)) { - solrQuery.setSorts(sortClauses.stream().map(SortClause::toSolrSortClause).toList()); + sortClauses.stream().filter(clause -> clause != null && StringUtils.hasText(clause.field())) + .map(SortClause::toSolrSortClause).forEach(solrQuery::addSort); } // pagination @@ -297,43 +308,50 @@ public SearchResponse search(@McpToolParam(description = "Solr collection to que solrQuery.setRows(rows); } - final QueryResponse queryResponse; try { - queryResponse = solrClient.query(collection, solrQuery); - } catch (SolrException e) { - throw withRemediationHint(e, collection); - } + final QueryResponse queryResponse = solrClient.query(collection, solrQuery); - // Add documents - final SolrDocumentList documents = queryResponse.getResults(); + // Add documents + final SolrDocumentList documents = queryResponse.getResults(); - // Convert SolrDocuments to Maps - // SolrDocument is a Map in Solr's field order, so no per-document copy - final List> docs = Collections.unmodifiableList(documents); + // Convert SolrDocuments to Maps + // SolrDocument is a Map in Solr's field order, so no per-document copy + final List> docs = Collections.unmodifiableList(documents); - // Add facets if present - final var facets = getFacets(queryResponse); + // Add facets if present + final var facets = getFacets(queryResponse); - return new SearchResponse(documents.getNumFound(), documents.getStart(), documents.getMaxScore(), docs, facets); + return new SearchResponse(documents.getNumFound(), documents.getStart(), documents.getMaxScore(), docs, + facets); + } catch (SolrException e) { + throw withRemediationHint(e, collection); + } catch (SolrServerException e) { + logger.debug("Solr query failed on collection {}", collection, e); + throw new SolrServerException(CONNECTION_ERROR); + } catch (IOException e) { + logger.debug("Solr query failed on collection {}", collection, e); + throw new IOException(CONNECTION_ERROR); + } catch (RuntimeException e) { + logger.debug("Solr query response failed on collection {}", collection, e); + throw new IllegalStateException(RESPONSE_ERROR); + } } /** - * Wraps common Solr query failures with a next-step hint. MCP clients receive - * the exception message as the tool error, so naming the follow-up tool lets - * them self-correct instead of retrying blind. + * Wraps common Solr query failures with a next-step hint. MCP unwraps exception + * causes, so client-facing exceptions must omit the cause to preserve safe + * guidance; the original failure is logged for server-side diagnostics. * * @param e * the Solr exception raised by the query * @param collection * the collection that was queried - * @return an exception carrying the original message plus a remediation hint, - * or the original exception when no hint applies + * @return an exception carrying safe guidance without a cause */ private static RuntimeException withRemediationHint(SolrException e, String collection) { final String message = String.valueOf(e.getMessage()); - // The MCP client only ever sees the exception message, so without this the - // server keeps no record of a failed query. + // Keep diagnostics in server logs, not in the client-facing exception chain. logger.debug("Solr query failed on collection {}", collection, e); // An unknown collection is a 404 whose body is Solr's HTML "not found" page, @@ -341,19 +359,34 @@ private static RuntimeException withRemediationHint(SolrException e, String coll // The status code is the only signal that survives; match it rather than the // message text, which mentions neither the collection nor "404". if (e.code() == SolrException.ErrorCode.NOT_FOUND.code) { - return new IllegalArgumentException(message + LIST_COLLECTIONS_HINT, e); + return new IllegalArgumentException("Search failed: the collection was not found" + LIST_COLLECTIONS_HINT); } // Everything below is a 400 carrying a generic SolrException, indistinguishable // except by Solr's message text. final String lower = message.toLowerCase(Locale.ROOT); - if (lower.contains(UNDEFINED_FIELD_TOKEN) || lower.contains(SORT_FIELD_NOT_FOUND_TOKEN)) { - return new IllegalArgumentException(message + GET_SCHEMA_HINT_FORMAT.formatted(collection), e); + if (e.code() == SolrException.ErrorCode.BAD_REQUEST.code) { + if (lower.contains(UNDEFINED_FIELD_TOKEN) || lower.contains(SORT_FIELD_NOT_FOUND_TOKEN)) { + return new IllegalArgumentException("Search failed: a referenced field is not defined" + + GET_SCHEMA_HINT_FORMAT.formatted(collection)); + } + if (lower.contains(SYNTAX_ERROR_TOKEN) || lower.contains(CANNOT_PARSE_TOKEN)) { + return new IllegalArgumentException( + "Search failed: Solr could not parse the query or filters" + LUCENE_SYNTAX_HINT); + } + return new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Search failed: Solr rejected the request. Check query syntax, filter queries, facet fields," + + " sort clauses, and pagination; call get-schema to verify the fields."); } - if (lower.contains(SYNTAX_ERROR_TOKEN) || lower.contains(CANNOT_PARSE_TOKEN)) { - return new IllegalArgumentException(message + LUCENE_SYNTAX_HINT, e); + if (e.code() == SolrException.ErrorCode.UNAUTHORIZED.code + || e.code() == SolrException.ErrorCode.FORBIDDEN.code) { + return new SolrException(SolrException.ErrorCode.getErrorCode(e.code()), + "Search failed: Solr denied access. Ask the operator to check the configured Solr credentials" + + " and collection permissions."); } - return e; + return new SolrException(SolrException.ErrorCode.getErrorCode(e.code()), + "Search failed: Solr could not complete the request. Retry later; if it persists," + + " ask the operator to check Solr health and server logs."); } /** diff --git a/src/test/java/org/apache/solr/mcp/server/McpClientStdioIntegrationTest.java b/src/test/java/org/apache/solr/mcp/server/McpClientStdioIntegrationTest.java index c6120a72..c42d070b 100644 --- a/src/test/java/org/apache/solr/mcp/server/McpClientStdioIntegrationTest.java +++ b/src/test/java/org/apache/solr/mcp/server/McpClientStdioIntegrationTest.java @@ -16,13 +16,21 @@ */ package org.apache.solr.mcp.server; +import static org.junit.jupiter.api.Assertions.*; + +import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; import io.modelcontextprotocol.client.McpClient; import io.modelcontextprotocol.client.McpSyncClient; import io.modelcontextprotocol.client.transport.ServerParameters; import io.modelcontextprotocol.client.transport.StdioClientTransport; import io.modelcontextprotocol.json.jackson.JacksonMcpJsonMapper; +import io.modelcontextprotocol.spec.McpSchema.CallToolRequest; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; import org.testcontainers.containers.SolrContainer; import org.testcontainers.junit.jupiter.Container; import org.testcontainers.junit.jupiter.Testcontainers; @@ -53,4 +61,30 @@ protected McpSyncClient createClient() { return McpClient.sync(transport).build(); } + @Test + @Order(39) + void blankOptionalSearchEntriesAreIgnoredThroughMcp() throws Exception { + var result = mcpClient.callTool(new CallToolRequest("search", + Map.of("collection", SHOWS_COLLECTION, "query", "*:*", "rows", 0, "filterQueries", List.of(" "), + "facetFields", List.of("", "platform"), "sortClauses", + List.of(Map.of("field", "", "order", ""))))); + assertNotError(result); + Map response = OBJECT_MAPPER.readValue(extractText(result), new TypeReference<>() { + }); + assertEquals(SHOWS_DOC_COUNT, getNumFound(response)); + Map facets = (Map) response.get("facets"); + Map platforms = (Map) facets.get("platform"); + assertEquals(20, ((Number) platforms.get("Netflix")).intValue()); + } + + @Test + @Order(40) + void searchFailureIsAnActionableMcpToolError() { + var result = mcpClient.callTool(new CallToolRequest("search", + Map.of("collection", SHOWS_COLLECTION, "facetFields", List.of("nonexistent_field_xyz")))); + assertEquals(Boolean.TRUE, result.isError()); + assertTrue(extractText(result).contains("get-schema"), extractText(result)); + assertFalse(extractText(result).contains("Exception"), extractText(result)); + } + } diff --git a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java index 55c1aabf..0dc84a45 100644 --- a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java +++ b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java @@ -20,13 +20,18 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.OptionalDouble; import org.apache.solr.client.solrj.SolrClient; +import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.GenericSolrRequest; import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.util.NamedList; import org.apache.solr.mcp.server.TestDocuments; import org.apache.solr.mcp.server.TestcontainersConfiguration; import org.apache.solr.mcp.server.indexing.IndexingService; @@ -70,6 +75,8 @@ void setUp() throws Exception { [ { "id": "book001", + "platform": ["netflix"], + "platform_ss": ["netflix"], "name": ["A Game of Thrones"], "author_ss": ["George R.R. Martin"], "price": [7.99], @@ -80,6 +87,8 @@ void setUp() throws Exception { }, { "id": "book002", + "platform": ["netflix"], + "platform_ss": ["netflix"], "name": ["A Clash of Kings"], "author_ss": ["George R.R. Martin"], "price": [8.99], @@ -90,6 +99,8 @@ void setUp() throws Exception { }, { "id": "book003", + "platform": ["hulu"], + "platform_ss": ["hulu"], "name": ["A Storm of Swords"], "author_ss": ["George R.R. Martin"], "price": [9.99], @@ -211,6 +222,41 @@ void facetingAQueryThatMatchesNothingReturnsEmptyFacets() throws SolrServerExcep () -> "expected no facet buckets, got: " + result.facets().get("genre_s")); } + @Test + void searchWithSchemalessPlatformFacetsReturnsCounts() throws Exception { + // Solr 9's _default infers text_general without docValues or uninversion, + // so its default faceting returns [] for platform despite matching documents. + // Preserve Solr's response (also across Solr versions), not a different facet + // algorithm. + NamedList raw = solrClient.request( + new GenericSolrRequest(SolrRequest.METHOD.GET, "/" + COLLECTION_NAME + "/select", + new ModifiableSolrParams().set("q", "id:book*").set("rows", 0).set("facet", true) + .set("facet.field", "platform").set("facet.mincount", 1).set("facet.sort", "count")), + COLLECTION_NAME); + NamedList facetCounts = assertInstanceOf(NamedList.class, raw.get("facet_counts")); + NamedList facetFields = assertInstanceOf(NamedList.class, facetCounts.get("facet_fields")); + NamedList platform = assertInstanceOf(NamedList.class, facetFields.get("platform")); + Map expectedPlatform = new HashMap<>(); + platform.forEach((term, count) -> expectedPlatform.put(term, ((Number) count).longValue())); + SearchResponse document = searchService.search(COLLECTION_NAME, "id:book001", null, null, null, null, null); + assertEquals(List.of("netflix"), document.documents().getFirst().get("platform")); + SearchResponse result = searchService.search(COLLECTION_NAME, "id:book*", null, + List.of("platform", "platform_ss", "genre_s"), null, null, 0); + assertEquals(10, result.numFound()); + assertTrue(result.documents().isEmpty()); + assertEquals(Map.of("platform", expectedPlatform, "platform_ss", Map.of("netflix", 2L, "hulu", 1L), "genre_s", + Map.of("fantasy", 7L, "scifi", 3L)), result.facets()); + } + + @Test + void searchWithBlankOptionsPreservesFiltersFacetsAndSort() throws Exception { + SearchResponse result = searchService.search(COLLECTION_NAME, " \t", List.of("", "platform:netflix", " "), + List.of(" ", "platform_ss", ""), List.of(new SortClause("", ""), new SortClause("id", "desc")), 0, 10); + assertEquals(2, result.numFound()); + assertEquals(List.of("book002", "book001"), getDocumentIds(result.documents())); + assertEquals(Map.of("netflix", 2L), result.facets().get("platform_ss")); + } + /** * Remediation hints classify Solr's error text, which this server cannot see at * compile time — the strings are produced by solr-core, and only solr-solrj is @@ -229,6 +275,7 @@ void searchWithUndefinedFieldInQueryReturnsGetSchemaHint() { .search(COLLECTION_NAME, "definitely_not_a_field:value", null, null, null, null, null)); assertTrue(e.getMessage().contains(SearchService.GET_SCHEMA_HINT_FORMAT.formatted(COLLECTION_NAME)), () -> "expected get-schema hint, got: " + e.getMessage()); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } @Test @@ -237,6 +284,7 @@ void searchWithUndefinedFieldInFilterQueryReturnsGetSchemaHint() { .search(COLLECTION_NAME, "*:*", List.of("definitely_not_a_field:value"), null, null, null, null)); assertTrue(e.getMessage().contains(SearchService.GET_SCHEMA_HINT_FORMAT.formatted(COLLECTION_NAME)), () -> "expected get-schema hint, got: " + e.getMessage()); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } /** Faceting words it differently: {@code undefined field: "name"}. */ @@ -246,6 +294,7 @@ void searchWithUndefinedFacetFieldReturnsGetSchemaHint() { .search(COLLECTION_NAME, "*:*", null, List.of("definitely_not_a_field"), null, null, null)); assertTrue(e.getMessage().contains(SearchService.GET_SCHEMA_HINT_FORMAT.formatted(COLLECTION_NAME)), () -> "expected get-schema hint, got: " + e.getMessage()); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } /** @@ -259,6 +308,7 @@ void searchWithUndefinedSortFieldReturnsGetSchemaHint() { () -> searchService.search(COLLECTION_NAME, "*:*", null, null, sort, null, null)); assertTrue(e.getMessage().contains(SearchService.GET_SCHEMA_HINT_FORMAT.formatted(COLLECTION_NAME)), () -> "expected get-schema hint, got: " + e.getMessage()); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } @Test @@ -267,6 +317,7 @@ void searchWithUnparseableQueryReturnsLuceneSyntaxHint() { () -> searchService.search(COLLECTION_NAME, "name:(", null, null, null, null, null)); assertTrue(e.getMessage().contains(SearchService.LUCENE_SYNTAX_HINT), () -> "expected Lucene syntax hint, got: " + e.getMessage()); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } /** @@ -282,19 +333,24 @@ void searchOnMissingCollectionReturnsListCollectionsHint() { () -> searchService.search("definitely_not_a_collection", "*:*", null, null, null, null, null)); assertTrue(e.getMessage().contains(SearchService.LIST_COLLECTIONS_HINT), () -> "expected list-collections hint, got: " + e.getMessage()); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } /** - * A Solr error we have no advice for must reach the client untouched. Negative - * {@code rows} is structurally identical to the undefined-field failures — a - * 400 carrying a generic {@code SolrException} — so it also pins that the text - * matching is not over-broad. + * An unclassified Solr error keeps its status but replaces backend text with + * safe request guidance. Negative {@code rows} is structurally identical to the + * undefined-field failures, so it also pins that text matching is not + * over-broad. */ @Test void searchWithUnrecognizedSolrErrorPropagatesWithoutHint() { SolrException e = assertThrows(SolrException.class, () -> searchService.search(COLLECTION_NAME, "*:*", null, null, null, null, -5)); assertFalse(e.getMessage().contains("Hint:"), () -> "expected no hint, got: " + e.getMessage()); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); + assertTrue(e.getMessage().contains("pagination")); + assertTrue(e.getMessage().contains("get-schema")); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } /** diff --git a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java index 28f10856..89cf1c91 100644 --- a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java +++ b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java @@ -23,6 +23,7 @@ import static org.mockito.Mockito.when; import java.io.IOException; +import java.util.Arrays; import java.util.List; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrServerException; @@ -47,6 +48,119 @@ void constructor_ShouldInitializeWithSolrClient() { assertNotNull(localService); } + @Test + void search_WithBlankFiltersAndFacets_ShouldOmitThem() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + when(mockResponse.getResults()).thenReturn(createMockDocumentList()); + when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenAnswer(invocation -> { + SolrQuery q = invocation.getArgument(1); + assertEquals("*:*", q.getQuery()); + assertNull(q.getFilterQueries()); + assertNull(q.getFacetFields()); + assertNull(q.get("facet")); + return mockResponse; + }); + SearchService localService = new SearchService(mockClient); + assertNotNull(localService.search("test_collection", " \t", Arrays.asList("", " \t", null), + Arrays.asList(null, "", " \n"), null, null, null)); + } + + @Test + void search_WithBlankSortFields_ShouldOmitSorting() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + when(mockResponse.getResults()).thenReturn(createMockDocumentList()); + when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenAnswer(invocation -> { + SolrQuery q = invocation.getArgument(1); + assertNull(q.getSortField()); + return mockResponse; + }); + SearchService localService = new SearchService(mockClient); + assertNotNull(localService.search("test_collection", null, null, null, + Arrays.asList(new SortClause("", ""), new SortClause(" \t", "asc"), null), null, null)); + } + + @Test + void search_WithMixedBlankAndValidOptions_ShouldPreserveValidValues() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + when(mockResponse.getResults()).thenReturn(createMockDocumentList()); + String filter = "{!tag=genre}genre_s:fantasy"; + String facet = "{!ex=genre}genre_s"; + when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenAnswer(invocation -> { + SolrQuery q = invocation.getArgument(1); + assertEquals("{!edismax qf='name author_ss'}george martin", q.getQuery()); + assertArrayEquals(new String[]{filter, "price:[0 TO 10]"}, q.getFilterQueries()); + assertArrayEquals(new String[]{facet, "author_ss"}, q.getFacetFields()); + assertEquals("true", q.get("facet")); + assertEquals(1, q.getFacetMinCount()); + assertEquals("count", q.getFacetSortString()); + assertEquals("price desc,name asc", q.getSortField()); + return mockResponse; + }); + SearchService localService = new SearchService(mockClient); + assertNotNull(localService.search("test_collection", "{!edismax qf='name author_ss'}george martin", + Arrays.asList("", filter, null, " \n", "price:[0 TO 10]"), + Arrays.asList("\t", facet, "", null, "author_ss"), Arrays.asList(null, new SortClause("", "asc"), + new SortClause("price", "DESC"), new SortClause(" \n", ""), new SortClause("name", " \t")), + null, null)); + } + + @Test + void search_WithInvalidNonblankSortOrder_ShouldStillFail() { + SearchService localService = new SearchService(mock(SolrClient.class)); + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + () -> localService.search("test_collection", null, null, null, + List.of(new SortClause("price", "descending")), null, null)); + assertTrue(e.getMessage().contains("'asc' or 'desc'")); + } + + @Test + void search_BackendFailures_ShouldReturnSafeActionableErrors() throws Exception { + String detail = "private-token http://internal-solr:8983 /private/config java.util.ArrayList"; + for (Exception failure : List.of(new IOException(detail), new SolrServerException(detail), + new SolrException(SolrException.ErrorCode.SERVER_ERROR, detail), + new SolrException(SolrException.ErrorCode.BAD_REQUEST, detail), + new SolrException(SolrException.ErrorCode.UNAUTHORIZED, detail), + new SolrException(SolrException.ErrorCode.FORBIDDEN, detail), new ClassCastException(detail), + new IllegalStateException(detail))) { + SolrClient mockClient = mock(SolrClient.class); + when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenThrow(failure); + SearchService localService = new SearchService(mockClient); + Exception e = assertThrows(Exception.class, + () -> localService.search("test_collection", "*:*", null, List.of("platform"), null, null, null)); + for (String sensitive : List.of("private-token", "internal-solr", "/private/config", + "java.util.ArrayList")) { + assertFalse(e.getMessage().contains(sensitive), e::getMessage); + } + assertTrue(e.getMessage().contains("Search failed"), e::getMessage); + assertTrue(e.getMessage().contains("operator") || e.getMessage().contains("get-schema"), e::getMessage); + assertNull(e.getCause(), "MCP must not unwrap a raw backend failure"); + if (failure instanceof SolrException solrFailure) { + assertEquals(solrFailure.code(), assertInstanceOf(SolrException.class, e).code()); + } + } + } + + @Test + void search_FacetConversionFailure_ShouldNotReturnSuccessfulSearch() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + when(mockResponse.getResults()).thenReturn(createMockDocumentListWithData()); + ClassCastException failure = new ClassCastException("java.util.ArrayList cannot be cast to NamedList"); + when(mockResponse.getFacetFields()).thenThrow(failure); + when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenReturn(mockResponse); + SearchService localService = new SearchService(mockClient); + IllegalStateException e = assertThrows(IllegalStateException.class, + () -> localService.search("test_collection", "*:*", null, List.of("platform"), null, null, null)); + assertFalse(e.getMessage().contains("ClassCastException")); + assertFalse(e.getMessage().contains("NamedList")); + assertTrue(e.getMessage().contains("facetFields")); + assertTrue(e.getMessage().contains("operator")); + assertNull(e.getCause(), "MCP must not unwrap a raw response-conversion failure"); + } + /* * These stub Solr's error text rather than observe it, so they can only show * that a matching message produces a hint — never that Solr still emits such a @@ -66,8 +180,9 @@ void search_WithUndefinedField_ShouldHintGetSchema() throws Exception { new SolrException(SolrException.ErrorCode.BAD_REQUEST, "undefined field bogus")); IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> localService.search("test_collection", "bogus:x", null, null, null, null, null)); - assertTrue(e.getMessage().contains("undefined field bogus"), "original Solr message must be preserved"); + assertFalse(e.getMessage().contains("undefined field bogus"), "raw Solr message must not be exposed"); assertTrue(e.getMessage().contains(SearchService.GET_SCHEMA_HINT_FORMAT.formatted("test_collection"))); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } @Test @@ -78,6 +193,7 @@ void search_WithUndefinedSortField_ShouldHintGetSchema() throws Exception { IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> localService.search("test_collection", "*:*", null, null, sort, null, null)); assertTrue(e.getMessage().contains(SearchService.GET_SCHEMA_HINT_FORMAT.formatted("test_collection"))); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } @Test @@ -87,6 +203,8 @@ void search_WithQuerySyntaxError_ShouldHintLuceneSyntax() throws Exception { IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> localService.search("test_collection", "name:(", null, null, null, null, null)); assertTrue(e.getMessage().contains(SearchService.LUCENE_SYNTAX_HINT)); + assertFalse(e.getMessage().contains("org.apache.solr.search.SyntaxError")); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } /** @@ -101,6 +219,8 @@ void search_WithNotFoundStatus_ShouldHintListCollections() throws Exception { IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> localService.search("test_collection", "*:*", null, null, null, null, null)); assertTrue(e.getMessage().contains(SearchService.LIST_COLLECTIONS_HINT)); + assertFalse(e.getMessage().contains("mime type")); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } @Test @@ -109,8 +229,11 @@ void search_WithUnrelatedSolrError_ShouldPropagateUnchanged() throws Exception { new SolrException(SolrException.ErrorCode.SERVER_ERROR, "internal failure")); SolrException e = assertThrows(SolrException.class, () -> localService.search("test_collection", null, null, null, null, null, null)); - assertTrue(e.getMessage().contains("internal failure")); - assertFalse(e.getMessage().contains("Hint:")); + assertEquals(SolrException.ErrorCode.SERVER_ERROR.code, e.code()); + assertFalse(e.getMessage().contains("internal failure")); + assertTrue(e.getMessage().contains("operator")); + assertTrue(e.getMessage().contains("Solr health")); + assertNull(e.getCause(), "MCP must not unwrap a raw Solr failure"); } @Test From 122ed6cda6f10c551c4a50a35fc8fde51370bdf7 Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Fri, 11 Sep 2026 15:07:43 -0400 Subject: [PATCH 2/4] test(search): cover MCP regressions over HTTP and STDIO Signed-off-by: Aditya Parikh Co-authored-by: Junie --- .../server/McpClientIntegrationTestBase.java | 26 ++++++++++++++ .../server/McpClientStdioIntegrationTest.java | 34 ------------------- 2 files changed, 26 insertions(+), 34 deletions(-) diff --git a/src/test/java/org/apache/solr/mcp/server/McpClientIntegrationTestBase.java b/src/test/java/org/apache/solr/mcp/server/McpClientIntegrationTestBase.java index 0a2a4c8d..f50e4616 100644 --- a/src/test/java/org/apache/solr/mcp/server/McpClientIntegrationTestBase.java +++ b/src/test/java/org/apache/solr/mcp/server/McpClientIntegrationTestBase.java @@ -828,6 +828,32 @@ void completePromptArg_ViewSchema_ReturnsCreatedCollection() { "view-schema prompt completion should resolve the created collection: " + result.completion().values()); } + @Test + @Order(39) + void blankOptionalSearchEntriesAreIgnoredThroughMcp() throws Exception { + var result = mcpClient.callTool(new CallToolRequest("search", + Map.of("collection", SHOWS_COLLECTION, "query", "*:*", "rows", 0, "filterQueries", List.of(" "), + "facetFields", List.of("", "platform"), "sortClauses", + List.of(Map.of("field", "", "order", ""))))); + assertNotError(result); + Map response = OBJECT_MAPPER.readValue(extractText(result), new TypeReference<>() { + }); + assertEquals(SHOWS_DOC_COUNT, getNumFound(response)); + Map facets = (Map) response.get("facets"); + Map platforms = (Map) facets.get("platform"); + assertEquals(20, ((Number) platforms.get("Netflix")).intValue()); + } + + @Test + @Order(40) + void searchFailureIsAnActionableMcpToolError() { + var result = mcpClient.callTool(new CallToolRequest("search", + Map.of("collection", SHOWS_COLLECTION, "facetFields", List.of("nonexistent_field_xyz")))); + assertEquals(Boolean.TRUE, result.isError()); + assertTrue(extractText(result).contains("get-schema"), extractText(result)); + assertFalse(extractText(result).contains("Exception"), extractText(result)); + } + private static String extractFirstMessageText(GetPromptResult result) { List messages = result.messages(); assertFalse(messages.isEmpty(), "messages must not be empty"); diff --git a/src/test/java/org/apache/solr/mcp/server/McpClientStdioIntegrationTest.java b/src/test/java/org/apache/solr/mcp/server/McpClientStdioIntegrationTest.java index c42d070b..c6120a72 100644 --- a/src/test/java/org/apache/solr/mcp/server/McpClientStdioIntegrationTest.java +++ b/src/test/java/org/apache/solr/mcp/server/McpClientStdioIntegrationTest.java @@ -16,21 +16,13 @@ */ package org.apache.solr.mcp.server; -import static org.junit.jupiter.api.Assertions.*; - -import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; import io.modelcontextprotocol.client.McpClient; import io.modelcontextprotocol.client.McpSyncClient; import io.modelcontextprotocol.client.transport.ServerParameters; import io.modelcontextprotocol.client.transport.StdioClientTransport; import io.modelcontextprotocol.json.jackson.JacksonMcpJsonMapper; -import io.modelcontextprotocol.spec.McpSchema.CallToolRequest; -import java.util.List; -import java.util.Map; -import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Tag; -import org.junit.jupiter.api.Test; import org.testcontainers.containers.SolrContainer; import org.testcontainers.junit.jupiter.Container; import org.testcontainers.junit.jupiter.Testcontainers; @@ -61,30 +53,4 @@ protected McpSyncClient createClient() { return McpClient.sync(transport).build(); } - @Test - @Order(39) - void blankOptionalSearchEntriesAreIgnoredThroughMcp() throws Exception { - var result = mcpClient.callTool(new CallToolRequest("search", - Map.of("collection", SHOWS_COLLECTION, "query", "*:*", "rows", 0, "filterQueries", List.of(" "), - "facetFields", List.of("", "platform"), "sortClauses", - List.of(Map.of("field", "", "order", ""))))); - assertNotError(result); - Map response = OBJECT_MAPPER.readValue(extractText(result), new TypeReference<>() { - }); - assertEquals(SHOWS_DOC_COUNT, getNumFound(response)); - Map facets = (Map) response.get("facets"); - Map platforms = (Map) facets.get("platform"); - assertEquals(20, ((Number) platforms.get("Netflix")).intValue()); - } - - @Test - @Order(40) - void searchFailureIsAnActionableMcpToolError() { - var result = mcpClient.callTool(new CallToolRequest("search", - Map.of("collection", SHOWS_COLLECTION, "facetFields", List.of("nonexistent_field_xyz")))); - assertEquals(Boolean.TRUE, result.isError()); - assertTrue(extractText(result).contains("get-schema"), extractText(result)); - assertFalse(extractText(result).contains("Exception"), extractText(result)); - } - } From 7ca165a0dfc99edca1301f4490588a031575fb44 Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Sat, 12 Sep 2026 23:10:46 -0400 Subject: [PATCH 3/4] fix(search): log backend failures at WARN and drop an off-topic test The client only sees the generic message, so the log is the only record of the cause; DEBUG hid it in production. The schemaless-facet integration test exercised Solr's faceting rather than this change and was the sole reason for the untyped platform fixture field. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01CiUHyyXLTo9ATdgg8eRFZJ Signed-off-by: Aditya Parikh --- .../solr/mcp/server/search/SearchService.java | 8 +++-- .../search/SearchServiceIntegrationTest.java | 34 ------------------- 2 files changed, 5 insertions(+), 37 deletions(-) diff --git a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java index 37ef3928..356209b6 100644 --- a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java +++ b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java @@ -288,6 +288,7 @@ public SearchResponse search(@McpToolParam(description = "Solr collection to que if (!CollectionUtils.isEmpty(facetFields)) { facetFields.stream().filter(StringUtils::hasText).forEach(solrQuery::addFacetField); } + // addFacetField sets facet=true; getFacetFields() is null until the first one if (solrQuery.getFacetFields() != null) { solrQuery.setFacetMinCount(1); solrQuery.setFacetSort(FacetParams.FACET_SORT_COUNT); @@ -326,13 +327,14 @@ public SearchResponse search(@McpToolParam(description = "Solr collection to que } catch (SolrException e) { throw withRemediationHint(e, collection); } catch (SolrServerException e) { - logger.debug("Solr query failed on collection {}", collection, e); + logger.warn("Solr query failed on collection {}", collection, e); throw new SolrServerException(CONNECTION_ERROR); } catch (IOException e) { - logger.debug("Solr query failed on collection {}", collection, e); + logger.warn("Solr query failed on collection {}", collection, e); throw new IOException(CONNECTION_ERROR); } catch (RuntimeException e) { - logger.debug("Solr query response failed on collection {}", collection, e); + // The client only sees RESPONSE_ERROR; the log is the sole record of the cause. + logger.warn("Solr query response failed on collection {}", collection, e); throw new IllegalStateException(RESPONSE_ERROR); } } diff --git a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java index 0dc84a45..b80ffffb 100644 --- a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java +++ b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java @@ -20,18 +20,13 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.OptionalDouble; import org.apache.solr.client.solrj.SolrClient; -import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.CollectionAdminRequest; -import org.apache.solr.client.solrj.request.GenericSolrRequest; import org.apache.solr.common.SolrException; -import org.apache.solr.common.params.ModifiableSolrParams; -import org.apache.solr.common.util.NamedList; import org.apache.solr.mcp.server.TestDocuments; import org.apache.solr.mcp.server.TestcontainersConfiguration; import org.apache.solr.mcp.server.indexing.IndexingService; @@ -75,7 +70,6 @@ void setUp() throws Exception { [ { "id": "book001", - "platform": ["netflix"], "platform_ss": ["netflix"], "name": ["A Game of Thrones"], "author_ss": ["George R.R. Martin"], @@ -87,7 +81,6 @@ void setUp() throws Exception { }, { "id": "book002", - "platform": ["netflix"], "platform_ss": ["netflix"], "name": ["A Clash of Kings"], "author_ss": ["George R.R. Martin"], @@ -99,7 +92,6 @@ void setUp() throws Exception { }, { "id": "book003", - "platform": ["hulu"], "platform_ss": ["hulu"], "name": ["A Storm of Swords"], "author_ss": ["George R.R. Martin"], @@ -222,32 +214,6 @@ void facetingAQueryThatMatchesNothingReturnsEmptyFacets() throws SolrServerExcep () -> "expected no facet buckets, got: " + result.facets().get("genre_s")); } - @Test - void searchWithSchemalessPlatformFacetsReturnsCounts() throws Exception { - // Solr 9's _default infers text_general without docValues or uninversion, - // so its default faceting returns [] for platform despite matching documents. - // Preserve Solr's response (also across Solr versions), not a different facet - // algorithm. - NamedList raw = solrClient.request( - new GenericSolrRequest(SolrRequest.METHOD.GET, "/" + COLLECTION_NAME + "/select", - new ModifiableSolrParams().set("q", "id:book*").set("rows", 0).set("facet", true) - .set("facet.field", "platform").set("facet.mincount", 1).set("facet.sort", "count")), - COLLECTION_NAME); - NamedList facetCounts = assertInstanceOf(NamedList.class, raw.get("facet_counts")); - NamedList facetFields = assertInstanceOf(NamedList.class, facetCounts.get("facet_fields")); - NamedList platform = assertInstanceOf(NamedList.class, facetFields.get("platform")); - Map expectedPlatform = new HashMap<>(); - platform.forEach((term, count) -> expectedPlatform.put(term, ((Number) count).longValue())); - SearchResponse document = searchService.search(COLLECTION_NAME, "id:book001", null, null, null, null, null); - assertEquals(List.of("netflix"), document.documents().getFirst().get("platform")); - SearchResponse result = searchService.search(COLLECTION_NAME, "id:book*", null, - List.of("platform", "platform_ss", "genre_s"), null, null, 0); - assertEquals(10, result.numFound()); - assertTrue(result.documents().isEmpty()); - assertEquals(Map.of("platform", expectedPlatform, "platform_ss", Map.of("netflix", 2L, "hulu", 1L), "genre_s", - Map.of("fantasy", 7L, "scifi", 3L)), result.facets()); - } - @Test void searchWithBlankOptionsPreservesFiltersFacetsAndSort() throws Exception { SearchResponse result = searchService.search(COLLECTION_NAME, " \t", List.of("", "platform:netflix", " "), From c9dfc407ed47a3af8bcced46e3d3b2afffc544d4 Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Sat, 12 Sep 2026 23:17:44 -0400 Subject: [PATCH 4/4] test(search): filter on the typed platform field The untyped platform fixture field went away with the schemaless-facet test, so the blank-options test now filters on platform_ss. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01CiUHyyXLTo9ATdgg8eRFZJ Signed-off-by: Aditya Parikh --- .../solr/mcp/server/search/SearchServiceIntegrationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java index b80ffffb..3493113e 100644 --- a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java +++ b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java @@ -216,7 +216,7 @@ void facetingAQueryThatMatchesNothingReturnsEmptyFacets() throws SolrServerExcep @Test void searchWithBlankOptionsPreservesFiltersFacetsAndSort() throws Exception { - SearchResponse result = searchService.search(COLLECTION_NAME, " \t", List.of("", "platform:netflix", " "), + SearchResponse result = searchService.search(COLLECTION_NAME, " \t", List.of("", "platform_ss:netflix", " "), List.of(" ", "platform_ss", ""), List.of(new SortClause("", ""), new SortClause("id", "desc")), 0, 10); assertEquals(2, result.numFound()); assertEquals(List.of("book002", "book001"), getDocumentIds(result.documents()));