gerlowskija commented on code in PR #1459:
URL: https://github.com/apache/solr/pull/1459#discussion_r1136901816
##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1828,28 +1835,21 @@ public Map<String, Object> execute(
});
/**
- * Places all prefixed properties in the sink map (or a new map) using the
prefix as the key and
- * a map of all prefixed properties as the value. The sub-map keys have
the prefix removed.
+ * Collects all prefixed properties in a new. The resulting keys have the
prefix removed.
Review Comment:
[0] Typo: "in a new." (You probably want 'map' at the end of that sentence?)
##########
solr/core/src/java/org/apache/solr/handler/admin/api/AliasPropertyAPI.java:
##########
@@ -0,0 +1,262 @@
+/*
+ * 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.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static
org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.Aliases;
+import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 APIs for managing and inspecting properties for collection aliases */
+@Path("/aliases/{aliasName}/properties")
+public class AliasPropertyAPI extends AdminAPIBase {
+
+ @Inject
+ public AliasPropertyAPI(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @GET
+ @PermissionName(COLL_READ_PERM)
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
+ @Operation(
+ summary = "Get properties for a collection alias.",
+ tags = {"aliases"})
+ public GetAllAliasPropertiesResponse getAllAliasProperties(
Review Comment:
[+1] Thanks for adding in this "missing" alias API, really rounds out the v2
pattern nicely.
##########
solr/solr-ref-guide/modules/deployment-guide/pages/alias-management.adoc:
##########
@@ -657,7 +657,7 @@ If a key is set with a value that is empty it will be
removed.
[source,bash]
----
-http://localhost:8983/admin/collections?action=ALIASPROP&name=techproducts_alias&property.foo=bar
+curl -X POST
'http://localhost:8983/admin/collections?action=ALIASPROP&name=techproducts_alias&property.foo=bar'
Review Comment:
[-1] Would you be willing to add some coverage here for the new v2 APIs that
you added (alias-prop listing, single-alias-prop fetching, etc.)
Would really help people use the alias prop functionality - I think right
now the ref-guide suggests that people look in ZK if they need to know alias
prop values!
##########
solr/core/src/java/org/apache/solr/handler/admin/api/AliasPropertyAPI.java:
##########
@@ -0,0 +1,262 @@
+/*
+ * 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.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static
org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.Aliases;
+import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 APIs for managing and inspecting properties for collection aliases */
+@Path("/aliases/{aliasName}/properties")
+public class AliasPropertyAPI extends AdminAPIBase {
+
+ @Inject
+ public AliasPropertyAPI(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @GET
+ @PermissionName(COLL_READ_PERM)
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
+ @Operation(
+ summary = "Get properties for a collection alias.",
+ tags = {"aliases"})
+ public GetAllAliasPropertiesResponse getAllAliasProperties(
+ @Parameter(description = "Alias Name") @PathParam("aliasName") String
aliasName)
+ throws Exception {
+ recordCollectionForLogAndTracing(null, solrQueryRequest);
+
+ final GetAllAliasPropertiesResponse response =
+ instantiateJerseyResponse(GetAllAliasPropertiesResponse.class);
+ final Aliases aliases = readAliasesFromZk();
+ if (aliases != null) {
+ response.properties = aliases.getCollectionAliasProperties(aliasName);
+ }
+
+ return response;
+ }
+
+ @GET
+ @Path("/{propName}")
+ @PermissionName(COLL_READ_PERM)
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
+ @Operation(
+ summary = "Get a specific property for a collection alias.",
+ tags = {"aliases"})
+ public GetAliasPropertyResponse getAliasProperty(
+ @Parameter(description = "Alias Name") @PathParam("aliasName") String
aliasName,
+ @Parameter(description = "Property Name") @PathParam("propName") String
propName)
+ throws Exception {
+ recordCollectionForLogAndTracing(null, solrQueryRequest);
+
+ final GetAliasPropertyResponse response =
+ instantiateJerseyResponse(GetAliasPropertyResponse.class);
+ final Aliases aliases = readAliasesFromZk();
+ if (aliases != null) {
+ response.property = propName;
+ response.value =
aliases.getCollectionAliasProperties(aliasName).get(propName);
Review Comment:
[0] What do you think about throwing a `new SolrException(NOT_FOUND, "Some
message")` here if the requested alias property didn't exist?
##########
solr/core/src/java/org/apache/solr/handler/admin/api/AliasPropertyAPI.java:
##########
@@ -0,0 +1,262 @@
+/*
+ * 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.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static
org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.Aliases;
+import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 APIs for managing and inspecting properties for collection aliases */
+@Path("/aliases/{aliasName}/properties")
+public class AliasPropertyAPI extends AdminAPIBase {
+
+ @Inject
+ public AliasPropertyAPI(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ @GET
+ @PermissionName(COLL_READ_PERM)
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
+ @Operation(
+ summary = "Get properties for a collection alias.",
+ tags = {"aliases"})
+ public GetAllAliasPropertiesResponse getAllAliasProperties(
+ @Parameter(description = "Alias Name") @PathParam("aliasName") String
aliasName)
+ throws Exception {
+ recordCollectionForLogAndTracing(null, solrQueryRequest);
+
+ final GetAllAliasPropertiesResponse response =
+ instantiateJerseyResponse(GetAllAliasPropertiesResponse.class);
+ final Aliases aliases = readAliasesFromZk();
+ if (aliases != null) {
+ response.properties = aliases.getCollectionAliasProperties(aliasName);
+ }
+
+ return response;
+ }
+
+ @GET
+ @Path("/{propName}")
+ @PermissionName(COLL_READ_PERM)
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
+ @Operation(
+ summary = "Get a specific property for a collection alias.",
+ tags = {"aliases"})
+ public GetAliasPropertyResponse getAliasProperty(
+ @Parameter(description = "Alias Name") @PathParam("aliasName") String
aliasName,
+ @Parameter(description = "Property Name") @PathParam("propName") String
propName)
+ throws Exception {
+ recordCollectionForLogAndTracing(null, solrQueryRequest);
+
+ final GetAliasPropertyResponse response =
+ instantiateJerseyResponse(GetAliasPropertyResponse.class);
+ final Aliases aliases = readAliasesFromZk();
+ if (aliases != null) {
+ response.property = propName;
+ response.value =
aliases.getCollectionAliasProperties(aliasName).get(propName);
+ }
+
+ return response;
+ }
+
+ private Aliases readAliasesFromZk() throws Exception {
+ final CoreContainer coreContainer =
fetchAndValidateZooKeeperAwareCoreContainer();
+ final ZkStateReader zkStateReader =
coreContainer.getZkController().getZkStateReader();
+ // Make sure we have the latest alias info, since a user has explicitly
invoked an alias API
+ zkStateReader.getAliasesManager().update();
+ return zkStateReader.getAliases();
+ }
+
+ @PUT
+ @PermissionName(COLL_EDIT_PERM)
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
+ @Operation(
+ summary = "Update properties for a collection alias.",
+ tags = {"aliases"})
+ public SolrJerseyResponse updateAliasProperties(
+ @Parameter(description = "Alias Name") @PathParam("aliasName") String
aliasName,
+ @RequestBody(description = "Properties that need to be updated",
required = true)
+ UpdateAliasPropertiesRequestBody requestBody)
+ throws Exception {
+
+ if (requestBody == null) {
Review Comment:
Good question!
You're right that any constraints added to the `@RequestBody` annotation are
only considered in the docs and generated OpenAPI spec.
Jersey does offer a way to validate requests based on constraint
annotations, but we're not using it currently. They call it "bean validation".
Basically you add an extra gradle dep on
`org.glassfish.jersey.ext:jersey-bean-validation`, enable validation in your
Jersey apps with a specific property
(`jersey.config.beanValidation.enableOutputValidationErrorEntity.server`), and
add any constraint annotations (like `@NotNull`) that you need. You can even
define your own annotations which is kindof cool.
I think that we'll eventually want to utilize this everywhere we can, but
right now it's hard to take advantage of it due to how the v1 code explicitly
calls the Jersey classes and methods internally. The v1 code isn't using the
Jersey framework, so it can't take advantage of any bean validation, so any
input checking still needs to happen explicitly as you've done here.
At least, unless there's some other option I'm missing? Maybe we should
enable bean-validation for the benefit of those v2 APIs that don't have v1
counterparts (like the list-aliases you've added in this PR)?
> checked some of the other apis and I could not find details
This is probably just looseness on my part. I'm probably not being as
rigorous in input-validation as I should be 😬
##########
solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java:
##########
@@ -291,20 +316,25 @@ public void testModifyPropertiesCAR() throws Exception {
setAliasProperty.addProperty("foo", "baz");
setAliasProperty.addProperty("bar", "bam");
setAliasProperty.process(cluster.getSolrClient());
- checkFooAndBarMeta(aliasName, zkStateReader);
+ checkFooAndBarMeta(aliasName, zkStateReader, "baz", "bam");
// now verify we can delete
setAliasProperty = CollectionAdminRequest.setAliasProperty(aliasName);
setAliasProperty.addProperty("foo", "");
setAliasProperty.process(cluster.getSolrClient());
+ checkFooAndBarMeta(aliasName, zkStateReader, null, "bam");
+
+ // TODO is this a bug? should "bar" be null after the following
Review Comment:
I'm not sure whether it's a bug or not, but I agree that it's very
surprising. It seems like empty-string is the only way to delete alias
properties. Might be worth a thread on dev@ to see whether anyone knows if
this is intentional or not.
--
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
