SOLR-18175: Return BAD_REQUEST for FieldExistsQuery IllegalStateException by openworld-maker · Pull Request #4369 · apache/solr

openworld-maker · 2026-04-27T04:08:29Z

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)

…tion

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 · 2026-04-27T11:40:13Z

+    Method method =
+        QueryComponent.class.getDeclaredMethod(
+            "doProcessUngroupedSearch", ResponseBuilder.class, QueryCommand.class);
+    method.setAccessible(true);
+    method.invoke(new QueryComponent(), rb, cmd);


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.

Addressed by replacing the reflection-heavy test entirely with a real request-driven regression test in solr/core/src/test/org/apache/solr/search/FieldExistsQueryBadRequestTest.java; no private-method reflection remains.

Copilot · 2026-04-27T11:40:14Z

+      } catch (IllegalStateException e) {
+        maybeThrowBadRequestForFieldExistsQueryFailure(e);
+        throw e;


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.

copilot is rightfully suspicious.

epugh · 2026-04-27T11:51:00Z

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.

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!

dsmiley

It'd be nice to see if this is fixed in the context of a facet.query as well.

dsmiley · 2026-05-06T04:04:26Z

 */
 public class QueryComponent extends SearchComponent {
  public static final String COMPONENT_NAME = "query";
+  private static final String FIELD_EXISTS_QUERY_REQUIRES_PREFIX = "FieldExistsQuery requires";


just inline this please; the constant isn't useful

dsmiley · 2026-05-06T04:08:05Z

+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class QueryComponentFieldExistsQueryErrorHandlingTest extends SolrTestCaseJ4 {


I don't trust Mocking to reflect the real world we want, not to mention it can be rather invasive as this test shows. And invasiveness leads to a bigger maintenance burden over time. Can you not provoke the problem as reported in a realistic test? I can try to help if needed or we just provoke it in a manual way and not even have an automated test. It's questionable if the problem is worth an extensive test.

Addressed by replacing the mock-based approach with a realistic regression test in solr/core/src/test/org/apache/solr/search/FieldExistsQueryBadRequestTest.java. The fix is now validated through normal request handling rather than private-method mocking.

dsmiley · 2026-05-06T04:11:00Z

+      } catch (IllegalStateException e) {
+        maybeThrowBadRequestForFieldExistsQueryFailure(e);
+        throw e;


copilot is rightfully suspicious.

dsmiley · 2026-05-06T04:15:28Z

I was hoping for a very different implementation... just modify org.apache.solr.schema.FieldType.getExistenceQuery to have similar checks that FieldExistsQuery has but done here so eagerly at the query parsing stage. Assuming this fixes the problem of course... see if we can provoke what I saw (prompting me to file the issue)

SOLR-18175: Return BAD_REQUEST for FieldExistsQuery IllegalStateExcep…

9c7b6ec

…tion

github-actions Bot added tests cat:search labels Apr 27, 2026

epugh requested a review from Copilot April 27, 2026 11:36

Copilot started reviewing on behalf of epugh April 27, 2026 11:37 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

epugh requested a review from dsmiley April 27, 2026 11:45

dsmiley requested changes May 6, 2026

View reviewed changes

SOLR-18175: handle FieldExistsQuery errors earlier

9454fd2

github-actions Bot added the cat:schema label May 10, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-18175: Return BAD_REQUEST for FieldExistsQuery IllegalStateException#4369

SOLR-18175: Return BAD_REQUEST for FieldExistsQuery IllegalStateException#4369
openworld-maker wants to merge 2 commits intoapache:mainfrom
openworld-maker:pr-18175-local

openworld-maker commented Apr 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

openworld-maker May 10, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

dsmiley May 6, 2026

Uh oh!

epugh commented Apr 27, 2026

Uh oh!

dsmiley left a comment

Uh oh!

dsmiley May 6, 2026

Uh oh!

dsmiley May 6, 2026

Uh oh!

openworld-maker May 10, 2026

Uh oh!

dsmiley May 6, 2026

Uh oh!

dsmiley commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

openworld-maker commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this fixes

Root cause

Change in this PR

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

openworld-maker May 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

dsmiley May 6, 2026

Choose a reason for hiding this comment

Uh oh!

epugh commented Apr 27, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

dsmiley May 6, 2026

Choose a reason for hiding this comment

Uh oh!

dsmiley May 6, 2026

Choose a reason for hiding this comment

Uh oh!

openworld-maker May 10, 2026

Choose a reason for hiding this comment

Uh oh!

dsmiley May 6, 2026

Choose a reason for hiding this comment

Uh oh!

dsmiley commented May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openworld-maker commented Apr 27, 2026 •

edited

Loading