This is an automated email from the ASF dual-hosted git repository.
epugh pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr-mcp.git
The following commit(s) were added to refs/heads/main by this push:
new 59089e1 feat(search): replace sort clause maps with a typed
SortClause record (#164)
59089e1 is described below
commit 59089e1f341adf11963af2c43f4a321338a1ee04
Author: Aditya Parikh <[email protected]>
AuthorDate: Fri Sep 11 10:05:05 2026 -0400
feat(search): replace sort clause maps with a typed SortClause record (#164)
The search tool's sortClauses parameter was List<Map<String,String>>
keyed by magic "item"/"order" strings that the generated JSON schema
cannot express, so MCP clients had to guess the keys. A record makes
the components named schema properties and the contract self-documenting.
The record also normalizes input the way LLM clients actually send it:
order is case-insensitive and defaults to asc when omitted; an unknown
order or missing field fails with a message that names the offending
value so clients can self-correct on retry.
Registered in SolrNativeHints alongside the response records: tool
parameter records are bound reflectively by Jackson and need the same
native-image treatment.
Signed-off-by: adityamparikh <[email protected]>
Co-authored-by: Claude Fable 5 <[email protected]>
---
.../solr/mcp/server/config/SolrNativeHints.java | 17 +++---
.../solr/mcp/server/search/SearchService.java | 47 ++--------------
.../apache/solr/mcp/server/search/SortClause.java | 65 ++++++++++++++++++++++
.../search/SearchServiceIntegrationTest.java | 11 ++--
.../solr/mcp/server/search/SearchServiceTest.java | 9 +--
.../solr/mcp/server/search/SortClauseTest.java | 62 +++++++++++++++++++++
6 files changed, 151 insertions(+), 60 deletions(-)
diff --git
a/src/main/java/org/apache/solr/mcp/server/config/SolrNativeHints.java
b/src/main/java/org/apache/solr/mcp/server/config/SolrNativeHints.java
index 2d1cedf..584f7ce 100644
--- a/src/main/java/org/apache/solr/mcp/server/config/SolrNativeHints.java
+++ b/src/main/java/org/apache/solr/mcp/server/config/SolrNativeHints.java
@@ -60,18 +60,19 @@ public class SolrNativeHints {
}
/**
- * Package-private record types returned by {@code @McpTool} methods.
Jackson
- * needs reflection access to serialize these as MCP tool responses in
native
- * image.
+ * Record types used by {@code @McpTool} methods as responses or
parameters.
+ * Jackson needs reflection access to serialize responses and to bind
request
+ * parameters in native image. Registered by name because some are
+ * package-private.
*/
- private static final List<String> MCP_RESPONSE_RECORDS = List.of(
+ private static final List<String> MCP_TOOL_RECORDS = List.of(
"org.apache.solr.mcp.server.collection.CollectionCreationResult",
"org.apache.solr.mcp.server.collection.SolrHealthStatus",
"org.apache.solr.mcp.server.collection.SolrMetrics",
"org.apache.solr.mcp.server.collection.IndexStats",
"org.apache.solr.mcp.server.collection.QueryStats",
"org.apache.solr.mcp.server.collection.CacheStats",
"org.apache.solr.mcp.server.collection.CacheInfo",
"org.apache.solr.mcp.server.collection.HandlerStats",
"org.apache.solr.mcp.server.collection.HandlerInfo",
"org.apache.solr.mcp.server.search.SearchResponse",
- "org.apache.solr.mcp.server.schema.SchemaUpdateResult");
+ "org.apache.solr.mcp.server.search.SortClause",
"org.apache.solr.mcp.server.schema.SchemaUpdateResult");
static class Registrar implements RuntimeHintsRegistrar {
@Override
@@ -110,8 +111,10 @@ public class SolrNativeHints {
hints.reflection().registerType(org.apache.solr.client.solrj.response.schema.SchemaRepresentation.class,
categories);
- // MCP tool response records (package-private,
registered by name)
- for (String className : MCP_RESPONSE_RECORDS) {
+ // MCP tool request/response records (some
package-private, registered by
+ // name); request records are deserialized reflectively
by Jackson when
+ // tool arguments are bound
+ for (String className : MCP_TOOL_RECORDS) {
hints.reflection().registerTypeIfPresent(classLoader, className, categories);
}
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 cff5168..2fb9800 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
@@ -108,14 +108,6 @@ import org.springframework.util.StringUtils;
@Observed
public class SearchService {
- /** Key for the field name within a sort clause map. */
- public static final String SORT_ITEM = "item";
- /**
- * Key for the sort direction ({@code asc} / {@code desc}) within a
sort clause
- * map.
- */
- public static final String SORT_ORDER = "order";
-
/**
* Fragments of Solr's own error text that identify a failure we can
advise on.
*
@@ -246,7 +238,8 @@ public class SearchService {
* @param facetFields
* List of fields to facet on
* @param sortClauses
- * List of sort clauses for ordering results
+ * List of sort clauses for ordering results; each names a
field and
+ * an optional {@code asc}/{@code desc} order (default
{@code asc})
* @param start
* Starting offset for pagination
* @param rows
@@ -303,8 +296,9 @@ public class SearchService {
required = false) @Nullable
List<String> filterQueries,
@McpToolParam(description = "Solr facet fields",
required = false) @Nullable List<String> facetFields,
@McpToolParam(
- description = "Solr sort parameter",
- required = false) @Nullable
List<Map<String, String>> sortClauses,
+ description = "Sort clauses applied in
order. Each has 'field' (field name to sort"
+ + " on) and 'order'
('asc' or 'desc', default 'asc')",
+ required = false) @Nullable
List<SortClause> sortClauses,
@McpToolParam(description = "Starting offset for
pagination", required = false) @Nullable Integer start,
@McpToolParam(description = "Number of rows to return",
required = false) @Nullable Integer rows)
throws SolrServerException, IOException {
@@ -330,7 +324,7 @@ public class SearchService {
// sorting
if (!CollectionUtils.isEmpty(sortClauses)) {
-
solrQuery.setSorts(sortClauses.stream().map(SearchService::toSortClause).toList());
+
solrQuery.setSorts(sortClauses.stream().map(SortClause::toSolrSortClause).toList());
}
// pagination
@@ -361,35 +355,6 @@ public class SearchService {
return new SearchResponse(documents.getNumFound(),
documents.getStart(), documents.getMaxScore(), docs, facets);
}
- /**
- * Builds a {@link SolrQuery.SortClause} from one caller-supplied map.
- *
- * <p>
- * Both keys are validated up front: {@code SortClause}'s constructor
calls
- * {@code ORDER.valueOf(order)}, which throws {@link
NullPointerException} on a
- * missing order and an opaque {@link IllegalArgumentException} on an
- * unrecognised one. Callers are LLMs, so the message needs to say what
to send.
- */
- private static SolrQuery.SortClause toSortClause(Map<String, String>
sortClause) {
- String field = sortClause.get(SORT_ITEM);
- String order = sortClause.get(SORT_ORDER);
- if (field == null || field.isBlank()) {
- throw new IllegalArgumentException("Each sort clause
requires a non-empty '" + SORT_ITEM + "' key");
- }
- if (order == null || order.isBlank()) {
- throw new IllegalArgumentException(
- "Sort clause for '" + field + "'
requires a '" + SORT_ORDER + "' key of 'asc' or 'desc'");
- }
- SolrQuery.ORDER parsed;
- try {
- parsed =
SolrQuery.ORDER.valueOf(order.toLowerCase(Locale.ROOT));
- } catch (IllegalArgumentException e) {
- throw new IllegalArgumentException(
- "Unsupported sort order '" + order + "'
for '" + field + "'; expected 'asc' or 'desc'", e);
- }
- return new SolrQuery.SortClause(field, parsed);
- }
-
/**
* 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
diff --git a/src/main/java/org/apache/solr/mcp/server/search/SortClause.java
b/src/main/java/org/apache/solr/mcp/server/search/SortClause.java
new file mode 100644
index 0000000..9d8260c
--- /dev/null
+++ b/src/main/java/org/apache/solr/mcp/server/search/SortClause.java
@@ -0,0 +1,65 @@
+/*
+ * 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.search;
+
+import java.util.Locale;
+import org.apache.solr.client.solrj.request.SolrQuery;
+import org.springframework.util.StringUtils;
+
+/**
+ * Immutable record representing a single sort clause of the {@code search} MCP
+ * tool.
+ *
+ * <p>
+ * Because this is a record, its components become named properties in the JSON
+ * schema that MCP clients receive, making the contract self-documenting —
+ * unlike a {@code Map} parameter whose expected keys are invisible to the
+ * schema.
+ *
+ * @param field
+ * the Solr field name to sort on
+ * @param order
+ * the sort direction, {@code asc} or {@code desc} (case
+ * insensitive); defaults to {@code asc} when omitted
+ */
+public record SortClause(String field, String order) {
+
+ /** Default sort order applied when {@code order} is omitted. */
+ private static final String DEFAULT_ORDER = "asc";
+
+ /**
+ * Converts this clause to a SolrJ {@link SolrQuery.SortClause},
normalizing the
+ * order to lowercase and defaulting a missing order to {@code asc}.
+ *
+ * @return the equivalent SolrJ sort clause
+ * @throws IllegalArgumentException
+ * if {@code field} is missing or {@code order} is neither
+ * {@code asc} nor {@code desc}; the message names the
offending
+ * value so MCP clients can correct the call
+ */
+ SolrQuery.SortClause toSolrSortClause() {
+ if (!StringUtils.hasText(field)) {
+ throw new IllegalArgumentException("Sort clause is
missing 'field': provide the field name to sort on");
+ }
+ String normalized = StringUtils.hasText(order) ?
order.toLowerCase(Locale.ROOT) : DEFAULT_ORDER;
+ if (!"asc".equals(normalized) && !"desc".equals(normalized)) {
+ throw new IllegalArgumentException(
+ "Invalid sort order '" + order + "' for
field '" + field + "': must be 'asc' or 'desc'");
+ }
+ return new SolrQuery.SortClause(field, normalized);
+ }
+}
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 99fb607..af19b90 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
@@ -253,8 +253,7 @@ class SearchServiceIntegrationTest {
*/
@Test
void searchWithUndefinedSortFieldReturnsGetSchemaHint() {
- List<Map<String, String>> sort = List
- .of(Map.of(SearchService.SORT_ITEM,
"definitely_not_a_field", SearchService.SORT_ORDER, "asc"));
+ List<SortClause> sort = List.of(new
SortClause("definitely_not_a_field", "asc"));
IllegalArgumentException e =
assertThrows(IllegalArgumentException.class,
() -> searchService.search(COLLECTION_NAME,
"*:*", null, null, sort, null, null));
assertTrue(e.getMessage().contains(SearchService.GET_SCHEMA_HINT_FORMAT.formatted(COLLECTION_NAME)),
@@ -343,7 +342,7 @@ class SearchServiceIntegrationTest {
@Test
void testSortByPriceAscending() throws Exception {
- List<Map<String, String>> sortClauses = List.of(Map.of("item",
"price", "order", "asc"));
+ List<SortClause> sortClauses = List.of(new SortClause("price",
"asc"));
SearchResponse result = searchService.search(COLLECTION_NAME,
null, null, null, sortClauses, null, null);
assertNotNull(result);
List<Map<String, Object>> documents = result.documents();
@@ -361,7 +360,7 @@ class SearchServiceIntegrationTest {
@Test
void testSortByPriceDescending() throws Exception {
- List<Map<String, String>> sortClauses = List.of(Map.of("item",
"price", "order", "desc"));
+ List<SortClause> sortClauses = List.of(new SortClause("price",
"desc"));
SearchResponse result = searchService.search(COLLECTION_NAME,
null, null, null, sortClauses, null, null);
assertNotNull(result);
List<Map<String, Object>> documents = result.documents();
@@ -379,7 +378,7 @@ class SearchServiceIntegrationTest {
@Test
void testSortBySequence() throws Exception {
- List<Map<String, String>> sortClauses = List.of(Map.of("item",
"sequence_i", "order", "asc"));
+ List<SortClause> sortClauses = List.of(new
SortClause("sequence_i", "asc"));
List<String> filterQueries = List.of("series_s:\"A Song of Ice
and Fire\"");
SearchResponse result = searchService.search(COLLECTION_NAME,
null, filterQueries, null, sortClauses, null,
null);
@@ -427,7 +426,7 @@ class SearchServiceIntegrationTest {
@Test
void testCombinedSortingAndFiltering() throws Exception {
- List<Map<String, String>> sortClauses = List.of(Map.of("item",
"price", "order", "desc"));
+ List<SortClause> sortClauses = List.of(new SortClause("price",
"desc"));
List<String> filterQueries = List.of("genre_s:fantasy");
SearchResponse result = searchService.search(COLLECTION_NAME,
null, filterQueries, null, sortClauses, null,
null);
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 798348b..ed80b17 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
@@ -24,7 +24,6 @@ import static org.mockito.Mockito.when;
import java.io.IOException;
import java.util.List;
-import java.util.Map;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.request.SolrQuery;
@@ -75,8 +74,7 @@ class SearchServiceTest {
void search_WithUndefinedSortField_ShouldHintGetSchema() throws
Exception {
SearchService localService = serviceThrowing(
new
SolrException(SolrException.ErrorCode.BAD_REQUEST, "sort param field can't be
found: bogus"));
- List<Map<String, String>> sort = List
- .of(Map.of(SearchService.SORT_ITEM, "bogus",
SearchService.SORT_ORDER, "asc"));
+ List<SortClause> sort = List.of(new SortClause("bogus", "asc"));
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")));
@@ -187,8 +185,7 @@ class SearchServiceTest {
void search_WithSortClauses_ShouldApplySorting() throws Exception {
SolrClient mockClient = mock(SolrClient.class);
QueryResponse mockResponse = mock(QueryResponse.class);
- List<Map<String, String>> sortClauses = List.of(Map.of("item",
"price", "order", "asc"),
- Map.of("item", "name", "order", "desc"));
+ List<SortClause> sortClauses = List.of(new SortClause("price",
"asc"), new SortClause("name", "desc"));
SolrDocumentList mockDocuments = createMockDocumentList();
when(mockResponse.getResults()).thenReturn(mockDocuments);
when(mockResponse.getFacetFields()).thenReturn(null);
@@ -225,7 +222,7 @@ class SearchServiceTest {
String query = "title:Java";
List<String> filterQueries = List.of("inStock:true");
List<String> facetFields = List.of("category");
- List<Map<String, String>> sortClauses = List.of(Map.of("item",
"price", "order", "asc"));
+ List<SortClause> sortClauses = List.of(new SortClause("price",
"asc"));
Integer start = 0;
Integer rows = 10;
SolrDocumentList mockDocuments = createMockDocumentList();
diff --git
a/src/test/java/org/apache/solr/mcp/server/search/SortClauseTest.java
b/src/test/java/org/apache/solr/mcp/server/search/SortClauseTest.java
new file mode 100644
index 0000000..bcbfac2
--- /dev/null
+++ b/src/test/java/org/apache/solr/mcp/server/search/SortClauseTest.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.search;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.apache.solr.client.solrj.request.SolrQuery;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for {@link SortClause} conversion to the SolrJ representation.
+ */
+class SortClauseTest {
+
+ @Test
+ void convertsFieldAndOrder() {
+ SolrQuery.SortClause solr = new SortClause("price",
"desc").toSolrSortClause();
+ assertEquals("price", solr.getItem());
+ assertEquals(SolrQuery.ORDER.desc, solr.getOrder());
+ }
+
+ @Test
+ void normalizesOrderCase() {
+ assertEquals(SolrQuery.ORDER.desc, new SortClause("price",
"DESC").toSolrSortClause().getOrder());
+ }
+
+ @Test
+ void defaultsMissingOrderToAscending() {
+ assertEquals(SolrQuery.ORDER.asc, new SortClause("price",
null).toSolrSortClause().getOrder());
+ assertEquals(SolrQuery.ORDER.asc, new SortClause("price", "
").toSolrSortClause().getOrder());
+ }
+
+ @Test
+ void rejectsInvalidOrderNamingTheValue() {
+ IllegalArgumentException e =
assertThrows(IllegalArgumentException.class,
+ () -> new SortClause("price",
"descending").toSolrSortClause());
+ assertTrue(e.getMessage().contains("descending"));
+ assertTrue(e.getMessage().contains("price"));
+ }
+
+ @Test
+ void rejectsMissingField() {
+ assertThrows(IllegalArgumentException.class, () -> new
SortClause(null, "asc").toSolrSortClause());
+ assertThrows(IllegalArgumentException.class, () -> new
SortClause(" ", "asc").toSolrSortClause());
+ }
+}
