[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r997139202


##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameClusterAPI.java:
##########
@@ -0,0 +1,46 @@
+package org.apache.solr.handler.admin.api;
+
+import org.apache.solr.api.Command;
+import org.apache.solr.api.EndPoint;
+import org.apache.solr.api.PayloadObj;
+import org.apache.solr.client.solrj.request.beans.RenameClusterPayload;
+import org.apache.solr.common.params.CollectionAdminParams;
+import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.handler.admin.CollectionsHandler;
+
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+
+import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+
+@EndPoint(
+        path = {"/clusters/rename/{cluster}"},
+        method = POST,
+        permission = COLL_EDIT_PERM)
+public class RenameClusterAPI {

Review Comment:
   [-1] The RENAME operation (afaik?) renames a "collection", not a "cluster".  (Actually, I don't even think a SolrCloud cluster has a name, in that sense...)
   
   So we should probably change the name of a few things here to make it clearer what is actually being renamed.  `RenameCollectionAPI` maybe?
   
   The API should probably also change, since that path implies that it's the cluster being renamed, not an individual collection.
   
   I put together a [spreadsheet](https://docs.google.com/spreadsheets/d/1HAoBBFPpSiT8mJmgNZKkZAPwfCfPvlc08m5jz3fQBpA/edit?usp=sharing) awhile back laying out Solr's current APIs (with some suggestions on how to close gaps and be a bit more consistent).  It's not authoritative strictly speaking, so we can definitely consider other options, but that sheet proposes the following for our v2 rename operation: `POST /api/collections/origCollName/commands/rename {"to": "newName"}`
   
   What do you think?
   
   [0] I suspect you just hadn't gotten to it yet, but don't forget to go into CollectionsHandler.getApis() and add a line there to register this new API! 



##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameClusterAPI.java:
##########
@@ -0,0 +1,46 @@
+package org.apache.solr.handler.admin.api;

Review Comment:
   [-1] Source code in Solr uses a particular copyright header in all files, and we'll need to add this here.  (You can find it at the very top of any `.java` source file and copy/paste it from there).
   
   I have a really hard time remembering this, so I've found it really helpful to have my editor add this into all new files automatically by setting up what's essentially a "template" for new source code files.  I use IntelliJ personally, so I'll link the docs for that one [here](https://www.jetbrains.com/help/idea/using-file-and-code-templates.html#create-new-template).



##########
solr/core/src/test/org/apache/solr/handler/admin/api/V2ClusterAPIMappingTest.java:
##########
@@ -0,0 +1,50 @@
+package org.apache.solr.handler.admin.api;
+
+import org.apache.solr.api.ApiBag;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.handler.ClusterAPI;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.handler.admin.ConfigSetsHandler;
+import org.apache.solr.handler.admin.V2ApiMappingTest;
+import org.apache.solr.request.SolrQueryRequest;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import static org.apache.solr.common.params.CollectionAdminParams.COLLECTION;
+import static org.apache.solr.common.params.CollectionAdminParams.TARGET;
+import static org.apache.solr.common.params.CommonParams.ACTION;
+import static org.mockito.Mockito.mock;
+
+
+public class V2ClusterAPIMappingTest extends V2ApiMappingTest<CollectionsHandler> {
+
+@Test
+public void testRenameClusterAllParams() throws Exception {

Review Comment:
   [-1] Is this the test that failed with the error message you mentioned in JIRA?
   
   ```
   org.apache.solr.handler.admin.api.V2CollectionAPIMappingTest > testRenameCluster FAILED
       java.lang.NullPointerException: Cannot invoke "org.apache.solr.api.Api.call(org.apache.solr.request.SolrQueryRequest, org.apache.solr.response.SolrQueryResponse)" because "api" is null
   ```
   
   Or has this test changed since you posted that message?  Well, obviously you moved the test-case into its own class I guess, but have there been other changes?  Are you still seeing the test fail in the same way?
   
   Assuming you are...
   
   One thing that jumps out to me is that `populateApiBag` is unimplemented here, which would probably cause the failure you mentioned in JIRA.
   
   Some helpful context, `ApiBag` is how Solr traditionally matches v2 API requests to specific API classes/methods.  On startup, Solr loads all the APIs into this "bag" class, and then at request time it [looks up](https://github.com/apache/solr/blob/6786ec032727d7d21cfdfb5705d60f4d8368a1cd/solr/core/src/java/org/apache/solr/api/ApiBag.java#L340) the right API based on the HTTP path, method, etc.
   
   This test doesn't run a full Solr though, so for it to find and run your API, you need to write a bit of code to add it yourself in the `populateApiBag` method.  Check out the `populateApiBag` and `createUnderlyingRequestHandler` implementations on [one of the other "Mapping" tests](https://github.com/apache/solr/blob/db23d7d48d5ef2cf84ff8f8de38209698db172d4/solr/core/src/test/org/apache/solr/handler/admin/V2CollectionsAPIMappingTest.java#L54) for examples on how this works.
   
   Hope that helps!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org