Skip to content

SOLR-18621: Add SolrParams.of(String...) and ModifiableSolrParams.of(String...) - #5291

Open
dsmiley wants to merge 1 commit into
apache:mainfrom
dsmiley:SOLR-18621
Open

dsmiley wants to merge 1 commit into
apache:mainfrom
dsmiley:SOLR-18621

Conversation

@dsmiley

@dsmiley dsmiley commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

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

Adds name/value varargs factories to SolrJ, mirroring SolrTestCaseJ4.params.

  • SolrParams.of(String...) returns unmodifiable params; ModifiableSolrParams.of(String...) returns modifiable params (plus of() and of(String, String) so every overload on the subclass returns the modifiable type).
  • Repeated names become multi-values; null name/value becomes empty string; an odd count throws IllegalArgumentException.
  • Existing params(String...) helpers (SolrTestCaseJ4, CollectionsHandler, CoreAdminHandler, SyncStrategy, StreamTool, SolrBenchState) delegate to the new method. Other callers are not migrated.
  • A literal ModifiableSolrParams.of(null) is now ambiguous; the one such test call is cast.

🤖 Generated with Claude Code

…String...)

- New factory methods build immutable and modifiable params from name/value pairs; repeated names become multi-values and null becomes empty string.
- Existing params(String...) helpers in SolrTestCaseJ4 and main code now delegate to them.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@dsmiley

dsmiley commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor Author

This is about freaking time to have this! In separate PRs I want to add withDefaults(SolrParams) instance method to make it easier to chain. And an isEmpty() method.

The part I'm unsure about in this PR is the null handling. Annoyingly, Map.of(...) can't handle a null, and yet there are some occasional spots where we pass null in our code. I thought the empty string would be a good substitute but I'm unsure... maybe we should just disallow -- treat as a non-existent pair maybe? I'm inclined to go that way.

@nick-boss-tech

nick-boss-tech commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖 (reviewed by Nick Shanin)

The failing TestMultiWordSynonyms.testPf check is unrelated; PR #5294 addresses it.

@dsmiley I finished the review and prepared a tested follow-up on top of your current head:

  • The immutable factory protects the map structure but exposes its stored arrays through getters, iteration, map views, writeMap, and modifiable copies. The follow-up uses a private implementation that returns array copies, protecting the immutable result through those paths.
  • Adding of(String...) makes the existing ModifiableSolrParams.of(null) call ambiguous with of(SolrParams). As an API alternative, the follow-up names both new varargs factories ofPairs(String...), updates the six helper delegates, and restores the uncast null test call. Empty and single-pair of overloads remain available. It retains this PR's null-to-empty-string policy.

Verified on Linux: 44 tests across the five SolrJ parameter suites and 20 tests across CoreAdminHandlerTest and StreamToolTest pass, with no skips. All six new isolation tests fail at their intended runtime assertions when the original shallow-wrapper algorithm is restored under the renamed API, and pass with the fix. The uncast of(null) call compiles and returns empty params.

The changelog entry passes the project's logchangeGenerate parser. The separate cross-branch validateChangelogs.py script crashes during branch enumeration before reading entries on that clone; its failure is not reported as a pass.

To take the tested commits:

git switch SOLR-18621
git fetch https://github.com/nick-boss-tech/solr.git solr-18621-params-factory-fixes
git merge --ff-only 199911b298fac80f4cea5e3de553675b64f6a6a9

Would you prefer ofPairs to preserve compatibility with the existing null call?

@dsmiley

dsmiley commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

@nick-boss-tech ModifiableSolrParams.of(null) -- single arg is invalid. ofPairs(String...) that is inconsistent with existing methods this takes inspiration from on Map. RE unmodifiable... I don't think we should go so far as protecting the array values; risk/verbosity-extra-ops isn't the right trade-off IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants