From 98c8ece81223e881b4ea3a4cfa7f5c4199e5ab4f Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Tue, 18 Aug 2026 17:25:20 -0400 Subject: [PATCH] fix: validate collection name consistently across all MCP tool methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only createCollection and the two schema-modification tools validated the collection name. checkHealth, getCollectionStats, search, the three index*Documents tools, getSchema and getSchemaResource accepted null or blank and failed downstream — a null collection reaches SolrJ and silently targets the client's default collection rather than reporting a bad argument. The message was already duplicated on main: a BLANK_COLLECTION_NAME_ERROR constant in CollectionService and a copied string literal in SchemaService's private requireCollection. Extract one shared ToolArguments.requireCollection(String) in the util package and route all eleven call sites through it, so the four services cannot drift apart. Message wording is aligned with the document-creator family (" cannot be null or empty"), which also covers SchemaService's sibling requireNonEmpty helper. The null half of the check is deliberate rather than redundant defensive coding: the package is @NullMarked with NullAway as a build error, but that analysis only binds callers the compiler can see. MCP tool methods are invoked reflectively by the Spring AI annotation runtime, which resolves each parameter with a plain lookup against the request's argument map and passes the result straight through — a missing or null JSON value therefore arrives as null no matter what the annotations declare. @McpToolParam(required = true) only marks the parameter required in the advertised JSON schema; the server does not validate incoming arguments against it. CollectionNameValidationTest asserts all nine entry points against the shared constant rather than a copied literal. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Aditya Parikh --- .../server/collection/CollectionService.java | 12 +- .../mcp/server/indexing/IndexingService.java | 8 ++ .../solr/mcp/server/schema/SchemaService.java | 13 +- .../solr/mcp/server/search/SearchService.java | 4 + .../solr/mcp/server/util/ToolArguments.java | 62 ++++++++ .../server/CollectionNameValidationTest.java | 134 ++++++++++++++++++ 6 files changed, 220 insertions(+), 13 deletions(-) create mode 100644 src/main/java/org/apache/solr/mcp/server/util/ToolArguments.java create mode 100644 src/test/java/org/apache/solr/mcp/server/CollectionNameValidationTest.java diff --git a/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java b/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java index 011d278e..9435d2dc 100644 --- a/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java +++ b/src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java @@ -20,6 +20,7 @@ import static org.apache.solr.mcp.server.collection.CollectionUtils.getInteger; import static org.apache.solr.mcp.server.collection.CollectionUtils.getLong; import static org.apache.solr.mcp.server.util.JsonUtils.toJson; +import static org.apache.solr.mcp.server.util.ToolArguments.requireCollection; import com.fasterxml.jackson.databind.ObjectMapper; import io.micrometer.observation.annotation.Observed; @@ -259,9 +260,6 @@ public class CollectionService { /** Default replication factor for new collections */ private static final int DEFAULT_REPLICATION_FACTOR = 1; - /** Error message for blank collection name validation */ - private static final String BLANK_COLLECTION_NAME_ERROR = "Collection name must not be blank"; - /** SolrJ client for communicating with Solr server */ private final SolrClient solrClient; @@ -519,6 +517,8 @@ public List listCollections() throws SolrServerException, IOException { public SolrMetrics getCollectionStats( @McpToolParam(description = "Solr collection to get stats/metrics for") String collection) throws SolrServerException, IOException { + requireCollection(collection); + // Extract actual collection name from shard name if needed String actualCollection = extractCollectionName(collection); @@ -1067,6 +1067,8 @@ private boolean validateCollectionExists(String collection) throws SolrServerExc annotations = @McpTool.McpAnnotations(readOnlyHint = true), description = "Check health of a Solr collection") public SolrHealthStatus checkHealth(@McpToolParam(description = "Solr collection") String collection) { + requireCollection(collection); + String actualCollection = extractCollectionName(collection); try { // Ping Solr @@ -1135,9 +1137,7 @@ public CollectionCreationResult createCollection( required = false) @Nullable Integer replicationFactor) throws SolrServerException, IOException { - if (name == null || name.isBlank()) { - throw new IllegalArgumentException(BLANK_COLLECTION_NAME_ERROR); - } + requireCollection(name); String effectiveConfigSet = configSet != null ? configSet : DEFAULT_CONFIGSET; int effectiveShards = numShards != null ? numShards : DEFAULT_NUM_SHARDS; diff --git a/src/main/java/org/apache/solr/mcp/server/indexing/IndexingService.java b/src/main/java/org/apache/solr/mcp/server/indexing/IndexingService.java index 34674852..df6d0964 100644 --- a/src/main/java/org/apache/solr/mcp/server/indexing/IndexingService.java +++ b/src/main/java/org/apache/solr/mcp/server/indexing/IndexingService.java @@ -16,6 +16,8 @@ */ package org.apache.solr.mcp.server.indexing; +import static org.apache.solr.mcp.server.util.ToolArguments.requireCollection; + import io.micrometer.observation.annotation.Observed; import java.io.IOException; import java.util.List; @@ -213,6 +215,8 @@ public IndexingService(SolrClient solrClient, IndexingDocumentCreator indexingDo public String indexJsonDocuments(@McpToolParam(description = "Solr collection to index into") String collection, @McpToolParam(description = "JSON string containing documents to index") String json) throws IOException, SolrServerException { + requireCollection(collection); + List schemalessDoc = indexingDocumentCreator.createSchemalessDocumentsFromJson(json); int successCount = indexDocuments(collection, schemalessDoc); return "Successfully indexed " + successCount + " of " + schemalessDoc.size() + " documents into collection '" @@ -288,6 +292,8 @@ public String indexJsonDocuments(@McpToolParam(description = "Solr collection to public String indexCsvDocuments(@McpToolParam(description = "Solr collection to index into") String collection, @McpToolParam(description = "CSV string containing documents to index") String csv) throws IOException, SolrServerException { + requireCollection(collection); + List schemalessDoc = indexingDocumentCreator.createSchemalessDocumentsFromCsv(csv); int successCount = indexDocuments(collection, schemalessDoc); return "Successfully indexed " + successCount + " of " + schemalessDoc.size() + " documents into collection '" @@ -387,6 +393,8 @@ public String indexCsvDocuments(@McpToolParam(description = "Solr collection to public String indexXmlDocuments(@McpToolParam(description = "Solr collection to index into") String collection, @McpToolParam(description = "XML string containing documents to index") String xml) throws ParserConfigurationException, SAXException, IOException, SolrServerException { + requireCollection(collection); + List schemalessDoc = indexingDocumentCreator.createSchemalessDocumentsFromXml(xml); int successCount = indexDocuments(collection, schemalessDoc); return "Successfully indexed " + successCount + " of " + schemalessDoc.size() + " documents into collection '" diff --git a/src/main/java/org/apache/solr/mcp/server/schema/SchemaService.java b/src/main/java/org/apache/solr/mcp/server/schema/SchemaService.java index 3f3bb96a..ba565cd5 100644 --- a/src/main/java/org/apache/solr/mcp/server/schema/SchemaService.java +++ b/src/main/java/org/apache/solr/mcp/server/schema/SchemaService.java @@ -18,6 +18,7 @@ import static org.apache.solr.mcp.server.util.JsonUtils.toJson; import static org.apache.solr.mcp.server.util.PromptText.optionalCodeBlock; +import static org.apache.solr.mcp.server.util.ToolArguments.requireCollection; import com.fasterxml.jackson.databind.ObjectMapper; import io.micrometer.observation.annotation.Observed; @@ -182,6 +183,8 @@ public SchemaService(SolrClient solrClient, ObjectMapper objectMapper) { description = "Schema definition for a Solr collection including fields, field types, and copy fields", mimeType = "application/json") public String getSchemaResource(String collection) { + requireCollection(collection); + try { return toJson(objectMapper, getSchema(collection)); } catch (Exception e) { @@ -277,6 +280,8 @@ public String getSchemaResource(String collection) { annotations = @McpTool.McpAnnotations(readOnlyHint = true), description = "Get schema for a Solr collection") public SchemaRepresentation getSchema(String collection) throws Exception { + requireCollection(collection); + SchemaRequest schemaRequest = new SchemaRequest(); return schemaRequest.process(solrClient, collection).getSchemaRepresentation(); } @@ -482,15 +487,9 @@ private AnalyzerDefinition toAnalyzerDefinition(Object raw) { return def; } - private static void requireCollection(String collection) { - if (collection == null || collection.isBlank()) { - throw new IllegalArgumentException("Collection name must not be blank"); - } - } - private static void requireNonEmpty(List list, String name) { if (list == null || list.isEmpty()) { - throw new IllegalArgumentException(name + " must not be empty"); + throw new IllegalArgumentException(name + " cannot be null or empty"); } } 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 cff51681..34e7a94d 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 @@ -16,6 +16,8 @@ */ package org.apache.solr.mcp.server.search; +import static org.apache.solr.mcp.server.util.ToolArguments.requireCollection; + import io.micrometer.observation.annotation.Observed; import java.io.IOException; import java.util.ArrayList; @@ -309,6 +311,8 @@ public SearchResponse search(@McpToolParam(description = "Solr collection to que @McpToolParam(description = "Number of rows to return", required = false) @Nullable Integer rows) throws SolrServerException, IOException { + requireCollection(collection); + // query final SolrQuery solrQuery = new SolrQuery("*:*"); if (StringUtils.hasText(query)) { diff --git a/src/main/java/org/apache/solr/mcp/server/util/ToolArguments.java b/src/main/java/org/apache/solr/mcp/server/util/ToolArguments.java new file mode 100644 index 00000000..1161cc0e --- /dev/null +++ b/src/main/java/org/apache/solr/mcp/server/util/ToolArguments.java @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.mcp.server.util; + +/** + * Validation of arguments arriving at MCP tool boundaries. + * + *

+ * Every {@code @McpTool} method that takes a collection name calls + * {@link #requireCollection(String)} first, so a client that omits the argument + * or sends an empty string gets one identical, actionable message instead of a + * per-service variant or an opaque downstream failure. + * + *

+ * Why a runtime check in null-marked code: the server is + * {@code @NullMarked} (see {@code org.apache.solr.mcp.server.package-info}) and + * NullAway runs as a build error, but that analysis only binds callers the + * compiler can see. MCP tool methods are invoked reflectively by the Spring AI + * annotation runtime, which resolves each parameter with a plain lookup against + * the request's argument map and passes the result straight through — a missing + * or null JSON value therefore reaches the method as {@code null} no matter + * what the annotations declare. {@code @McpToolParam(required = true)} only + * marks the parameter required in the advertised JSON schema; the server does + * not validate incoming arguments against it. These checks are the trust + * boundary, not redundant defensive coding. + */ +public final class ToolArguments { + + /** Message used whenever a collection name is missing, empty, or blank. */ + public static final String BLANK_COLLECTION_NAME_ERROR = "Collection name cannot be null or empty"; + + private ToolArguments() { + } + + /** + * Rejects a missing, empty, or whitespace-only collection name. + * + * @param collection + * the collection name supplied by the MCP client + * @throws IllegalArgumentException + * if {@code collection} is null or contains only whitespace + */ + public static void requireCollection(String collection) { + if (collection == null || collection.isBlank()) { + throw new IllegalArgumentException(BLANK_COLLECTION_NAME_ERROR); + } + } +} diff --git a/src/test/java/org/apache/solr/mcp/server/CollectionNameValidationTest.java b/src/test/java/org/apache/solr/mcp/server/CollectionNameValidationTest.java new file mode 100644 index 00000000..f22ed6b2 --- /dev/null +++ b/src/test/java/org/apache/solr/mcp/server/CollectionNameValidationTest.java @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.mcp.server; + +import static org.apache.solr.mcp.server.util.ToolArguments.BLANK_COLLECTION_NAME_ERROR; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.solr.client.solrj.SolrClient; +import org.apache.solr.mcp.server.collection.CollectionService; +import org.apache.solr.mcp.server.indexing.IndexingService; +import org.apache.solr.mcp.server.indexing.documentcreator.IndexingDocumentCreator; +import org.apache.solr.mcp.server.schema.SchemaService; +import org.apache.solr.mcp.server.search.SearchService; +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledInNativeImage; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +/** + * Verifies that every MCP tool method taking a collection name rejects a + * missing, empty, or whitespace-only value with the same message. + * + *

+ * The services are exercised directly with mocked collaborators: validation + * must happen before any Solr call, so a client that omits the argument gets an + * actionable message rather than an opaque downstream failure. Asserting + * against {@code BLANK_COLLECTION_NAME_ERROR} rather than a copied string + * literal is deliberate — it is what keeps the four services from drifting + * apart. + */ +@ExtendWith(MockitoExtension.class) +@DisabledInNativeImage +class CollectionNameValidationTest { + + private static final String BLANK = " "; + + @Mock + private SolrClient solrClient; + + @Mock + private IndexingDocumentCreator indexingDocumentCreator; + + private CollectionService collectionService; + private IndexingService indexingService; + private SchemaService schemaService; + private SearchService searchService; + + @BeforeEach + void setUp() { + ObjectMapper objectMapper = new ObjectMapper(); + collectionService = new CollectionService(solrClient, objectMapper); + indexingService = new IndexingService(solrClient, indexingDocumentCreator); + schemaService = new SchemaService(solrClient, objectMapper); + searchService = new SearchService(solrClient); + } + + private static void assertRejectsBlankCollection(ThrowingCallable withNull, ThrowingCallable withBlank) { + assertThatThrownBy(withNull).isInstanceOf(IllegalArgumentException.class) + .hasMessage(BLANK_COLLECTION_NAME_ERROR); + assertThatThrownBy(withBlank).isInstanceOf(IllegalArgumentException.class) + .hasMessage(BLANK_COLLECTION_NAME_ERROR); + } + + @Test + void createCollectionRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> collectionService.createCollection(null, null, null, null), + () -> collectionService.createCollection(BLANK, null, null, null)); + } + + @Test + void getCollectionStatsRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> collectionService.getCollectionStats(null), + () -> collectionService.getCollectionStats(BLANK)); + } + + @Test + void checkHealthRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> collectionService.checkHealth(null), + () -> collectionService.checkHealth(BLANK)); + } + + @Test + void indexJsonDocumentsRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> indexingService.indexJsonDocuments(null, "[]"), + () -> indexingService.indexJsonDocuments(BLANK, "[]")); + } + + @Test + void indexCsvDocumentsRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> indexingService.indexCsvDocuments(null, "id\n1"), + () -> indexingService.indexCsvDocuments(BLANK, "id\n1")); + } + + @Test + void indexXmlDocumentsRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> indexingService.indexXmlDocuments(null, ""), + () -> indexingService.indexXmlDocuments(BLANK, "")); + } + + @Test + void getSchemaRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> schemaService.getSchema(null), () -> schemaService.getSchema(BLANK)); + } + + @Test + void getSchemaResourceRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> schemaService.getSchemaResource(null), + () -> schemaService.getSchemaResource(BLANK)); + } + + @Test + void searchRejectsBlankCollectionName() { + assertRejectsBlankCollection(() -> searchService.search(null, null, null, null, null, null, null), + () -> searchService.search(BLANK, null, null, null, null, null, null)); + } +}