Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley merged PR #3140: URL: https://github.com/apache/solr/pull/3140 -- 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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on PR #3140: URL: https://github.com/apache/solr/pull/3140#issuecomment-2640800039 Planning on merging this tomorrow night. LMK if I should hold up. -- 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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1938528244
##
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:
I brought this back and audited with data flow analysis that it'll never be
null.
--
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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on PR #3140: URL: https://github.com/apache/solr/pull/3140#issuecomment-2629459790 Not thinking a JIRA/CHANGES.txt is worth it but will do if asked. Proposed commit message: > Clarify getParams not null in SolrRequest, SolrQueryRequest, QParser > Clarify mutability of params for SolrRequest (SolrJ). UpdateRequest changed; doesn't lazy-create params anymore. > New SolrParams.of() and of(key, value) for an empty or single-pair instance. Planning to merge Tuesday if no further comments. -- 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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1938508213
##
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:
It turns out SolrQueryRequest.getParams will always be non-null (because
`org.apache.solr.servlet.SolrRequestParsers#parseQueryString(java.lang.String)`
is what initializes it, always non-null) but this fact is not documented. I'll
increase the scope here a little to cover that since there's little to it.
--
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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1935063818
##
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
implements IsUpdateRequest {
- protected ModifiableSolrParams params;
+ protected ModifiableSolrParams params = new ModifiableSolrParams(); // TODO
make final
Review Comment:
It's easy for me to enforce that setParams arg isn't null. I'll do that.
setParams is called kinda a lot BTW, although I feel it's usually not needed.
I can do a follow-up draft PR to show you what it looks like if *nobody* calls
it.
--
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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1934927357
##
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:
Coincidentally I recently had the same conversation for the new
SimpleOrderedMap.of.It's modelled off JDK data structure utilities using
this name, thus its consistent. Once we see additional overloads, I think
you'll like it better. I could add one or two in this PR.
--
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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1934925281
##
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:
yeah it's a very nice Java 16 thing
--
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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140: URL: https://github.com/apache/solr/pull/3140#discussion_r1934921025 ## 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: I want to consolidate deprecations elsewhere so I will remove this change from the PR. We can discuss when I post that. -- 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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1934914237
##
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:
Okay I'll remove. I suppose the scope of this PR should be more focused on
SolrJ QueryRequest not server side SolrQueryRequest. Granted, I'd wish for the
same for SolrQueryRequest.getParams; same rationale for doing it in SolrJ. I
_could_ increase the scope to that but maybe too much in one PR; not sure.
--
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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
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
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]
Re: [PR] SolrRequest.getParams never null; and clarify mutability [solr]
dsmiley commented on code in PR #3140:
URL: https://github.com/apache/solr/pull/3140#discussion_r1929924679
##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -282,7 +282,6 @@ private void commitOnLeader(String leaderBaseUrl, String
coreName)
throws SolrServerException, IOException {
try (SolrClient client = recoverySolrClientBuilder(leaderBaseUrl,
coreName).build()) {
UpdateRequest ureq = new UpdateRequest();
- ureq.setParams(new ModifiableSolrParams());
Review Comment:
This was clearly a hack to work around it's weird mutability situation. Now
not needed here and elsewhere.
##
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:
There's a lot I want to deprecate and remove in Solr 10; this is one. The
annotation snuck in; arguably for another PR. On the other hand, Solr isn't
using or testing this class, so let's get on with removing!
##
solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java:
##
@@ -625,11 +625,8 @@ protected long addDocAndGetVersion(Object... fields)
throws Exception {
SolrInputDocument doc = new SolrInputDocument();
addFields(doc, fields);
-ModifiableSolrParams params = new ModifiableSolrParams();
-params.add("versions", "true");
-
UpdateRequest ureq = new UpdateRequest();
-ureq.setParams(params);
+ureq.getParams().set("versions", true);
Review Comment:
With a clear mutability story, the requirement here is now easy with this
one-liner. (same elsewhere)
##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##
@@ -380,7 +380,7 @@ public InputStreamResponseListener getResponseListener() {
public OutStream initOutStream(String baseUrl, UpdateRequest updateRequest,
String collection)
throws IOException {
String contentType = requestWriter.getUpdateContentType();
-final ModifiableSolrParams origParams = new
ModifiableSolrParams(updateRequest.getParams());
+final SolrParams origParams = updateRequest.getParams();
Review Comment:
My suspicion is that `new ModifiableSolrParams` is here to implicilty handle
null (as it handles that in its constructor). We have no need to modify the
result, I confirm.
##
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
implements IsUpdateRequest {
- protected ModifiableSolrParams params;
+ protected ModifiableSolrParams params = new ModifiableSolrParams(); // TODO
make final
Review Comment:
Up for debate on eventually final. That means we lose the setter, which is
called in a ton of places, in lieu of calling `getParams().add(params)`. I
actually went down that path and shelved the work. I was motivated for clarity
on the getParams value never being potentially shared with something else.
##
solr/modules/cross-dc/src/java/org/apache/solr/crossdc/common/MirroredSolrRequest.java:
##
@@ -72,7 +71,7 @@ public SolrParams getParams() {
return params;
}
-public void setParams(ModifiableSolrParams params) {
+public void setParams(SolrParams params) {
Review Comment:
getter & setter should have same type
##
solr/core/src/test/org/apache/solr/update/processor/TemplateUpdateProcessorTest.java:
##
@@ -69,14 +71,11 @@ public void testSimple() throws Exception {
SolrInputDocument solrDoc = new SolrInputDocument();
solrDoc.addField("id", "1");
-params =
-new ModifiableSolrParams()
-.add("processor", "template")
-.add("commit", "true")
-.add("template.field", "x_s:key_{id}");
-params.add("commit", "true");
UpdateRequest add = new UpdateRequest().add(solrDoc);
-add.setParams(params);
+add.getParams()
+.add("processor", "template")
+.add("template.field", "x_s:key_{id}")
+.add("commit", "true");
Review Comment:
so nice IMO
##
solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java:
##
@@ -16,36 +16,39 @@
*/
package org.apache.solr.client.solrj.request;
+import java.util.Objects;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.
