yuqi1129 commented on code in PR #7821: URL: https://github.com/apache/gravitino/pull/7821#discussion_r2258738050
########## common/src/main/java/org/apache/gravitino/dto/requests/PolicyCreateRequest.java: ########## @@ -0,0 +1,170 @@ +/* + * 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.gravitino.dto.requests; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import com.google.common.base.Preconditions; +import java.util.Set; +import javax.annotation.Nullable; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.dto.policy.PolicyContentDTO; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.rest.RESTRequest; + +/** Represents a request to create a policy. */ +@Getter +@EqualsAndHashCode +@ToString +public class PolicyCreateRequest implements RESTRequest { + + @JsonProperty("name") + private final String name; + + @JsonProperty("comment") + @Nullable + private final String comment; + + @JsonProperty("policyType") + private final String policyType; + + @JsonProperty(value = "enabled", defaultValue = "true") + private final Boolean enabled; + + @JsonProperty("content") + @JsonTypeInfo( + use = JsonTypeInfo.Id.NAME, + include = JsonTypeInfo.As.EXTERNAL_PROPERTY, + property = "policyType", + defaultImpl = PolicyContentDTO.CustomContentDTO.class) + @JsonSubTypes({ + // add mappings for built-in types here + // For example: @JsonSubTypes.Type(value = DataCompactionContent.class, name = + // "system_data_compaction") + }) + private final PolicyContentDTO policyContent; + + @JsonProperty("exclusive") + private final Boolean exclusive; + + @JsonProperty("inheritable") + private final Boolean inheritable; + + @JsonProperty("supportedObjectTypes") + private final Set<MetadataObject.Type> supportedObjectTypes; + + /** + * Creates a new PolicyCreateRequest. + * + * @param name The name of the policy. + * @param comment The comment of the policy. + * @param type The type of the policy. + * @param enabled Whether the policy is enabled. + * @param exclusive Whether the policy is exclusive. + * @param inheritable Whether the policy is inheritable. + * @param supportedObjectTypes The set of metadata object types that the policy can be applied to. + * @param content The content of the policy. + */ + public PolicyCreateRequest( + String name, + String type, + String comment, + boolean enabled, + boolean exclusive, + boolean inheritable, + Set<MetadataObject.Type> supportedObjectTypes, + PolicyContentDTO content) { + this.name = name; + this.policyType = type; + this.comment = comment; + this.enabled = enabled; + this.exclusive = exclusive; + this.inheritable = inheritable; + this.supportedObjectTypes = supportedObjectTypes; + this.policyContent = content; + } + + /** This is the constructor that is used by Jackson deserializer */ + private PolicyCreateRequest() { + this.name = null; + this.policyType = null; + this.comment = null; + this.enabled = null; + this.exclusive = null; + this.inheritable = null; + this.supportedObjectTypes = null; + this.policyContent = null; + } + + /** + * Validates the request. + * + * @throws IllegalArgumentException If the request is invalid, this exception is thrown. + */ + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + StringUtils.isNotBlank(name), "\"name\" is required and cannot be empty"); Review Comment: You'd better unify the workd `can't be empty` is `null or empty` or just `empty`. I see you use `cannot be null or empty` in L287. ########## common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java: ########## @@ -1188,4 +1244,25 @@ public static JobTemplate fromDTO(JobTemplateDTO jobTemplateDTO) { "Unsupported job template type: " + jobTemplateDTO.jobType()); } } + + /** + * Converts a PolicyContentDTO to a PolicyContent. + * + * @param policyContentDTO The policy content DTO to be converted. + * @return The policy content. + */ + public static PolicyContent fromDTO(PolicyContentDTO policyContentDTO) { + if (policyContentDTO == null) { + return null; + } + + if (policyContentDTO instanceof PolicyContentDTO.CustomContentDTO) { + PolicyContentDTO.CustomContentDTO customContentDTO = + (PolicyContentDTO.CustomContentDTO) policyContentDTO; + return PolicyContents.custom(customContentDTO.customRules(), customContentDTO.properties()); + } + + throw new IllegalArgumentException( Review Comment: Why does `toDTO` supports two cases `PolicyContentDTO` and `olicyContents.CustomContent` and one here? ########## server/src/main/java/org/apache/gravitino/server/web/rest/PolicyOperations.java: ########## @@ -0,0 +1,273 @@ +/* + * 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.gravitino.server.web.rest; + +import static org.apache.gravitino.dto.util.DTOConverters.fromDTO; + +import com.codahale.metrics.annotation.ResponseMetered; +import com.codahale.metrics.annotation.Timed; +import java.util.Arrays; +import java.util.Optional; +import javax.inject.Inject; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.DELETE; +import javax.ws.rs.DefaultValue; +import javax.ws.rs.GET; +import javax.ws.rs.PATCH; +import javax.ws.rs.POST; +import javax.ws.rs.PUT; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; +import javax.ws.rs.QueryParam; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.Response; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.gravitino.dto.policy.PolicyDTO; +import org.apache.gravitino.dto.requests.PolicyCreateRequest; +import org.apache.gravitino.dto.requests.PolicySetRequest; +import org.apache.gravitino.dto.requests.PolicyUpdateRequest; +import org.apache.gravitino.dto.requests.PolicyUpdatesRequest; +import org.apache.gravitino.dto.responses.BaseResponse; +import org.apache.gravitino.dto.responses.DropResponse; +import org.apache.gravitino.dto.responses.NameListResponse; +import org.apache.gravitino.dto.responses.PolicyListResponse; +import org.apache.gravitino.dto.responses.PolicyResponse; +import org.apache.gravitino.dto.util.DTOConverters; +import org.apache.gravitino.metrics.MetricNames; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyChange; +import org.apache.gravitino.policy.PolicyDispatcher; +import org.apache.gravitino.server.web.Utils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Path("metalakes/{metalake}/policies") +public class PolicyOperations { + + private static final Logger LOG = LoggerFactory.getLogger(PolicyOperations.class); + + private final PolicyDispatcher policyDispatcher; + + @Context private HttpServletRequest httpRequest; + + @Inject + public PolicyOperations(PolicyDispatcher policyDispatcher) { + this.policyDispatcher = policyDispatcher; + } + + @GET + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "list-policies." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "list-policies", absolute = true) + public Response listPolicies( + @PathParam("metalake") String metalake, + @QueryParam("details") @DefaultValue("false") boolean verbose) { + LOG.info( + "Received list policy {} request for metalake: {}", verbose ? "infos" : "names", metalake); + + try { + return Utils.doAs( + httpRequest, + () -> { + if (verbose) { + Policy[] policies = policyDispatcher.listPolicyInfos(metalake); + PolicyDTO[] policyDTOs; + if (ArrayUtils.isEmpty(policies)) { + policyDTOs = new PolicyDTO[0]; + } else { + policyDTOs = + Arrays.stream(policies) + .map(p -> DTOConverters.toDTO(p, Optional.empty())) + .toArray(PolicyDTO[]::new); + } + + LOG.info("List {} policies info under metalake: {}", policyDTOs.length, metalake); + return Utils.ok(new PolicyListResponse(policyDTOs)); + + } else { + String[] policyNames = policyDispatcher.listPolicies(metalake); + policyNames = policyNames == null ? new String[0] : policyNames; + + LOG.info("List {} policies under metalake: {}", policyNames.length, metalake); + return Utils.ok(new NameListResponse(policyNames)); + } + }); + } catch (Exception e) { + return ExceptionHandlers.handlePolicyException(OperationType.LIST, "", metalake, e); + } + } + + @POST + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "create-policy." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "create-policy", absolute = true) + public Response createPolicy( + @PathParam("metalake") String metalake, PolicyCreateRequest request) { + LOG.info("Received create policy request under metalake: {}", metalake); + + try { + return Utils.doAs( + httpRequest, + () -> { + request.validate(); + Policy policy = + Policy.BuiltInType.fromPolicyType(request.getPolicyType()) + == Policy.BuiltInType.CUSTOM + ? policyDispatcher.createPolicy( + metalake, + request.getName(), + request.getPolicyType(), + request.getComment(), + request.getEnabled(), + request.getExclusive(), + request.getInheritable(), + request.getSupportedObjectTypes(), + fromDTO(request.getPolicyContent())) + : policyDispatcher.createPolicy( + metalake, + request.getName(), + request.getPolicyType(), + request.getComment(), + request.getEnabled(), + fromDTO(request.getPolicyContent())); + + LOG.info("Created policy: {} under metalake: {}", policy.name(), metalake); + return Utils.ok(new PolicyResponse(DTOConverters.toDTO(policy, Optional.empty()))); + }); + } catch (Exception e) { + return ExceptionHandlers.handlePolicyException( + OperationType.CREATE, request.getName(), metalake, e); + } + } + + @GET + @Path("{policy}") + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "get-policy." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "get-policy", absolute = true) + public Response getPolicy( + @PathParam("metalake") String metalake, @PathParam("policy") String name) { + LOG.info("Received get policy request for policy: {} under metalake: {}", name, metalake); + + try { + return Utils.doAs( + httpRequest, + () -> { + Policy policy = policyDispatcher.getPolicy(metalake, name); + LOG.info("Get policy: {} under metalake: {}", name, metalake); + return Utils.ok(new PolicyResponse(DTOConverters.toDTO(policy, Optional.empty()))); + }); + } catch (Exception e) { + return ExceptionHandlers.handlePolicyException(OperationType.GET, name, metalake, e); + } + } + + @PUT + @Path("{policy}") + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "alter-policy." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "alter-policy", absolute = true) + public Response alterPolicy( Review Comment: There are two methods name `alterPolicy`, I believe it's more suitble to differentate them. ########## common/src/main/java/org/apache/gravitino/dto/requests/PolicyCreateRequest.java: ########## @@ -0,0 +1,170 @@ +/* + * 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.gravitino.dto.requests; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import com.google.common.base.Preconditions; +import java.util.Set; +import javax.annotation.Nullable; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.dto.policy.PolicyContentDTO; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.rest.RESTRequest; + +/** Represents a request to create a policy. */ +@Getter +@EqualsAndHashCode +@ToString +public class PolicyCreateRequest implements RESTRequest { + + @JsonProperty("name") + private final String name; + + @JsonProperty("comment") + @Nullable + private final String comment; + + @JsonProperty("policyType") + private final String policyType; + + @JsonProperty(value = "enabled", defaultValue = "true") + private final Boolean enabled; + + @JsonProperty("content") + @JsonTypeInfo( + use = JsonTypeInfo.Id.NAME, + include = JsonTypeInfo.As.EXTERNAL_PROPERTY, + property = "policyType", + defaultImpl = PolicyContentDTO.CustomContentDTO.class) + @JsonSubTypes({ + // add mappings for built-in types here + // For example: @JsonSubTypes.Type(value = DataCompactionContent.class, name = + // "system_data_compaction") + }) + private final PolicyContentDTO policyContent; + + @JsonProperty("exclusive") + private final Boolean exclusive; Review Comment: Can't we use primitive type `exclusive` here? ########## server/src/main/java/org/apache/gravitino/server/web/rest/PolicyOperations.java: ########## @@ -0,0 +1,273 @@ +/* + * 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.gravitino.server.web.rest; + +import static org.apache.gravitino.dto.util.DTOConverters.fromDTO; + +import com.codahale.metrics.annotation.ResponseMetered; +import com.codahale.metrics.annotation.Timed; +import java.util.Arrays; +import java.util.Optional; +import javax.inject.Inject; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.DELETE; +import javax.ws.rs.DefaultValue; +import javax.ws.rs.GET; +import javax.ws.rs.PATCH; +import javax.ws.rs.POST; +import javax.ws.rs.PUT; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; +import javax.ws.rs.QueryParam; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.Response; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.gravitino.dto.policy.PolicyDTO; +import org.apache.gravitino.dto.requests.PolicyCreateRequest; +import org.apache.gravitino.dto.requests.PolicySetRequest; +import org.apache.gravitino.dto.requests.PolicyUpdateRequest; +import org.apache.gravitino.dto.requests.PolicyUpdatesRequest; +import org.apache.gravitino.dto.responses.BaseResponse; +import org.apache.gravitino.dto.responses.DropResponse; +import org.apache.gravitino.dto.responses.NameListResponse; +import org.apache.gravitino.dto.responses.PolicyListResponse; +import org.apache.gravitino.dto.responses.PolicyResponse; +import org.apache.gravitino.dto.util.DTOConverters; +import org.apache.gravitino.metrics.MetricNames; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyChange; +import org.apache.gravitino.policy.PolicyDispatcher; +import org.apache.gravitino.server.web.Utils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Path("metalakes/{metalake}/policies") +public class PolicyOperations { + + private static final Logger LOG = LoggerFactory.getLogger(PolicyOperations.class); + + private final PolicyDispatcher policyDispatcher; + + @Context private HttpServletRequest httpRequest; + + @Inject + public PolicyOperations(PolicyDispatcher policyDispatcher) { + this.policyDispatcher = policyDispatcher; + } + + @GET + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "list-policies." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "list-policies", absolute = true) + public Response listPolicies( + @PathParam("metalake") String metalake, + @QueryParam("details") @DefaultValue("false") boolean verbose) { + LOG.info( + "Received list policy {} request for metalake: {}", verbose ? "infos" : "names", metalake); + + try { + return Utils.doAs( + httpRequest, + () -> { + if (verbose) { + Policy[] policies = policyDispatcher.listPolicyInfos(metalake); Review Comment: `policies` will never be null, I think we can simplify the code below. -- 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]
