From 17554721a51a15d422e7b2b557314827e7001d9b Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Sun, 2 Aug 2026 12:01:33 -0400 Subject: [PATCH 1/2] fix(config): keep empty facet arrays as NamedList, not List JsonResponseParser classified arrays purely by shape: an array of [String, non-String, ...] pairs became a NamedList, anything else a List. An empty array has no shape to inspect, so a facet on a field that matched zero documents was converted to an empty List. SolrJ's QueryResponse casts every facet_counts/facet_fields entry to NamedList, so that produced a ClassCastException whenever a faceted field had no matches - a plausible query, not an edge case. Give the traversal positional context instead of guessing: thread the node path through toNamedList/convertValue and treat anything directly under facet_counts/facet_fields as a NamedList regardless of shape. The shape heuristic still covers other flat-NamedList sites. Adds JsonResponseParserTest, which was confirmed to fail without this change (2 of its 5 cases) and to pass with it, including an end-to-end case that feeds the parsed response into a real QueryResponse. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Aditya Parikh --- .../mcp/server/config/JsonResponseParser.java | 51 ++++++-- .../server/config/JsonResponseParserTest.java | 123 ++++++++++++++++++ 2 files changed, 161 insertions(+), 13 deletions(-) create mode 100644 src/test/java/org/apache/solr/mcp/server/config/JsonResponseParserTest.java diff --git a/src/main/java/org/apache/solr/mcp/server/config/JsonResponseParser.java b/src/main/java/org/apache/solr/mcp/server/config/JsonResponseParser.java index 545d5e87..8e50490c 100644 --- a/src/main/java/org/apache/solr/mcp/server/config/JsonResponseParser.java +++ b/src/main/java/org/apache/solr/mcp/server/config/JsonResponseParser.java @@ -88,22 +88,33 @@ public Collection getContentTypes() { return List.of(MediaType.APPLICATION_JSON_VALUE, MediaType.TEXT_PLAIN_VALUE); } + /** + * Path of the object whose direct children are per-field facet arrays. Arrays + * found one level below this path are always NamedLists, regardless of shape. + */ + private static final String FACET_FIELDS_PATH = "facet_counts/facet_fields"; + @Override public NamedList processResponse(InputStream body, String encoding) { try { - return toNamedList(mapper.readTree(body)); + return toNamedList(mapper.readTree(body), ""); } catch (IOException e) { throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Failed to parse Solr JSON response", e); } } - private SimpleOrderedMap toNamedList(JsonNode objectNode) { + private SimpleOrderedMap toNamedList(JsonNode objectNode, String path) { SimpleOrderedMap result = new SimpleOrderedMap<>(); - objectNode.fields().forEachRemaining(entry -> result.add(entry.getKey(), convertValue(entry.getValue()))); + objectNode.fields().forEachRemaining( + entry -> result.add(entry.getKey(), convertValue(entry.getValue(), child(path, entry.getKey())))); return result; } - private @Nullable Object convertValue(JsonNode node) { + private static String child(String path, String key) { + return path.isEmpty() ? key : path + "/" + key; + } + + private @Nullable Object convertValue(JsonNode node, String path) { if (node.isNull()) return null; if (node.isBoolean()) @@ -117,21 +128,28 @@ private SimpleOrderedMap toNamedList(JsonNode objectNode) { if (node.isDouble() || node.isFloat()) return node.floatValue(); if (node.isObject()) - return convertObject(node); + return convertObject(node, path); if (node.isArray()) - return convertArray(node); + return convertArray(node, path); return node.asText(); } - private Object convertObject(JsonNode node) { + private Object convertObject(JsonNode node, String path) { // Detect a Solr query result set by the presence of numFound + docs if (node.has("numFound") && node.has("docs")) { return toSolrDocumentList(node); } - return toNamedList(node); + return toNamedList(node, path); } - private Object convertArray(JsonNode arrayNode) { + private Object convertArray(JsonNode arrayNode, String path) { + // Facet field values are always NamedLists, even when empty. The shape + // heuristic below cannot recognise an empty array, and returning a List + // for a facet on a zero-hit field would break SolrJ's QueryResponse, + // which casts each facet_fields entry to NamedList. + if (isFacetFieldValue(path)) { + return flatArrayToNamedList(arrayNode); + } // Detect Solr's flat NamedList encoding: [String, non-String, String, // non-String, ...] // Used for facet counts (json.nl=flat default). Distinguished from plain string @@ -141,10 +159,16 @@ private Object convertArray(JsonNode arrayNode) { return flatArrayToNamedList(arrayNode); } List list = new ArrayList<>(arrayNode.size()); - arrayNode.forEach(element -> list.add(convertValue(element))); + arrayNode.forEach(element -> list.add(convertValue(element, path))); return list; } + /** True for {@code facet_counts/facet_fields/}. */ + private static boolean isFacetFieldValue(String path) { + int lastSlash = path.lastIndexOf('/'); + return lastSlash > 0 && path.substring(0, lastSlash).equals(FACET_FIELDS_PATH); + } + /** * Returns true when the array has even length, every even-indexed element is a * string (the key), and every odd-indexed element is NOT a string (the value). @@ -168,7 +192,8 @@ private boolean isFlatNamedList(JsonNode arrayNode) { private SimpleOrderedMap flatArrayToNamedList(JsonNode arrayNode) { SimpleOrderedMap result = new SimpleOrderedMap<>(); for (int i = 0; i < arrayNode.size(); i += 2) { - result.add(arrayNode.get(i).textValue(), convertValue(arrayNode.get(i + 1))); + // Values here are facet counts (scalars), never nested facet arrays. + result.add(arrayNode.get(i).textValue(), convertValue(arrayNode.get(i + 1), "")); } return result; } @@ -196,10 +221,10 @@ private SolrDocument toSolrDocument(JsonNode node) { if (val.isArray()) { // Multi-valued field — always a plain list, never a flat NamedList List values = new ArrayList<>(val.size()); - val.forEach(v -> values.add(convertValue(v))); + val.forEach(v -> values.add(convertValue(v, ""))); doc.setField(entry.getKey(), values); } else { - doc.setField(entry.getKey(), convertValue(val)); + doc.setField(entry.getKey(), convertValue(val, "")); } }); return doc; diff --git a/src/test/java/org/apache/solr/mcp/server/config/JsonResponseParserTest.java b/src/test/java/org/apache/solr/mcp/server/config/JsonResponseParserTest.java new file mode 100644 index 00000000..1b70cb3c --- /dev/null +++ b/src/test/java/org/apache/solr/mcp/server/config/JsonResponseParserTest.java @@ -0,0 +1,123 @@ +/* + * 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.config; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.fasterxml.jackson.databind.ObjectMapper; +import java.io.ByteArrayInputStream; +import java.nio.charset.StandardCharsets; +import org.apache.solr.client.solrj.response.QueryResponse; +import org.apache.solr.common.util.NamedList; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link JsonResponseParser}'s conversion of Solr's JSON wire + * format into the {@link NamedList} tree SolrJ expects. + * + *

+ * The facet cases are regression coverage: SolrJ's {@link QueryResponse} casts + * every {@code facet_counts/facet_fields} entry to a {@link NamedList}, so an + * empty facet array must not be converted to a {@link java.util.List}. + */ +class JsonResponseParserTest { + + private final JsonResponseParser parser = new JsonResponseParser(new ObjectMapper()); + + private NamedList parse(String json) { + return parser.processResponse(new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8)), "UTF-8"); + } + + @SuppressWarnings("unchecked") + private static NamedList facetFields(NamedList response) { + NamedList facetCounts = (NamedList) response.get("facet_counts"); + return (NamedList) facetCounts.get("facet_fields"); + } + + @Test + @DisplayName("populated facet array converts to a NamedList of counts") + void populatedFacetBecomesNamedList() { + NamedList response = parse(""" + {"facet_counts":{"facet_fields":{"genre":["fantasy",10,"scifi",5]}}} + """); + + Object genre = facetFields(response).get("genre"); + NamedList counts = assertInstanceOf(NamedList.class, genre); + assertEquals(2, counts.size()); + assertEquals(10, counts.get("fantasy")); + assertEquals(5, counts.get("scifi")); + } + + @Test + @DisplayName("empty facet array still converts to a NamedList, not a List") + void emptyFacetBecomesEmptyNamedList() { + // A facet on a field where nothing matched. The shape heuristic cannot + // recognise [] as a flat NamedList, so position in the tree must decide. + NamedList response = parse(""" + {"facet_counts":{"facet_fields":{"genre":[]}}} + """); + + Object genre = facetFields(response).get("genre"); + NamedList counts = assertInstanceOf(NamedList.class, genre); + assertEquals(0, counts.size()); + } + + @Test + @DisplayName("empty array outside facet_fields stays a List") + void emptyArrayElsewhereStaysList() { + NamedList response = parse(""" + {"responseHeader":{"warnings":[]}} + """); + + @SuppressWarnings("unchecked") + NamedList header = (NamedList) response.get("responseHeader"); + assertInstanceOf(java.util.List.class, header.get("warnings")); + } + + @Test + @DisplayName("plain string array is not mistaken for a flat NamedList") + void plainStringArrayStaysList() { + NamedList response = parse(""" + {"responseHeader":{"fields":["col1","col2"]}} + """); + + @SuppressWarnings("unchecked") + NamedList header = (NamedList) response.get("responseHeader"); + assertInstanceOf(java.util.List.class, header.get("fields")); + } + + @Test + @DisplayName("QueryResponse can read facets when one field has zero matches") + void queryResponseHandlesEmptyFacet() { + // End-to-end guard: this is the cast that used to throw ClassCastException. + NamedList response = parse(""" + {"responseHeader":{"status":0,"QTime":1}, + "response":{"numFound":0,"start":0,"docs":[]}, + "facet_counts":{"facet_fields":{"genre":[],"author":["asimov",3]}}} + """); + + QueryResponse queryResponse = new QueryResponse(); + queryResponse.setResponse(response); + + assertEquals(2, queryResponse.getFacetFields().size()); + assertTrue(queryResponse.getFacetField("genre").getValues().isEmpty()); + assertEquals(1, queryResponse.getFacetField("author").getValues().size()); + } +} From e4d0646fc9feb60552fd901a163c8aab915945e7 Mon Sep 17 00:00:00 2001 From: Aditya Parikh Date: Thu, 20 Aug 2026 10:59:42 -0400 Subject: [PATCH 2/2] test(search): cover the empty facet end to end against real Solr JsonResponseParserTest pins the decoding at the parser boundary using a hand-written payload. That leaves one assumption untested: that a real Solr actually emits [] for a facet on a zero-hit query. If Solr ever emitted {} instead, the unit tests would keep passing while the bug they guard no longer matched reality. Adds the end-to-end counterpart via Testcontainers: facet a filter designed to match nothing, and assert an empty facet map comes back rather than an exception. Verified by reverting only the JsonResponseParser change on this branch and re-running: the test fails with java.lang.ClassCastException. With the fix it passes, and the full build is 378 tests, 0 failures. Ported from #185, which duplicated this PR and is being closed in its favour. Signed-off-by: Aditya Parikh --- .../search/SearchServiceIntegrationTest.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) 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 bb3c842c..99fb607a 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 @@ -185,6 +185,31 @@ void testBasicSearch() throws SolrServerException, IOException { assertEquals(10, documents.size()); } + /** + * Zero matches is an ordinary search outcome, not an error. Solr writes an + * empty facet as {@code []}, which must still reach SolrJ as a NamedList — + * {@code QueryResponse.getFacetFields()} casts to one, so a plain list surfaces + * as {@code ClassCastException: ArrayList cannot be cast to + * NamedList} instead of an empty result. + * + *

+ * {@link org.apache.solr.mcp.server.config.JsonResponseParserTest} pins the + * same behaviour at the parser boundary against a hand-written payload. This + * test is the end-to-end counterpart: it proves a real Solr actually emits + * {@code []} for a zero-hit facet, which is the premise the unit tests assume. + */ + @Test + void facetingAQueryThatMatchesNothingReturnsEmptyFacets() throws SolrServerException, IOException { + SearchResponse result = searchService.search(COLLECTION_NAME, "genre_s:no_such_genre_exists", null, + List.of("genre_s"), null, null, 0); + + assertNotNull(result); + assertEquals(0, result.numFound(), "the filter is designed to match nothing"); + assertNotNull(result.facets(), "facets must be present even when nothing matched"); + assertTrue(result.facets().getOrDefault("genre_s", Map.of()).isEmpty(), + () -> "expected no facet buckets, got: " + result.facets().get("genre_s")); + } + /** * 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