Skip to content

SOLR-18612: SolrJ: use SimpleOrderedMap where keys never repeat - #5311

Open
isaric wants to merge 5 commits into
apache:mainfrom
isaric:SOLR-18612-solrj
Open

isaric wants to merge 5 commits into
apache:mainfrom
isaric:SOLR-18612-solrj

Conversation

@isaric

@isaric isaric commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Description

First slice of SIP-22 step 5 (SOLR-18612): in solr/solrj, create a SimpleOrderedMap instead of a NamedList wherever the keys provably never repeat. Instantiation sites only; no declared types, method signatures or wire formats change.

Solution

The solrj main sources have 46 new NamedList sites in 13 files. 27 are converted; 19 stay, each for a stated reason.

Converted (27)

  • SchemaRequest (23 of 24): Schema API request bodies: field-type and analyzer definitions built from Maps, and the one-key wrappers such as {"add-field": …}. Utils.writeJson serializes NamedList and SimpleOrderedMap through the same MapWriter path, so the JSON sent to the server is unchanged.
  • QueryResponse._facetPivot (keyed by pivot spec), JsonMapResponseParser's result (filled from a parsed Map), ConcurrentUpdateBaseSolrClient's dummy response, and CloudSolrClient's per-route exception list: client-side objects with unique keys.

Kept as NamedList (19)

  • Wire format, since javabin picks the tag from the runtime type: JavaBinCodec.readNamedList, and the JavaBinUpdateRequestCodec request envelope and its reader (the server's streaming reader only hooks the NAMED_LST tag; the existing comment on params in that file documents the same constraint).
  • Repeated keys by design: SchemaRequest.MultiUpdate (repeats "add-field" etc. in one body), XML <lst> parsing in DOMUtil and ConfigNode (SIP step 8), and in CloudSolrClient: shardResponses (the non-routable request reuses a route URL as key), per-id version lists (a batch may contain an id twice), and exception metadata merged from several exceptions. RemoteSolrException's metadata rebuild, where repeats are not provably absent.
  • Preserve the receiver's kind: NamedList.clone() / getImmutableCopy(), ResponseCanonicalizer.
  • SOLR-18398's scope (response header): SolrResponseBase and the merged header in CloudSolrClient.condenseResponse.

One thing tried and reverted: a SimpleOrderedMap(Map) constructor mirroring NamedList's. It is ambiguous with the existing SimpleOrderedMap(MapWriter) for arguments implementing both (SolrInputDocument, SimpleOrderedMap itself), so existing callers would stop compiling. The map-fed sites in SchemaRequest go through a private helper instead.

Compatibility of this slice: no JSON-visible and no javabin-visible change. The JSON-visible effects the ticket describes come with the solr/core slice, which follows as a separate PR.

AI disclosure: the site classification, the edits and this description were drafted with Claude Code and reviewed by me.

Tests

New SchemaRequestBodyTest (solrj, no server) pins the request bodies: an AddFieldType with analyzer and similarity, and an AddField, must serialize byte-for-byte like the same structure built from plain LinkedHashMaps, which is this PR's claim stated as an assertion; and a MultiUpdate must keep its repeated add-field keys, which guards the one site deliberately left as NamedList.

Also ran: ./gradlew tidy; the full :solr:solrj:test suite; the 30 solr/core test classes that use SchemaRequest, JsonMapResponseParser or pivot facets (schema API, bulk schema, managed schema, pivot-facet distributed tests, V2 API integration, ZooKeeper admin handlers); and ./gradlew check -x test. All pass. (The Antora ref-guide task is excluded locally because its UI-bundle download can't go through my environment's proxy; no ref-guide content changes here.)

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide
  • I have added a changelog entry for my change

isaric and others added 4 commits November 4, 2026 16:40
Converts the NamedList instantiations in solrj whose keys provably never
repeat: Schema API request bodies (SchemaRequest, except MultiUpdate,
which repeats keys on purpose), JsonMapResponseParser's result, the
pivot-facet map in QueryResponse, ConcurrentUpdateBaseSolrClient's dummy
response and CloudSolrClient's per-route exception list. No declared
types or wire formats change. Adds a SimpleOrderedMap(Map) constructor
mirroring NamedList's.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XL2ChskWQPpS1niMQgCaQa
It is ambiguous with SimpleOrderedMap(MapWriter) for arguments that
implement both, such as SolrInputDocument, so existing callers would stop
compiling. SchemaRequest builds its map-fed entries through a private
helper instead.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XL2ChskWQPpS1niMQgCaQa
Asserts that AddFieldType and AddField serialize exactly like the same
structure built from plain maps, and that MultiUpdate keeps repeated
command keys.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XL2ChskWQPpS1niMQgCaQa
@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

Automated PR triage (LLM-generated; may be wrong). Checked once, when the PR was opened or marked ready; not updated on later pushes.

2 possible mismatch(es) between what this change warrants and what the PR includes.

Warranted? In PR? Confidence Why
✅ Jira issue in title — yes — Already present; not judged.
⚠️ Changelog — differs from the entry other changed high The change is a large-scale internal refactoring (replacing 27 `NamedList` instantiations with `SimpleOrderedMap` in SolrJ) that explicitly states 'No wire format changes' and 'no JSON-visible and no javabin-visible change', making it a performance/​cleanup improvement rather than a user-facing feature or fix.​
✅ Reference Guide update no no high The PR modifies internal implementation details of SolrJ classes (instantiation types) without changing any request parameters, API endpoints, configuration settings, or user-facing behavior documented in the Reference Guide.​
✅ Upgrade notes no no high The change is backwards-compatible; it only changes the concrete class used for instantiation where keys are unique, with no changes to method signatures, wire formats, or public API contracts that would require user action upon upgrading.​
⚠️ Tests — probably not needed no yes medium The changes are pure refactoring of instantiation sites with no change to logic or behavior (keys are provably unique, and serialization paths are identical), so new tests are not strictly required to validate the change, although the author included a test to pin the serialization output.​

⚠️ mismatch, ❔ uncertain or unanswered, ✅ agrees. Matching changelog, docs and test files were hidden from the model; the PR description may still mention them. File presence does not establish adequacy. Assessed commit 7fc3553dbe12cbd046843692e837adb1992725fe.

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.

1 participant