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


##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameCollectionAPI.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+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.RenameCollectionPayload;
+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 = {"/collections/{collection}"},
+        method = POST,
+        permission = COLL_EDIT_PERM)
+public class RenameCollectionAPI {

Review Comment:
   [-1] While this API might be correct, it needs to be registered with 
`CollectionsHandler` to be accessible.
   
   See `CollectionsHandler.getApis()` and add a similar line for this class to 
that method.



##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameCollectionAPI.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+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.RenameCollectionPayload;
+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 = {"/collections/{collection}"},

Review Comment:
   I appreciate the switch you made away from the `/cluster` path, but this 
still deviates a bit from the API syntax I mentioned in my previous comment:
   
   > 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"}
   
   Specifically: here "rename" appears as a key inside the JSON body/payload 
instead of at the end of the path (`/collections/origCollName/commands/rename`) 
as I proposed previously.  Was there a reason you kept "rename" where it is 
currently?
   
   If there was a particular reason, let's discuss and decide what's best.  If 
there's no particular reason though; I'd vote we aim for the syntax I mentioned 
above, since it's gotten a loose 👍 from the community and is consistent with 
what we're doing with other APIs going forward.
   
   (The `{"rename": {...}}` syntax is consistent with a lot of APIs currently, 
but we're aiming to move away from that format.  See SOLR-15781 for some 
context on that.)
   
   It might be a bit tough to do this with the `PayloadObj<>` method signature, 
but we should be able to do it either with something like the snippet below, or 
a larger change to the JAX-RS based API framework which is much more flexible 
on these sort of things:
   
   ```
       private static final ObjectMapper mapper = 
SolrJacksonAnnotationInspector.createObjectMapper()
               .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, 
false)
               .disable(MapperFeature.AUTO_DETECT_FIELDS);
   
       @EndPoint(
               path = {"/collections/{collection}/commands/rename"},
               method = POST,
               permission = COLL_EDIT_PERM)
       public void renameCollection(SolrQueryRequest req, SolrQueryResponse 
rsp) throws Exception {
           final ContentStream cs = req.getContentStreams().iterator().next(); 
// Bit hacky, error handling needed
           RenameCollectionPayload v2Body = mapper.readValue(cs.getStream(), 
RenameCollectionPayload.class);
       ...
   }
   ```



##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameCollectionAPI.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+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.RenameCollectionPayload;
+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 = {"/collections/{collection}"},
+        method = POST,
+        permission = COLL_EDIT_PERM)
+public class RenameCollectionAPI {
+
+    private static final String V2_RENAME_COLLECTION_CMD = "rename";
+    private final CollectionsHandler collectionsHandler;
+
+    public RenameCollectionAPI(CollectionsHandler collectionsHandler) {
+        this.collectionsHandler = collectionsHandler;
+    }
+
+    @Command(name = V2_RENAME_COLLECTION_CMD)
+    public void renameCollection(PayloadObj<RenameCollectionPayload> obj) 
throws Exception {
+       final RenameCollectionPayload v2Body = obj.get();
+        final Map<String, Object> v1Params = v2Body.toMap(new HashMap<>());
+
+        v1Params.put(
+                CollectionParams.ACTION, 
CollectionParams.CollectionAction.RENAME.name().toLowerCase(Locale.ROOT));
+        v1Params.put(
+                CollectionAdminParams.COLLECTION, 
obj.getRequest().getPathTemplateValues().get(CollectionAdminParams.COLLECTION));

Review Comment:
   [-1] As far as I can tell, the v1 parameter representing the existing 
collection to "rename" is `name`, not `collection`.
   
   At least, that's what I'm gathering from the docs 
[here](https://solr.apache.org/guide/8_10/collection-management.html#rename-command-parameters).
  Are those docs wrong? (That's not sarcasm - it wouldn't surprise me at all if 
they were)  Or are the docs correct and the first argument to `v1Params.put` 
should be `name` or some constant with that value?



##########
solr/core/src/test/org/apache/solr/handler/admin/api/V2CollectionAPIMappingTest.java:
##########
@@ -85,6 +85,18 @@ public void testGetCollectionStatus() throws Exception {
     assertEquals("shard2", v1Params.get(SHARD));
   }
 
+
+  @Test
+  public void testRenameCollectionAllParams() throws Exception {
+    final SolrParams v1Params = captureConvertedV1Params(
+            "/collections/collName", "POST", "{\"rename\": {\"to\": 
\"targetColl\"}}");

Review Comment:
   [0] This test is named "AllParams" but it doesn't actually cover all of the 
params accepted by the rename API.
   
   What do you think about adding "followAliases" and "async" to the payload in 
this line, and adding assertions for those parameters below to ensure they get 
mapped correctly?



-- 
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-unsubscr...@solr.apache.org

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


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

Reply via email to