SOLR-7298: Fix Collections API SolrJ calls to not add name parameter when not needed

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1669426 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -328,6 +328,9 @@ Bug Fixes
 * SOLR-7293: Fix bug that Solr server does not listen on IPv6 interfaces by default.
   (Uwe Schindler, Sebastian Pesman)
 
+* SOLR-7298: Fix Collections API calls (SolrJ) to not add name parameter when not needed.
+  (Shai Erera, Anshum Gupta)
+
 Optimizations
 ----------------------
 

solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java

@@ -47,27 +47,25 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
 
   private static String PROPERTY_PREFIX = "property.";
 
-  protected void setAction( CollectionAction action ) {
+  protected void setAction(CollectionAction action) {
     this.action = action;
   }
 
-  public CollectionAdminRequest()
+  public CollectionAdminRequest() {
-  {
+    super(METHOD.GET, "/admin/collections");
-    super( METHOD.GET, "/admin/collections" );
   }
 
-  public CollectionAdminRequest( String path )
+  public CollectionAdminRequest(String path) {
-  {
+    super(METHOD.GET, path);
-    super( METHOD.GET, path );
   }
 
   @Override
   public SolrParams getParams() {
-    if( action == null ) {
+    if (action == null) {
       throw new RuntimeException( "no action specified!" );
     }
     ModifiableSolrParams params = new ModifiableSolrParams();
-    params.set( CoreAdminParams.ACTION, action.toString() );
+    params.set(CoreAdminParams.ACTION, action.toString());
     return params;
   }
 
@@ -98,8 +96,7 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
   protected static class CollectionSpecificAdminRequest extends CollectionAdminRequest {
     protected String collection = null;
 
-    public final void setCollectionName( String collectionName )
+    public final void setCollectionName(String collectionName) {
-    {
       this.collection = collectionName;
     }
     
@@ -113,16 +110,30 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
 
   }
   
-  protected static class CollectionShardAdminRequest extends CollectionSpecificAdminRequest {
+  protected static class CollectionShardAdminRequest extends CollectionAdminRequest {
     protected String shardName = null;
+    protected String collection = null;
 
-    public void setShardName(String shard) { this.shardName = shard; }
+    public void setCollectionName(String collectionName) {
-    public String getShardName() { return this.shardName; }
+      this.collection = collectionName;
+    }
+    
+    public String getCollectionName() {
+      return collection;
+    }
+    
+    public void setShardName(String shard) {
+      this.shardName = shard;
+    }
+    
+    public String getShardName() {
+      return this.shardName;
+    }
 
     public ModifiableSolrParams getCommonParams() {
       ModifiableSolrParams params = (ModifiableSolrParams) super.getParams();
-      params.set( "collection", collection );
+      params.set(CoreAdminParams.COLLECTION, collection);
-      params.set( "shard", shardName);
+      params.set(CoreAdminParams.SHARD, shardName);
       return params;
     }
 
@@ -180,7 +191,6 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
     protected Integer stateFormat;
     protected String asyncId;
 
-
     public Create() {
       action = CollectionAction.CREATE;
     }
@@ -297,8 +307,10 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = getCommonParams();
-      params.set( "createNodeSet", nodeSet);
+      if (nodeSet != null) {
-      if(properties != null) {
+        params.set("createNodeSet", nodeSet);
+      }
+      if (properties != null) {
         addProperties(params, properties);
       }
       return params;
@@ -376,13 +388,18 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
       action = CollectionAction.REQUESTSTATUS;
     }
 
-    public void setRequestId(String requestId) {this.requestId = requestId; }
+    public void setRequestId(String requestId) {
-    public String getRequestId() { return this.requestId; }
+      this.requestId = requestId;
+    }
+    
+    public String getRequestId() {
+      return this.requestId;
+    }
 
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = (ModifiableSolrParams) super.getParams();
-      params.set("requestid", requestId);
+      params.set(CoreAdminParams.REQUESTID, requestId);
       return params;
     }
   }
@@ -404,8 +421,13 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
       return aliasName;
     }
     
-    public void setAliasedCollections(String alias) { this.aliasedCollections = alias; }
+    public void setAliasedCollections(String alias) {
-    public String getAliasedCollections() { return this.aliasedCollections; }
+      this.aliasedCollections = alias;
+    }
+    
+    public String getAliasedCollections() {
+      return this.aliasedCollections;
+    }
     
     @Deprecated
     public void setCollectionName(String aliasName) {
@@ -415,8 +437,8 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = (ModifiableSolrParams) super.getParams();
-      params.set("name", aliasName);
+      params.set(CoreAdminParams.NAME, aliasName);
-      params.set( "collections", aliasedCollections );
+      params.set("collections", aliasedCollections);
       return params;
     }
   }
@@ -436,7 +458,7 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = new ModifiableSolrParams(super.getParams());
-      params.set("name", aliasName);
+      params.set(CoreAdminParams.NAME, aliasName);
       return params;
     }
   }
@@ -498,13 +520,15 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
     public SolrParams getParams() {
       ModifiableSolrParams params = new ModifiableSolrParams(super.getParams());
       if (shardName == null || shardName.isEmpty()) {
-        params.remove("shard");
+        params.remove(CoreAdminParams.SHARD);
         if (routeKey == null) {
           throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Either shard or routeKey must be provided");
         }
         params.add(ShardParams._ROUTE_, routeKey);
       }
+      if (asyncId != null) {
         params.set("async", asyncId);
+      }
       if (node != null) {
         params.add("node", node);
       }
@@ -591,9 +615,10 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
       return this.propertyValue;
     }
     
+    @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = new ModifiableSolrParams(super.getParams());
-      params.add("name", propertyName);
+      params.add(CoreAdminParams.NAME, propertyName);
       params.add("val", propertyValue);
       
       return params;
@@ -602,7 +627,8 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
   }
   
   // MIGRATE request
-  public static class Migrate extends CollectionSpecificAdminRequest {
+  public static class Migrate extends CollectionAdminRequest {
+    private String collection;
     private String targetCollection;
     private String splitKey;
     private Integer forwardTimeout;
@@ -613,6 +639,14 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
       action = CollectionAction.MIGRATE;
     }
     
+    public void setCollectionName(String collection) {
+      this.collection = collection;
+    }
+    
+    public String getCollectionName() {
+      return collection;
+    }
+    
     public void setTargetCollection(String targetCollection) {
       this.targetCollection = targetCollection;
     }
@@ -648,15 +682,15 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = new ModifiableSolrParams(super.getParams());
-      params.set( "collection", collection );
+      params.set(CoreAdminParams.COLLECTION, collection);
       params.set("target.collection", targetCollection);
       params.set("split.key", splitKey);
-      if(forwardTimeout != null) {
+      if (forwardTimeout != null) {
         params.set("forward.timeout", forwardTimeout);
       }
       params.set("async", asyncId);
       
-      if(properties != null) {
+      if (properties != null) {
         addProperties(params, properties);
       }
       
@@ -694,12 +728,43 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
   }
 
   // CLUSTERSTATUS request
-  public static class ClusterStatus extends CollectionShardAdminRequest {
+  public static class ClusterStatus extends CollectionAdminRequest {
+    
+    protected String shardName = null;
+    protected String collection = null;
     
     public ClusterStatus () {
       action = CollectionAction.CLUSTERSTATUS;
     }
     
+    public void setCollectionName(String collectionName) {
+      this.collection = collectionName;
+    }
+    
+    public String getCollectionName() {
+      return collection;
+    }
+    
+    public void setShardName(String shard) {
+      this.shardName = shard;
+    }
+    
+    public String getShardName() {
+      return this.shardName;
+    }
+
+    @Override
+    public SolrParams getParams() {
+      ModifiableSolrParams params = (ModifiableSolrParams) super.getParams();
+      if (collection != null) {
+        params.set(CoreAdminParams.COLLECTION, collection);
+      }
+      if (shardName != null) {
+        params.set(CoreAdminParams.SHARD, shardName);
+      }
+      return params;
+    }
+    
   }
 
   // LIST request
@@ -755,12 +820,13 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = new ModifiableSolrParams(super.getParams());
-      params.set("replica", replica);
+      params.set(CoreAdminParams.REPLICA, replica);
       params.set("property", propertyName);
       params.set("property.value", propertyValue);
       
-      if(shardUnique != null)
+      if (shardUnique != null) {
         params.set("shardUnique", shardUnique);
+      }
       
       return params;
     }
@@ -847,7 +913,7 @@ public class CollectionAdminRequest extends SolrRequest<CollectionAdminResponse>
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = new ModifiableSolrParams(super.getParams());
-      params.set("collection", collection);
+      params.set(CoreAdminParams.COLLECTION, collection);
       params.set("property", propertyName);
       if(onlyActiveNodes != null)
         params.set("onlyactivenodes", onlyActiveNodes);

solr/solrj/src/java/org/apache/solr/common/params/CoreAdminParams.java

@@ -74,6 +74,9 @@ public abstract class CoreAdminParams
   /** The collection name in solr cloud */
   public final static String COLLECTION = "collection";
 
+  /** The replica name in solr cloud */
+  public final static String REPLICA = "replica";
+  
   /** The shard id in solr cloud */
   public final static String SHARD = "shard";
   

solr/solrj/src/test/org/apache/solr/client/solrj/CollectionAdminRequestRequiredParamsTest.java

@@ -0,0 +1,200 @@
+package org.apache.solr.client.solrj;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import static org.apache.solr.common.params.CoreAdminParams.*;
+
+import java.util.Iterator;
+import java.util.Set;
+
+import org.apache.lucene.util.LuceneTestCase;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.common.params.ShardParams;
+import org.apache.solr.common.params.SolrParams;
+
+import com.google.common.collect.Sets;
+
+/**
+ * Tests that default {@link CollectionAdminRequest#getParams()} returns only
+ * the required parameters of this request, and none other.
+ */
+public class CollectionAdminRequestRequiredParamsTest extends LuceneTestCase {
+
+  public void testBalanceShardUnique() {
+    final CollectionAdminRequest.BalanceShardUnique request = new CollectionAdminRequest.BalanceShardUnique();
+    request.setCollection("foo");
+    request.setPropertyName("prop");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, "property");
+  }
+  
+  public void testClusterProp() {
+    final CollectionAdminRequest.ClusterProp request = new CollectionAdminRequest.ClusterProp();
+    request.setPropertyName("foo");
+    request.setPropertyValue("bar");
+    assertContainsParams(request.getParams(), ACTION, NAME, "val");
+  }
+  
+  public void testAddRole() {
+    final CollectionAdminRequest.AddRole request = new CollectionAdminRequest.AddRole();
+    request.setNode("node");
+    request.setRole("role");
+    assertContainsParams(request.getParams(), ACTION, "node", "role");
+  }
+  
+  public void testRemoveRole() {
+    final CollectionAdminRequest.RemoveRole request = new CollectionAdminRequest.RemoveRole();
+    request.setNode("node");
+    request.setRole("role");
+    assertContainsParams(request.getParams(), ACTION, "node", "role");
+  }
+  
+  public void testAddReplica() {
+    // with shard parameter
+    CollectionAdminRequest.AddReplica request = new CollectionAdminRequest.AddReplica();
+    request.setShardName("shard");
+    request.setCollectionName("collection");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, SHARD);
+    
+    // with route parameter
+    request = new CollectionAdminRequest.AddReplica();
+    request.setRouteKey("route");
+    request.setCollectionName("collection");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, ShardParams._ROUTE_);
+  }
+  
+  public void testAddReplicaProp() {
+    final CollectionAdminRequest.AddReplicaProp request = new CollectionAdminRequest.AddReplicaProp();
+    request.setShardName("shard");
+    request.setCollectionName("collection");
+    request.setReplica("replica");
+    request.setPropertyName("prop");
+    request.setPropertyValue("value");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, SHARD, REPLICA, "property", "property.value");
+  }
+  
+  public void testClusterStatus() {
+    final CollectionAdminRequest.ClusterStatus request = new CollectionAdminRequest.ClusterStatus();
+    assertContainsParams(request.getParams(), ACTION);
+  }
+  
+  public void testCreateShard() {
+    final CollectionAdminRequest.CreateShard request = new CollectionAdminRequest.CreateShard();
+    request.setCollectionName("collection");
+    request.setShardName("shard");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, SHARD);
+  }
+  
+  public void testDeleteReplica() {
+    final CollectionAdminRequest.DeleteReplica request = new CollectionAdminRequest.DeleteReplica();
+    request.setCollectionName("collection");
+    request.setShardName("shard");
+    request.setReplica("replica");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, SHARD, REPLICA);
+  }
+  
+  public void testDeleteReplicaProp() {
+    final CollectionAdminRequest.DeleteReplicaProp request = new CollectionAdminRequest.DeleteReplicaProp();
+    request.setCollectionName("collection");
+    request.setShardName("shard");
+    request.setReplica("replica");
+    request.setPropertyName("foo");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, SHARD, REPLICA, "property");
+  }
+  
+  public void testDeleteShard() {
+    final CollectionAdminRequest.DeleteShard request = new CollectionAdminRequest.DeleteShard();
+    request.setCollectionName("collection");
+    request.setShardName("shard");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, SHARD);
+  }
+  
+  public void testSplitShard() {
+    final CollectionAdminRequest.SplitShard request = new CollectionAdminRequest.SplitShard();
+    request.setCollectionName("collection");
+    request.setShardName("shard");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, SHARD);
+  }
+
+  public void testCreateCollection() {
+    final CollectionAdminRequest.Create request = new CollectionAdminRequest.Create();
+    request.setCollectionName("collection");
+    assertContainsParams(request.getParams(), ACTION, NAME);
+  }
+  
+  public void testReloadCollection() {
+    final CollectionAdminRequest.Reload request = new CollectionAdminRequest.Reload();
+    request.setCollectionName("collection");
+    assertContainsParams(request.getParams(), ACTION, NAME);
+  }
+  
+  public void testDeleteCollection() {
+    final CollectionAdminRequest.Delete request = new CollectionAdminRequest.Delete();
+    request.setCollectionName("collection");
+    assertContainsParams(request.getParams(), ACTION, NAME);
+  }
+  
+  public void testCreateAlias() {
+    final CollectionAdminRequest.CreateAlias request = new CollectionAdminRequest.CreateAlias();
+    request.setAliasName("name");
+    request.setAliasedCollections("collections");
+    assertContainsParams(request.getParams(), ACTION, NAME, "collections");
+  }
+  
+  public void testDeleteAlias() {
+    final CollectionAdminRequest.DeleteAlias request = new CollectionAdminRequest.DeleteAlias();
+    request.setAliasName("name");
+    assertContainsParams(request.getParams(), ACTION, NAME);
+  }
+  
+  public void testListCollections() {
+    final CollectionAdminRequest.List request = new CollectionAdminRequest.List();
+    assertContainsParams(request.getParams(), ACTION);
+  }
+
+  public void testMigrate() {
+    final CollectionAdminRequest.Migrate request = new CollectionAdminRequest.Migrate();
+    request.setCollectionName("collection");
+    request.setTargetCollection("target");
+    request.setSplitKey("splitKey");
+    assertContainsParams(request.getParams(), ACTION, COLLECTION, "target.collection", "split.key");
+  }
+  
+  public void testOverseerStatus() {
+    final CollectionAdminRequest.OverseerStatus request = new CollectionAdminRequest.OverseerStatus();
+    assertContainsParams(request.getParams(), ACTION);
+  }
+  
+  public void testRequestStatus() {
+    final CollectionAdminRequest.RequestStatus request = new CollectionAdminRequest.RequestStatus();
+    request.setRequestId("request");
+    assertContainsParams(request.getParams(), ACTION, REQUESTID);
+  }
+
+  private void assertContainsParams(SolrParams solrParams, String... requiredParams) {
+    final Set<String> requiredParamsSet = Sets.newHashSet(requiredParams);
+    final Set<String> solrParamsSet = Sets.newHashSet();
+    for (Iterator<String> iter = solrParams.getParameterNamesIterator(); iter.hasNext();) {
+      solrParamsSet.add(iter.next());
+    }
+    assertTrue("required params missing: required=" + requiredParamsSet + ", params=" + solrParamsSet, 
+        solrParamsSet.containsAll(requiredParamsSet));
+    assertTrue("extra parameters included in request: required=" + requiredParamsSet + ", params=" + solrParams, 
+        Sets.difference(solrParamsSet, requiredParamsSet).isEmpty());
+  }
+  
+}