gerlowskija commented on code in PR #2072: URL: https://github.com/apache/solr/pull/2072#discussion_r1393262795
########## solr/solr-ref-guide/modules/configuration-guide/pages/coreadmin-api.adoc: ########## @@ -349,9 +349,19 @@ This parameter is required in v1, and part of the url in the v2 API. The `RENAME` action changes the name of a Solr core. +[.dynamic-tabs] Review Comment: [+1] Thanks as always for your attention to the docs! But.... [-1] I definitely see what you were going for in creating a different "Parameter" section for each API version. It's tough otherwise to be clear about which parameter names are used in v1 vs. v2. But in handling similar cases for other APIs, we've found a pattern that does this a bit more concisely without (I think) any loss of clarity. Essentially what we've been doing is just to put the v1 and v2 parameter names in the same "list item" separated by a comma, and with the API version for each in parenthesis. See the shards/shardNames parameter entry [here](https://solr.apache.org/guide/solr/latest/deployment-guide/collection-management.html#create-parameters) for an example. Would you mind merging the "Parameters" sections here to be closer to that example? (It's a bit of a hard thing to describe. If I wasn't clear enough or you're not quite sure what I'm getting at, lmk I can push up a commit to show what I mean.) ########## solr/core/src/java/org/apache/solr/handler/admin/CoreAdminOperation.java: ########## @@ -172,12 +175,14 @@ public enum CoreAdminOperation implements CoreAdminOp { RENAME, it -> { SolrParams params = it.req.getParams(); - String name = params.required().get(CoreAdminParams.OTHER); - String cname = params.required().get(CoreAdminParams.CORE); Review Comment: I think your rationale there makes sense as long as the parameter names are the same in v2 as in v1, but it might be a little confusing in this case since the param names are different. e.g. A v1 user will be confused when their request `/admin/cores?action=RENAME&core=foo` returns the error message `Missing required parameter 'to'`. The v1 API doesn't have a 'to' param! And they'll be even more confused if they add a "to" param, and get the same error again! So I'd vote to add the validation back in here to avoid this. Though if you've got another approach you like better to keep the error messages accurate, that works too 👍 ########## solr/core/src/test/org/apache/solr/handler/admin/CoreAdminOperationTest.java: ########## @@ -167,23 +167,29 @@ public void testRenameUnexpectedFailuresResultIn500SolrException() { @Test public void testRenameMissingCoreParamResultsIn400SolrException() { + final var coreAdminHandler = mock(CoreAdminHandler.class); + final var callInfo1 = new CoreAdminHandler.CallInfo(coreAdminHandler, mockRequest, null, null); + Review Comment: Thanks for calling these out. > That's why I created new mock and passed it The mock makes sense IMO 👍 You likely already know this, but I'll mention it as an FYI, since it's a common gotcha in writing Solr tests: any tests that use mocks need to have a `assumeWorkingMockito` call in a `@BeforeClass` method. It looks like we've already got in this class, so no need to change anything here! Just wanted to put the FYI out there. > Let me know If you want to change the variable name. Haha, 'callInfo1' isn't the best name, maybe. I probably would've chosen something like `renameCallInfo` or 'callInfoWithMockHandler`. (That latter option makes it clear why we're not using the 'callInfo' like all the other tests.). But the test methods are small and clear, so it's not a big deal, ultimately. So it's up to you. ########## solr/core/src/test/org/apache/solr/handler/admin/CoreAdminOperationTest.java: ########## @@ -167,23 +167,29 @@ public void testRenameUnexpectedFailuresResultIn500SolrException() { @Test public void testRenameMissingCoreParamResultsIn400SolrException() { + final var coreAdminHandler = mock(CoreAdminHandler.class); + final var callInfo1 = new CoreAdminHandler.CallInfo(coreAdminHandler, mockRequest, null, null); + Review Comment: Thanks for calling these out. > That's why I created new mock and passed it The mock makes sense IMO 👍 You likely already know this, but I'll mention it as an FYI, since it's a common gotcha in writing Solr tests: any tests that use mocks need to have a `assumeWorkingMockito` call in a `@BeforeClass` method. It looks like we've already got in this class, so no need to change anything here! Just wanted to put the FYI out there. > Let me know If you want to change the variable name. Haha, 'callInfo1' isn't the best name, maybe. I probably would've chosen something like `renameCallInfo` or 'callInfoWithMockHandler`. (That latter option makes it clear why we're not using the 'callInfo' like all the other tests.). But the test methods are small and clear, so it's not a big deal, ultimately. So it's up to you. ########## solr/core/src/java/org/apache/solr/handler/admin/CoreAdminOperation.java: ########## @@ -172,12 +175,14 @@ public enum CoreAdminOperation implements CoreAdminOp { RENAME, it -> { SolrParams params = it.req.getParams(); - String name = params.required().get(CoreAdminParams.OTHER); - String cname = params.required().get(CoreAdminParams.CORE); - - if (cname.equals(name)) return; - - it.handler.coreContainer.rename(cname, name); + final String cname = params.get(CoreAdminParams.CORE); + final var renameCoreRequestBody = new RenameCoreRequestBody(); + renameCoreRequestBody.to = params.get((CoreAdminParams.OTHER)); + RenameCoreApi renameCoreApi = + new RenameCore( + it.handler.coreContainer, it.handler.getCoreAdminAsyncTracker(), it.req, it.rsp); Review Comment: [+0] It's up to you, either will get the job done. But I probably would've used it here, yep. ``` final var renameCoreApi = new RenameCore(...); ``` ########## solr/CHANGES.txt: ########## @@ -94,6 +94,9 @@ Improvements * SOLR-17041: Make CommitTracker currentTlogSize lazy (Alex Deparvu) +* SOLR-16397: Rename core v2 endpoints have been updated to be more REST-ful. + SWAP is now available at `POST /api/cores/coreName/rename` (Sanjay Dutt via Jason Gerlowski) Review Comment: [-1] Oops, looks like a copy/paste error here at the very start of the line - "SWAP" should be "RENAME" or something similar. ########## solr/api/src/java/org/apache/solr/client/api/model/RenameCoreRequestBody.java: ########## @@ -0,0 +1,30 @@ +/* + * 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.client.api.model; + +import com.fasterxml.jackson.annotation.JsonProperty; +import io.swagger.v3.oas.annotations.media.Schema; + +public class RenameCoreRequestBody { + @Schema(description = "The new name for the Solr core.") Review Comment: [-0] AFAIK, users _must_ provide a 'to'. Adding a `required = true` to the `@Schema` annotation will ensure this gets noticed by OpenAPI and reflected in the generated clients. ########## solr/core/src/java/org/apache/solr/handler/admin/CoreAdminOperation.java: ########## @@ -172,12 +175,14 @@ public enum CoreAdminOperation implements CoreAdminOp { RENAME, it -> { SolrParams params = it.req.getParams(); - String name = params.required().get(CoreAdminParams.OTHER); - String cname = params.required().get(CoreAdminParams.CORE); Review Comment: I think your rationale there makes sense as long as the parameter names are the same in v2 as in v1, but it might be a little confusing in this case since the param names are different. e.g. A v1 user will be confused when their request `/admin/cores?action=RENAME&core=foo` returns the error message `Missing required parameter 'to'`. The v1 API doesn't have a 'to' param! And they'll be even more confused if they add a "to" param, and get the same error again! So I'd vote to add the validation back in here to avoid this. Though if you've got another approach you like better to keep the error messages accurate, that works too 👍 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
