From e616ed49a6688c011e4854afc509dd13a7222b6f Mon Sep 17 00:00:00 2001 From: Andrzej Bialecki Date: Fri, 5 Jul 2019 09:17:57 +0200 Subject: [PATCH] SOLR-13583: Return 400 Bad Request instead of 500 Server Error when a complex alias is found but a simple alias was expected. --- .../api/collections/DeleteCollectionCmd.java | 5 +-- .../solr/cloud/AliasIntegrationTest.java | 33 +++++++++++++++++-- .../org/apache/solr/common/cloud/Aliases.java | 25 +++++++------- 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java index 98c7ca31443..6fe02ee84c1 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java @@ -202,13 +202,14 @@ public class DeleteCollectionCmd implements OverseerCollectionMessageHandler.Cmd zkStateReader.aliasesManager.update(); // aliases may have been stale; get latest from ZK aliases = zkStateReader.getAliases(); aliasesRefs = referencedByAlias(extCollection, aliases, followAliases); + String collection = followAliases ? aliases.resolveSimpleAlias(extCollection) : extCollection; if (aliasesRefs.size() > 0) { for (String alias : aliasesRefs) { // for back-compat in 8.x we don't automatically remove other // aliases that point only to this collection if (!extCollection.equals(alias)) { - throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, - "Collection : " + extCollection + " is part of aliases: " + aliasesRefs + ", remove or modify the aliases before removing this collection."); + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Collection : " + collection + " is part of aliases: " + aliasesRefs + ", remove or modify the aliases before removing this collection."); } else { aliasesToDelete.add(alias); } diff --git a/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java b/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java index c0d63ba11a0..60d2e3b2dfc 100644 --- a/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java @@ -317,7 +317,7 @@ public class AliasIntegrationTest extends SolrCloudTestCase { try { String resolved = stateProvider.resolveSimpleAlias(aliasName); fail("this is not a simple alias but it resolved to " + resolved); - } catch (IllegalArgumentException e) { + } catch (SolrException e) { // expected } } @@ -517,10 +517,37 @@ public class AliasIntegrationTest extends SolrCloudTestCase { // Now delete one of the collections, should fail since an alias points to it. RequestStatusState delResp = CollectionAdminRequest.deleteCollection("collection_one").processAndWait(cluster.getSolrClient(), 60); - + // failed because the collection is a part of a compound alias assertEquals("Should have failed to delete collection: ", delResp, RequestStatusState.FAILED); - // Now redefine the alias to only point to colletion two + CollectionAdminRequest.Delete delete = CollectionAdminRequest.deleteCollection("collection_alias_pair"); + delResp = delete.processAndWait(cluster.getSolrClient(), 60); + // failed because we tried to delete an alias with followAliases=false + assertEquals("Should have failed to delete alias: ", delResp, RequestStatusState.FAILED); + + delete.setFollowAliases(true); + delResp = delete.processAndWait(cluster.getSolrClient(), 60); + // failed because we tried to delete compound alias + assertEquals("Should have failed to delete collection: ", delResp, RequestStatusState.FAILED); + + CollectionAdminRequest.createAlias("collection_alias_one", "collection_one").process(cluster.getSolrClient()); + lastVersion = waitForAliasesUpdate(lastVersion, zkStateReader); + + delete = CollectionAdminRequest.deleteCollection("collection_one"); + delResp = delete.processAndWait(cluster.getSolrClient(), 60); + // failed because we tried to delete collection referenced by multiple aliases + assertEquals("Should have failed to delete collection: ", delResp, RequestStatusState.FAILED); + + delete = CollectionAdminRequest.deleteCollection("collection_alias_one"); + delete.setFollowAliases(true); + delResp = delete.processAndWait(cluster.getSolrClient(), 60); + // failed because we tried to delete collection referenced by multiple aliases + assertEquals("Should have failed to delete collection: ", delResp, RequestStatusState.FAILED); + + CollectionAdminRequest.deleteAlias("collection_alias_one").process(cluster.getSolrClient()); + lastVersion = waitForAliasesUpdate(lastVersion, zkStateReader); + + // Now redefine the alias to only point to collection two CollectionAdminRequest.createAlias("collection_alias_pair", "collection_two").process(cluster.getSolrClient()); lastVersion = waitForAliasesUpdate(lastVersion, zkStateReader); diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/Aliases.java b/solr/solrj/src/java/org/apache/solr/common/cloud/Aliases.java index f1fa3f1365a..3ba61b7ce14 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/Aliases.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/Aliases.java @@ -27,6 +27,7 @@ import java.util.Objects; import java.util.function.UnaryOperator; import java.util.stream.Collectors; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.CollectionAdminParams; import org.apache.solr.common.util.StrUtils; import org.apache.solr.common.util.Utils; @@ -207,29 +208,29 @@ public class Aliases { * @param aliasName alias name * @return original name if there's no such alias, or a resolved name. If an alias points to more than 1 * collection (directly or indirectly) an exception is thrown - * @throws IllegalArgumentException if either direct or indirect alias points to more than 1 name. + * @throws SolrException if either direct or indirect alias points to more than 1 name. */ - public String resolveSimpleAlias(String aliasName) throws IllegalArgumentException { + public String resolveSimpleAlias(String aliasName) throws SolrException { return resolveSimpleAliasGivenAliasMap(collectionAliases, aliasName); } /** @lucene.internal */ @SuppressWarnings("JavaDoc") public static String resolveSimpleAliasGivenAliasMap(Map> collectionAliasListMap, - String aliasName) throws IllegalArgumentException { + String aliasName) throws SolrException { List level1 = collectionAliasListMap.get(aliasName); if (level1 == null || level1.isEmpty()) { return aliasName; // simple collection name } if (level1.size() > 1) { - throw new IllegalArgumentException("Simple alias '" + aliasName + "' points to more than 1 collection: " + level1); + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Simple alias '" + aliasName + "' points to more than 1 collection: " + level1); } List level2 = collectionAliasListMap.get(level1.get(0)); if (level2 == null || level2.isEmpty()) { return level1.get(0); // simple alias } if (level2.size() > 1) { - throw new IllegalArgumentException("Simple alias '" + aliasName + "' resolves to '" + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Simple alias '" + aliasName + "' resolves to '" + level1.get(0) + "' which points to more than 1 collection: " + level2); } return level2.get(0); @@ -318,10 +319,10 @@ public class Aliases { * @param after new alias name. If this is null then it's equivalent to calling {@link #cloneWithCollectionAlias(String, String)} * with the second argument set to null, ie. removing an alias. * @return new instance with the renamed alias - * @throws IllegalArgumentException when either before or after is empty, or + * @throws SolrException when either before or after is empty, or * the before name is a routed alias */ - public Aliases cloneWithRename(String before, String after) { + public Aliases cloneWithRename(String before, String after) throws SolrException { if (before == null) { throw new NullPointerException("'before' and 'after' cannot be null"); } @@ -329,7 +330,7 @@ public class Aliases { return cloneWithCollectionAlias(before, after); } if (before.isEmpty() || after.isEmpty()) { - throw new IllegalArgumentException("'before' and 'after' cannot be empty"); + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'before' and 'after' cannot be empty"); } if (before.equals(after)) { return this; @@ -337,7 +338,7 @@ public class Aliases { Map props = collectionAliasProperties.get(before); if (props != null) { if (props.keySet().stream().anyMatch(k -> k.startsWith(CollectionAdminParams.ROUTER_PREFIX))) { - throw new IllegalArgumentException("source name '" + before + "' is a routed alias."); + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "source name '" + before + "' is a routed alias."); } } Map> newColProperties = new LinkedHashMap<>(this.collectionAliasProperties); @@ -397,12 +398,12 @@ public class Aliases { * @param properties the properties to add/replace, null values in the map will remove the key. * @return An immutable copy of the aliases with the new properties. */ - public Aliases cloneWithCollectionAliasProperties(String alias, Map properties) { + public Aliases cloneWithCollectionAliasProperties(String alias, Map properties) throws SolrException { if (!collectionAliases.containsKey(alias)) { - throw new IllegalArgumentException(alias + " is not a valid alias"); + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, alias + " is not a valid alias"); } if (properties == null) { - throw new IllegalArgumentException("Null is not a valid properties map"); + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Null is not a valid properties map"); } Map> newColProperties = new LinkedHashMap<>(this.collectionAliasProperties);//clone to modify Map newMetaMap = new LinkedHashMap<>(newColProperties.getOrDefault(alias, Collections.emptyMap()));