You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/11/04 19:47:51 UTC
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1014393162
##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameCollectionAPI.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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 com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.MapperFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.api.EndPoint;
+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.common.util.ContentStream;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+
+import java.util.Locale;
+
+import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
+import static org.apache.solr.common.cloud.ZkStateReader.ALIASES;
+import static org.apache.solr.common.params.CollectionAdminParams.TARGET;
+import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
+import static org.apache.solr.common.params.CommonParams.ACTION;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+
+
+public class RenameCollectionAPI {
+
+
+
+ private final CollectionsHandler collectionsHandler;
+ public RenameCollectionAPI(CollectionsHandler collectionsHandler) {
+ this.collectionsHandler = collectionsHandler;
+ }
+
+ public static final String RENAME_COLLECTION_CMD = "rename";
Review Comment:
[0] This constant doesn't look like it's used anymore; can you delete?
##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameCollectionAPI.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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 com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.MapperFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.api.EndPoint;
+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.common.util.ContentStream;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+
+import java.util.Locale;
+
+import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
+import static org.apache.solr.common.cloud.ZkStateReader.ALIASES;
+import static org.apache.solr.common.params.CollectionAdminParams.TARGET;
+import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
+import static org.apache.solr.common.params.CommonParams.ACTION;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+
+
+public class RenameCollectionAPI {
+
+
+
+ private final CollectionsHandler collectionsHandler;
+ public RenameCollectionAPI(CollectionsHandler collectionsHandler) {
+ this.collectionsHandler = collectionsHandler;
+ }
+
+ public static final String RENAME_COLLECTION_CMD = "rename";
+
+ private static final ObjectMapper mapper = SolrJacksonAnnotationInspector.createObjectMapper()
+ .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
+ .disable(MapperFeature.AUTO_DETECT_FIELDS);
+
+ @EndPoint(
+ path = {"/collections/{collection}/command/rename"},
+ method = POST,
+ permission = COLL_EDIT_PERM)
+ public void renameCollection(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception{
+
+ final ContentStream cs = req.getContentStreams().iterator().next();
+ RenameCollectionPayload v2Body = mapper.readValue(cs.getStream(), RenameCollectionPayload.class);
+
+ req =
+ wrapParams(
+ req,
+ ACTION,
+ CollectionParams.CollectionAction.RENAME.name().toLowerCase(Locale.ROOT),
+ NAME,
+ req.getPathTemplateValues().get(CollectionAdminParams.COLLECTION),
+ TARGET,
+ v2Body.to,
+ ASYNC,
+ v2Body.async,
+ ALIASES,
Review Comment:
[-1] The value of this `ALIASES` constant is "/aliases.json"....probably not what you intended. The v1 parameter name is "followAliases"...there's a constant `FOLLOW_ALIASES` that you can import that has that value.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameCollectionAPI.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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 com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.MapperFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.api.EndPoint;
+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.common.util.ContentStream;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+
+import java.util.Locale;
+
+import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
+import static org.apache.solr.common.cloud.ZkStateReader.ALIASES;
+import static org.apache.solr.common.params.CollectionAdminParams.TARGET;
+import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
+import static org.apache.solr.common.params.CommonParams.ACTION;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+
+
+public class RenameCollectionAPI {
+
+
+
+ private final CollectionsHandler collectionsHandler;
+ public RenameCollectionAPI(CollectionsHandler collectionsHandler) {
+ this.collectionsHandler = collectionsHandler;
+ }
+
+ public static final String RENAME_COLLECTION_CMD = "rename";
+
+ private static final ObjectMapper mapper = SolrJacksonAnnotationInspector.createObjectMapper()
+ .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
+ .disable(MapperFeature.AUTO_DETECT_FIELDS);
+
+ @EndPoint(
+ path = {"/collections/{collection}/command/rename"},
Review Comment:
[-1] Oof, just noticing this now, I think we want "command" to be plural in this path. i.e. `/collections/{collection}/commands/rename`
##########
solr/core/src/test/org/apache/solr/handler/admin/api/V2CollectionAPIMappingTest.java:
##########
@@ -87,13 +96,15 @@ public void testGetCollectionStatus() throws Exception {
@Test
- public void testRenameCollectionAllParams() throws Exception {
- final SolrParams v1Params = captureConvertedV1Params(
- "/collections/collName", "POST", "{\"rename\": {\"to\": \"targetColl\"}}");
+ public void testRenameCollectionAllParams(){
+ final SolrQueryRequest request = runRenameCollectionsApi("/collections/collName");
Review Comment:
+1 to what Josh said. AddReplicaPropertyAPITest is a good example on how to use mocks, but it's setup for APIs that use the JAX-RS framework, so you wouldn't want to follow it too closely. But it's good if you're just looking for an example on the mock stuff.
And I'm sure we'd both be happy to clarify if you can elaborate a bit on **what** you need clarified.
[Q] One thing that jumps out at me on this line is the `runRenameCollectionsAPI`: is there a reason you've created a separate method for this instead of reusing one of the `captureConvertedV1Params` methods that's already out there. If there's not some reason for `runRenameCollectionsAPI` that I'm missing, you could presumably just replace its usage here with a snippet like:
```
final SolrParams v1Params = captureConvertedV1Params("/collections/collName/commands/rename", "POST",
"{\"to\": \"targetColl\", \"async\": \"requestTrackingId\", \"followAliases\": true}");
```
The second thing that jumps out at me here is the path being sent as an argument to this method - it doesn't match the path of the API you created in this PR (see [here](https://github.com/apache/solr/pull/1080/files#diff-e1a70c63cd0e418b5027514b9833ab6ebef6d4dd71026a7e440d9c061ef181c6R60)).
--
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