SOLR-18603: Seal the NamedList class hierarchy

changelog/unreleased/SOLR-18603-seal-namedlist.yml

@@ -0,0 +1,14 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+
+title: >
+  NamedList is now a sealed class whose only subclass is SimpleOrderedMap, which is final. Third-party
+  code can no longer extend either. CloudSolrClient.RouteResponse is removed: a routed update now
+  returns a plain SimpleOrderedMap whose responseHeader carries each route's achieved replication
+  factor under "rfByRoute", which getShardReplicationFactor() reads.
+type: changed
+authors:
+  - name: Ivan Šarić
+    nick: isaric
+links:
+  - name: SOLR-18603
+    url: https://issues.apache.org/jira/browse/SOLR-18603

solr/core/src/test/org/apache/solr/search/facet/DistributedFacetSimpleRefinementLongTailTest.java

@@ -200,7 +200,7 @@ private void sanityCheckIndividualShards() throws Exception {
     {
       List<NamedList> bar_buckets =
           (List<NamedList>)
-              ((NamedList<NamedList>) shardFooBuckets[2].get(6).get("bar")).get("buckets");
+              ((NamedList<Object>) shardFooBuckets[2].get(6).get("bar")).get("buckets");
       assertEquals(6, bar_buckets.size());
       for (int j = 0; j < 5; j++) {
         NamedList bucket = bar_buckets.get(j);

solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-11.adoc

@@ -41,3 +41,14 @@ bin/solr start -Dsolr.node.roles=data:on,overseer:preferred
 
 A node started this way asks the Overseer to re-run its node prioritization, so a preferred node takes over without waiting for the current Overseer to restart.
 Note that node roles are fixed for the lifetime of a node: unlike `ADDROLE`, they cannot be changed on a running node.
+
+== SolrJ
+
+=== NamedList is a sealed class
+
+`NamedList` is now a sealed class whose only permitted subclass is `SimpleOrderedMap`, which is `final`.
+Plugins or client code that extended either class will no longer compile; hold a `NamedList` rather than being one.
+
+`CloudSolrClient.RouteResponse` has been removed.
+A routed update request now returns a plain `SimpleOrderedMap`, whose `responseHeader` carries the achieved replication factor of each route under `rfByRoute` (`CloudSolrClient.REPFACT_BY_ROUTE`), keyed by route URL.
+`CloudSolrClient.getShardReplicationFactor()` reads that entry, so its signature and behavior are unchanged; `condenseResponse()` now returns a `SimpleOrderedMap`.

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java

@@ -29,7 +29,6 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -46,7 +45,6 @@
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
-import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import org.apache.solr.client.solrj.RequestNotSentException;
 import org.apache.solr.client.solrj.SolrClient;
@@ -414,13 +412,8 @@ private NamedList<Object> directUpdate(UpdateRequest request, String collection)
 
     long end = System.nanoTime();
 
-    @SuppressWarnings({"rawtypes"})
-    RouteResponse rr =
-        condenseResponse(
-            shardResponses, (int) TimeUnit.MILLISECONDS.convert(end - start, TimeUnit.NANOSECONDS));
-    rr.setRouteResponses(shardResponses);
-    rr.setRoutes(routes);
-    return rr;
+    return condenseResponse(
+        shardResponses, (int) TimeUnit.MILLISECONDS.convert(end - start, TimeUnit.NANOSECONDS));
   }
 
   protected RouteException getRouteException(
@@ -487,11 +480,22 @@ private Map<String, List<String>> buildUrlMap(
     return urlMap;
   }
 
-  protected <T extends RouteResponse<?>> T condenseResponse(
-      NamedList<?> response, int timeMillis, Supplier<T> supplier) {
-    T condensed = supplier.get();
+  /**
+   * Key in the merged {@code responseHeader} of a routed update request: the achieved replication
+   * factor of each route, keyed by route URL.
+   */
+  public static final String REPFACT_BY_ROUTE = "rfByRoute";
+
+  /**
+   * Merges the per-route responses of a routed update request into a single response. The merged
+   * {@code responseHeader} carries the lowest achieved replication factor as {@link
+   * UpdateRequest#REPFACT} and each route's own value under {@link #REPFACT_BY_ROUTE}.
+   */
+  public SimpleOrderedMap<Object> condenseResponse(NamedList<?> response, int timeMillis) {
+    SimpleOrderedMap<Object> condensed = new SimpleOrderedMap<>();
     int status = 0;
     Integer rf = null;
+    SimpleOrderedMap<Integer> rfByRoute = null;
 
     // TolerantUpdateProcessor
     List<SimpleOrderedMap<String>> toleratedErrors = null;
@@ -511,6 +515,8 @@ protected <T extends RouteResponse<?>> T condenseResponse(
       Object rfObj = header.get(UpdateRequest.REPFACT);
       if (rfObj != null && rfObj instanceof Integer routeRf) {
         if (rf == null || routeRf < rf) rf = routeRf;
+        if (rfByRoute == null) rfByRoute = new SimpleOrderedMap<>();
+        rfByRoute.put(response.getName(i), routeRf);
       }
 
       @SuppressWarnings("unchecked")
@@ -549,6 +555,7 @@ protected <T extends RouteResponse<?>> T condenseResponse(
     cheader.add("status", status);
     cheader.add("QTime", timeMillis);
     if (rf != null) cheader.add(UpdateRequest.REPFACT, rf);
+    if (rfByRoute != null) cheader.add(REPFACT_BY_ROUTE, rfByRoute);
     if (null != toleratedErrors) {
       cheader.add("maxErrors", ToleratedUpdateError.getUserFriendlyMaxErrors(maxToleratedErrors));
       cheader.add("errors", toleratedErrors);
@@ -584,11 +591,6 @@ protected <T extends RouteResponse<?>> T condenseResponse(
     return condensed;
   }
 
-  @SuppressWarnings({"rawtypes"})
-  public RouteResponse condenseResponse(NamedList<?> response, int timeMillis) {
-    return condenseResponse(response, timeMillis, RouteResponse::new);
-  }
-
   @Override
   public NamedList<Object> request(SolrRequest<?> request, String collection)
       throws SolrServerException, IOException {
@@ -1241,14 +1243,15 @@ public int getMinAchievedReplicationFactor(String collection, NamedList<?> resp)
   }
 
   /**
-   * Walks the NamedList response after performing an update request looking for the replication
-   * factor that was achieved in each shard involved in the request. For single doc updates, there
+   * Reads the replication factor that was achieved in each shard involved in a routed update
+   * request, from {@link #REPFACT_BY_ROUTE} in the response header. For single doc updates, there
    * will be only one shard in the return value.
    */
   public Map<String, Integer> getShardReplicationFactor(String collection, NamedList<?> resp) {
     Map<String, Integer> results = new HashMap<>();
-    if (resp instanceof RouteResponse) {
-      NamedList<NamedList<?>> routes = ((RouteResponse<?>) resp).getRouteResponses();
+    NamedList<?> header = (NamedList<?>) resp.get("responseHeader");
+    Object rfByRoute = header == null ? null : header.get(REPFACT_BY_ROUTE);
+    if (rfByRoute instanceof NamedList<?> routes) {
       DocCollection coll = getDocCollection(collection, null);
       Map<String, String> leaders = new HashMap<>();
       for (Slice slice : coll.getActiveSlices()) {
@@ -1261,13 +1264,9 @@ public Map<String, Integer> getShardReplicationFactor(String collection, NamedLi
         }
       }
 
-      Iterator<Map.Entry<String, NamedList<?>>> routeIter = routes.iterator();
-      while (routeIter.hasNext()) {
-        Map.Entry<String, NamedList<?>> next = routeIter.next();
+      for (Map.Entry<String, ?> next : routes) {
         String host = next.getKey();
-        NamedList<?> hostResp = next.getValue();
-        Integer rf =
-            (Integer) ((NamedList<?>) hostResp.get("responseHeader")).get(UpdateRequest.REPFACT);
+        Integer rf = (Integer) next.getValue();
         if (rf != null) {
           String shard = leaders.get(host);
           if (shard == null) {
@@ -1681,28 +1680,6 @@ void evictStale() {
     }
   }
 
-  @SuppressWarnings({"rawtypes"})
-  public static class RouteResponse<T extends LBSolrClient.Req> extends NamedList<Object> {
-    private NamedList<NamedList<?>> routeResponses;
-    private Map<String, T> routes;
-
-    public void setRouteResponses(NamedList<NamedList<?>> routeResponses) {
-      this.routeResponses = routeResponses;
-    }
-
-    public NamedList<NamedList<?>> getRouteResponses() {
-      return routeResponses;
-    }
-
-    public void setRoutes(Map<String, T> routes) {
-      this.routes = routes;
-    }
-
-    public Map<String, T> getRoutes() {
-      return routes;
-    }
-  }
-
   public static class RouteException extends SolrException {
 
     private NamedList<Throwable> throwables;

solr/solrj/src/java/org/apache/solr/common/util/NamedList.java

@@ -53,10 +53,13 @@
  * allows order to be easily preserved in various clients (i.e. not a straight map). If access by
  * key is more important for serialization, see {@link SimpleOrderedMap}, or simply use a regular
  * {@link Map}
+ *
+ * <p>This class is sealed: {@link SimpleOrderedMap} is its only direct subclass.
  */
 @SuppressWarnings({"unchecked", "rawtypes"})
-public class NamedList<T>
-    implements Cloneable, Serializable, Iterable<Map.Entry<String, T>>, MapWriter {
+public sealed class NamedList<T>
+    implements Cloneable, Serializable, Iterable<Map.Entry<String, T>>, MapWriter
+    permits SimpleOrderedMap {
 
   private static final long serialVersionUID = 1957981902839867821L;
   protected final List<Object> nvPairs;

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

@@ -43,7 +43,7 @@
  * hold data to be serialized. It aims to minimize overhead and to be efficient at adding new
  * elements.
  */
-public class SimpleOrderedMap<T> extends NamedList<T> implements Map<String, T> {
+public final class SimpleOrderedMap<T> extends NamedList<T> implements Map<String, T> {
 
   private static final SimpleOrderedMap<Object> EMPTY = new SimpleOrderedMap<>(List.of());
 

solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java

@@ -18,7 +18,6 @@
 
 import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
 import static org.apache.solr.client.solrj.impl.BaseHttpClusterStateProvider.SYS_PROP_CACHE_TIMEOUT_SECONDS;
-import static org.apache.solr.client.solrj.impl.CloudSolrClient.RouteResponse;
 
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
@@ -361,6 +360,54 @@ private CloudSolrClient createHttpCSPBasedCloudSolrClient() {
     return new CloudSolrClient.Builder(solrUrls).build();
   }
 
+  @Test
+  public void testShardReplicationFactors() throws Exception {
+    final String collection = "replication_factor_collection";
+    CloudSolrClient client = cluster.getSolrClient();
+    CollectionAdminRequest.createCollection(collection, "conf", 2, 1).process(client);
+    cluster.waitForActiveCollection(collection, 2, 2);
+
+    DocCollection col = client.getClusterState().getCollection(collection);
+    String shard1Url = col.getSlice("shard1").getLeader().getCoreUrl();
+    String shard2Url = col.getSlice("shard2").getLeader().getCoreUrl();
+    NamedList<NamedList<?>> routeResponses = new NamedList<>();
+    routeResponses.add(
+        shard1Url,
+        new NamedList<>(
+            Map.of(
+                "responseHeader", new NamedList<>(Map.of("status", 0, UpdateRequest.REPFACT, 2)))));
+    routeResponses.add(
+        shard2Url,
+        new NamedList<>(
+            Map.of(
+                "responseHeader", new NamedList<>(Map.of("status", 0, UpdateRequest.REPFACT, 3)))));
+
+    NamedList<Object> response = client.condenseResponse(routeResponses, 7);
+    NamedList<?> header = (NamedList<?>) response.get("responseHeader");
+    assertEquals(Map.of(shard1Url, 2, shard2Url, 3), header.get(CloudSolrClient.REPFACT_BY_ROUTE));
+    assertEquals(
+        Map.of("shard1", 2, "shard2", 3), client.getShardReplicationFactor(collection, response));
+    assertEquals(2, client.getMinAchievedReplicationFactor(collection, response));
+  }
+
+  @Test
+  public void testShardReplicationFactorsWithoutRouteFactors() {
+    CloudSolrClient client = cluster.getSolrClient();
+    NamedList<NamedList<?>> routeResponses = new NamedList<>();
+    routeResponses.add(
+        "http://unused/solr/core",
+        new NamedList<>(Map.of("responseHeader", new NamedList<>(Map.of("status", 0)))));
+
+    NamedList<Object> response = client.condenseResponse(routeResponses, 7);
+    NamedList<?> header = (NamedList<?>) response.get("responseHeader");
+    assertNull(header.get(CloudSolrClient.REPFACT_BY_ROUTE));
+    assertNull(header.get(UpdateRequest.REPFACT));
+    assertEquals(Map.of(), client.getShardReplicationFactor("missing_collection", response));
+    assertEquals(-1, client.getMinAchievedReplicationFactor("missing_collection", response));
+    assertEquals(
+        Map.of(), client.getShardReplicationFactor("missing_collection", new NamedList<>()));
+  }
+
   @Test
   public void testRouting() throws Exception {
     CollectionAdminRequest.createCollection("routing_collection", "conf", 2, 1)
@@ -378,29 +425,7 @@ public void testRouting() throws Exception {
     if (getRandomClient().isDirectUpdatesToLeadersOnly()) {
       checkSingleServer(response);
     }
-    RouteResponse<?> rr = (RouteResponse<?>) response;
-    Map<String, ? extends LBSolrClient.Req> routes = rr.getRoutes();
-    Iterator<? extends Map.Entry<String, ? extends LBSolrClient.Req>> it =
-        routes.entrySet().iterator();
-    while (it.hasNext()) {
-      Map.Entry<String, ? extends LBSolrClient.Req> entry = it.next();
-      String coreUrl = entry.getKey();
-      final String baseUrl = URLUtil.extractBaseUrl(coreUrl);
-      final String coreName = URLUtil.extractCoreFromCoreUrl(coreUrl);
-      UpdateRequest updateRequest = (UpdateRequest) entry.getValue().getRequest();
-      SolrInputDocument doc = updateRequest.getDocuments().get(0);
-      String id = doc.getField("id").getValue().toString();
-      ModifiableSolrParams params = new ModifiableSolrParams();
-      params.add("q", "id:" + id);
-      params.add("distrib", "false");
-      QueryRequest queryRequest = new QueryRequest(params);
-
-      SolrClient solrClient =
-          new CollectionScopedSolrClient(cluster.getJetty(baseUrl).getSolrClient(), coreName);
-      QueryResponse queryResponse = queryRequest.process(solrClient);
-      SolrDocumentList docList = queryResponse.getResults();
-      assertEquals(1, docList.getNumFound());
-    }
+    assertDocsOnShardLeaders("routing_collection", "0", "2");
 
     // Test the deleteById routing for UpdateRequest
 
@@ -429,27 +454,7 @@ public void testRouting() throws Exception {
       if (threadedClient.isDirectUpdatesToLeadersOnly()) {
         checkSingleServer(response);
       }
-      rr = (RouteResponse) response;
-      routes = rr.getRoutes();
-      it = routes.entrySet().iterator();
-      while (it.hasNext()) {
-        Map.Entry<String, ? extends LBSolrClient.Req> entry = it.next();
-        String coreUrl = entry.getKey();
-        final String baseUrl = URLUtil.extractBaseUrl(coreUrl);
-        final String coreName = URLUtil.extractCoreFromCoreUrl(coreUrl);
-        UpdateRequest updateRequest = (UpdateRequest) entry.getValue().getRequest();
-        SolrInputDocument doc = updateRequest.getDocuments().get(0);
-        String id = doc.getField("id").getValue().toString();
-        ModifiableSolrParams params = new ModifiableSolrParams();
-        params.add("q", "id:" + id);
-        params.add("distrib", "false");
-        QueryRequest queryRequest = new QueryRequest(params);
-        SolrClient solrClient =
-            new CollectionScopedSolrClient(cluster.getJetty(baseUrl).getSolrClient(), coreName);
-        QueryResponse queryResponse = queryRequest.process(solrClient);
-        SolrDocumentList docList = queryResponse.getResults();
-        assertEquals(1, docList.getNumFound());
-      }
+      assertDocsOnShardLeaders("routing_collection", "0", "2");
     }
 
     // Test that queries with _route_ params are routed by the client
@@ -1096,18 +1101,40 @@ public void testRetryUpdatesWhenClusterStateIsStale() throws Exception {
     }
   }
 
+  /** Asserts that each doc is on the leader of the shard it routes to, querying that core alone. */
+  private static void assertDocsOnShardLeaders(String collection, String... ids) throws Exception {
+    DocCollection col = cluster.getSolrClient().getClusterState().getCollection(collection);
+    for (String id : ids) {
+      String coreUrl =
+          col.getRouter().getTargetSlice(id, null, null, null, col).getLeader().getCoreUrl();
+      QueryResponse queryResponse =
+          new QueryRequest(params("q", "id:" + id, "distrib", "false"))
+              .process(
+                  cluster.getJetty(URLUtil.extractBaseUrl(coreUrl)).getSolrClient(),
+                  URLUtil.extractCoreFromCoreUrl(coreUrl));
+      assertEquals(1, queryResponse.getResults().getNumFound());
+    }
+  }
+
+  /** Asserts that a routed update was sent to shard leaders only, via the per-route rf header. */
   private static void checkSingleServer(NamedList<Object> response) {
-    final RouteResponse<?> rr = (RouteResponse<?>) response;
-    final Map<String, ? extends LBSolrClient.Req> routes = rr.getRoutes();
-    final Iterator<? extends Map.Entry<String, ? extends LBSolrClient.Req>> it =
-        routes.entrySet().iterator();
-    while (it.hasNext()) {
-      Map.Entry<String, ? extends LBSolrClient.Req> entry = it.next();
-      assertEquals(
-          "wrong number of servers: " + entry.getValue().getEndpoints(),
-          1,
-          entry.getValue().getEndpoints().size());
+    NamedList<?> header = (NamedList<?>) response.get("responseHeader");
+    NamedList<?> rfByRoute = (NamedList<?>) header.get(CloudSolrClient.REPFACT_BY_ROUTE);
+    assertNotNull("no per-route rf in " + header, rfByRoute);
+    DocCollection col =
+        cluster.getSolrClient().getClusterState().getCollection("routing_collection");
+    Set<String> leaderUrls = new HashSet<>();
+    for (Slice slice : col.getSlices()) {
+      leaderUrls.add(slice.getLeader().getCoreUrl());
     }
+    assertEquals("missing routes in " + rfByRoute, leaderUrls.size(), rfByRoute.size());
+    for (Map.Entry<String, ?> route : rfByRoute) {
+      assertTrue("not a leader: " + route.getKey(), leaderUrls.contains(route.getKey()));
+      assertEquals(Integer.valueOf(1), route.getValue());
+    }
+    assertEquals(
+        Map.of("shard1", 1, "shard2", 1),
+        cluster.getSolrClient().getShardReplicationFactor("routing_collection", response));
   }
 
   /**