SOLR-18085: Review use of "wt=standard" and see if we can remove it. by epugh · Pull Request #4080 · apache/solr

epugh · 2026-01-27T19:12:51Z

https://issues.apache.org/jira/browse/SOLR-18085

Description

The use of wt=standard is odd.. It really means return json formatted code. I dug around some more and we have werid complex logic around this parameter. Digging furthur, I think most of the complexity is not needed.

The one thing that I DID change is that before if you did "wt=fake" then you would get JSON back in many places. And in the V2 api's if you did "wt=fake" you would get a 400 back with an error. I think the right think in either of those cases is a 500 server error.

Summary of 63d4284

I've updated the code so that when an invalid/unknown wt parameter is provided, Solr returns a 500 Server Error instead of silently falling back or returning a null writer.

Changes Made to default behavior:

SolrQueryRequest.java (lines 207-222):
- Added validation that throws a SolrException with SERVER_ERROR (500) when the response writer is not found for the given wt parameter
- The error message clearly states: "Unknown response writer type: {wt}"
V2ApiIntegrationTest.java:
- Split the original testWTParam test into two tests:
  - testInvalidWTParamReturnsError - verifies that wt=bleh returns a 500 error with the appropriate message
  - testWTParam - verifies that when no wt is specified, JSON is used by default
TestPrometheusResponseWriter.java:
- Updated the comment in testUnsupportedMetricsFormat to clarify that unknown wt parameters return a 500 error
- The test already asserted assertEquals(500, res.get("responseStatus")) which now passes

Solution

Mostly refactoring, except for the big change in 63d4284 which changed the response logic.

Tests

Re ran test

…s to work Some failing tests for plugins like CborResponseWriter that aren't in the content type --> wt converter.

…t found you get a 500 error. I think using a bad responsewriter should be an error, and we shoudln't cover it up, or return json...

solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java

epugh · 2026-01-27T19:17:02Z

solr/core/src/java/org/apache/solr/core/RequestHandlers.java

    handlers.init(Collections.emptyMap(), core, modifiedInfos);
-    handlers.alias(handlers.getDefault(), "");
+    // Curious if this is actually needed!
+    // handlers.alias(handlers.getDefault(), "");


tests all pass!

epugh · 2026-01-27T19:18:07Z

solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java

+      }
    }
+    // Default to JSON when no Accept header or unrecognized content type
+    return CommonParams.JSON;


i remain ambivalent about these... maybe okay? I mean, if I do a curl command don't provide wt=json OR the content types, maybe okay? It might be a perfectly nice thign for our users...

epugh · 2026-01-27T19:18:18Z

solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java

-  public static final String SIMPLE = "simple";
-  public static final String STANDARD = "standard";
+  // public static final String MULTIPART = "multipart";
+  // public static final String FORMDATA = "formdata";


so many unused statics!

epugh · 2026-01-27T19:18:37Z

solr/core/src/test/org/apache/solr/handler/V2ApiIntegrationTest.java

+  public void testInvalidWTParamReturnsError() throws Exception {
    V2Request request = new V2Request.Builder("/c/" + COLL_NAME + "/get/_introspect").build();
-    // TODO: If possible do this in a better way
+    // Using an invalid wt parameter should return a 500 error


This is the Big Change.

Copilot

Pull request overview

Updates Solr’s response-writer (wt) handling to reduce reliance on the legacy "standard" writer and to fail fast when an unknown response writer is requested.

Changes:

Adds stricter wt validation and introduces an Accept-header fallback when wt is absent.
Removes/neutralizes legacy "standard" response-writer wiring and adjusts handler aliasing behavior.
Updates tests and test utilities to reflect the new error behavior and defaults.

Reviewed changes

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

Show a summary per file

File	Description
solr/test-framework/src/java/org/apache/solr/util/TestHarness.java	Forces `wt=xml` in a common local test request path to preserve historical XML expectations.
solr/core/src/test/org/apache/solr/response/TestPrometheusResponseWriter.java	Updates expected status for unknown `wt` to 500.
solr/core/src/test/org/apache/solr/handler/V2ApiIntegrationTest.java	Splits `wt` tests and asserts 500 + message for invalid writer types.
solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java	Comments out legacy constants/registry code (effectively disabling/removing it).
solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java	Adds `Accept` header → `wt` mapping and throws 500 for unknown response writers.
solr/core/src/java/org/apache/solr/core/SolrCore.java	Removes `"standard"` alias from default writers and stops setting a default writer.
solr/core/src/java/org/apache/solr/core/RequestHandlers.java	Stops aliasing the configured default handler to the empty path.

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

solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java

solr/core/src/java/org/apache/solr/core/RequestHandlers.java

solr/core/src/java/org/apache/solr/core/SolrCore.java

solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java

…here.

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (1)

solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java:121

There is a large block of commented-out code in init() referencing removed fields/constants (e.g., parsers, STANDARD, standard). Leaving this dead code around makes the refactor harder to follow and risks it getting stale. Prefer deleting it entirely or replacing it with a brief comment explaining the current design (single StandardRequestParser).


  private static RTimerTree getRequestTimer(HttpServletRequest req) {
    final Object reqTimer = req.getAttribute(REQUEST_TIMER_SERVLET_ATTRIBUTE);
    if (reqTimer instanceof RTimerTree) {
      return ((RTimerTree) reqTimer);
    }

    return new RTimerTree();
  }

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

solr/core/src/test/org/apache/solr/OutputWriterTest.java

Copilot · 2026-02-04T00:52:42Z

solr/core/src/java/org/apache/solr/core/SolrCore.java

@@ -3131,14 +3131,11 @@ private void initWriters() {

    // Initialize with the built defaults
    responseWriters.init(defaultWriters, this);


initWriters() no longer ensures a default response writer is set on responseWriters. If no <queryResponseWriter default="true"> is defined in solrconfig (e.g., configsets where response writers are only implicit), PluginBag's default (def) can remain null, which breaks default writer resolution and can change the default output unexpectedly. Consider restoring a safe default (e.g., set default to json) when responseWriters.getDefault() is null after init.

Suggested change

responseWriters.init(defaultWriters, this);

responseWriters.init(defaultWriters, this);

// Ensure there is always a default response writer configured.

// If no <queryResponseWriter default="true"> is defined and PluginBag

// did not pick a default, fall back to "json" when available.

if (responseWriters.getDefault() == null) {

QueryResponseWriter jsonWriter = responseWriters.get("json", false);

if (jsonWriter != null) {

responseWriters.setDefault("json");

}

}

solr/core/src/java/org/apache/solr/core/SolrCore.java

solr/core/src/java/org/apache/solr/response/ResponseWritersRegistry.java

Copilot · 2026-02-04T00:52:42Z

solr/core/src/test/org/apache/solr/response/TestResponseWritersRegistry.java

+    QueryResponseWriter standardWriter = ResponseWritersRegistry.getWriter("json");

    // Built-in writers should NOT include extended format writers (csv, geojson, etc.)
-    // These should all fall back to standard
-    // I think this standard thing is weird...   I think it should throw an exception!
+    // These should all fall back to json
+    // I think this json thing is weird...   I think it should throw an exception!
    assertThat(
-        "geojson should fall back to standard",
+        "geojson should fall back to json",
        ResponseWritersRegistry.getWriter("geojson"),
-        is(standardWriter));
+        is(nullValue()));


In testBuiltInWriterLimitedSet, the local variable name (standardWriter) and the assertion message/comments say these formats "fall back to json", but the assertion expects nullValue(). Either update the test to assert the intended fallback behavior, or rename/update the comments/messages so they match the new contract (unknown built-in writer returns null).

Copilot · 2026-02-04T00:52:43Z

solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java


  /**
-   * Pass in an xml configuration. A null configuration will enable everything with maximum values.
+   * Pass in a xml configuration. A null configuration will enable everything with maximum values.


Grammar in this Javadoc is off: "Pass in a xml configuration" should be "Pass in an XML configuration" (and typically XML is capitalized).

Suggested change

* Pass in a xml configuration. A null configuration will enable everything with maximum values.

* Pass in an XML configuration. A null configuration will enable everything with maximum values.

Copilot · 2026-02-04T00:52:43Z

solr/core/src/test/org/apache/solr/handler/V2ApiIntegrationTest.java

    NamedList<Object> res = cluster.getSolrClient().request(request);
    String respString = InputStreamResponseParser.consumeResponseToString(res);



This test asserts on substrings like "500" in the raw response body, which is brittle (error pages/formatting can change). Since you already have the NamedList response from InputStreamResponseParser, consider asserting res.get("responseStatus") == 500 (and then optionally assert the message substring) for a more stable check.

changelog/unreleased/SOLR-18085.yml

epugh · 2026-02-04T00:52:57Z

@dsmiley assuming all the tests pass, this is ready for merging.

epugh · 2026-02-04T00:53:14Z

@dsmiley assuming all the tests pass, this is ready for review and potentially merging.

changelog/unreleased/SOLR-18085.yml

dsmiley · 2026-02-04T06:05:16Z

solr/core/src/java/org/apache/solr/response/ResponseWritersRegistry.java

-            prometheusWriter,
-            ReplicationAPIBase.FILE_STREAM,
-            new FileStreamResponseWriter());
+        Map.ofEntries(


nice and tidy :-)

dsmiley · 2026-02-04T06:08:15Z

solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java

  protected void logAndFlushAdminRequest(SolrQueryResponse solrResp) throws IOException {
    if (solrResp.getToLog().size() > 0) {
-      // has to come second and in it's own if to keep ./gradlew check happy.
+      // has to come second and in its own "if" to keep ./gradlew check happy.


minor: I prefer that we don't change source files for out-of-scope reasons that we aren't already touching for in-scope reasons.

yeah, I really struggle with seeing these things and not fixing them. I will try not to do it in the futre. I swear I went through the entire code base and fixed all the typos, and yet I keep finding them.

dsmiley · 2026-02-04T06:10:54Z

solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java

Are these changes in-scope?

dsmiley · 2026-02-04T06:13:30Z

solr/core/src/test/org/apache/solr/response/TestRawResponseWriter.java

what's going on here?

note: commented code is typically avoided

dsmiley · 2026-02-04T06:14:28Z

solr/packaging/test/test_rolling_upgrade.bats

out-of-scope

dsmiley · 2026-02-04T06:15:06Z

solr/test-framework/src/java/org/apache/solr/util/TestHarness.java

    @SuppressWarnings({"unchecked"})
    public LocalSolrQueryRequest makeRequest(String... q) {
      if (q.length == 1) {
+        args.computeIfAbsent("wt", k -> "xml");


hmm... how did this end up failing without this?

Clarified the changelog entry by removing redundant information.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

testSOLR59responseHeaderVersions looks like it tests wt, but the assertQ always sets wt=xml, so it doesn't do anything. We test qtime and status plenty of other places. I was confused by USELESS_OUTPUT till I realized it was part of UselessOutputWriter.

…null raw response writer

epugh added 3 commits January 27, 2026 13:02

Commenting out code and adding a content type --> wt converter appear…

99a5114

…s to work Some failing tests for plugins like CborResponseWriter that aren't in the content type --> wt converter.

don't overwrite the wt if it's been set.

d976a1a

Change the logic so that if you use a bad wt then instead of a 400 no…

63d4284

…t found you get a 500 error. I think using a bad responsewriter should be an error, and we shoudln't cover it up, or return json...

github-actions bot added test-framework jetty-server tests cat:search labels Jan 27, 2026

epugh requested review from Copilot and dsmiley January 27, 2026 19:15

Copilot started reviewing on behalf of epugh January 27, 2026 19:16 View session

epugh commented Jan 27, 2026

View reviewed changes

solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java Outdated Show resolved Hide resolved

epugh commented Jan 27, 2026

View reviewed changes

Copilot AI reviewed Jan 27, 2026

View reviewed changes

epugh added 6 commits January 27, 2026 16:28

json is the new standard

90dd987

No longer supporting a "default" since we specify what we want everyw…

947464b

…here.

Merge remote-tracking branch 'upstream/main' into SOLR-18085

5753e33

fix merge

c76ad67

lots of review and rethinking

c9e9bea

Merge remote-tracking branch 'upstream/main' into SOLR-18085

951e21e

epugh requested a review from Copilot February 4, 2026 00:42

Copilot started reviewing on behalf of epugh February 4, 2026 00:42 View session

epugh added 2 commits February 3, 2026 19:50

use the correct name of this bats test

5194e8b

Reviewing PR, looking to remove commented code and simplify some logic

8cbba6a

Copilot AI reviewed Feb 4, 2026

View reviewed changes

dsmiley reviewed Feb 4, 2026

View reviewed changes

Update changelog for SOLR-18085

53b11b0

Clarified the changelog entry by removing redundant information.

Copilot AI reviewed Feb 4, 2026

View reviewed changes

epugh added 4 commits February 4, 2026 13:50

Merge remote-tracking branch 'upstream/main' into SOLR-18085

aa15e40

back out previous chnage

441ae7a

I dont think this is right but cant prove it. I dont think we need a …

efbb007

…null raw response writer

		@@ -3131,14 +3131,11 @@ private void initWriters() {

		// Initialize with the built defaults
		responseWriters.init(defaultWriters, this);

-    responseWriters.init(defaultWriters, this);
+    responseWriters.init(defaultWriters, this);
+    // Ensure there is always a default response writer configured.
+    // If no <queryResponseWriter default="true"> is defined and PluginBag
+    // did not pick a default, fall back to "json" when available.
+    if (responseWriters.getDefault() == null) {
+      QueryResponseWriter jsonWriter = responseWriters.get("json", false);
+      if (jsonWriter != null) {
+        responseWriters.setDefault("json");
+      }
+    }

	* Pass in a xml configuration. A null configuration will enable everything with maximum values.
	* Pass in an XML configuration. A null configuration will enable everything with maximum values.

		NamedList<Object> res = cluster.getSolrClient().request(request);
		String respString = InputStreamResponseParser.consumeResponseToString(res);

Conversation

epugh commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes Made to default behavior:

Solution

Tests

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

epugh commented Feb 4, 2026

Uh oh!

epugh commented Feb 4, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

epugh commented Jan 27, 2026 •

edited

Loading