SolrRequest.getParams never null; and clarify mutability #3140

dsmiley · 2025-01-27T01:48:15Z

SolrJ SolrRequest.getParams lacked javadocs and thus also clarity on null & mutability. This PR addresses that. It can return null but I don't want it to, so I made some changes and removed needless null checks. Too much code implicitly assumes it's non-null but that's an iffy assumption. Additionally mutability is unclear. So I propose that if the return type is ModifiableSolrParams, then it's safe to modify it. UpdateRequest had issues with this; I removed it's lazy instantiation so we can always count on an instance being returned that we can modify.

This PR introduces SolrParams.of(), the widespread usage of which can occur later. Or I could do all of this piece in another PR if asked.

Many changes here aren't strictly required but felt good with these changes.

https://issues.apache.org/jira/browse/SOLR-XXXXX

Add SolrParams.of()

dsmiley

Solr 10 only, albeit could backport Solr.of, for example.

dsmiley · 2025-01-27T01:49:15Z

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());


This was clearly a hack to work around it's weird mutability situation. Now not needed here and elsewhere.

dsmiley · 2025-01-27T01:51:05Z

solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java

    UpdateRequest ureq = new UpdateRequest();
-    ureq.setParams(params);
+    ureq.getParams().set("versions", true);


With a clear mutability story, the requirement here is now easy with this one-liner. (same elsewhere)

dsmiley · 2025-01-27T01:53:21Z

solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java

-    ureq.setParams(params);
+    ureq.getParams().add(params);


I chose to call SolrParams.add(SolrParams) here but could have called setParams. A nice effect of this is that we copy the params instead of use in-place. Previously this deleteDocAndGetVersion method actually added params to its argument, which is very bad taste.

dsmiley · 2025-01-27T01:54:10Z

solr/core/src/test/org/apache/solr/update/processor/TemplateUpdateProcessorTest.java

+    add.getParams()
+        .add("processor", "template")
+        .add("template.field", "x_s:key_{id}")
+        .add("commit", "true");


so nice IMO

dsmiley · 2025-01-27T01:55:55Z

solr/modules/cross-dc/src/java/org/apache/solr/crossdc/common/MirroredSolrRequest.java

-    public void setParams(ModifiableSolrParams params) {
+    public void setParams(SolrParams params) {


getter & setter should have same type

solr/solrj/src/java/org/apache/solr/client/solrj/request/DirectXmlRequest.java

dsmiley · 2025-01-27T02:03:55Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/DocumentAnalysisRequest.java

-  public ModifiableSolrParams getParams() {
+  public SolrParams getParams() {


With new clarity from the Javadocs, we should never declare the return type as ModifiableSolrParams unless we have a consistent instance to return.

dsmiley · 2025-01-27T02:04:55Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/GenericV2SolrRequest.java

@@ -40,8 +40,7 @@ public GenericV2SolrRequest(METHOD m, String path) {
   * @param params query parameter names and values for making this request.
   */
  public GenericV2SolrRequest(METHOD m, String path, SolrParams params) {
-    super(m, path);
-    this.params = params;
+    super(m, removeLeadingApiRoot(path), params);


@gerlowskija this includes a minor bug fix (to call removeLeadingApiRoot as the other overloaded constructor does) but maybe this code wasn't used

dsmiley · 2025-01-27T02:11:41Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java

    // NOTE: if the update request contains only delete commands the params
    // must be loaded now
-    if (updateRequest.getParams() == null) {
+    if (updateRequest.getParams().iterator().hasNext() == false) { // no params


This check is evil; took a while to find it as I debugged a test for hours. Behavior is changing based on the presence of params or not. Like nowhere is there a similar check (at least based on my adventure with this PR). The history behind these lines of code is what I consider to be an ugly/messy solution to propagating a commitWithin param. Tempting to add that on my long TODO list of messes to fix but our longer future is not "javabin", and the mess thankfully seems localized here.

dsmiley · 2025-01-27T02:12:18Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java


  public QueryRequest() {
    super(METHOD.GET, null);
+    query = new ModifiableSolrParams(); // TODO SolrParams.of()


at some point I eventually added SolrParams.of; TODO to use it in more places

gerlowskija

Left a few inline comments, but happy enough with things to give this a 'LGTM'

gerlowskija · 2025-01-29T19:53:30Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/AbstractUpdateRequest.java

 public abstract class AbstractUpdateRequest extends CollectionRequiringSolrRequest<UpdateResponse>
    implements IsUpdateRequest {
-  protected ModifiableSolrParams params;
+  protected ModifiableSolrParams params = new ModifiableSolrParams(); // TODO make final


+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

gerlowskija · 2025-01-29T20:45:30Z

solr/modules/cross-dc/src/java/org/apache/solr/crossdc/common/MirroredSolrRequest.java

-      ((MirroredAdminRequest) request).setParams(params);
-    } else if (request instanceof UpdateRequest) {
-      ((UpdateRequest) request).setParams(params);
+    if (request instanceof MirroredAdminRequest mReq) {


[+1] I haven't seen this instanceof-and-declare-variable syntax before - very cool!

yeah it's a very nice Java 16 thing

solr/solrj/src/java/org/apache/solr/client/solrj/request/DirectXmlRequest.java

solr/solrj/src/java/org/apache/solr/common/params/SolrParams.java

dsmiley · 2025-02-02T16:22:32Z

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.

SolrRequest.getParams never null; and define mutability

bfdec90

Add SolrParams.of()

dsmiley requested a review from gerlowskija January 27, 2025 01:48

github-actions bot added test-framework client:solrj tests cat:cloud cat:index cat:api labels Jan 27, 2025

fix nocommit

04eac02

dsmiley commented Jan 27, 2025

View reviewed changes

dsmiley mentioned this pull request Jan 27, 2025

SolrParams.equals implementation #3141

Open

gerlowskija approved these changes Jan 29, 2025

View reviewed changes

PR feedback. Added SolrParams.of(k, v).

e428006

github-actions bot added the cat:cli label Jan 30, 2025

dsmiley added 2 commits January 30, 2025 02:05

Null isn't supported

010ae6f

Also clarify SolrRequest.getParams nad QParser params

44a83b0

github-actions bot added cat:search cat:security labels Feb 2, 2025

dsmiley marked this pull request as ready for review February 2, 2025 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SolrRequest.getParams never null; and clarify mutability #3140

SolrRequest.getParams never null; and clarify mutability #3140

dsmiley commented Jan 27, 2025

dsmiley left a comment

dsmiley Jan 27, 2025

dsmiley Jan 27, 2025

dsmiley Jan 27, 2025

dsmiley Jan 27, 2025

dsmiley Jan 27, 2025

dsmiley Jan 27, 2025

dsmiley Jan 27, 2025

dsmiley Jan 27, 2025

dsmiley Jan 27, 2025

gerlowskija left a comment

gerlowskija Jan 29, 2025

gerlowskija Jan 29, 2025

dsmiley Jan 30, 2025

dsmiley commented Feb 2, 2025

		public void setParams(ModifiableSolrParams params) {
		public void setParams(SolrParams params) {

		public ModifiableSolrParams getParams() {
		public SolrParams getParams() {

SolrRequest.getParams never null; and clarify mutability #3140

Are you sure you want to change the base?

SolrRequest.getParams never null; and clarify mutability #3140

Conversation

dsmiley commented Jan 27, 2025

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerlowskija left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley commented Feb 2, 2025