epugh commented on code in PR #4943: URL: https://github.com/apache/solr/pull/4943#discussion_r4105110613
########## solr/core/src/test/org/apache/solr/handler/admin/api/V2CopyFieldApiTest.java: ########## @@ -0,0 +1,317 @@ +/* + * 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 java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import org.apache.solr.client.solrj.RemoteSolrException; +import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.V2Request; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.params.MapSolrParams; +import org.apache.solr.common.util.NamedList; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +/** Tests the v2 copy-field endpoints, which address rules by their source field. */ +public class V2CopyFieldApiTest extends SolrCloudTestCase { + + private static final String COLLECTION = "v2CopyFieldApiTest"; + private static final String SOURCE = "cf_source"; + private static final String DEST_ONE = "cf_dest_one"; + private static final String DEST_TWO = "cf_dest_two"; + + @BeforeClass + public static void setupCluster() throws Exception { + System.setProperty("managed.schema.mutable", "true"); + configureCluster(1).addConfig("conf", configset("cloud-managed")).configure(); + + CollectionAdminRequest.createCollection(COLLECTION, "conf", 1, 1) + .process(cluster.getSolrClient()); + cluster.waitForActiveCollection(COLLECTION, 1, 1); + + for (String field : List.of(SOURCE, DEST_ONE, DEST_TWO)) { + new V2Request.Builder(schemaPath("/fields/" + field)) Review Comment: I was surprised to see this V2Request.builder used, as I didn't think that was our pattern. I checked and the pattern is like "new AliasesApi.DeleteAlias(aliasName)" in the tests.. Claude tells me we are missing the corresponding SolrJ methods... Not sure why yet. ########## solr/solr-ref-guide/modules/indexing-guide/pages/schema-api.adoc: ########## @@ -490,6 +490,7 @@ s|Required |Default: none |=== + A field or an array of fields to which the source field will be copied. +In the v2 API this attribute is named `destinations`; `dest` is still accepted as an alias. Review Comment: i don't think at this point, with v2 being experimental, that we need to have back compat... adds confusion and we definitly don't do it in ohter places! ########## solr/api/src/java/org/apache/solr/client/api/endpoint/UpdateSchemaApi.java: ########## @@ -112,25 +113,54 @@ SolrJerseyResponse addFieldType( SolrJerseyResponse deleteFieldType(@PathParam("fieldTypeName") String fieldTypeName) throws Exception; - // TODO Gah! Copyfields don't currently have names for us to create/delete by name in an API like Review Comment: Gah! ;-). Makes me smile this morning. ########## solr/api/src/java/org/apache/solr/client/api/endpoint/UpdateSchemaApi.java: ########## @@ -112,25 +113,54 @@ SolrJerseyResponse addFieldType( SolrJerseyResponse deleteFieldType(@PathParam("fieldTypeName") String fieldTypeName) throws Exception; - // TODO Gah! Copyfields don't currently have names for us to create/delete by name in an API like - // /schema/copyfields/{copyFieldName}...figure out how to address this in a way consistent with - // all our other APIs. (In the meantime, the functionality is exposed through the bulk API at - // least. - // @POST - // @Path("/copyfields/{copyFieldName}") - // @StoreApiParameters - // @Operation( - // summary = "Add a new copy-field with the specified name.", - // tags = {"schema"}) - // SolrJerseyResponse addCopyField(@PathParam("copyFieldName") String copyFieldName, - // @RequestBody SchemaChangeOperation.AddCopyField requestBody) throws Exception; - // - // @DELETE - // @Path("/copyfields/{copyFieldName}") - // @StoreApiParameters - // @Operation(summary = "Remove the copy-field with the specified name.", tags = {"schema"}) - // SolrJerseyResponse deleteCopyField(@PathParam("copyFieldName") String copyFieldName) throws - // Exception; + // Copy-field rules have no name of their own, so unlike the other schema resources they are + // addressed by their source field. A single request therefore covers all of the rules that + // share that source. + @PUT + @Path("/copyfields/{sourceField}") + @StoreApiParameters + @Operation( + summary = + "Set the copy-field rules for the specified source field. The given destinations " + + "replace any rules that source already has.", Review Comment: Looking back above... The more fine grained "you must provide maxChars" seems totally fine and better than 1 value to set them all version! These apis aren't used much, it's not like this traffic is going to stress the network back and forth ;-). -- 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]
