From 60208203de023852357de23cd3f1cabd9d8276b9 Mon Sep 17 00:00:00 2001 From: Yufei Gu Date: Thu, 3 Apr 2025 19:01:56 -0700 Subject: [PATCH 1/6] Implement Policy API detach/attach/getApplicablePolicy --- .../PolicyMappingAlreadyExistsException.java | 6 + .../TransactionalMetaStoreManagerImpl.java | 1 + .../exceptions/PolicyAttachException.java | 34 +++ .../policy/validator/PolicyValidators.java | 9 + .../quarkus/catalog/PolicyCatalogTest.java | 137 ++++++++--- .../service/catalog/policy/PolicyCatalog.java | 219 ++++++++++++++++-- .../exception/PolarisExceptionMapper.java | 3 + 7 files changed, 364 insertions(+), 45 deletions(-) create mode 100644 polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/PolicyAttachException.java diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolicyMappingAlreadyExistsException.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolicyMappingAlreadyExistsException.java index 2cd714f25d..793cea5332 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolicyMappingAlreadyExistsException.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolicyMappingAlreadyExistsException.java @@ -18,6 +18,7 @@ */ package org.apache.polaris.core.persistence; +import com.google.errorprone.annotations.FormatMethod; import org.apache.polaris.core.policy.PolarisPolicyMappingRecord; /** @@ -35,6 +36,11 @@ public PolicyMappingAlreadyExistsException(PolarisPolicyMappingRecord existingRe this.existingRecord = existingRecord; } + @FormatMethod + public PolicyMappingAlreadyExistsException(String message, Object... arg) { + super(String.format(message, arg)); + } + public PolarisPolicyMappingRecord getExistingRecord() { return this.existingRecord; } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java index 27b049e917..94f4ac239b 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java @@ -47,6 +47,7 @@ import org.apache.polaris.core.entity.PolarisPrivilege; import org.apache.polaris.core.entity.PolarisTaskConstants; import org.apache.polaris.core.persistence.*; +import org.apache.polaris.core.persistence.PolicyMappingAlreadyExistsException; import org.apache.polaris.core.persistence.dao.entity.BaseResult; import org.apache.polaris.core.persistence.dao.entity.ChangeTrackingResult; import org.apache.polaris.core.persistence.dao.entity.CreateCatalogResult; diff --git a/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/PolicyAttachException.java b/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/PolicyAttachException.java new file mode 100644 index 0000000000..e47d978d0f --- /dev/null +++ b/polaris-core/src/main/java/org/apache/polaris/core/policy/exceptions/PolicyAttachException.java @@ -0,0 +1,34 @@ +/* + * 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.exceptions; + +import com.google.errorprone.annotations.FormatMethod; +import org.apache.polaris.core.exceptions.PolarisException; + +public class PolicyAttachException extends PolarisException { + @FormatMethod + public PolicyAttachException(String message, Object... args) { + super(String.format(message, args)); + } + + @FormatMethod + public PolicyAttachException(Throwable cause, String message, Object... args) { + super(String.format(message, args), cause); + } +} diff --git a/polaris-core/src/main/java/org/apache/polaris/core/policy/validator/PolicyValidators.java b/polaris-core/src/main/java/org/apache/polaris/core/policy/validator/PolicyValidators.java index 3ccb2f6d2e..4e1b4e288b 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/policy/validator/PolicyValidators.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/policy/validator/PolicyValidators.java @@ -26,6 +26,7 @@ import org.apache.polaris.core.policy.content.maintenance.MetadataCompactionPolicyContent; import org.apache.polaris.core.policy.content.maintenance.OrphanFileRemovalPolicyContent; import org.apache.polaris.core.policy.content.maintenance.SnapshotRetentionPolicyContent; +import org.apache.polaris.core.policy.exceptions.PolicyAttachException; import org.apache.polaris.core.policy.validator.maintenance.BaseMaintenancePolicyValidator; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -102,4 +103,12 @@ public static boolean canAttach(PolicyEntity policy, PolarisEntity targetEntity) return false; } } + + public static void validateAttach(PolicyEntity policy, PolarisEntity targetEntity) { + if (!canAttach(policy, targetEntity)) { + throw new PolicyAttachException( + "Cannot attach policy '%s' to target entity '%s'", + policy.getName(), targetEntity.getName()); + } + } } diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java index 83bdecaaa4..56976f51be 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java @@ -18,6 +18,11 @@ */ package org.apache.polaris.service.quarkus.catalog; +import static org.apache.polaris.core.policy.PredefinedPolicyTypes.DATA_COMPACTION; +import static org.apache.polaris.core.policy.PredefinedPolicyTypes.METADATA_COMPACTION; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.when; @@ -58,6 +63,7 @@ import org.apache.polaris.core.persistence.MetaStoreManagerFactory; import org.apache.polaris.core.persistence.PolarisEntityManager; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.PolicyMappingAlreadyExistsException; import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet; import org.apache.polaris.core.persistence.cache.EntityCache; import org.apache.polaris.core.persistence.dao.entity.BaseResult; @@ -82,8 +88,8 @@ import org.apache.polaris.service.storage.PolarisStorageIntegrationProviderImpl; import org.apache.polaris.service.task.TaskExecutor; import org.apache.polaris.service.types.Policy; +import org.apache.polaris.service.types.PolicyAttachmentTarget; import org.apache.polaris.service.types.PolicyIdentifier; -import org.assertj.core.api.Assertions; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; @@ -124,6 +130,8 @@ public Map getConfigOverrides() { private static final PolicyIdentifier POLICY2 = new PolicyIdentifier(NS1, "p2"); private static final PolicyIdentifier POLICY3 = new PolicyIdentifier(NS1, "p3"); private static final PolicyIdentifier POLICY4 = new PolicyIdentifier(NS1, "p4"); + private static final PolicyAttachmentTarget POLICY_ATTACH_TARGET_NS = + new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.NAMESPACE, List.of(NS1.levels())); @Inject MetaStoreManagerFactory managerFactory; @Inject PolarisConfigurationStore configurationStore; @@ -305,7 +313,7 @@ public Map purgeRealms(Iterable realms) { @Test public void testCreatePolicyDoesNotThrow() { icebergCatalog.createNamespace(NS1); - Assertions.assertThatCode( + assertThatCode( () -> policyCatalog.createPolicy( POLICY1, @@ -320,8 +328,7 @@ public void testCreatePolicyAlreadyExists() { icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - - Assertions.assertThatThrownBy( + assertThatThrownBy( () -> policyCatalog.createPolicy( POLICY1, @@ -330,7 +337,7 @@ public void testCreatePolicyAlreadyExists() { "{\"enable\": false}")) .isInstanceOf(AlreadyExistsException.class); - Assertions.assertThatThrownBy( + assertThatThrownBy( () -> policyCatalog.createPolicy( POLICY1, @@ -362,8 +369,8 @@ public void testListPolicies() { "{\"enable\": false}"); List listResult = policyCatalog.listPolicies(NS1, null); - Assertions.assertThat(listResult).hasSize(4); - Assertions.assertThat(listResult).containsExactlyInAnyOrder(POLICY1, POLICY2, POLICY3, POLICY4); + assertThat(listResult).hasSize(4); + assertThat(listResult).containsExactlyInAnyOrder(POLICY1, POLICY2, POLICY3, POLICY4); } @Test @@ -389,8 +396,8 @@ public void testListPoliciesFilterByPolicyType() { List listResult = policyCatalog.listPolicies(NS1, PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL); - Assertions.assertThat(listResult).hasSize(1); - Assertions.assertThat(listResult).containsExactlyInAnyOrder(POLICY4); + assertThat(listResult).hasSize(1); + assertThat(listResult).containsExactlyInAnyOrder(POLICY4); } @Test @@ -400,19 +407,18 @@ public void testLoadPolicy() { POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); Policy policy = policyCatalog.loadPolicy(POLICY1); - Assertions.assertThat(policy.getVersion()).isEqualTo(0); - Assertions.assertThat(policy.getPolicyType()) - .isEqualTo(PredefinedPolicyTypes.DATA_COMPACTION.getName()); - Assertions.assertThat(policy.getContent()).isEqualTo("{\"enable\": false}"); - Assertions.assertThat(policy.getName()).isEqualTo("p1"); - Assertions.assertThat(policy.getDescription()).isEqualTo("test"); + assertThat(policy.getVersion()).isEqualTo(0); + assertThat(policy.getPolicyType()).isEqualTo(PredefinedPolicyTypes.DATA_COMPACTION.getName()); + assertThat(policy.getContent()).isEqualTo("{\"enable\": false}"); + assertThat(policy.getName()).isEqualTo("p1"); + assertThat(policy.getDescription()).isEqualTo("test"); } @Test public void testCreatePolicyWithInvalidContent() { icebergCatalog.createNamespace(NS1); - Assertions.assertThatThrownBy( + assertThatThrownBy( () -> policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "invalid")) @@ -423,7 +429,7 @@ public void testCreatePolicyWithInvalidContent() { public void testLoadPolicyNotExist() { icebergCatalog.createNamespace(NS1); - Assertions.assertThatThrownBy(() -> policyCatalog.loadPolicy(POLICY1)) + assertThatThrownBy(() -> policyCatalog.loadPolicy(POLICY1)) .isInstanceOf(NoSuchPolicyException.class); } @@ -432,16 +438,14 @@ public void testUpdatePolicy() { icebergCatalog.createNamespace(NS1); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - policyCatalog.updatePolicy(POLICY1, "updated", "{\"enable\": true}", 0); Policy policy = policyCatalog.loadPolicy(POLICY1); - Assertions.assertThat(policy.getVersion()).isEqualTo(1); - Assertions.assertThat(policy.getPolicyType()) - .isEqualTo(PredefinedPolicyTypes.DATA_COMPACTION.getName()); - Assertions.assertThat(policy.getContent()).isEqualTo("{\"enable\": true}"); - Assertions.assertThat(policy.getName()).isEqualTo("p1"); - Assertions.assertThat(policy.getDescription()).isEqualTo("updated"); + assertThat(policy.getVersion()).isEqualTo(1); + assertThat(policy.getPolicyType()).isEqualTo(PredefinedPolicyTypes.DATA_COMPACTION.getName()); + assertThat(policy.getContent()).isEqualTo("{\"enable\": true}"); + assertThat(policy.getName()).isEqualTo("p1"); + assertThat(policy.getDescription()).isEqualTo("updated"); } @Test @@ -450,7 +454,7 @@ public void testUpdatePolicyWithWrongVersion() { policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - Assertions.assertThatThrownBy( + assertThatThrownBy( () -> policyCatalog.updatePolicy(POLICY1, "updated", "{\"enable\": true}", 1)) .isInstanceOf(PolicyVersionMismatchException.class); } @@ -461,8 +465,7 @@ public void testUpdatePolicyWithInvalidContent() { policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - Assertions.assertThatThrownBy( - () -> policyCatalog.updatePolicy(POLICY1, "updated", "invalid", 0)) + assertThatThrownBy(() -> policyCatalog.updatePolicy(POLICY1, "updated", "invalid", 0)) .isInstanceOf(InvalidPolicyException.class); } @@ -470,7 +473,7 @@ public void testUpdatePolicyWithInvalidContent() { public void testUpdatePolicyNotExist() { icebergCatalog.createNamespace(NS1); - Assertions.assertThatThrownBy( + assertThatThrownBy( () -> policyCatalog.updatePolicy(POLICY1, "updated", "{\"enable\": true}", 0)) .isInstanceOf(NoSuchPolicyException.class); } @@ -481,8 +484,8 @@ public void testDropPolicy() { policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - Assertions.assertThat(policyCatalog.dropPolicy(POLICY1, false)).isTrue(); - Assertions.assertThatThrownBy(() -> policyCatalog.loadPolicy(POLICY1)) + assertThat(policyCatalog.dropPolicy(POLICY1, false)).isTrue(); + assertThatThrownBy(() -> policyCatalog.loadPolicy(POLICY1)) .isInstanceOf(NoSuchPolicyException.class); } @@ -490,7 +493,79 @@ public void testDropPolicy() { public void testDropPolicyNotExist() { icebergCatalog.createNamespace(NS1); - Assertions.assertThatThrownBy(() -> policyCatalog.dropPolicy(POLICY1, false)) + assertThatThrownBy(() -> policyCatalog.dropPolicy(POLICY1, false)) .isInstanceOf(NoSuchPolicyException.class); } + + @Test + public void testAttachPolicy() { + icebergCatalog.createNamespace(NS1); + policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + + var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); + assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + assertThat(policyCatalog.getApplicablePolicies(null, null, null).size()).isEqualTo(1); + } + + @Test + public void testAttachPolicyConflict() { + icebergCatalog.createNamespace(NS1); + policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + policyCatalog.createPolicy(POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); + + var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); + assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + // Attempt to attach a conflicting second policy and expect an exception + assertThatThrownBy(() -> policyCatalog.attachPolicy(POLICY2, target, null)) + .isInstanceOf(PolicyMappingAlreadyExistsException.class) + .hasMessage( + String.format( + "The policy mapping of same type(%s) for %s already exists", + DATA_COMPACTION.getName(), CATALOG_NAME)); + } + + @Test + public void testDetachPolicy() { + icebergCatalog.createNamespace(NS1); + policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + + assertThat(policyCatalog.attachPolicy(POLICY1, POLICY_ATTACH_TARGET_NS, null)).isTrue(); + assertThat(policyCatalog.getApplicablePolicies(NS1, null, null).size()).isEqualTo(1); + assertThat(policyCatalog.detachPolicy(POLICY1, POLICY_ATTACH_TARGET_NS)).isTrue(); + assertThat(policyCatalog.getApplicablePolicies(NS1, null, null).size()).isEqualTo(0); + } + + @Test + public void testAttachPolicyOverwrite() { + icebergCatalog.createNamespace(NS1); + policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + policyCatalog.createPolicy(POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); + + // attach to catalog + var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); + assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + + // attach to namespace + assertThat(policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null)).isTrue(); + var applicablePolicies = policyCatalog.getApplicablePolicies(NS1, null, null); + assertThat(applicablePolicies.size()).isEqualTo(1); + assertThat(applicablePolicies.getFirst().getName()).isEqualTo(POLICY2.getName()); + } + + @Test + public void testAttachPolicyInheritance() { + icebergCatalog.createNamespace(NS1); + policyCatalog.createPolicy( + POLICY1, METADATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + policyCatalog.createPolicy(POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); + + // attach to catalog + var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); + assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + + // attach to namespace + assertThat(policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null)).isTrue(); + var applicablePolicies = policyCatalog.getApplicablePolicies(NS1, null, null); + assertThat(applicablePolicies.size()).isEqualTo(2); + } } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index a4fb23da7d..778e4af047 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -18,21 +18,33 @@ */ package org.apache.polaris.service.catalog.policy; +import static org.apache.polaris.core.persistence.dao.entity.BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS; +import static org.apache.polaris.service.types.PolicyAttachmentTarget.TypeEnum.CATALOG; + +import com.google.common.base.Strings; +import jakarta.annotation.Nonnull; +import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.AlreadyExistsException; import org.apache.iceberg.exceptions.BadRequestException; +import org.apache.iceberg.exceptions.NoSuchNamespaceException; +import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; -import org.apache.polaris.core.persistence.dao.entity.DropEntityResult; +import org.apache.polaris.core.persistence.PolicyMappingAlreadyExistsException; import org.apache.polaris.core.persistence.dao.entity.EntityResult; +import org.apache.polaris.core.persistence.dao.entity.LoadPolicyMappingsResult; import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView; import org.apache.polaris.core.policy.PolicyEntity; import org.apache.polaris.core.policy.PolicyType; @@ -40,6 +52,7 @@ import org.apache.polaris.core.policy.exceptions.PolicyVersionMismatchException; import org.apache.polaris.core.policy.validator.PolicyValidators; import org.apache.polaris.service.types.Policy; +import org.apache.polaris.service.types.PolicyAttachmentTarget; import org.apache.polaris.service.types.PolicyIdentifier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -251,25 +264,203 @@ public Policy updatePolicy( public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) { // TODO: Implement detachAll when we support attach/detach policy - PolarisResolvedPathWrapper resolvedEntities = + var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); + var catalogPath = resolvedPolicyPath.getRawParentPath(); + var policyEntity = resolvedPolicyPath.getRawLeafEntity(); + + return metaStoreManager + .dropEntityIfExists( + callContext.getPolarisCallContext(), + PolarisEntity.toCoreList(catalogPath), + policyEntity, + Map.of(), + false) + .isSuccess(); + } + + public boolean attachPolicy( + PolicyIdentifier policyIdentifier, + PolicyAttachmentTarget target, + Map parameters) { + + var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); + var policyCatalogPath = PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath()); + var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity()); + + var resolvedTargetPath = getResolvedPathWrapper(target); + var targetCatalogPath = PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath()); + var targetEntity = resolvedTargetPath.getRawLeafEntity(); + + PolicyValidators.validateAttach(policyEntity, targetEntity); + + var result = + metaStoreManager.attachPolicyToEntity( + callContext.getPolarisCallContext(), + targetCatalogPath, + targetEntity, + policyCatalogPath, + policyEntity, + parameters); + + if (result.getReturnStatus() == POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS) { + var targetId = catalogEntity.getName(); + if (target.getType() != CATALOG) { + targetId += "." + String.join(".", target.getPath()); + } + throw new PolicyMappingAlreadyExistsException( + "The policy mapping of same type(%s) for %s already exists", + policyEntity.getPolicyType().getName(), targetId); + } + + return result.isSuccess(); + } + + public boolean detachPolicy(PolicyIdentifier policyIdentifier, PolicyAttachmentTarget target) { + var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); + var policyCatalogPath = PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath()); + var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity()); + + var resolvedTargetPath = getResolvedPathWrapper(target); + var targetCatalogPath = PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath()); + var targetEntity = resolvedTargetPath.getRawLeafEntity(); + + return metaStoreManager + .detachPolicyFromEntity( + callContext.getPolarisCallContext(), + targetCatalogPath, + targetEntity, + policyCatalogPath, + policyEntity) + .isSuccess(); + } + + public List getApplicablePolicies( + Namespace namespace, String targetName, PolicyType policyType) { + var targetFullPath = getFullPath(namespace, targetName); + return getEffectivePolicies(targetFullPath, policyType); + } + + private List getEffectivePolicies(List path, PolicyType policyType) { + if (path == null || path.isEmpty()) { + return List.of(); + } + + Map inheritedPolicies = new LinkedHashMap<>(); + // Final list of effective policies (inheritable + last-entity non-inheritable) + List finalPolicies = new ArrayList<>(); + + // Process all entities except the last one + for (int i = 0; i < path.size() - 1; i++) { + PolarisEntity entity = path.get(i); + List currentPolicies = getPolicies(entity, policyType); + + for (Policy policy : currentPolicies) { + // For non-last entities, we only carry forward inheritable policies + if (policy.getInheritable()) { + // Put in map; overwrites by policyType if encountered again + inheritedPolicies.put(policy.getPolicyType(), policy); + } + } + } + + // Now handle the last entity's policies + List lastPolicies = getPolicies(path.getLast(), policyType); + + for (Policy policy : lastPolicies) { + if (policy.getInheritable()) { + // Overwrite anything by the same policyType in the inherited map + inheritedPolicies.put(policy.getPolicyType(), policy); + } else { + // Non-inheritable => goes directly to final list + finalPolicies.add(policy); + } + } + + // Append all inherited policies at the end, preserving insertion order + finalPolicies.addAll(inheritedPolicies.values()); + + return finalPolicies; + } + + private List getPolicies(PolarisEntity target, PolicyType policyType) { + LoadPolicyMappingsResult result; + if (policyType == null) { + result = metaStoreManager.loadPoliciesOnEntity(callContext.getPolarisCallContext(), target); + } else { + result = + metaStoreManager.loadPoliciesOnEntityByType( + callContext.getPolarisCallContext(), target, policyType); + } + + return result.getEntities().stream().map(PolicyCatalog::toPolicy).toList(); + } + + private List getFullPath(Namespace namespace, String targetName) { + if (namespace == null || namespace.isEmpty()) { + // catalog + return List.of(catalogEntity); + } else if (Strings.isNullOrEmpty(targetName)) { + // namespace + var resolvedTargetEntity = resolvedEntityView.getResolvedPath(namespace); + if (resolvedTargetEntity == null) { + throw new NoSuchNamespaceException("Namespace does not exist: %s", namespace); + } + return resolvedTargetEntity.getRawFullPath(); + } else { + // table + var tableIdentifier = TableIdentifier.of(namespace, targetName); + // only Iceberg tables are supported + var resolvedTableEntity = + resolvedEntityView.getResolvedPath( + tableIdentifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE); + if (resolvedTableEntity == null) { + throw new NoSuchTableException("Iceberg Table does not exist: %s", tableIdentifier); + } + return resolvedTableEntity.getRawFullPath(); + } + } + + private static Policy toPolicy(PolarisBaseEntity polarisBaseEntity) { + var policyEntity = PolicyEntity.of(polarisBaseEntity); + return constructPolicy(policyEntity); + } + + private PolarisResolvedPathWrapper getResolvedPathWrapper(PolicyIdentifier policyIdentifier) { + var resolvedEntities = resolvedEntityView.getPassthroughResolvedPath( policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); if (resolvedEntities == null) { throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); } + return resolvedEntities; + } - List catalogPath = resolvedEntities.getRawParentPath(); - PolarisEntity leafEntity = resolvedEntities.getRawLeafEntity(); - - DropEntityResult dropEntityResult = - metaStoreManager.dropEntityIfExists( - callContext.getPolarisCallContext(), - PolarisEntity.toCoreList(catalogPath), - leafEntity, - Map.of(), - false); - - return dropEntityResult.isSuccess(); + private PolarisResolvedPathWrapper getResolvedPathWrapper( + @Nonnull PolicyAttachmentTarget target) { + return switch (target.getType()) { + // get the current catalog entity, since policy cannot apply across catalog at this moment + case CATALOG -> resolvedEntityView.getResolvedReferenceCatalogEntity(); + case NAMESPACE -> { + var namespace = Namespace.of(target.getPath().toArray(new String[0])); + var resolvedTargetEntity = resolvedEntityView.getResolvedPath(namespace); + if (resolvedTargetEntity == null) { + throw new NoSuchNamespaceException("Namespace does not exist: %s", namespace); + } + yield resolvedTargetEntity; + } + case TABLE_LIKE -> { + var tableIdentifier = TableIdentifier.of(target.getPath().toArray(new String[0])); + // only Iceberg tables are supported + var resolvedTableEntity = + resolvedEntityView.getResolvedPath( + tableIdentifier, PolarisEntityType.TABLE_LIKE, PolarisEntitySubType.ICEBERG_TABLE); + if (resolvedTableEntity == null) { + throw new NoSuchTableException("Iceberg Table does not exist: %s", tableIdentifier); + } + yield resolvedTableEntity; + } + default -> throw new IllegalArgumentException("Unsupported target type: " + target.getType()); + }; } private static Policy constructPolicy(PolicyEntity policyEntity) { diff --git a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java index 22b9e187db..037baa3be7 100644 --- a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java @@ -26,6 +26,7 @@ import org.apache.polaris.core.exceptions.AlreadyExistsException; import org.apache.polaris.core.exceptions.PolarisException; import org.apache.polaris.core.policy.exceptions.NoSuchPolicyException; +import org.apache.polaris.core.policy.exceptions.PolicyAttachException; import org.apache.polaris.core.policy.exceptions.PolicyVersionMismatchException; import org.apache.polaris.core.policy.validator.InvalidPolicyException; import org.apache.polaris.service.context.UnresolvableRealmContextException; @@ -49,6 +50,8 @@ private Response.Status getStatus(PolarisException exception) { return Response.Status.NOT_FOUND; } else if (exception instanceof InvalidPolicyException) { return Response.Status.BAD_REQUEST; + } else if (exception instanceof PolicyAttachException) { + return Response.Status.BAD_REQUEST; } else if (exception instanceof NoSuchPolicyException) { return Response.Status.NOT_FOUND; } else if (exception instanceof PolicyVersionMismatchException) { From 50ba63395f6f17d97bb988ba1c8d350f152250e2 Mon Sep 17 00:00:00 2001 From: Yufei Gu Date: Fri, 4 Apr 2025 12:03:33 -0700 Subject: [PATCH 2/6] Add more test cases --- .../quarkus/catalog/PolicyCatalogTest.java | 156 ++++++++++++------ .../service/catalog/policy/PolicyCatalog.java | 59 ++++--- 2 files changed, 135 insertions(+), 80 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java index 56976f51be..ce0e91235e 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java @@ -18,8 +18,10 @@ */ package org.apache.polaris.service.quarkus.catalog; +import static org.apache.iceberg.types.Types.NestedField.required; import static org.apache.polaris.core.policy.PredefinedPolicyTypes.DATA_COMPACTION; import static org.apache.polaris.core.policy.PredefinedPolicyTypes.METADATA_COMPACTION; +import static org.apache.polaris.core.policy.PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -42,9 +44,11 @@ import java.util.function.Supplier; import org.apache.commons.lang3.NotImplementedException; import org.apache.iceberg.CatalogProperties; +import org.apache.iceberg.Schema; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.AlreadyExistsException; +import org.apache.iceberg.types.Types; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; import org.apache.polaris.core.admin.model.AwsStorageConfigInfo; @@ -118,20 +122,26 @@ public Map getConfigOverrides() { } } - protected static final Namespace NS = Namespace.of("newdb"); - protected static final TableIdentifier TABLE = TableIdentifier.of(NS, "table"); - public static final String CATALOG_NAME = "polaris-catalog"; - public static final String TEST_ACCESS_KEY = "test_access_key"; - public static final String SECRET_ACCESS_KEY = "secret_access_key"; - public static final String SESSION_TOKEN = "session_token"; - - private static final Namespace NS1 = Namespace.of("ns1"); - private static final PolicyIdentifier POLICY1 = new PolicyIdentifier(NS1, "p1"); - private static final PolicyIdentifier POLICY2 = new PolicyIdentifier(NS1, "p2"); - private static final PolicyIdentifier POLICY3 = new PolicyIdentifier(NS1, "p3"); - private static final PolicyIdentifier POLICY4 = new PolicyIdentifier(NS1, "p4"); + private static final Namespace NS = Namespace.of("ns1"); + private static final TableIdentifier TABLE = TableIdentifier.of(NS, "table"); + private static final String CATALOG_NAME = "polaris-catalog"; + private static final String TEST_ACCESS_KEY = "test_access_key"; + private static final String SECRET_ACCESS_KEY = "secret_access_key"; + private static final String SESSION_TOKEN = "session_token"; + private static final Schema SCHEMA = + new Schema( + required(3, "id", Types.IntegerType.get(), "unique ID"), + required(4, "data", Types.StringType.get())); + + private static final PolicyIdentifier POLICY1 = new PolicyIdentifier(NS, "p1"); + private static final PolicyIdentifier POLICY2 = new PolicyIdentifier(NS, "p2"); + private static final PolicyIdentifier POLICY3 = new PolicyIdentifier(NS, "p3"); + private static final PolicyIdentifier POLICY4 = new PolicyIdentifier(NS, "p4"); private static final PolicyAttachmentTarget POLICY_ATTACH_TARGET_NS = - new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.NAMESPACE, List.of(NS1.levels())); + new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.NAMESPACE, List.of(NS.levels())); + private static final PolicyAttachmentTarget POLICY_ATTACH_TARGET_TBL = + new PolicyAttachmentTarget( + PolicyAttachmentTarget.TypeEnum.TABLE_LIKE, List.of(TABLE.toString().split("\\."))); @Inject MetaStoreManagerFactory managerFactory; @Inject PolarisConfigurationStore configurationStore; @@ -312,7 +322,7 @@ public Map purgeRealms(Iterable realms) { @Test public void testCreatePolicyDoesNotThrow() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); assertThatCode( () -> policyCatalog.createPolicy( @@ -325,7 +335,7 @@ public void testCreatePolicyDoesNotThrow() { @Test public void testCreatePolicyAlreadyExists() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); assertThatThrownBy( @@ -349,7 +359,7 @@ public void testCreatePolicyAlreadyExists() { @Test public void testListPolicies() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); @@ -363,19 +373,16 @@ public void testListPolicies() { POLICY3, PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy( - POLICY4, - PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL.getName(), - "test", - "{\"enable\": false}"); + POLICY4, ORPHAN_FILE_REMOVAL.getName(), "test", "{\"enable\": false}"); - List listResult = policyCatalog.listPolicies(NS1, null); + List listResult = policyCatalog.listPolicies(NS, null); assertThat(listResult).hasSize(4); assertThat(listResult).containsExactlyInAnyOrder(POLICY1, POLICY2, POLICY3, POLICY4); } @Test public void testListPoliciesFilterByPolicyType() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); @@ -389,20 +396,16 @@ public void testListPoliciesFilterByPolicyType() { POLICY3, PredefinedPolicyTypes.SNAPSHOT_RETENTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy( - POLICY4, - PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL.getName(), - "test", - "{\"enable\": false}"); + POLICY4, ORPHAN_FILE_REMOVAL.getName(), "test", "{\"enable\": false}"); - List listResult = - policyCatalog.listPolicies(NS1, PredefinedPolicyTypes.ORPHAN_FILE_REMOVAL); + List listResult = policyCatalog.listPolicies(NS, ORPHAN_FILE_REMOVAL); assertThat(listResult).hasSize(1); assertThat(listResult).containsExactlyInAnyOrder(POLICY4); } @Test public void testLoadPolicy() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); @@ -416,7 +419,7 @@ public void testLoadPolicy() { @Test public void testCreatePolicyWithInvalidContent() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); assertThatThrownBy( () -> @@ -427,7 +430,7 @@ public void testCreatePolicyWithInvalidContent() { @Test public void testLoadPolicyNotExist() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); assertThatThrownBy(() -> policyCatalog.loadPolicy(POLICY1)) .isInstanceOf(NoSuchPolicyException.class); @@ -435,7 +438,7 @@ public void testLoadPolicyNotExist() { @Test public void testUpdatePolicy() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); policyCatalog.updatePolicy(POLICY1, "updated", "{\"enable\": true}", 0); @@ -450,7 +453,7 @@ public void testUpdatePolicy() { @Test public void testUpdatePolicyWithWrongVersion() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); @@ -461,7 +464,7 @@ public void testUpdatePolicyWithWrongVersion() { @Test public void testUpdatePolicyWithInvalidContent() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); @@ -471,7 +474,7 @@ public void testUpdatePolicyWithInvalidContent() { @Test public void testUpdatePolicyNotExist() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); assertThatThrownBy( () -> policyCatalog.updatePolicy(POLICY1, "updated", "{\"enable\": true}", 0)) @@ -480,7 +483,7 @@ public void testUpdatePolicyNotExist() { @Test public void testDropPolicy() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); @@ -491,7 +494,7 @@ public void testDropPolicy() { @Test public void testDropPolicyNotExist() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); assertThatThrownBy(() -> policyCatalog.dropPolicy(POLICY1, false)) .isInstanceOf(NoSuchPolicyException.class); @@ -499,7 +502,7 @@ public void testDropPolicyNotExist() { @Test public void testAttachPolicy() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); @@ -509,7 +512,7 @@ public void testAttachPolicy() { @Test public void testAttachPolicyConflict() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy(POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); @@ -526,18 +529,18 @@ public void testAttachPolicyConflict() { @Test public void testDetachPolicy() { - icebergCatalog.createNamespace(NS1); + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); assertThat(policyCatalog.attachPolicy(POLICY1, POLICY_ATTACH_TARGET_NS, null)).isTrue(); - assertThat(policyCatalog.getApplicablePolicies(NS1, null, null).size()).isEqualTo(1); + assertThat(policyCatalog.getApplicablePolicies(NS, null, null).size()).isEqualTo(1); assertThat(policyCatalog.detachPolicy(POLICY1, POLICY_ATTACH_TARGET_NS)).isTrue(); - assertThat(policyCatalog.getApplicablePolicies(NS1, null, null).size()).isEqualTo(0); + assertThat(policyCatalog.getApplicablePolicies(NS, null, null).size()).isEqualTo(0); } @Test - public void testAttachPolicyOverwrite() { - icebergCatalog.createNamespace(NS1); + public void testPolicyOverwrite() { + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); policyCatalog.createPolicy(POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); @@ -547,25 +550,72 @@ public void testAttachPolicyOverwrite() { // attach to namespace assertThat(policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null)).isTrue(); - var applicablePolicies = policyCatalog.getApplicablePolicies(NS1, null, null); + var applicablePolicies = policyCatalog.getApplicablePolicies(NS, null, null); assertThat(applicablePolicies.size()).isEqualTo(1); - assertThat(applicablePolicies.getFirst().getName()).isEqualTo(POLICY2.getName()); + assertThat(applicablePolicies.getFirst().getName()) + .isEqualTo(POLICY2.getName()) + .as("Namespace level policy overwrite the catalog level policy with the same type"); } @Test - public void testAttachPolicyInheritance() { - icebergCatalog.createNamespace(NS1); + public void testPolicyInheritance() { + icebergCatalog.createNamespace(NS); + var p1 = + policyCatalog.createPolicy( + POLICY1, METADATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + var p2 = + policyCatalog.createPolicy( + POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); + + // attach a policy to catalog + var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); + assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + + // attach a different type of policy to namespace + assertThat(policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null)).isTrue(); + var applicablePolicies = policyCatalog.getApplicablePolicies(NS, null, null); + assertThat(applicablePolicies.size()).isEqualTo(2); + assertThat(applicablePolicies.contains(p1)).isTrue(); + assertThat(applicablePolicies.contains(p2)).isTrue(); + + // attach policies to a table + icebergCatalog.createTable(TABLE, SCHEMA); + applicablePolicies = policyCatalog.getApplicablePolicies(NS, TABLE.name(), null); + assertThat(applicablePolicies.size()).isEqualTo(2); + // attach a third type of policy to a table + policyCatalog.createPolicy( + POLICY3, ORPHAN_FILE_REMOVAL.getName(), "test", "{\"enable\": false}"); + assertThat(policyCatalog.attachPolicy(POLICY3, POLICY_ATTACH_TARGET_TBL, null)).isTrue(); + applicablePolicies = policyCatalog.getApplicablePolicies(NS, TABLE.name(), null); + assertThat(applicablePolicies.size()).isEqualTo(3); + // create policy 4 with one of types from its parent + var p4 = + policyCatalog.createPolicy( + POLICY4, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); + assertThat(policyCatalog.attachPolicy(POLICY4, POLICY_ATTACH_TARGET_TBL, null)).isTrue(); + applicablePolicies = policyCatalog.getApplicablePolicies(NS, TABLE.name(), null); + // p2 should be overwritten by p4, as they are the same type + assertThat(applicablePolicies.contains(p4)).isTrue(); + assertThat(applicablePolicies.contains(p2)).isFalse(); + } + + @Test + public void testGetApplicablePoliciesFilterOnType() { + icebergCatalog.createNamespace(NS); policyCatalog.createPolicy( POLICY1, METADATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - policyCatalog.createPolicy(POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); + var p2 = + policyCatalog.createPolicy( + POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); - // attach to catalog + // attach a policy to catalog var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); - // attach to namespace + // attach a different type of policy to namespace assertThat(policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null)).isTrue(); - var applicablePolicies = policyCatalog.getApplicablePolicies(NS1, null, null); - assertThat(applicablePolicies.size()).isEqualTo(2); + var applicablePolicies = policyCatalog.getApplicablePolicies(NS, null, DATA_COMPACTION); + // only p2 is with the type fetched + assertThat(applicablePolicies.contains(p2)).isTrue(); } } diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index 778e4af047..05b14b6f8e 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -190,16 +190,8 @@ public List listPolicies(Namespace namespace, PolicyType polic } public Policy loadPolicy(PolicyIdentifier policyIdentifier) { - PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getPassthroughResolvedPath( - policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); - - PolicyEntity policy = - PolicyEntity.of(resolvedEntities == null ? null : resolvedEntities.getRawLeafEntity()); - - if (policy == null) { - throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); - } + var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); + var policy = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity()); return constructPolicy(policy); } @@ -208,16 +200,8 @@ public Policy updatePolicy( String newDescription, String newContent, int currentPolicyVersion) { - PolarisResolvedPathWrapper resolvedEntities = - resolvedEntityView.getPassthroughResolvedPath( - policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); - - PolicyEntity policy = - PolicyEntity.of(resolvedEntities == null ? null : resolvedEntities.getRawLeafEntity()); - - if (policy == null) { - throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); - } + var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); + var policy = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity()); // Verify that the current version of the policy matches the version that the user is trying to // update @@ -242,7 +226,7 @@ public Policy updatePolicy( PolicyValidators.validate(newPolicyEntity); - List catalogPath = resolvedEntities.getRawParentPath(); + List catalogPath = resolvedPolicyPath.getRawParentPath(); newPolicyEntity = Optional.ofNullable( metaStoreManager @@ -340,6 +324,27 @@ public List getApplicablePolicies( return getEffectivePolicies(targetFullPath, policyType); } + /** + * Returns the effective policies for a given hierarchical path and policy type. + * + *

Potential Performance Improvements: + * + *

    + *
  • Range Query Optimization: Enhance the query mechanism to fetch policies for all entities + * in a single range query, reducing the number of individual queries against the mapping + * table. + *
  • Filtering on Inheritable: Improve the filtering process by applying the inheritable + * condition at the data retrieval level, so that only the relevant policies for non-leaf + * nodes are processed. + *
  • Caching: Implement caching for up-level policies to avoid redundant calculations and + * lookups, especially for frequently accessed paths. + *
+ * + * @param path the list of entities representing the hierarchical path + * @param policyType the type of policy to filter on + * @return a list of effective policies, combining inherited policies from upper levels and + * non-inheritable policies from the final entity + */ private List getEffectivePolicies(List path, PolicyType policyType) { if (path == null || path.isEmpty()) { return List.of(); @@ -420,16 +425,11 @@ private List getFullPath(Namespace namespace, String targetName) } } - private static Policy toPolicy(PolarisBaseEntity polarisBaseEntity) { - var policyEntity = PolicyEntity.of(polarisBaseEntity); - return constructPolicy(policyEntity); - } - private PolarisResolvedPathWrapper getResolvedPathWrapper(PolicyIdentifier policyIdentifier) { var resolvedEntities = resolvedEntityView.getPassthroughResolvedPath( policyIdentifier, PolarisEntityType.POLICY, PolarisEntitySubType.NULL_SUBTYPE); - if (resolvedEntities == null) { + if (resolvedEntities == null || resolvedEntities.getResolvedLeafEntity() == null) { throw new NoSuchPolicyException(String.format("Policy does not exist: %s", policyIdentifier)); } return resolvedEntities; @@ -463,6 +463,11 @@ private PolarisResolvedPathWrapper getResolvedPathWrapper( }; } + private static Policy toPolicy(PolarisBaseEntity polarisBaseEntity) { + var policyEntity = PolicyEntity.of(polarisBaseEntity); + return constructPolicy(policyEntity); + } + private static Policy constructPolicy(PolicyEntity policyEntity) { return Policy.builder() .setPolicyType(policyEntity.getPolicyType().getName()) From 3b32d8fc22a587b9bf7bf597fa6e9de4b2664602 Mon Sep 17 00:00:00 2001 From: Yufei Gu Date: Fri, 4 Apr 2025 22:48:55 -0700 Subject: [PATCH 3/6] Resolve comments --- .../PolicyMappingAlreadyExistsException.java | 3 +- .../service/catalog/policy/PolicyCatalog.java | 32 ++++++++----------- .../exception/PolarisExceptionMapper.java | 3 ++ 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolicyMappingAlreadyExistsException.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolicyMappingAlreadyExistsException.java index 793cea5332..a31e500b0b 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolicyMappingAlreadyExistsException.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolicyMappingAlreadyExistsException.java @@ -19,13 +19,14 @@ package org.apache.polaris.core.persistence; import com.google.errorprone.annotations.FormatMethod; +import org.apache.polaris.core.exceptions.PolarisException; import org.apache.polaris.core.policy.PolarisPolicyMappingRecord; /** * Exception raised when an existing policy mapping preveents the attempted creation of a new policy * mapping record. */ -public class PolicyMappingAlreadyExistsException extends RuntimeException { +public class PolicyMappingAlreadyExistsException extends PolarisException { private PolarisPolicyMappingRecord existingRecord; /** diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index 05b14b6f8e..88f9d7063b 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -36,7 +36,6 @@ import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.entity.CatalogEntity; -import org.apache.polaris.core.entity.PolarisBaseEntity; import org.apache.polaris.core.entity.PolarisEntity; import org.apache.polaris.core.entity.PolarisEntitySubType; import org.apache.polaris.core.entity.PolarisEntityType; @@ -350,31 +349,31 @@ private List getEffectivePolicies(List path, PolicyType p return List.of(); } - Map inheritedPolicies = new LinkedHashMap<>(); + Map inheritedPolicies = new LinkedHashMap<>(); // Final list of effective policies (inheritable + last-entity non-inheritable) - List finalPolicies = new ArrayList<>(); + List finalPolicies = new ArrayList<>(); // Process all entities except the last one for (int i = 0; i < path.size() - 1; i++) { PolarisEntity entity = path.get(i); - List currentPolicies = getPolicies(entity, policyType); + var currentPolicies = getPolicies(entity, policyType); - for (Policy policy : currentPolicies) { + for (var policy : currentPolicies) { // For non-last entities, we only carry forward inheritable policies - if (policy.getInheritable()) { + if (policy.getPolicyType().isInheritable()) { // Put in map; overwrites by policyType if encountered again - inheritedPolicies.put(policy.getPolicyType(), policy); + inheritedPolicies.put(policy.getPolicyType().getName(), policy); } } } // Now handle the last entity's policies - List lastPolicies = getPolicies(path.getLast(), policyType); + List lastPolicies = getPolicies(path.getLast(), policyType); - for (Policy policy : lastPolicies) { - if (policy.getInheritable()) { + for (var policy : lastPolicies) { + if (policy.getPolicyType().isInheritable()) { // Overwrite anything by the same policyType in the inherited map - inheritedPolicies.put(policy.getPolicyType(), policy); + inheritedPolicies.put(policy.getPolicyType().getName(), policy); } else { // Non-inheritable => goes directly to final list finalPolicies.add(policy); @@ -384,10 +383,10 @@ private List getEffectivePolicies(List path, PolicyType p // Append all inherited policies at the end, preserving insertion order finalPolicies.addAll(inheritedPolicies.values()); - return finalPolicies; + return finalPolicies.stream().map(PolicyCatalog::constructPolicy).toList(); } - private List getPolicies(PolarisEntity target, PolicyType policyType) { + private List getPolicies(PolarisEntity target, PolicyType policyType) { LoadPolicyMappingsResult result; if (policyType == null) { result = metaStoreManager.loadPoliciesOnEntity(callContext.getPolarisCallContext(), target); @@ -397,7 +396,7 @@ private List getPolicies(PolarisEntity target, PolicyType policyType) { callContext.getPolarisCallContext(), target, policyType); } - return result.getEntities().stream().map(PolicyCatalog::toPolicy).toList(); + return result.getEntities().stream().map(PolicyEntity::of).toList(); } private List getFullPath(Namespace namespace, String targetName) { @@ -463,11 +462,6 @@ private PolarisResolvedPathWrapper getResolvedPathWrapper( }; } - private static Policy toPolicy(PolarisBaseEntity polarisBaseEntity) { - var policyEntity = PolicyEntity.of(polarisBaseEntity); - return constructPolicy(policyEntity); - } - private static Policy constructPolicy(PolicyEntity policyEntity) { return Policy.builder() .setPolicyType(policyEntity.getPolicyType().getName()) diff --git a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java index 037baa3be7..57df2cbc53 100644 --- a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java @@ -25,6 +25,7 @@ import org.apache.iceberg.rest.responses.ErrorResponse; import org.apache.polaris.core.exceptions.AlreadyExistsException; import org.apache.polaris.core.exceptions.PolarisException; +import org.apache.polaris.core.persistence.PolicyMappingAlreadyExistsException; import org.apache.polaris.core.policy.exceptions.NoSuchPolicyException; import org.apache.polaris.core.policy.exceptions.PolicyAttachException; import org.apache.polaris.core.policy.exceptions.PolicyVersionMismatchException; @@ -56,6 +57,8 @@ private Response.Status getStatus(PolarisException exception) { return Response.Status.NOT_FOUND; } else if (exception instanceof PolicyVersionMismatchException) { return Response.Status.CONFLICT; + } else if (exception instanceof PolicyMappingAlreadyExistsException) { + return Response.Status.CONFLICT; } else { return Response.Status.INTERNAL_SERVER_ERROR; } From 44150d11a073a0b2d63b42c900cb18e18a19053d Mon Sep 17 00:00:00 2001 From: Yufei Gu Date: Mon, 7 Apr 2025 10:35:27 -0700 Subject: [PATCH 4/6] Resolve Robert's comments --- .../quarkus/catalog/PolicyCatalogTest.java | 28 ++++---- .../service/catalog/policy/PolicyCatalog.java | 66 +++++++++++++------ 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java index ce0e91235e..9e4d4b38a6 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java @@ -487,7 +487,7 @@ public void testDropPolicy() { policyCatalog.createPolicy( POLICY1, PredefinedPolicyTypes.DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - assertThat(policyCatalog.dropPolicy(POLICY1, false)).isTrue(); + policyCatalog.dropPolicy(POLICY1, false); assertThatThrownBy(() -> policyCatalog.loadPolicy(POLICY1)) .isInstanceOf(NoSuchPolicyException.class); } @@ -506,7 +506,7 @@ public void testAttachPolicy() { policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); - assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + policyCatalog.attachPolicy(POLICY1, target, null); assertThat(policyCatalog.getApplicablePolicies(null, null, null).size()).isEqualTo(1); } @@ -517,13 +517,13 @@ public void testAttachPolicyConflict() { policyCatalog.createPolicy(POLICY2, DATA_COMPACTION.getName(), "test", "{\"enable\": true}"); var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); - assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + policyCatalog.attachPolicy(POLICY1, target, null); // Attempt to attach a conflicting second policy and expect an exception assertThatThrownBy(() -> policyCatalog.attachPolicy(POLICY2, target, null)) .isInstanceOf(PolicyMappingAlreadyExistsException.class) .hasMessage( String.format( - "The policy mapping of same type(%s) for %s already exists", + "The policy mapping of same type (%s) for %s already exists", DATA_COMPACTION.getName(), CATALOG_NAME)); } @@ -532,9 +532,9 @@ public void testDetachPolicy() { icebergCatalog.createNamespace(NS); policyCatalog.createPolicy(POLICY1, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - assertThat(policyCatalog.attachPolicy(POLICY1, POLICY_ATTACH_TARGET_NS, null)).isTrue(); + policyCatalog.attachPolicy(POLICY1, POLICY_ATTACH_TARGET_NS, null); assertThat(policyCatalog.getApplicablePolicies(NS, null, null).size()).isEqualTo(1); - assertThat(policyCatalog.detachPolicy(POLICY1, POLICY_ATTACH_TARGET_NS)).isTrue(); + policyCatalog.detachPolicy(POLICY1, POLICY_ATTACH_TARGET_NS); assertThat(policyCatalog.getApplicablePolicies(NS, null, null).size()).isEqualTo(0); } @@ -546,10 +546,10 @@ public void testPolicyOverwrite() { // attach to catalog var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); - assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + policyCatalog.attachPolicy(POLICY1, target, null); // attach to namespace - assertThat(policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null)).isTrue(); + policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null); var applicablePolicies = policyCatalog.getApplicablePolicies(NS, null, null); assertThat(applicablePolicies.size()).isEqualTo(1); assertThat(applicablePolicies.getFirst().getName()) @@ -569,10 +569,10 @@ public void testPolicyInheritance() { // attach a policy to catalog var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); - assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + policyCatalog.attachPolicy(POLICY1, target, null); // attach a different type of policy to namespace - assertThat(policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null)).isTrue(); + policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null); var applicablePolicies = policyCatalog.getApplicablePolicies(NS, null, null); assertThat(applicablePolicies.size()).isEqualTo(2); assertThat(applicablePolicies.contains(p1)).isTrue(); @@ -585,14 +585,14 @@ public void testPolicyInheritance() { // attach a third type of policy to a table policyCatalog.createPolicy( POLICY3, ORPHAN_FILE_REMOVAL.getName(), "test", "{\"enable\": false}"); - assertThat(policyCatalog.attachPolicy(POLICY3, POLICY_ATTACH_TARGET_TBL, null)).isTrue(); + policyCatalog.attachPolicy(POLICY3, POLICY_ATTACH_TARGET_TBL, null); applicablePolicies = policyCatalog.getApplicablePolicies(NS, TABLE.name(), null); assertThat(applicablePolicies.size()).isEqualTo(3); // create policy 4 with one of types from its parent var p4 = policyCatalog.createPolicy( POLICY4, DATA_COMPACTION.getName(), "test", "{\"enable\": false}"); - assertThat(policyCatalog.attachPolicy(POLICY4, POLICY_ATTACH_TARGET_TBL, null)).isTrue(); + policyCatalog.attachPolicy(POLICY4, POLICY_ATTACH_TARGET_TBL, null); applicablePolicies = policyCatalog.getApplicablePolicies(NS, TABLE.name(), null); // p2 should be overwritten by p4, as they are the same type assertThat(applicablePolicies.contains(p4)).isTrue(); @@ -610,10 +610,10 @@ public void testGetApplicablePoliciesFilterOnType() { // attach a policy to catalog var target = new PolicyAttachmentTarget(PolicyAttachmentTarget.TypeEnum.CATALOG, List.of()); - assertThat(policyCatalog.attachPolicy(POLICY1, target, null)).isTrue(); + policyCatalog.attachPolicy(POLICY1, target, null); // attach a different type of policy to namespace - assertThat(policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null)).isTrue(); + policyCatalog.attachPolicy(POLICY2, POLICY_ATTACH_TARGET_NS, null); var applicablePolicies = policyCatalog.getApplicablePolicies(NS, null, DATA_COMPACTION); // only p2 is with the type fetched assertThat(applicablePolicies.contains(p2)).isTrue(); diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index 88f9d7063b..e04daf6fd1 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -48,6 +48,7 @@ import org.apache.polaris.core.policy.PolicyEntity; import org.apache.polaris.core.policy.PolicyType; import org.apache.polaris.core.policy.exceptions.NoSuchPolicyException; +import org.apache.polaris.core.policy.exceptions.PolicyAttachException; import org.apache.polaris.core.policy.exceptions.PolicyVersionMismatchException; import org.apache.polaris.core.policy.validator.PolicyValidators; import org.apache.polaris.service.types.Policy; @@ -245,23 +246,29 @@ public Policy updatePolicy( return constructPolicy(newPolicyEntity); } - public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) { + public void dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) { // TODO: Implement detachAll when we support attach/detach policy var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); var catalogPath = resolvedPolicyPath.getRawParentPath(); var policyEntity = resolvedPolicyPath.getRawLeafEntity(); - return metaStoreManager - .dropEntityIfExists( + var result = + metaStoreManager.dropEntityIfExists( callContext.getPolarisCallContext(), PolarisEntity.toCoreList(catalogPath), policyEntity, Map.of(), - false) - .isSuccess(); + false); + + if (!result.isSuccess()) { + throw new IllegalStateException( + String.format( + "Failed to drop policy %s error status: %s with extraInfo: %s", + policyIdentifier, result.getReturnStatus(), result.getExtraInformation())); + } } - public boolean attachPolicy( + public void attachPolicy( PolicyIdentifier policyIdentifier, PolicyAttachmentTarget target, Map parameters) { @@ -285,20 +292,21 @@ public boolean attachPolicy( policyEntity, parameters); - if (result.getReturnStatus() == POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS) { - var targetId = catalogEntity.getName(); - if (target.getType() != CATALOG) { - targetId += "." + String.join(".", target.getPath()); + if (!result.isSuccess()) { + var targetId = getIdentifier(target); + if (result.getReturnStatus() == POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS) { + throw new PolicyMappingAlreadyExistsException( + "The policy mapping of same type (%s) for %s already exists", + policyEntity.getPolicyType().getName(), targetId); } - throw new PolicyMappingAlreadyExistsException( - "The policy mapping of same type(%s) for %s already exists", - policyEntity.getPolicyType().getName(), targetId); - } - return result.isSuccess(); + throw new PolicyAttachException( + "Failed to attach policy %s to %s: %s with extraInfo: %s", + policyIdentifier, targetId, result.getReturnStatus(), result.getExtraInformation()); + } } - public boolean detachPolicy(PolicyIdentifier policyIdentifier, PolicyAttachmentTarget target) { + public void detachPolicy(PolicyIdentifier policyIdentifier, PolicyAttachmentTarget target) { var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); var policyCatalogPath = PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath()); var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity()); @@ -307,14 +315,23 @@ public boolean detachPolicy(PolicyIdentifier policyIdentifier, PolicyAttachmentT var targetCatalogPath = PolarisEntity.toCoreList(resolvedTargetPath.getRawParentPath()); var targetEntity = resolvedTargetPath.getRawLeafEntity(); - return metaStoreManager - .detachPolicyFromEntity( + var result = + metaStoreManager.detachPolicyFromEntity( callContext.getPolarisCallContext(), targetCatalogPath, targetEntity, policyCatalogPath, - policyEntity) - .isSuccess(); + policyEntity); + + if (!result.isSuccess()) { + throw new IllegalStateException( + String.format( + "Failed to detach policy %s from %s error status: %s with extraInfo: %s", + policyIdentifier, + getIdentifier(target), + result.getReturnStatus(), + result.getExtraInformation())); + } } public List getApplicablePolicies( @@ -424,6 +441,15 @@ private List getFullPath(Namespace namespace, String targetName) } } + private String getIdentifier(PolicyAttachmentTarget target) { + String identifier = catalogEntity.getName(); + // If the target is not of type CATALOG, append the additional path segments. + if (target.getType() != CATALOG) { + identifier += "." + String.join(".", target.getPath()); + } + return identifier; + } + private PolarisResolvedPathWrapper getResolvedPathWrapper(PolicyIdentifier policyIdentifier) { var resolvedEntities = resolvedEntityView.getPassthroughResolvedPath( From 2cf8e1ce3d50f1a8581837123a865d1efd9d7a24 Mon Sep 17 00:00:00 2001 From: Yufei Gu Date: Mon, 7 Apr 2025 10:37:27 -0700 Subject: [PATCH 5/6] Resolve Robert's comments --- .../transactional/TransactionalMetaStoreManagerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java index 94f4ac239b..27b049e917 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java @@ -47,7 +47,6 @@ import org.apache.polaris.core.entity.PolarisPrivilege; import org.apache.polaris.core.entity.PolarisTaskConstants; import org.apache.polaris.core.persistence.*; -import org.apache.polaris.core.persistence.PolicyMappingAlreadyExistsException; import org.apache.polaris.core.persistence.dao.entity.BaseResult; import org.apache.polaris.core.persistence.dao.entity.ChangeTrackingResult; import org.apache.polaris.core.persistence.dao.entity.CreateCatalogResult; From 4c4273bff36a4cc5b0555a3c8eb9d2fce0c15880 Mon Sep 17 00:00:00 2001 From: Yufei Gu Date: Mon, 7 Apr 2025 16:46:52 -0700 Subject: [PATCH 6/6] Resolve comments --- .../service/catalog/policy/PolicyCatalog.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java index e04daf6fd1..718b4ef9e0 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java @@ -246,7 +246,7 @@ public Policy updatePolicy( return constructPolicy(newPolicyEntity); } - public void dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) { + public boolean dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) { // TODO: Implement detachAll when we support attach/detach policy var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); var catalogPath = resolvedPolicyPath.getRawParentPath(); @@ -266,9 +266,11 @@ public void dropPolicy(PolicyIdentifier policyIdentifier, boolean detachAll) { "Failed to drop policy %s error status: %s with extraInfo: %s", policyIdentifier, result.getReturnStatus(), result.getExtraInformation())); } + + return true; } - public void attachPolicy( + public boolean attachPolicy( PolicyIdentifier policyIdentifier, PolicyAttachmentTarget target, Map parameters) { @@ -304,9 +306,11 @@ public void attachPolicy( "Failed to attach policy %s to %s: %s with extraInfo: %s", policyIdentifier, targetId, result.getReturnStatus(), result.getExtraInformation()); } + + return true; } - public void detachPolicy(PolicyIdentifier policyIdentifier, PolicyAttachmentTarget target) { + public boolean detachPolicy(PolicyIdentifier policyIdentifier, PolicyAttachmentTarget target) { var resolvedPolicyPath = getResolvedPathWrapper(policyIdentifier); var policyCatalogPath = PolarisEntity.toCoreList(resolvedPolicyPath.getRawParentPath()); var policyEntity = PolicyEntity.of(resolvedPolicyPath.getRawLeafEntity()); @@ -332,6 +336,8 @@ public void detachPolicy(PolicyIdentifier policyIdentifier, PolicyAttachmentTarg result.getReturnStatus(), result.getExtraInformation())); } + + return true; } public List getApplicablePolicies(