gerlowskija commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1934494649
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/AbstractUpdateRequest.java:
##########
@@ -21,10 +21,9 @@
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.UpdateParams;
-/** */
public abstract class AbstractUpdateRequest extends
CollectionRequiringSolrRequest<UpdateResponse>
implements IsUpdateRequest {
- protected ModifiableSolrParams params;
+ protected ModifiableSolrParams params = new ModifiableSolrParams(); // TODO
make final
Review Comment:
+1 to make this `final`. Otherwise it's tough to rely on the field being
non-null, since the signature/contract lets folks set it to null.
That's my main concern about this PR in general - I love that we're saying
that `SolrRequest.getParams()` always returns non-null. But I'm unsure how
much confidence we can have in that guarantee while some SolrRequest
implementations offer a `setParams(...)` method that would allow any SolrJ user
to break that promise (in their client side code at least). Maybe javadocs on
the `setParams(...)` methods is enough to address that? Or maybe something
else in this PR already addresses that and I've just missed it?
##########
solr/core/src/java/org/apache/solr/handler/api/V2ApiUtils.java:
##########
@@ -91,9 +91,6 @@ public static void squashIntoNamedListWithoutHeader(
}
public static String getMediaTypeFromWtParam(SolrParams params, String
defaultMediaType) {
- if (params == null) {
Review Comment:
The null-check is still needed here, I think. Some callers of this method
(e.g. MediaTypeOverridingFilter) may not have a SolrParams to pass in (because
they don't have a SolrQueryRequest at all).
If we really want to remove the null-check here, we can, but we'd need to
also go into some of those call-sites and ensure they're passing in an empty
SolrParams where necessary.
I guess those call-sites could check for that case and pass in a
`SolrParams.of()`, but we can't remove the null check here without that.
##########
solr/modules/cross-dc/src/java/org/apache/solr/crossdc/common/MirroredSolrRequest.java:
##########
@@ -247,10 +246,10 @@ public boolean equals(final Object o) {
}
public static void setParams(SolrRequest<?> request, ModifiableSolrParams
params) {
- if (request instanceof MirroredAdminRequest) {
- ((MirroredAdminRequest) request).setParams(params);
- } else if (request instanceof UpdateRequest) {
- ((UpdateRequest) request).setParams(params);
+ if (request instanceof MirroredAdminRequest mReq) {
Review Comment:
[+1] I haven't seen this instanceof-and-declare-variable syntax before -
very cool!
##########
solr/solrj/src/java/org/apache/solr/common/params/SolrParams.java:
##########
@@ -526,4 +526,9 @@ public String toString() {
}
return sb.toString();
}
+
+ /** An empty, immutable SolrParams. */
+ public static SolrParams of() {
Review Comment:
[0] When I see `of()` it reminds me of the various var-arg methods with the
same name that Java's "Collections" classes offer
([List.of](https://docs.oracle.com/javase/9/docs/api/java/util/List.html#of--)).
It's a little jarring to see that we don't accept any args here, and that
this is only for creating empty param-sets. Maybe a name like `emptyParams()`
or `empty()` would convey things more accurately?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/DirectXmlRequest.java:
##########
@@ -20,13 +20,15 @@
import
org.apache.solr.client.solrj.request.RequestWriter.StringPayloadContentWriter;
import org.apache.solr.client.solrj.response.UpdateResponse;
import org.apache.solr.client.solrj.util.ClientUtils;
+import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
/**
* Send arbitrary XML to a request handler
*
* @since solr 1.3
*/
+@Deprecated
Review Comment:
Happy to look the other way in terms of you sneaking a tiny deprecation into
this PR.
But if we're deprecating we should probably also put a user-friendly note
somewhere so that folks have a sense why this went away, and what they should
use instead.
(Not sure where that note should go: in CHANGES.txt's deprecation section?
as a PR comment? in the code itself near this annotation?)
--
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]
