SOLR-13407: Reject update requests sent to non-routed multi collection aliases.

solr/CHANGES.txt

@@ -94,6 +94,10 @@ Upgrade Notes
   Please see https://velocity.apache.org/engine/2.0/upgrading.html about upgrading. Velocity Tools upgraded from 2.0 to
   3.0. For more details, please see https://velocity.apache.org/tools/3.0/upgrading.html for details about the upgrade.
 
+* SOLR-13407: Update requests sent to non-routed aliases that point to multiple collections are no longer accepted. Until
+  now Solr followed an obscure convention of updating only the first collection from the list, which usually was not what
+  the user intended. This change explicitly rejects such update requests.
+
 
 New Features
 ----------------------
@@ -226,6 +230,8 @@ Improvements
 
 * SOLR-13398: Move log line "Processing SSL Credential Provider chain..." from INFO to DEBUG (janhoy)
 
+* SOLR-13407: Reject update requests sent to non-routed multi collection aliases. (ab)
+
 Other Changes
 ----------------------
 

solr/core/src/java/org/apache/solr/api/V2HttpCall.java

@@ -196,6 +196,11 @@ public class V2HttpCall extends HttpSolrCall {
 
     Supplier<DocCollection> logic = () -> {
       this.collectionsList = resolveCollectionListOrAlias(collectionStr); // side-effect
+      if (collectionsList.size() > 1) {
+        throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Request must be sent to a single collection " +
+            "or an alias that points to a single collection," +
+            " but '" + collectionStr + "' resolves to " + this.collectionsList);
+      }
       String collectionName = collectionsList.get(0); // first
       //TODO an option to choose another collection in the list if can't find a local replica of the first?
 

solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java

@@ -371,17 +371,29 @@ public class HttpSolrCall {
    * {@link #getCollectionsList()}
    */
   protected List<String> resolveCollectionListOrAlias(String collectionStr) {
-    if (collectionStr == null) {
+    if (collectionStr == null || collectionStr.trim().isEmpty()) {
       return Collections.emptyList();
     }
-    LinkedHashSet<String> resultList = new LinkedHashSet<>();
+    List<String> result = null;
+    LinkedHashSet<String> uniqueList = null;
     Aliases aliases = getAliases();
     List<String> inputCollections = StrUtils.splitSmart(collectionStr, ",", true);
+    if (inputCollections.size() > 1) {
+      uniqueList = new LinkedHashSet<>();
+    }
     for (String inputCollection : inputCollections) {
       List<String> resolvedCollections = aliases.resolveAliases(inputCollection);
-      resultList.addAll(resolvedCollections);
+      if (uniqueList != null) {
+        uniqueList.addAll(resolvedCollections);
+      } else {
+        result = resolvedCollections;
+      }
+    }
+    if (uniqueList != null) {
+      return new ArrayList<>(uniqueList);
+    } else {
+      return result;
     }
-    return new ArrayList<>(resultList);
   }
 
   /**
@@ -430,7 +442,7 @@ public class HttpSolrCall {
 
   protected void extractRemotePath(String collectionName, String origCorename) throws UnsupportedEncodingException, KeeperException, InterruptedException {
     assert core == null;
-    coreUrl = getRemotCoreUrl(collectionName, origCorename);
+    coreUrl = getRemoteCoreUrl(collectionName, origCorename);
     // don't proxy for internal update requests
     invalidStates = checkStateVersionsAreValid(queryParams.get(CloudSolrClient.STATE_VERSION));
     if (coreUrl != null
@@ -927,7 +939,7 @@ public class HttpSolrCall {
     }
   }
 
-  protected String getRemotCoreUrl(String collectionName, String origCorename) {
+  protected String getRemoteCoreUrl(String collectionName, String origCorename) {
     ClusterState clusterState = cores.getZkController().getClusterState();
     final DocCollection docCollection = clusterState.getCollectionOrNull(collectionName);
     Slice[] slices = (docCollection != null) ? docCollection.getActiveSlicesArr() : null;
@@ -951,7 +963,12 @@ public class HttpSolrCall {
       return null;
     }
 
-    collectionsList.add(collectionName);
+    // XXX (ab) most likely this is not needed? it seems all code paths
+    // XXX already make sure the collectionName is on the list
+    if (!collectionsList.contains(collectionName)) {
+      collectionsList = new ArrayList<>(collectionsList);
+      collectionsList.add(collectionName);
+    }
     String coreUrl = getCoreUrl(collectionName, origCorename, clusterState,
         activeSlices, byCoreName, true);
 

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

@@ -34,8 +34,10 @@ import org.apache.lucene.util.IOUtils;
 import org.apache.solr.client.solrj.SolrQuery;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.cloud.SolrCloudManager;
 import org.apache.solr.client.solrj.embedded.JettySolrRunner;
 import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.ClusterStateProvider;
 import org.apache.solr.client.solrj.impl.HttpSolrClient;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.UpdateRequest;
@@ -46,6 +48,7 @@ import org.apache.solr.common.SolrException;
 import org.apache.solr.common.cloud.Aliases;
 import org.apache.solr.common.cloud.SolrZkClient;
 import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CollectionAdminParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.Utils;
@@ -269,6 +272,43 @@ public class AliasIntegrationTest extends SolrCloudTestCase {
 
   }
 
+  @Test
+  public void testClusterStateProviderAPI() throws Exception {
+    final String aliasName = getSaferTestName();
+    ZkStateReader zkStateReader = createColectionsAndAlias(aliasName);
+    CollectionAdminRequest.SetAliasProperty setAliasProperty = CollectionAdminRequest.setAliasProperty(aliasName);
+    setAliasProperty.addProperty("foo","baz");
+    setAliasProperty.addProperty("bar","bam");
+    setAliasProperty.process(cluster.getSolrClient());
+    checkFooAndBarMeta(aliasName, zkStateReader);
+    SolrCloudManager cloudManager = cluster.getJettySolrRunner(0).getCoreContainer().getZkController().getSolrCloudManager();
+    ClusterStateProvider stateProvider = cloudManager.getClusterStateProvider();
+    List<String> collections = stateProvider.resolveAlias(aliasName);
+    assertEquals(collections.toString(), 2, collections.size());
+    assertTrue(collections.toString(), collections.contains("collection1meta"));
+    assertTrue(collections.toString(), collections.contains("collection2meta"));
+    Map<String, String> props = stateProvider.getAliasProperties(aliasName);
+    assertEquals(props.toString(), 2, props.size());
+    assertEquals(props.toString(), "baz", props.get("foo"));
+    assertEquals(props.toString(), "bam", props.get("bar"));
+
+    assertFalse("should not be a routed alias", stateProvider.isRoutedAlias(aliasName));
+    // now make it a routed alias, according to the criteria in the API
+    setAliasProperty = CollectionAdminRequest.setAliasProperty(aliasName);
+    setAliasProperty.addProperty(CollectionAdminParams.ROUTER_PREFIX + "foo","baz");
+    setAliasProperty.process(cluster.getSolrClient());
+    // refresh
+    stateProvider = cloudManager.getClusterStateProvider();
+    assertTrue("should be a routed alias", stateProvider.isRoutedAlias(aliasName));
+
+    try {
+      String resolved = stateProvider.resolveSimpleAlias(aliasName);
+      fail("this is not a simple alias but it resolved to " + resolved);
+    } catch (IllegalArgumentException e) {
+      // expected
+    }
+  }
+
   private void checkFooAndBarMeta(String aliasName, ZkStateReader zkStateReader) throws Exception {
     zkStateReader.aliasesManager.update(); // ensure our view is up to date
     Map<String, String> meta = zkStateReader.getAliases().getCollectionAliasProperties(aliasName);
@@ -555,11 +595,16 @@ public class AliasIntegrationTest extends SolrCloudTestCase {
 
     searchSeveralWays("testalias6", new SolrQuery("*:*"), 6);
 
-    // add one document to testalias6, which will route to collection2 because it's the first
-    new UpdateRequest()
-        .add("id", "12", "a_t", "humpty dumpy5 sat on a walls")
-        .commit(cluster.getSolrClient(), "testalias6"); // thus gets added to collection2
-    searchSeveralWays("collection2", new SolrQuery("*:*"), 4);
+    // add one document to testalias6. this should fail because it's a multi-collection non-routed alias
+    try {
+      new UpdateRequest()
+          .add("id", "12", "a_t", "humpty dumpy5 sat on a walls")
+          .commit(cluster.getSolrClient(), "testalias6");
+      fail("Update to a multi-collection non-routed alias should fail");
+    } catch (SolrException e) {
+      // expected
+      assertEquals(e.toString(), SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+    }
 
     ///////////////
     for (int i = 1; i <= 6 ; i++) {

solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java

@@ -2145,6 +2145,11 @@ public class SimClusterStateProvider implements ClusterStateProvider {
     throw new UnsupportedOperationException("resolveAlias not implemented");
   }
 
+  @Override
+  public Map<String, String> getAliasProperties(String alias) {
+    throw new UnsupportedOperationException("getAliasProperties not implemented");
+  }
+
   @Override
   public ClusterState getClusterState() throws IOException {
     ensureNotClosed();

solr/solr-ref-guide/src/aliases.adoc

@@ -25,10 +25,9 @@ alternative names for collections are known as aliases, and are useful when you
 . Insulate the client programming versus changes in collection names
 . Issue a single query against several collections with identical schemas
 
-It's also possible to send update commands to aliases, but this is rarely useful if the
-  alias refers to more than one collection (as in case 3 above).
-Since there is no logic by which to distribute documents among the collections, all updates will simply be
-  directed to the first collection in the list.
+It's also possible to send update commands to aliases, but only to those that either resolve to a single collection
+or those that define the routing between multiple collections (Routed Aliases). In other cases update commands are
+rejected with an error since there is no logic by which to distribute documents among the multiple collections.
 
 Standard aliases are created and updated using the <<collections-api.adoc#createalias,CREATEALIAS>> command.
 The current list of collections that are members of an alias can be verified via the
@@ -56,7 +55,7 @@ collection name is expected. This works only when the following criteria are sat
 * an alias must not refer to a Routed Alias (see below)
 
 If all criteria are satisfied then the command will resolve alias names and operate on the collections the aliases
-refer to, as if it was invoked with the collection names instead. Otherwise the command will not be executed and
+refer to as if it was invoked with the collection names instead. Otherwise the command will not be executed and
 an exception will be thrown.
 
 == Routed Aliases

solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/DelegatingClusterStateProvider.java

@@ -63,6 +63,15 @@ public class DelegatingClusterStateProvider implements ClusterStateProvider {
     }
   }
 
+  @Override
+  public Map<String, String> getAliasProperties(String alias) {
+    if (delegate != null) {
+      return delegate.getAliasProperties(alias);
+    } else {
+      return Collections.emptyMap();
+    }
+  }
+
   @Override
   public String resolveSimpleAlias(String alias) throws IllegalArgumentException {
     if (delegate != null) {

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

@@ -413,9 +413,15 @@ public abstract class BaseCloudSolrClient extends SolrClient {
       throw new SolrServerException("No collection param specified on request and no default collection has been set.");
     }
 
-    //Check to see if the collection is an alias.
+    //Check to see if the collection is an alias. Updates to multi-collection aliases are ok as long
+    // as they are routed aliases
     List<String> aliasedCollections = getClusterStateProvider().resolveAlias(collection);
-    collection = aliasedCollections.get(0); // pick 1st (consistent with HttpSolrCall behavior)
+    if (getClusterStateProvider().isRoutedAlias(collection) || aliasedCollections.size() == 1) {
+      collection = aliasedCollections.get(0); // pick 1st (consistent with HttpSolrCall behavior)
+    } else {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Update request to non-routed multi-collection alias not supported: "
+        + collection + " -> " + aliasedCollections);
+    }
 
     DocCollection col = getDocCollection(collection, null);
 
@@ -786,8 +792,9 @@ public abstract class BaseCloudSolrClient extends SolrClient {
       isCollectionRequestOfV2 = ((V2Request) request).isPerCollectionRequest();
     }
     boolean isAdmin = ADMIN_PATHS.contains(request.getPath());
+    boolean isUpdate = (request instanceof IsUpdateRequest) && (request instanceof UpdateRequest);
     if (!inputCollections.isEmpty() && !isAdmin && !isCollectionRequestOfV2) { // don't do _stateVer_ checking for admin, v2 api requests
-      Set<String> requestedCollectionNames = resolveAliases(inputCollections);
+      Set<String> requestedCollectionNames = resolveAliases(inputCollections, isUpdate);
 
       StringBuilder stateVerParamBuilder = null;
       for (String requestedCollection : requestedCollectionNames) {
@@ -957,9 +964,15 @@ public abstract class BaseCloudSolrClient extends SolrClient {
     connect();
 
     boolean sendToLeaders = false;
+    boolean isUpdate = false;
 
     if (request instanceof IsUpdateRequest) {
       if (request instanceof UpdateRequest) {
+        isUpdate = true;
+        if (inputCollections.size() > 1) {
+          throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Update request must be sent to a single collection " +
+              "or an alias: " + inputCollections);
+        }
         String collection = inputCollections.isEmpty() ? null : inputCollections.get(0); // getting first mimics HttpSolrCall
         NamedList<Object> response = directUpdate((AbstractUpdateRequest) request, collection);
         if (response != null) {
@@ -993,7 +1006,7 @@ public abstract class BaseCloudSolrClient extends SolrClient {
       }
 
     } else { // Typical...
-      Set<String> collectionNames = resolveAliases(inputCollections);
+      Set<String> collectionNames = resolveAliases(inputCollections, isUpdate);
       if (collectionNames.isEmpty()) {
         throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
             "No collection param specified on request and no default collection has been set: " + inputCollections);
@@ -1057,17 +1070,20 @@ public abstract class BaseCloudSolrClient extends SolrClient {
   }
 
   /** Resolves the input collections to their possible aliased collections. Doesn't validate collection existence. */
-  private LinkedHashSet<String> resolveAliases(List<String> inputCollections) {
-    LinkedHashSet<String> collectionNames = new LinkedHashSet<>(); // consistent ordering
+  private Set<String> resolveAliases(List<String> inputCollections, boolean isUpdate) {
+    if (inputCollections.isEmpty()) {
+      return Collections.emptySet();
+    }
+    LinkedHashSet<String> uniqueNames = new LinkedHashSet<>(); // consistent ordering
     for (String collectionName : inputCollections) {
       if (getClusterStateProvider().getState(collectionName) == null) {
         // perhaps it's an alias
-        collectionNames.addAll(getClusterStateProvider().resolveAlias(collectionName));
+        uniqueNames.addAll(getClusterStateProvider().resolveAlias(collectionName));
       } else {
-        collectionNames.add(collectionName); // it's a collection
+        uniqueNames.add(collectionName); // it's a collection
       }
     }
-    return collectionNames;
+    return uniqueNames;
   }
 
   public boolean isUpdatesToLeaders() {

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

@@ -31,6 +31,7 @@ import org.apache.solr.client.solrj.SolrClient;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.client.solrj.response.CollectionAdminResponse;
 import org.apache.solr.common.cloud.Aliases;
 import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.cloud.ZkStateReader;
@@ -50,6 +51,7 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid
   volatile Set<String> liveNodes;
   long liveNodesTimestamp = 0;
   volatile Map<String, List<String>> aliases;
+  volatile Map<String, Map<String, String>> aliasProperties;
   long aliasesTimestamp = 0;
 
   private int cacheTimeout = 5; // the liveNodes and aliases cache will be invalidated after 5 secs
@@ -212,7 +214,9 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid
         String baseUrl = Utils.getBaseUrlForNodeName(nodeName, urlScheme);
         try (SolrClient client = getSolrClient(baseUrl)) {
 
-          this.aliases = new CollectionAdminRequest.ListAliases().process(client).getAliasesAsLists();
+          CollectionAdminResponse response = new CollectionAdminRequest.ListAliases().process(client);
+          this.aliases = response.getAliasesAsLists();
+          this.aliasProperties = response.getAliasProperties(); // side-effect
           this.aliasesTimestamp = System.nanoTime();
           return Collections.unmodifiableMap(this.aliases);
         } catch (SolrServerException | RemoteSolrException | IOException e) {
@@ -221,6 +225,7 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid
             log.warn("LISTALIASES not found, possibly using older Solr server. Aliases won't work"
                 + " unless you re-create the CloudSolrClient using zkHost(s) or upgrade Solr server", e);
             this.aliases = Collections.emptyMap();
+            this.aliasProperties = Collections.emptyMap();
             this.aliasesTimestamp = System.nanoTime();
             return aliases;
           }
@@ -238,6 +243,12 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid
     }
   }
 
+  @Override
+  public Map<String, String> getAliasProperties(String alias) {
+    getAliases(false);
+    return Collections.unmodifiableMap(aliasProperties.get(alias));
+  }
+
   @Override
   public ClusterState getClusterState() throws IOException {
     for (String nodeName: liveNodes) {

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

@@ -24,6 +24,7 @@ import java.util.Set;
 import org.apache.solr.common.SolrCloseable;
 import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.params.CollectionAdminParams;
 
 public interface ClusterStateProvider extends SolrCloseable {
 
@@ -44,6 +45,11 @@ public interface ClusterStateProvider extends SolrCloseable {
    */
   List<String> resolveAlias(String alias);
 
+  /**
+   * Return alias properties, or an empty map if the alias has no properties.
+   */
+  Map<String, String> getAliasProperties(String alias);
+
   /**
    * Given a collection alias, return a single collection it points to, or the original name if it's not an
    * alias.
@@ -57,6 +63,13 @@ public interface ClusterStateProvider extends SolrCloseable {
     return aliases.get(0);
   }
 
+  /**
+   * Returns true if an alias exists and is a routed alias, false otherwise.
+   */
+  default boolean isRoutedAlias(String alias) {
+    return getAliasProperties(alias).entrySet().stream().anyMatch(e -> e.getKey().startsWith(CollectionAdminParams.ROUTER_PREFIX));
+  }
+
   /**
    * Obtain the current cluster state.
    */

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

@@ -88,6 +88,11 @@ public class ZkClientClusterStateProvider implements ClusterStateProvider {
     return getZkStateReader().getAliases().resolveAliases(alias); // if not an alias, returns itself
   }
 
+  @Override
+  public Map<String, String> getAliasProperties(String alias) {
+    return getZkStateReader().getAliases().getCollectionAliasProperties(alias);
+  }
+
   @Override
   public String resolveSimpleAlias(String alias) throws IllegalArgumentException {
     return getZkStateReader().getAliases().resolveSimpleAlias(alias);

solr/solrj/src/java/org/apache/solr/client/solrj/response/CollectionAdminResponse.java

@@ -83,6 +83,14 @@ public class CollectionAdminResponse extends SolrResponseBase
     return Aliases.convertMapOfCommaDelimitedToMapOfList(getAliases());
   }
 
+  public Map<String, Map<String, String>> getAliasProperties() {
+    NamedList<Object> response = getResponse();
+    if (response.get("properties") != null) {
+      return ((Map<String, Map<String, String>>)response.get("properties"));
+    }
+    return Collections.emptyMap();
+  }
+
   @SuppressWarnings("unchecked")
   public Map<String, NamedList<Integer>> getCollectionNodesStatus()
   {

solr/solrj/src/java/org/apache/solr/common/cloud/Aliases.java

@@ -189,6 +189,20 @@ public class Aliases {
     return collectionAliases.containsKey(aliasName);
   }
 
+  /**
+   * Returns true if an alias exists and is a routed alias, false otherwise.
+   */
+  public boolean isRoutedAlias(String aliasName) {
+    if (!collectionAliases.containsKey(aliasName)) {
+      return false;
+    }
+    Map<String, String> props = collectionAliasProperties.get(aliasName);
+    if (props == null) {
+      return false;
+    }
+    return props.entrySet().stream().anyMatch(e -> e.getKey().startsWith(CollectionAdminParams.ROUTER_PREFIX));
+  }
+
   /**
    * Resolve an alias that points to a single collection. One level of alias indirection is supported.
    * @param aliasName alias name