-
Notifications
You must be signed in to change notification settings - Fork 809
SOLR-18085: Review use of "wt=standard" and see if we can remove it. #4080
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
99a5114
d976a1a
63d4284
90dd987
947464b
5753e33
c76ad67
c9e9bea
951e21e
5194e8b
8cbba6a
53b11b0
aa15e40
441ae7a
37bdfab
efbb007
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | ||
| title: Removed the wt=standard concept that was used internally by Solr. | ||
| type: removed # added, changed, fixed, deprecated, removed, dependency_update, security, other | ||
| authors: | ||
| - name: Eric Pugh | ||
| links: | ||
| - name: SOLR-18085 | ||
| url: https://issues.apache.org/jira/browse/SOLR-18085 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,9 @@ | |
| import static org.apache.solr.util.stats.MetricUtils.PROMETHEUS_METRICS_WT; | ||
|
|
||
| import java.util.Map; | ||
| import org.apache.solr.common.SolrException; | ||
| import org.apache.solr.common.params.CommonParams; | ||
| import org.apache.solr.core.SolrCore; | ||
| import org.apache.solr.handler.admin.api.ReplicationAPIBase; | ||
|
|
||
| /** | ||
|
|
@@ -46,21 +48,14 @@ private ResponseWritersRegistry() { | |
| PrometheusResponseWriter prometheusWriter = new PrometheusResponseWriter(); | ||
|
|
||
| BUILTIN_WRITERS = | ||
| Map.of( | ||
| CommonParams.JAVABIN, | ||
| new JavaBinResponseWriter(), | ||
| CommonParams.JSON, | ||
| jsonWriter, | ||
| "standard", | ||
| jsonWriter, // Alias for JSON | ||
| "xml", | ||
| new XMLResponseWriter(), | ||
| PROMETHEUS_METRICS_WT, | ||
| prometheusWriter, | ||
| OPEN_METRICS_WT, | ||
| prometheusWriter, | ||
| ReplicationAPIBase.FILE_STREAM, | ||
| new FileStreamResponseWriter()); | ||
| Map.ofEntries( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice and tidy :-) |
||
| Map.entry(CommonParams.JAVABIN, new JavaBinResponseWriter()), | ||
| Map.entry(CommonParams.JSON, jsonWriter), | ||
| Map.entry("xml", new XMLResponseWriter()), | ||
| Map.entry("raw", new RawResponseWriter()), | ||
| Map.entry(PROMETHEUS_METRICS_WT, prometheusWriter), | ||
| Map.entry(OPEN_METRICS_WT, prometheusWriter), | ||
| Map.entry(ReplicationAPIBase.FILE_STREAM, new FileStreamResponseWriter())); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -69,17 +64,68 @@ private ResponseWritersRegistry() { | |
| * <p>Built-in writers are always available and provide essential formats needed by admin APIs and | ||
| * core functionality. They do not depend on core configuration or ImplicitPlugins.json settings. | ||
| * | ||
| * <p>If the requested writer is not available, returns the "standard" (JSON) writer as a | ||
| * fallback. This ensures requests always get a valid response format. | ||
| * <p>If the requested writer is not available, returns the JSON writer as a fallback. This | ||
| * ensures requests always get a valid response format. | ||
| * | ||
| * @param writerName the writer name (e.g., "json", "xml", "javabin"), or null for default | ||
| * @return the response writer, never null (returns "standard"/JSON if not found) | ||
| * @return the response writer, never null (returns JSON if not found) | ||
epugh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| */ | ||
| public static QueryResponseWriter getWriter(String writerName) { | ||
| if (writerName == null || writerName.isEmpty()) { | ||
| return BUILTIN_WRITERS.get("standard"); | ||
| writerName = CommonParams.JSON; | ||
| } | ||
| return BUILTIN_WRITERS.getOrDefault(writerName, BUILTIN_WRITERS.get("standard")); | ||
| return BUILTIN_WRITERS.get(writerName); | ||
| } | ||
|
|
||
| /** | ||
| * Gets a response writer, trying the core's registry first, then falling back to built-in | ||
| * writers. This is the unified entry point for all writer resolution. | ||
| * | ||
| * <p>Resolution order: | ||
| * | ||
| * <ol> | ||
| * <li>If core is provided, check core's writer registry | ||
| * <li>If not found in core (or no core), check built-in writers | ||
| * <li>If writer name is explicitly specified but not found anywhere, throw exception | ||
| * <li>If writer name is null/empty, return default (JSON) | ||
| * </ol> | ||
| * | ||
| * @param writerName the writer name (e.g., "json", "xml", "javabin"), or null for default | ||
| * @param core the SolrCore to check first, or null for node-level requests | ||
| * @return the response writer, never null | ||
| * @throws SolrException if an explicitly requested writer type is not found | ||
| */ | ||
| public static QueryResponseWriter getWriter(String writerName, SolrCore core) { | ||
| QueryResponseWriter writer = null; | ||
|
|
||
| // Try core registry first if available | ||
| if (core != null) { | ||
| writer = core.getQueryResponseWriter(writerName); | ||
| } | ||
|
|
||
| // If not found and writer is explicitly requested, validate it exists in built-in | ||
| if (writer == null) { | ||
| if (!hasWriter(writerName)) { | ||
| throw new SolrException( | ||
| SolrException.ErrorCode.SERVER_ERROR, "Unknown response writer type: " + writerName); | ||
| } else { | ||
| writer = getWriter(writerName); | ||
| } | ||
| } | ||
| return writer; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a writer with the given name exists in the built-in writers. | ||
| * | ||
| * @param writerName the writer name to check | ||
| * @return true if the writer exists, false otherwise | ||
| */ | ||
| public static boolean hasWriter(String writerName) { | ||
| if (writerName == null || writerName.isEmpty()) { | ||
| return true; // null/empty is valid, will use default | ||
| } | ||
| return BUILTIN_WRITERS.containsKey(writerName); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -727,7 +727,7 @@ private void handleAdminRequest() throws IOException { | |
|
|
||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if (log.isInfoEnabled()) { | ||
| log.info( | ||
| handler != null | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these changes in-scope? |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -67,22 +67,14 @@ public class SolrRequestParsers { | |||||
|
|
||||||
| private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); | ||||||
|
|
||||||
| // Should these constants be in a more public place? | ||||||
| public static final String MULTIPART = "multipart"; | ||||||
| public static final String FORMDATA = "formdata"; | ||||||
| public static final String RAW = "raw"; | ||||||
| public static final String SIMPLE = "simple"; | ||||||
| public static final String STANDARD = "standard"; | ||||||
|
|
||||||
| private static final Charset CHARSET_US_ASCII = StandardCharsets.US_ASCII; | ||||||
|
|
||||||
| public static final String INPUT_ENCODING_KEY = "ie"; | ||||||
| private static final byte[] INPUT_ENCODING_BYTES = INPUT_ENCODING_KEY.getBytes(CHARSET_US_ASCII); | ||||||
|
|
||||||
| public static final String REQUEST_TIMER_SERVLET_ATTRIBUTE = "org.apache.solr.RequestTimer"; | ||||||
|
|
||||||
| private final HashMap<String, SolrRequestParser> parsers = new HashMap<>(); | ||||||
| private StandardRequestParser standard; | ||||||
| private StandardRequestParser parser; | ||||||
|
|
||||||
| /** | ||||||
| * Default instance for e.g. admin requests. Limits to 2 MB uploads and does not allow remote | ||||||
|
|
@@ -91,7 +83,7 @@ public class SolrRequestParsers { | |||||
| public static final SolrRequestParsers DEFAULT = new SolrRequestParsers(); | ||||||
|
|
||||||
| /** | ||||||
| * 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. | ||||||
|
||||||
| * 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initWriters()no longer ensures a default response writer is set onresponseWriters. 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 tojson) whenresponseWriters.getDefault()is null after init.