HonahX commented on code in PR #1238: URL: https://github.com/apache/polaris/pull/1238#discussion_r2008485793
########## polaris-core/src/main/java/org/apache/polaris/core/policy/validator/GenericPolicyValidator.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.polaris.core.policy.validator; + +import com.google.common.base.Preconditions; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.policy.PolicyEntity; +import org.apache.polaris.core.policy.PredefinedPolicyTypes; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Validates a given {@link PolicyEntity} against its defined policy type. + * + * <p>This class maps the policy type code from the {@code PolicyEntity} to a predefined policy + * type, then delegates parsing/validation to a specific validator implementation. + */ +public class GenericPolicyValidator { + private static final Logger LOGGER = LoggerFactory.getLogger(GenericPolicyValidator.class); + + /** + * Validates the given policy. + * + * @param policy the policy entity to validate + * @throws InvalidPolicyException if the policy type is unknown or unsupported, or if the policy + * content is invalid + */ + public static void validate(PolicyEntity policy) { + Preconditions.checkNotNull(policy, "Policy must not be null"); + + var type = PredefinedPolicyTypes.fromCode(policy.getPolicyTypeCode()); + Preconditions.checkArgument(type != null, "Unknown policy type: " + policy.getPolicyTypeCode()); + + switch (type) { + case DATA_COMPACTION: + DataCompactionPolicyValidator.INSTANCE.parse(policy.getContent()); + break; + + // To support additional policy types in the future, add cases here. + case METADATA_COMPACTION: + case SNAPSHOT_RETENTION: + case ORPHAN_FILE_REMOVAL: + default: Review Comment: Just check my understanding: in the future when we add support for custom type, we will need to load the custom type's validator here? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/validator/DataCompactionPolicy.java: ########## @@ -0,0 +1,61 @@ +/* + * 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.polaris.core.policy.validator; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; +import java.util.Map; + +public class DataCompactionPolicy { Review Comment: ```suggestion public class DataCompactionPolicyContent { ``` Shall we append "Content" to the name to differentiate it with the `Policy` class generated by open api generator? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/validator/InvalidPolicyException.java: ########## @@ -0,0 +1,36 @@ +/* + * 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.polaris.core.policy.validator; + +import org.apache.polaris.core.exceptions.PolarisException; + +/** Exception thrown when a policy is invalid or violates defined rules. */ +public class InvalidPolicyException extends PolarisException { Review Comment: Should this map to status code 400, BadRequestError in rest? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/validator/DataCompactionPolicy.java: ########## @@ -0,0 +1,61 @@ +/* + * 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.polaris.core.policy.validator; Review Comment: Shall we move Data Compaction Policy Validator to a submodule `data-compaction`? I think a submodule for each policy type can offer a more organized layout. ########## polaris-core/src/main/java/org/apache/polaris/core/policy/validator/DataCompactionPolicyValidator.java: ########## @@ -0,0 +1,80 @@ +/* + * 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.polaris.core.policy.validator; + +import static org.apache.polaris.core.entity.PolarisEntityType.CATALOG; +import static org.apache.polaris.core.entity.PolarisEntityType.ICEBERG_TABLE_LIKE; +import static org.apache.polaris.core.entity.PolarisEntityType.NAMESPACE; + +import com.google.common.base.Strings; +import java.util.Set; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; + +public class DataCompactionPolicyValidator implements PolicyValidator<DataCompactionPolicy> { + static final DataCompactionPolicyValidator INSTANCE = new DataCompactionPolicyValidator(); + + private static final String DEFAULT_POLICY_SCHEMA_VERSION = "2025-02-03"; + private static final Set<String> POLICY_SCHEMA_VERSIONS = Set.of(DEFAULT_POLICY_SCHEMA_VERSION); + private static final Set<PolarisEntityType> ATTACHABLE_ENTITY_TYPES = + Set.of(CATALOG, NAMESPACE, ICEBERG_TABLE_LIKE); + + @Override + public DataCompactionPolicy parse(String content) { + if (Strings.isNullOrEmpty(content)) { + throw new InvalidPolicyException("Policy is empty"); + } + + try { + var policy = PolicyValidatorUtil.MAPPER.readValue(content, DataCompactionPolicy.class); Review Comment: ```suggestion DataCompactionPolicy policy = PolicyValidatorUtil.MAPPER.readValue(content, DataCompactionPolicy.class); ``` Shall we explicitly write out the type here? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/validator/GenericPolicyValidator.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.polaris.core.policy.validator; + +import com.google.common.base.Preconditions; +import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.policy.PolicyEntity; +import org.apache.polaris.core.policy.PredefinedPolicyTypes; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Validates a given {@link PolicyEntity} against its defined policy type. + * + * <p>This class maps the policy type code from the {@code PolicyEntity} to a predefined policy + * type, then delegates parsing/validation to a specific validator implementation. + */ +public class GenericPolicyValidator { + private static final Logger LOGGER = LoggerFactory.getLogger(GenericPolicyValidator.class); + + /** + * Validates the given policy. + * + * @param policy the policy entity to validate + * @throws InvalidPolicyException if the policy type is unknown or unsupported, or if the policy + * content is invalid + */ + public static void validate(PolicyEntity policy) { + Preconditions.checkNotNull(policy, "Policy must not be null"); + + var type = PredefinedPolicyTypes.fromCode(policy.getPolicyTypeCode()); + Preconditions.checkArgument(type != null, "Unknown policy type: " + policy.getPolicyTypeCode()); + + switch (type) { + case DATA_COMPACTION: + DataCompactionPolicyValidator.INSTANCE.parse(policy.getContent()); + break; + + // To support additional policy types in the future, add cases here. + case METADATA_COMPACTION: + case SNAPSHOT_RETENTION: + case ORPHAN_FILE_REMOVAL: + default: + throw new InvalidPolicyException("Unsupported policy type: " + type.getName()); + } + + LOGGER.info("Policy validated successfully: {}", type.getName()); + } + + /** + * Determines whether the given policy can be attached to the specified target entity. + * + * @param policy the policy entity to check + * @param targetEntity the target Polaris entity to attach the policy to + * @return {@code true} if the policy is attachable to the target entity; {@code false} otherwise + */ + public static boolean canAttach(PolicyEntity policy, PolarisEntity targetEntity) { Review Comment: Just brainstorming about the name, may be "isAttachable"? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/validator/DataCompactionPolicyValidator.java: ########## @@ -0,0 +1,80 @@ +/* + * 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.polaris.core.policy.validator; + +import static org.apache.polaris.core.entity.PolarisEntityType.CATALOG; +import static org.apache.polaris.core.entity.PolarisEntityType.ICEBERG_TABLE_LIKE; +import static org.apache.polaris.core.entity.PolarisEntityType.NAMESPACE; + +import com.google.common.base.Strings; +import java.util.Set; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; + +public class DataCompactionPolicyValidator implements PolicyValidator<DataCompactionPolicy> { + static final DataCompactionPolicyValidator INSTANCE = new DataCompactionPolicyValidator(); + + private static final String DEFAULT_POLICY_SCHEMA_VERSION = "2025-02-03"; + private static final Set<String> POLICY_SCHEMA_VERSIONS = Set.of(DEFAULT_POLICY_SCHEMA_VERSION); + private static final Set<PolarisEntityType> ATTACHABLE_ENTITY_TYPES = + Set.of(CATALOG, NAMESPACE, ICEBERG_TABLE_LIKE); + + @Override + public DataCompactionPolicy parse(String content) { + if (Strings.isNullOrEmpty(content)) { + throw new InvalidPolicyException("Policy is empty"); + } + + try { + var policy = PolicyValidatorUtil.MAPPER.readValue(content, DataCompactionPolicy.class); + if (policy == null) { + throw new InvalidPolicyException("Invalid policy"); + } + + if (Strings.isNullOrEmpty(policy.getVersion())) { + policy.setVersion(DEFAULT_POLICY_SCHEMA_VERSION); + } + + if (!POLICY_SCHEMA_VERSIONS.contains(policy.getVersion())) { + throw new InvalidPolicyException("Invalid policy version: " + policy.getVersion()); + } + Review Comment: This may be a follow up: Do we need to validate data compaction configs (if they present). For example, `target_file_size_bytes` needs to be a value larger than 0. -- 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]
