Skip to content

fix(config): keep empty facet arrays as NamedList, not List - #175

Merged
epugh merged 2 commits into
apache:mainfrom
adityamparikh:port/empty-facet-upstream
Sep 11, 2026
Merged

epugh merged 2 commits into
apache:mainfrom
adityamparikh:port/empty-facet-upstream

Conversation

@adityamparikh

Copy link
Copy Markdown
Contributor

JsonResponseParser classifies JSON arrays purely by shape: an array of
alternating [String, non-String, ...] pairs becomes a NamedList, anything
else becomes a List. An empty array has no shape to inspect, so a facet on a
field that matched zero documents is converted to an empty List.

SolrJ's QueryResponse casts every facet_counts/facet_fields entry to
NamedList, so that produces a ClassCastException whenever a faceted field
has no matches
— a plausible query, not an edge case:

{"facet_counts":{"facet_fields":{"genre":[],"author":["asimov",3]}}}
                                          ^^ empty -> List -> CCE

Fix

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 existing
shape heuristic still covers the other flat-NamedList sites.

Tests

Adds JsonResponseParserTest (5 cases), covering populated facets, the empty
facet, empty/plain arrays outside facet_fields (which must stay List),
and an end-to-end case that feeds the parsed response into a real
QueryResponse.

Verified against main: reverting only the JsonResponseParser change while
keeping the new test fails 2 of the 5 cases, one of them with the
ClassCastException above. With the fix, all 5 pass alongside the existing
JsonResponseParserContentTypesTest.

./gradlew spotlessCheck test is green.

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
@adityamparikh
adityamparikh force-pushed the port/empty-facet-upstream branch from 87a3552 to 1755472 Compare August 19, 2026 11:57
@adityamparikh

Copy link
Copy Markdown
Contributor Author

Rebased onto main (a84033b).

One conflict, in JsonResponseParser.convertValue: main added a @Nullable return annotation (jspecify rollout) on the same signature this PR threads a path parameter through. Kept both — private @Nullable Object convertValue(JsonNode node, String path).

No other changes; the fix and its tests are as reviewed. Verified: ./gradlew build green on JDK 25.

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 apache#185, which duplicated this PR and is being closed in its favour.

Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
@adityamparikh

Copy link
Copy Markdown
Contributor Author

Added an end-to-end test and closed a duplicate PR in this one's favour.

Duplicate: I opened #185 against the same bug without spotting this PR — my duplicate check
searched issues, not pull requests. #185 is now closed, and issue #182 (which I filed for the
same defect) points here.

Comparing the two implementations, this one is better and is what should merge. It anchors on
the full path facet_counts/facet_fields/<field>, so a key named facet_fields appearing
anywhere else in a response cannot be misread. #185 matched the bare key at any depth. #185 also
covered facet_queries and facet_intervals, but that turned out to be dead weight — Solr emits
both as JSON objects, which already decode correctly through convertObject.

Ported across (e4d0646): the one thing #185 had that this PR didn't — an integration test
against real Solr via Testcontainers.

The existing JsonResponseParserTest pins decoding at the parser boundary with a hand-written
payload, which is the right place for it. But that leaves one assumption unverified: that a real
Solr genuinely emits [] for a zero-hit facet. If Solr ever emitted {} instead, those unit
tests would keep passing while the bug they guard had stopped existing in that form. The new test
closes that gap by faceting a filter designed to match nothing and asserting an empty facet map
comes back.

Verified rather than assumed: reverting only the JsonResponseParser change on this branch and
re-running makes the new test fail with java.lang.ClassCastException. With the fix it passes.
Full build is 378 tests, 0 failures, 7 skipped.

@epugh
epugh merged commit 9b1d0e7 into apache:main Sep 11, 2026
4 checks passed
adityamparikh added a commit to adityamparikh/solr-mcp that referenced this pull request Sep 11, 2026
Resolves the conflicts between main and the Spring Boot 4 / Spring AI 2
upgrade, keeping the Boot 4 line throughout:

- build.gradle.kts, libs.versions.toml: keep spring-boot-starter-opentelemetry,
  mcp-server-security 0.1.14 and the MCP SDK 2.0.1 pin; add CommonMark for
  markdown indexing (apache#144). JSpecify stays transitive (built into Boot 4).
- Service classes: keep the Spring AI 2 annotation package
  (org.springframework.ai.mcp.annotation) and take the SLF4J loggers from apache#110.
- JsonResponseParser: Jackson 3 API (properties(), JacksonException) combined
  with the facet-path tracking from apache#175.
- JsonUtils, IndexingServiceIntegrationTest and the new JsonResponseParserTest:
  Jackson 3 (tools.jackson) instead of com.fasterxml; MarkdownDocumentCreator
  wired into the indexing test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQhKFCmP75K71UV7CiYTqB
Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
@adityamparikh
adityamparikh deleted the port/empty-facet-upstream branch October 7, 2026 02:13
adityamparikh added a commit to adityamparikh/solr-mcp that referenced this pull request Oct 12, 2026
… build

The coc2026 image, native images and JAR are now built from the Spring
Boot 4.1 / Spring AI 2.0 line (apache#23), which carries
everything on main, so the card describes what that build does:

- Fifteen tools, with the three alias tools in the toolbox table.
- Step 3: CSV, XML and Markdown samples come from main's
  src/test/resources. CSV and XML go to Solr's own handlers, which
  report the payload accepted instead of a count. XML is Solr update
  XML, so its field names match JSON and the show_ prefix notes go;
  the Option B XML paste is converted to <add><doc> form. Markdown is
  one multi-record file and one tool call; its description is in
  content, which the step 6 note now says.
- Step 4: one outcome, an empty facet (apache#175 is in the image).
- Native Docker images are prebuilt for amd64 and arm64.
- Observability: OTLP/HTTP on 4318 with a URL per signal,
  ClassName#method span names, timers in milliseconds, JVM metrics over
  OTLP rather than /actuator/prometheus.
- Security: protected resource metadata for /mcp is served at the
  RFC 9728 path-suffixed URL.
- Sizes (52 MB JAR, ~170 MB image) and the verified-against footer.
- Table cards: fifteen tools, and ChatGPT in the client list.

Every step was replayed against the published image for all four
formats, including the Option B pastes, the observability panel against
grafana/otel-lgtm 0.33.0, and the security panel against Keycloak.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Aditya Parikh <aditya.m.parikh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants