[isaric]

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

Description

Step 4 of SIP-22: NamedList Reduction: use Java 17 sealed classes so that the NamedList type hierarchy is exactly what we want. Agreed as the next step on the dev list (thread).

NamedList is now sealed, permitting only SimpleOrderedMap, which is final. No public method signatures change.

Solution

An audit of main found three subclasses of NamedList, not one:

1. SimpleOrderedMap – expected.
2. CloudSolrClient.RouteResponse – a public nested class returned by CloudSolrClient.request() for routed updates, carrying the per-route responses so that getShardReplicationFactor() could read each shard's achieved replication factor. SolrJ has no module-info.java, so a permitted subclass must live in NamedList's package, which rules out keeping it as a subclass. As agreed on the Jira, it is removed: condenseResponse now records each route's rf in the merged responseHeader under rfByRoute (CloudSolrClient.REPFACT_BY_ROUTE), next to the existing minimum rf and in the same way the tolerant-update errors are merged, and getShardReplicationFactor() reads that entry instead of casting. Its signature and result are unchanged; getMinAchievedReplicationFactor() is untouched. condenseResponse(NamedList, int) now returns SimpleOrderedMap<Object>, and the protected Supplier overload is gone (nothing overrode it).
3. Twelve anonymous subclasses in NestableJsonFacetTest (double-brace initialisation), rewritten to plain add() calls.

One further consequence of sealing: a cast from a NamedList-typed expression to List is now a compile error, since no permitted subtype implements List. One test (DistributedFacetSimpleRefinementLongTailTest) did that on a value that was never a NamedList; the containing map is now typed NamedList<Object>.

Sealing a public SolrJ class is a plugin-compatibility change: third-party subclasses of NamedList or SimpleOrderedMap will no longer compile. This is main / 11.0 only, with a changelog entry and an upgrade note in major-changes-in-solr-11.adoc that also documents rfByRoute.

AI disclosure: the audit, code changes, changelog/doc text and this description were drafted with Claude Code and reviewed by me.

Tests

No new test classes; the compiler enforces the hierarchy. CloudHttp2SolrClientTest.testRouting previously inspected RouteResponse.getRoutes(); it now checks each document on the leader of the shard DocRouter routes it to, and checkSingleServer asserts that every route URL in rfByRoute is a shard leader (and that the entry is present). Ran:

• ./gradlew tidy – applied.
• ./gradlew check -x test – passes across all modules, so nothing else in the repo subclasses NamedList. The Antora ref-guide site task could not fetch its UI bundle in my environment; I built the ref-guide separately with a local bundle to confirm the upgrade-note page renders with no warnings.
• ./gradlew :solr:solrj:test --tests NestableJsonFacetTest --tests CloudHttp2SolrClientTest – both pass.

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

[dsmiley]

no point in using CollectionScopedSolrClient when in the next line you could pass the extracted core name to the 1st arg of the process method.

[dsmiley]

the method name confused me as to what it does. Note this could simply call SimpleOrderedMap.of("count", count) and you could inline that trivial thing, dropping the method.

[dsmiley]

It's pleasantly ironic to see that a short-cut trick/hack somebody did to try to be concise ended up being much more verbose than what we're coming up with now.

[dsmiley]

I love that the changes here are simpler than before :-)

I suppose a side-effect of this change is that if someone were to serialize the response NamedList, they'd see information in the responseHeader that previously wasn't there. But so what.

[isaric]

@dsmiley Thanks for the review; both suggestions are in (aa3e2ab). The first Crave run failed on TestCloudConsistency.testOutOfSyncReplicasCannotBecomeLeaderAfterRestart; passes locally, it looks flaky. The rerun on the current head is green across all four workflows.

[nick-boss-tech]

🤖 AI text below 🤖 (reviewed by Nick Shanin)

Hi @isaric, I have a tested follow-up that adds missing coverage for the per-shard replication-factor API: diff. The current tests exercise the minimum factor, but nothing pins the full shard-to-factor map the API returns, so a regression that returned the same factor for every shard would pass unnoticed.

It adds the missing tests to CloudHttp2SolrClientTest, test-only, with no production changes: testShardReplicationFactors feeds distinct per-shard factors (2 and 3) through real shard leaders and checks the full shard-to-factor map the public API returns, and testShardReplicationFactorsWithoutRouteFactors covers the omitted-metadata case. The existing checkSingleServer helper now asserts the per-shard map for its real updates too.

Verification on Linux at `b244ae94d6` (JDK 21.0.12.1, Gradle 9.7.0, seed `18603C0DE`)

• CloudHttp2SolrClientTest passes in full (22 tests, including the two new ones) and NestableJsonFacetTest passes; tidy is clean and root check -x test passes.
• The new assertions bite: with the production code made to drop the per-shard factors from the response, the first test fails on the missing factors; with the factors corrupted to 0, it fails on the map.

The commits are a fast-forward of your current PR head:

git switch SOLR-18603-seal-namedlist
git fetch https://github.com/nick-boss-tech/solr.git solr-18603-submit
git merge --ff-only b244ae94d6c84eec06bf1de5754e566b0fdb88d1

You're welcome to take the commits or adapt the changes. If you would rather I open it as a separate PR after this one merges, I can do that instead.

Separately, the Crave failure in TestCloudConsistency appears unrelated to this change: I could not reproduce it on your head or on main with the recorded seed, so it should clear on a rerun.