gerlowskija commented on code in PR #2788: URL: https://github.com/apache/solr/pull/2788#discussion_r1817160658
########## solr/api/src/java/org/apache/solr/client/api/endpoint/SetClusterPropertyApi.java: ########## @@ -0,0 +1,43 @@ +/* + * 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.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.Parameter; +import io.swagger.v3.oas.annotations.parameters.RequestBody; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import org.apache.solr.client.api.model.SetClusterPropertyRequestBody; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +@Path("/cluster/properties/{propertyName}") +public interface SetClusterPropertyApi { + + @PUT + @Operation( + summary = "Set a cluster property in this Solr cluster", Review Comment: ```suggestion summary = "Set a single new or existing cluster property in this Solr cluster", ``` ########## solr/api/src/java/org/apache/solr/client/api/endpoint/SetNestedClusterPropertyApi.java: ########## @@ -0,0 +1,38 @@ +/* + * 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.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.parameters.RequestBody; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; +import org.apache.solr.client.api.model.SetNestedClusterPropertyRequestBody; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +@Path("/cluster/properties") +public interface SetNestedClusterPropertyApi { + + @PUT + @Operation( + summary = "Set nested cluster properties in this Solr cluster", + tags = {"cluster-properties"}) + SolrJerseyResponse createOrUpdateNestedClusterProperty( Review Comment: [Q] It's interesting that this API is both the only way to set "nested"/complex cluster properties, and the only way to set multiple properties simultaneously. I guess that's fine, since it mirrors what's supported in v1? I don't really have a question or suggestion here, mostly just making a note of it... ########## solr/core/src/test/org/apache/solr/handler/admin/api/ClusterPropsAPITest.java: ########## @@ -0,0 +1,178 @@ +/* + * 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 static org.apache.solr.common.util.Utils.getObjectByPath; + +import java.net.URL; +import java.util.List; +import org.apache.http.HttpResponse; +import org.apache.http.client.methods.HttpDelete; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpPut; +import org.apache.http.entity.StringEntity; +import org.apache.solr.client.solrj.impl.HttpSolrClient; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.util.Utils; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +public class ClusterPropsAPITest extends SolrCloudTestCase { + + private URL baseUrl; + private String baseUrlV2ClusterProps; + + private static final String testClusterProperty = "ext.test"; + private static final String testClusterPropertyValue = "test value"; + private static final String testClusterPropertyNestedKeyAndValue = + " \"defaults\": {" + + " \"collection\": {" + + " \"numShards\": 4," + + " \"nrtReplicas\": 2," + + " \"tlogReplicas\": 2," + + " \"pullReplicas\": 2" + + " }" + + " }"; + private static final String testClusterPropertyBulkAndNestedValues = + "{ \"properties\": {" + + testClusterPropertyNestedKeyAndValue + + "," + + " \"" + + testClusterProperty + + "\": " + + "\"" + + testClusterPropertyValue + + "\"" + + " } }"; + + @BeforeClass + public static void setupCluster() throws Exception { + configureCluster(1).addConfig("conf", configset("cloud-minimal")).configure(); + } + + @Before + @Override + public void setUp() throws Exception { + super.setUp(); + + baseUrl = cluster.getJettySolrRunner(0).getBaseUrl(); + baseUrlV2ClusterProps = + cluster.getJettySolrRunner(0).getBaseURLV2().toString() + "/cluster/properties"; + } + + @After + @Override + public void tearDown() throws Exception { + super.tearDown(); + } + + @Test + public void testClusterPropertyOpsAllGood() throws Exception { Review Comment: [+1] Great tests - afaict we have very little coverage around cluster-props so this is a huge improvement! [-0] These tests seem like a good candidate for using the SolrJ classes generated by your API definitions (i.e. `o.a.s.client.solrj.request.ClusterPropertiesApi`). Doing so feels a bit circular (i.e. any bugs in the JAX-RS annotations would get propagated to the generated code and might not be caught)...but it'd help the tests to scan a lot nicer IMO, and it'd get us coverage for the SolrJ classes to boot, so it sees like a worthwhile tradeoff IMO. Wdyt? ########## solr/core/src/java/org/apache/solr/handler/ClusterAPI.java: ########## @@ -278,14 +280,13 @@ public void removeRole(PayloadObj<RoleInfo> obj) throws Exception { @Command(name = "set-obj-property") public void setObjProperty(PayloadObj<ClusterPropPayload> obj) { // Not using the object directly here because the API differentiate between {name:null} and {} - Map<String, Object> m = obj.getDataMap(); - ClusterProperties clusterProperties = - new ClusterProperties(getCoreContainer().getZkController().getZkClient()); - try { - clusterProperties.setClusterProperties(m); - } catch (Exception e) { - throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error in API", e); - } + SetNestedClusterPropertyRequestBody setNestedClusterPropertyRequestBody = Review Comment: [-1] SetNestedClusterPropertyApi/SetNestedClusterProperty provide an improved v2 API for this functionality, so IMO we should delete this method outright, rather than keeping the older v2 API around. (v2 APIs are "experimental", so there's no backcompat concerns with removing/changing them as necessary) ########## solr/core/src/java/org/apache/solr/core/CoreContainer.java: ########## @@ -907,6 +912,11 @@ private void loadInternal() { ClusterAPI clusterAPI = new ClusterAPI(collectionsHandler, configSetsHandler); registerV2ApiIfEnabled(clusterAPI); registerV2ApiIfEnabled(clusterAPI.commands); + registerV2ApiIfEnabled(ListClusterProperties.class); Review Comment: [0] Not a huge deal either way, but it's prob a bit cleaner to register these APIs from within `CollectionsHandler`, where the v1 logic (such as it is) lives. (See `CollectionsHandler.getJerseyResources`) CoreContainer is already pretty bloated so the more API-registration we can defer to individual request-handler's, the better IMO ########## solr/core/src/java/org/apache/solr/handler/admin/api/ListClusterProperties.java: ########## @@ -0,0 +1,53 @@ +/* + * 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 jakarta.inject.Inject; +import java.io.IOException; +import java.util.ArrayList; +import org.apache.solr.client.api.endpoint.ListClusterPropertiesApi; +import org.apache.solr.client.api.model.ListClusterPropertiesResponse; +import org.apache.solr.core.CoreContainer; + +/** + * V2 API for listing cluster properties. + * + * <p>This API (GET /api/cluster/properties) has no v1 equivalent. + */ +public class ListClusterProperties extends ClusterPropertiesAPIBase + implements ListClusterPropertiesApi { + + @Inject + public ListClusterProperties(CoreContainer coreContainer) { + super(coreContainer.getZkController().getZkClient()); + } + + @Override Review Comment: [-1] We want all APIs to have an `@PermissionName` annotation, so that they can be mapped to the predefined permissions that Solr's RuleBasedAuth plugin knows about. In terms of the what permission - `COLL_READ` seems like the best fit at a glance to me. Wdyt? (Same comment applies to GetClusterProperty, but I think all the others are good!) ########## solr/core/src/java/org/apache/solr/handler/admin/api/SetNestedClusterProperty.java: ########## @@ -0,0 +1,50 @@ +/* + * 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 static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM; + +import jakarta.inject.Inject; +import org.apache.solr.client.api.endpoint.SetNestedClusterPropertyApi; +import org.apache.solr.client.api.model.SetNestedClusterPropertyRequestBody; +import org.apache.solr.client.api.model.SolrJerseyResponse; +import org.apache.solr.common.SolrException; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.jersey.PermissionName; + +public class SetNestedClusterProperty extends ClusterPropertiesAPIBase + implements SetNestedClusterPropertyApi { + + @Inject + public SetNestedClusterProperty(CoreContainer coreContainer) { + super(coreContainer.getZkController().getZkClient()); + } + + @Override + @PermissionName(COLL_EDIT_PERM) + public SolrJerseyResponse createOrUpdateNestedClusterProperty( + SetNestedClusterPropertyRequestBody requestBody) { + SolrJerseyResponse response = instantiateJerseyResponse(SolrJerseyResponse.class); + try { + clusterProperties.setClusterProperties(requestBody.properties); + } catch (Exception e) { + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error in API", e); Review Comment: [0] Not the most helpful error message, but I can see it came straight from the pre-existing "set-obj-property" API. Commenting here as a reminder to myself to fix this and a few other places in a subsequent PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
