Copilot commented on code in PR #4101:
URL: https://github.com/apache/solr/pull/4101#discussion_r2764528693
##########
solr/core/src/test/org/apache/solr/BasicFunctionalityTest.java:
##########
@@ -785,9 +787,9 @@ public void testLazyField() throws IOException {
DocList dl = ((ResultContext) rsp.getResponse()).getDocList();
DocIterator di = dl.iterator();
Document d1 = req.getSearcher().getDocFetcher().doc(di.nextDoc());
- IndexableField[] values1 = null;
+ IndexableField[] values1;
- // ensure fl field is non lazy, and non-fl field is lazy
+ // ensure fl field are not lazy, and non-fl field is lazy
Review Comment:
Grammar nit in comment: "ensure fl field are not lazy" should be "ensure fl
fields are not lazy" (or "ensure fields in fl are not lazy").
```suggestion
// ensure fl fields are not lazy, and non-fl field is lazy
```
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java:
##########
@@ -209,11 +226,21 @@ public List<CommandOperation> getCommands(boolean
validateInput) {
return CommandOperation.clone(parsedCommands);
}
- protected ValidatingJsonMap getSpec() {
- return null;
- }
-
protected Map<String, JsonSchemaValidator> getValidators() {
return Collections.emptyMap();
}
+
+ private record LocalPrincipal(String user) implements Principal {
+
+ @Override
+ public String getName() {
+ return user;
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ return Objects.equals(this.getClass(), other.getClass())
+ && Objects.equals(this.getName(), ((LocalPrincipal)
other).getName());
+ }
Review Comment:
`LocalPrincipal.equals` is not null-safe: calling `equals(null)` will throw
a NullPointerException due to `other.getClass()`. Since this is a `record`, you
can simply delete the custom `equals` override (letting the generated
implementation handle null/type checks), or rewrite it using an `instanceof`
pattern check that safely handles null.
```suggestion
```
##########
solr/core/src/test/org/apache/solr/handler/admin/TestCollectionAPIs.java:
##########
@@ -163,16 +163,32 @@ public List<CommandOperation> getCommands(boolean
validateInput) {
public Map<String, String> getPathTemplateValues() {
return parts;
}
-
- @Override
- public String getHttpMethod() {
- return method.toString();
- }
};
api.call(req, rsp);
Review Comment:
This test request no longer overrides `getHttpMethod()` and also doesn't
populate `req.getContext().put("httpMethod", ...)`. Since
`SolrQueryRequest#getHttpMethod()`'s default implementation reads and casts the
`httpMethod` context entry, `BaseHandlerApiSupport` will hit an NPE when it
calls `req.getHttpMethod()`. Set `req.getContext().put("httpMethod", method)`
(where `method` is the `SolrRequest.METHOD` enum) or add back a
`getHttpMethod()` override.
##########
solr/core/src/test/org/apache/solr/handler/admin/TestCollectionAPIs.java:
##########
@@ -163,16 +163,32 @@ public List<CommandOperation> getCommands(boolean
validateInput) {
public Map<String, String> getPathTemplateValues() {
return parts;
}
-
- @Override
- public String getHttpMethod() {
- return method.toString();
- }
};
api.call(req, rsp);
return new Pair<>(req, rsp);
}
+ private SolrQueryRequestBase createTestRequest(
+ SolrParams params,
+ Map<String, String> pathTemplateValues,
+ String httpMethod,
+ String requestBody,
+ Api api) {
+ return new SolrQueryRequestBase(null, params) {
+ @Override
+ public List<CommandOperation> getCommands(boolean validateInput) {
+ if (requestBody == null) return Collections.emptyList();
+ return ApiBag.getCommandOperations(
+ new ContentStreamBase.StringStream(requestBody),
api.getCommandSchema(), true);
+ }
+
+ @Override
+ public Map<String, String> getPathTemplateValues() {
+ return pathTemplateValues;
+ }
+ };
+ }
Review Comment:
`createTestRequest(...)` is unused, and it has an unused parameter
(`httpMethod`). Either remove this helper entirely or refactor `makeCall(...)`
to use it (and ensure it sets the request context/overrides needed for
`getHttpMethod()` as well).
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java:
##########
@@ -31,10 +31,10 @@
import org.apache.solr.common.util.JsonSchemaValidator;
import org.apache.solr.common.util.ObjectReleaseTracker;
import org.apache.solr.common.util.SuppressForbidden;
-import org.apache.solr.common.util.ValidatingJsonMap;
import org.apache.solr.core.SolrCore;
import org.apache.solr.schema.IndexSchema;
import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.security.PKIAuthenticationPlugin;
import org.apache.solr.util.RTimerTree;
Review Comment:
`PKIAuthenticationPlugin` is imported but only referenced in Javadoc. If the
build enforces no-unused-imports (common with Checkstyle/Spotless), this will
fail. Either remove the import and fully-qualify it in the `@see`, or reference
the class in code.
--
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]
