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/10/15 19:02:46 UTC
[GitHub] [solr] AAnakhe opened a new pull request, #1080: SOLR 15749 created a v2 equivalent of v1 'rename' #1061
AAnakhe opened a new pull request, #1080:
URL: https://github.com/apache/solr/pull/1080
https://issues.apache.org/jira/browse/SOLR-15749
# Description
Create a v2 equivalent of v1 'RENAME' /solr/admin/collections endpoint
# Solution
Created a RenameClusterAPI class and RenameClusterPayload class with an endpoint @EndPoint(path ={"/clusters/rename/{cluster}"}, method = POST, permission = COLL_EDIT_PERM)
# Tests
Created a V2ClusterAPIMappingTest class in solr.handler.admin.api and tested mapping to API endpoint
# Checklist
Please review the following and check all that apply:
- [√] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [√] I have created a Jira issue and added the issue ID to my pull request title.
- [√] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [√] I have developed this patch against the `main` branch.
- [√] I have run `./gradlew check`.
- [√] I have added tests for my changes.
- [] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1006815561
##########
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 was not sure how to fetch values from the payload class and implement them in my code but that was addressed in your last comment. This is what I have done
` @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,
v2Body.followAliases);
collectionsHandler.handleRequestBody(req, rsp);
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r998354973
##########
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:
Copyright header added to files, files template comes in handy, thanks
--
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
[GitHub] [solr] joshgog commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
joshgog commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1010113677
##########
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:
`Test result returns java.lang.AssertionError: expected:<rename> but was:<null>`
What if we mock `SolrQueryRequest` and `SolrQueryResponse` , pass them to the`public void renameCollection(SolrQueryRequest req, SolrQueryResponse rsp)` method of the API then perform the assertions
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1016093656
##########
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:
I thought you said we were moving away from the old captureConvertedV1Params syntax, I wasn't aware there are different method implementations
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r998016163
##########
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:
yes, It has changed since I last updated the failed test on Jira and the test fail the same way, following this review I will implement the test method on V2CollectionAPIMappingTest class and also recommendations made
--
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
[GitHub] [solr] joshgog commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
joshgog commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1010708916
##########
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:
I'm not sure what exactly you would like to be clarified. If its about mock checkout `AddReplicaPropertyAPITest` which uses mocked` SolrQueryRequest` and `SolrQueryResponse`
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r997255652
##########
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 (and on any other new files in this PR). (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 called 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).
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1017416496
##########
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:
It's clearer now, and I'm so glad to hear that this will be merged thank you.
--
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
[GitHub] [solr] gerlowskija merged pull request #1080: SOLR-15749: Create v2 equivalent for 'rename' collection API
Posted by GitBox <gi...@apache.org>.
gerlowskija merged PR #1080:
URL: https://github.com/apache/solr/pull/1080
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r997255652
##########
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 (and on any other new files in this PR). (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).
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
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-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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1010155274
##########
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:
I will try to implement that, thanks
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1006078777
##########
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:
Can you be a little more specific about what part is unclear or that you're stuck on? The code snippet in your last comment looks generally 👌 at a glance, except that:
1. The last argument in your `wrapParams` call should be the value of the "target" parameter. We used Jackson to parse that into a payload instance just a few lines above, so we can fetch the target value from that payload instance (e.g. `v2Body.to`)
2. You should add `async` and `followAliases` to the `wrapParams` invocation, handling those similarly to "target".
Happy to expand or clarify anything else as well, but you'll need to be a bit more specific what bit you're unclear on.
--
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
[GitHub] [solr] AAnakhe commented on pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on PR #1080:
URL: https://github.com/apache/solr/pull/1080#issuecomment-1297175272
Okay thanks, I will fix that shortly
On Mon, Oct 31, 2022 at 3:21 PM Jason Gerlowski ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In
> solr/core/src/test/org/apache/solr/handler/admin/api/V2CollectionAPIMappingTest.java
> <https://github.com/apache/solr/pull/1080#discussion_r1009477355>:
>
> > @@ -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\"}}");
>
> The first thing that jumps out at me in this test code is that you
> restructured the API endpoint to look the way we discussed. (i.e. now it
> looks like POST /collections/collName/commands/rename)....but this test
> still uses the old appearance/syntax. The path, request-body, etc. that
> gets sent into captureConvertedV1Params is all using the old syntax we
> moved away from. If you fix that, I'm betting this error will go away!
>
> As a general note on "process": generally speaking its easier for me (or
> others) to help review if you push up the code changes you've made, even if
> things aren't 100% working yet. That makes it easier for me to comment on
> particular lines. It makes it easier for me to pull the code down and run
> that test myself, etc. It saves you from having to paste your code in a
> comment box and fight the markdown/formatting, etc.
>
> Totally fine if you had reasons for not doing that here; just figured I'd
> mention the convention as a "heads up" in case it's helpful since you're
> newer to the community
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/solr/pull/1080#discussion_r1009477355>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AWIEDZG2KINRYWJVZZDM74TWF7IYLANCNFSM6AAAAAARGAWM5E>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by 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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1009477355
##########
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:
The first thing that jumps out at me in this test code is that you restructured the API endpoint to look the way we discussed. (i.e. now it looks like `POST /collections/collName/commands/rename`)....but this test still uses the old appearance/syntax. The path, request-body, etc. that gets sent into `captureConvertedV1Params` is all using the old syntax we moved away from. If you fix that, I'm betting this error will go away!
As a general note on "process": generally speaking its easier for me (or others) to help review if you push up the code changes you've made, even if things aren't 100% working yet. That makes it easier for me to comment on particular lines. It makes it easier for me to pull the code down and run that test myself, etc. It saves you from having to paste your code in a comment box and fight the markdown/formatting, etc.
Totally fine if you had reasons for not doing that here; just figured I'd mention the convention as a "heads up" in case it's helpful since you're newer to the community
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1005900863
##########
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:
API now registered with CollectionHandler.getApis()
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1005997856
##########
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 had no particular reason for placing it there. A little more clarification is needed here in other to implement this method, Here is my progress, kindly have a look
`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,
CollectionAdminParams.TARGET);
collectionsHandler.handleRequestBody(req, rsp);
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1010192031
##########
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:
> `Test result returns java.lang.AssertionError: expected:<rename> but was:<null>`
>
> What if we mock `SolrQueryRequest` and `SolrQueryResponse` , pass them to the`public void renameCollection(SolrQueryRequest req, SolrQueryResponse rsp)` method of the API then perform the assertions
Please I need more clarification on this
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1009724283
##########
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:
is this in order?
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1017074323
##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameCollectionAPI.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.params.CollectionAdminParams.FOLLOW_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;
+ }
+
+
+ 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"},
Review Comment:
[0] There was some [discussion on a different v2 API PR](https://github.com/apache/solr/pull/1115#issuecomment-1303995329) about the `/commands` path segment in this path, and the consensus was that we should drop that path segment.
I'll update the code here accordingly.
##########
build.gradle:
##########
@@ -207,5 +207,5 @@ apply from: file('gradle/ant-compat/solr.post-jar.gradle')
apply from: file('gradle/node.gradle')
-sourceCompatibility = JavaVersion.VERSION_17
-targetCompatibility = JavaVersion.VERSION_17
+//sourceCompatibility = JavaVersion.VERSION_16_PREVIEW
Review Comment:
[Q] Ooh, interesting. Did you intend to edit these lines @AAnakhe ?
A few months back I inadvertently committed changes to these two lines, but to this day I have no memory of editing them. I suspected it came from importing into my IDE or some similar operation but I never was able to figure out what did it.
Anyway, just curious if you intended these changes here. And if you didn't, if you had any guesses what might've edited them on your behalf?
--
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
[GitHub] [solr] joshgog commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
joshgog commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1010708916
##########
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:
I'm not sure what exactly you would like to be clarified. If its about mocks checkout `AddReplicaPropertyAPITest` which uses mocked` SolrQueryRequest` and `SolrQueryResponse`
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1006834433
##########
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:
This is my test method
` @Test
public void testRenameCollectionAllParams() throws Exception {
final SolrParams v1Params = captureConvertedV1Params(
"/collections/collName",
"POST",
"{ 'rename' : {"
+ "'to': 'targetColl',"
+ "'async': 'requestTrackingId',"
+ "'followAliases': true}}");
assertEquals("rename", v1Params.get(ACTION));
assertEquals("collName", v1Params.get(NAME));
assertEquals("targetColl", v1Params.get(TARGET));
assertEquals("requestTrackingId", v1Params.get(ASYNC));
assertEquals(true, v1Params.getPrimitiveBool("followAliases"));`
but I get this error
`Error in command payload, errors: [{errorMessages=[Unknown operation 'rename' available ops are '[modify, balance-shard-unique, delete-replica-property, migrate-docs, reload, move-replica, rebalance-leaders, set-collection-property]'], rename={to=targetColl, async=requestTrackingId, followAliases=true}}], `
--
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
[GitHub] [solr] joshgog commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
joshgog commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1010113677
##########
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:
`Test result returns java.lang.AssertionError: expected:<rename> but was:<null>`
What if we mock `SolrQueryRequest` and `SolrQueryResponse` and pass them to the`public void renameCollection(SolrQueryRequest req, SolrQueryResponse rsp)` method of the API then perform the assertions
--
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
[GitHub] [solr] AAnakhe commented on pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on PR #1080:
URL: https://github.com/apache/solr/pull/1080#issuecomment-1304264336
Thank you Jason, you do the same.
On Sat, Oct 15, 2022 at 8:51 PM sonatype-lift[bot] ***@***.***>
wrote:
> ⚠️ *314 God Classes* were detected by Lift in this project. Visit the
> Lift web console
> <https://lift.sonatype.com/results/github.com/apache/solr/01GFEGDZ81KM2ZP1W0ZBMPP3N9?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache%20solr>
> for more details.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/solr/pull/1080#issuecomment-1279821768>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AWIEDZD65G22DAN66STNGC3WDMDM5ANCNFSM6AAAAAARGAWM5E>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1014416757
##########
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:
Overall yep; I'll resolve this conversation. I did notice one smaller issue (particularly the ALIASES constant you're using towards the bottom of that snippet), but I've left a comment about that elsewhere so let's just close this out.
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR-15749: Create v2 equivalent for 'rename' collection API
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1020649278
##########
build.gradle:
##########
@@ -207,5 +207,5 @@ apply from: file('gradle/ant-compat/solr.post-jar.gradle')
apply from: file('gradle/node.gradle')
-sourceCompatibility = JavaVersion.VERSION_17
-targetCompatibility = JavaVersion.VERSION_17
+//sourceCompatibility = JavaVersion.VERSION_16_PREVIEW
Review Comment:
I didn't intend to make any changes there, it was imported by my IDE when I clicked on a confirmation request from IntelliJ requesting support for Java 16 or so, I can not recall 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-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
[GitHub] [solr] gerlowskija commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r1017025758
##########
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:
Ah, not quite. Or at least, I didn't mean to suggest that?
What I probably meant to say is that Solr is moving away from its traditional v2 API framework to a framework based on JAX-RS. `captureConvertedV1Params` is a nice utility for testing APIs implemented using the "traditional" (i.e. non JAX-RS) framework.
In the long run I'd expect to see usage of `captureConvertedV1Params` decline, but not because it's deprecated or shouldn't be used - just because things will switch over to JAX-RS as time goes by. This PR uses the traditional framework (which is absolutely still valid), so it's fine/normal to use `captureConvertedV1Params` in your tests.
I'll push up a commit here to help get the unit test over the line and then hopefully we can get this merged 👍
--
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
[GitHub] [solr] sonatype-lift[bot] commented on pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on PR #1080:
URL: https://github.com/apache/solr/pull/1080#issuecomment-1279821768
:warning: **314 God Classes** were detected by Lift in this project. [Visit the Lift web console](https://lift.sonatype.com/results/github.com/apache/solr/01GFEGDZ81KM2ZP1W0ZBMPP3N9?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache\%20solr) for more details.
--
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
[GitHub] [solr] AAnakhe commented on a diff in pull request #1080: SOLR 15749 created a v2 equivalent of v1 'rename'
Posted by GitBox <gi...@apache.org>.
AAnakhe commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r998008800
##########
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:
Thanks for the review, working on recommended changes. I will update you as I progress
--
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