SOLR-18175: Return BAD_REQUEST for FieldExistsQuery IllegalStateException

[openworld-maker]

What this fixes

SOLR-18175 reports that some FieldExistsQuery failures are returned as server errors. These should be client errors (400 BAD_REQUEST).

Root cause

During query execution, FieldExistsQuery can throw an IllegalStateException with a message starting with FieldExistsQuery requires ... when the field does not support the required data structures. This exception was not being mapped to BAD_REQUEST in QueryComponent execution paths.

Change in this PR

This PR adds a narrow exception mapping in QueryComponent:

• In grouped execution path
• In ungrouped execution path

If an IllegalStateException (including nested causes) has a message starting with FieldExistsQuery requires, it is rethrown as SolrException(BAD_REQUEST).

All other IllegalStateExceptions are left unchanged.

Tests

Added QueryComponentFieldExistsQueryErrorHandlingTest covering:

• direct FieldExistsQuery requires ... exception -> BAD_REQUEST
• nested-cause FieldExistsQuery requires ... exception -> BAD_REQUEST
• unrelated IllegalStateException -> unchanged behavior

Validation run in Docker included:

• new test class above
• TestSolrQueryParser
• selected QueryComponent/SearchHandler suites
• *QueryComponent* test pattern
• formatting checks (spotlessCheck)

[Copilot]

Pull request overview

Updates Solr’s QueryComponent error handling so that FieldExistsQuery execution failures that currently surface as server errors are instead returned as 400 BAD_REQUEST.

Changes:

• Add targeted mapping for IllegalStateException whose (possibly nested) message starts with FieldExistsQuery requires..., converting it to SolrException(BAD_REQUEST) in both grouped and ungrouped execution paths.
• Introduce a helper method and constant to detect this specific failure signature.
• Add a new unit test class to validate BAD_REQUEST mapping for direct and nested IllegalStateException cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java	Adds the narrow IllegalStateException → BAD_REQUEST remapping for grouped and ungrouped query execution.
solr/core/src/test/org/apache/solr/handler/component/QueryComponentFieldExistsQueryErrorHandlingTest.java	Adds tests for the ungrouped execution path to ensure correct exception mapping behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This test uses reflection with setAccessible(true) to reach a private method, but it isn’t annotated with @SuppressForbidden. The build enables forbidden-apis’ jdk-reflection signatures (see gradle/validation/forbidden-apis.gradle), and similar reflection-based tests in this package suppress it (e.g., AsyncTrackerSemaphoreLeakTest.java:236). Add an appropriate @SuppressForbidden annotation (method or class) with a brief reason to avoid forbidden-apis failures.

[Copilot]

The PR changes error mapping for the grouped execution path (the groupingSpec != null branch), but the added tests only exercise the ungrouped doProcessUngroupedSearch path. Please add a test that triggers the grouped path and asserts the same BAD_REQUEST mapping, to prevent regressions specific to grouped execution.

[epugh]

I have tagged @dsmiley for review. I did look at the tests and they appear very "mock heavy", and we tend as a project to try and minimize the use of mocks where possible, b/c many of our bugs occur when real objects interact with other real objects ;-). There are a lot of mock objects in the tests so I asked copilot about this and got this response:

Yes—very likely. If the tests for this change are “mock-heavy”, you can usually cover this behavior with one of these lower-mock options (often with zero mocks), because the behavior you’re asserting is fundamentally an HTTP/response mapping concern: when Lucene/Solr query parsing/search throws a specific IllegalStateException (FieldExistsQuery “requires …”), Solr should return 400 BAD_REQUEST instead of 500.

1. Best option: a Solr integration-style test (no mocks)
   Write a test that boots a minimal SolrCore (SolrTestCaseJ4-style) with a tiny schema and then issues a request that triggers the FieldExistsQuery requires ... exception. Assert: HTTP status code is 400 (or SolrException code == BAD_REQUEST depending on harness) error message contains the “FieldExistsQuery requires …” text. This approach tests the real wiring through QueryComponent, not an artificial mocked SolrIndexSearcher / ResponseBuilder graph.

Could you take a stab at reducing/removing the use of mocks? I know QueryComponent can be tough to work with directly because of how it sets up state through ResponseBuilders etc... If a non mock version isn't feasible, then it's better to have this test, even if it's brittle!