Copilot commented on code in PR #4080:
URL: https://github.com/apache/solr/pull/4080#discussion_r2733504901
##########
solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java:
##########
@@ -68,11 +68,11 @@ 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";
+ // 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";
Review Comment:
This change leaves large blocks of commented-out code (public constants and
the parser registry) rather than removing them. Since these are now dead code
paths, it’s better to delete them to avoid confusion and reduce maintenance
overhead (or keep them active if they’re still intended to be supported).
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3094,7 +3094,6 @@ public PluginBag<QueryResponseWriter>
getResponseWriters() {
HashMap<String, QueryResponseWriter> m = new HashMap<>(15, 1);
m.put("xml", new XMLResponseWriter());
m.put(CommonParams.JSON, new JacksonJsonWriter());
Review Comment:
Removing the "standard" response writer alias is a backwards-incompatible
change: existing code/tests in this repo still use wt=standard (e.g.,
solr/core/src/test/org/apache/solr/OutputWriterTest.java sets
lrf.args.put("wt","standard")). If the intent is to deprecate it, consider
keeping the alias (mapping to json) while emitting a deprecation warning, or
update all internal and documented usages before removing it outright.
```suggestion
m.put(CommonParams.JSON, new JacksonJsonWriter());
m.put("standard", new JacksonJsonWriter());
```
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -3154,7 +3153,7 @@ default String getContentType() {
private void initWriters() {
responseWriters.init(DEFAULT_RESPONSE_WRITERS, this);
// configure the default response writer; this one should never be null
- if (responseWriters.getDefault() == null)
responseWriters.setDefault("standard");
+ // if (responseWriters.getDefault() == null)
responseWriters.setDefault("standard");
Review Comment:
The default response writer is no longer being set when none is explicitly
marked default in solrconfig. This can cause core.getQueryResponseWriter(req) /
core.getQueryResponseWriter(null) to return null, which leads to
NullPointerExceptions in call sites that assume a non-null default (e.g.,
GeoTransformerFactory uses req.getCore().getQueryResponseWriter(req) and then
calls getClass() on it). Consider restoring a non-null default (likely "json"
now that "standard" is being removed) and keep unknown-`wt` validation separate
from default selection.
```suggestion
if (responseWriters.getDefault() == null) {
// Use "json" as the default writer now that the legacy "standard"
writer is being removed.
responseWriters.setDefault("json");
}
```
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java:
##########
@@ -200,12 +201,55 @@ default QueryResponseWriter getResponseWriter() {
// it's weird this method is here instead of SolrQueryResponse, but it's
practical/convenient
SolrCore core = getCore();
String wt = getParams().get(CommonParams.WT);
+
+ if (wt == null || wt.isEmpty()) {
+ // Fall back to Accept header if wt parameter is not provided
+ wt = getWtFromAcceptHeader();
+ }
+
+ QueryResponseWriter writer;
if (core != null) {
- return core.getQueryResponseWriter(wt);
+ writer = core.getQueryResponseWriter(wt);
} else {
- return SolrCore.DEFAULT_RESPONSE_WRITERS.getOrDefault(
- wt, SolrCore.DEFAULT_RESPONSE_WRITERS.get("standard"));
+ writer = SolrCore.DEFAULT_RESPONSE_WRITERS.get(wt);
+ }
+ if (writer == null) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unknown response writer type:
" + wt);
+ }
+ return writer;
+ }
+
+ /**
+ * Maps the HTTP Accept header to a wt parameter value. Returns "json" as
default if no Accept
+ * header is present or if the content type is not recognized.
+ *
+ * <p>This is a quick implmentation, but it's not pluggable. For example,
how do we support CBOR
+ * without modifying this. In V2ApiUtils.getMediaTypeFromWtParam() we do the
same basic thing. i
+ * also dont' see cbor.
Review Comment:
Typo in comment: "dont'" should be "don't".
```suggestion
* also don't see cbor.
```
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java:
##########
@@ -200,12 +201,55 @@ default QueryResponseWriter getResponseWriter() {
// it's weird this method is here instead of SolrQueryResponse, but it's
practical/convenient
SolrCore core = getCore();
String wt = getParams().get(CommonParams.WT);
+
+ if (wt == null || wt.isEmpty()) {
+ // Fall back to Accept header if wt parameter is not provided
+ wt = getWtFromAcceptHeader();
+ }
+
+ QueryResponseWriter writer;
if (core != null) {
- return core.getQueryResponseWriter(wt);
+ writer = core.getQueryResponseWriter(wt);
Review Comment:
Using core.getQueryResponseWriter(wt) here won’t reliably detect an unknown
`wt`, because SolrCore#getQueryResponseWriter(String) falls back to the
configured default when a writer name isn’t found. That means `writer` may be
non-null even for invalid `wt` values, and the "Unknown response writer"
exception won’t be thrown in configs that have a default writer set. Consider
looking up the writer without default fallback (e.g., via
core.getResponseWriters().get(wt) or a new SolrCore API that does not use the
default) and only using defaults when `wt` is absent.
```suggestion
// Avoid using SolrCore#getQueryResponseWriter(wt) for explicit wt
values, since it
// falls back to the default writer when the name is not found. That
would mask
// unknown wt values and prevent us from throwing an error below.
if (wt == null || wt.isEmpty()) {
// No explicit wt: allow SolrCore to apply its default writer
behavior.
writer = core.getQueryResponseWriter(wt);
} else {
// Explicit wt: look up without default fallback.
writer = core.getResponseWriters().get(wt);
}
```
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java:
##########
@@ -200,12 +201,55 @@ default QueryResponseWriter getResponseWriter() {
// it's weird this method is here instead of SolrQueryResponse, but it's
practical/convenient
SolrCore core = getCore();
String wt = getParams().get(CommonParams.WT);
+
+ if (wt == null || wt.isEmpty()) {
+ // Fall back to Accept header if wt parameter is not provided
+ wt = getWtFromAcceptHeader();
+ }
+
+ QueryResponseWriter writer;
if (core != null) {
- return core.getQueryResponseWriter(wt);
+ writer = core.getQueryResponseWriter(wt);
} else {
- return SolrCore.DEFAULT_RESPONSE_WRITERS.getOrDefault(
- wt, SolrCore.DEFAULT_RESPONSE_WRITERS.get("standard"));
+ writer = SolrCore.DEFAULT_RESPONSE_WRITERS.get(wt);
+ }
+ if (writer == null) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unknown response writer type:
" + wt);
+ }
+ return writer;
+ }
+
+ /**
+ * Maps the HTTP Accept header to a wt parameter value. Returns "json" as
default if no Accept
+ * header is present or if the content type is not recognized.
+ *
+ * <p>This is a quick implmentation, but it's not pluggable. For example,
how do we support CBOR
Review Comment:
Typo in comment: "implmentation" should be "implementation".
```suggestion
* <p>This is a quick implementation, but it's not pluggable. For example,
how do we support CBOR
```
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequest.java:
##########
@@ -200,12 +201,55 @@ default QueryResponseWriter getResponseWriter() {
// it's weird this method is here instead of SolrQueryResponse, but it's
practical/convenient
SolrCore core = getCore();
String wt = getParams().get(CommonParams.WT);
+
+ if (wt == null || wt.isEmpty()) {
+ // Fall back to Accept header if wt parameter is not provided
+ wt = getWtFromAcceptHeader();
+ }
+
+ QueryResponseWriter writer;
if (core != null) {
- return core.getQueryResponseWriter(wt);
+ writer = core.getQueryResponseWriter(wt);
} else {
- return SolrCore.DEFAULT_RESPONSE_WRITERS.getOrDefault(
- wt, SolrCore.DEFAULT_RESPONSE_WRITERS.get("standard"));
+ writer = SolrCore.DEFAULT_RESPONSE_WRITERS.get(wt);
+ }
+ if (writer == null) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unknown response writer type:
" + wt);
+ }
+ return writer;
+ }
+
+ /**
+ * Maps the HTTP Accept header to a wt parameter value. Returns "json" as
default if no Accept
+ * header is present or if the content type is not recognized.
+ *
+ * <p>This is a quick implmentation, but it's not pluggable. For example,
how do we support CBOR
+ * without modifying this. In V2ApiUtils.getMediaTypeFromWtParam() we do the
same basic thing. i
+ * also dont' see cbor.
+ *
Review Comment:
The comment says you “don’t see cbor”, but CBOR is present in
SolrCore.DEFAULT_RESPONSE_WRITERS (key "cbor"). Either update the comment to
match reality, or extend getWtFromAcceptHeader() to map Accept headers for the
supported writers (including CBOR) so the comment and behavior are aligned.
##########
solr/core/src/java/org/apache/solr/core/RequestHandlers.java:
##########
@@ -117,7 +117,8 @@ void initHandlersFromConfig(SolrConfig config) {
modifiedInfos.add(applyInitParams(config, info));
}
handlers.init(Collections.emptyMap(), core, modifiedInfos);
- handlers.alias(handlers.getDefault(), "");
+ // Curious if this is actually needed!
+ // handlers.alias(handlers.getDefault(), "");
Review Comment:
Commenting out the aliasing of the configured default handler to the empty
path ("") can change runtime behavior for configs that set a non-/select
handler as the default. For example,
solr/modules/langid/src/test-files/langid/solr/collection1/conf/solrconfig-languageidentifier.xml
defines a default handler named "search"; without this aliasing, requests to
the core root will be routed to /select instead of the declared default.
Consider restoring alias(handlers.getDefault(), "") (or otherwise ensuring
plugin-bag default takes precedence over the /select fallback).
```suggestion
// Ensure the configured default handler (if any) is aliased to the
empty path ("")
String defaultHandler = handlers.getDefault();
if (defaultHandler != null) {
handlers.alias(defaultHandler, "");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
