flyrain commented on code in PR #1104: URL: https://github.com/apache/polaris/pull/1104#discussion_r2008466129
########## polaris-core/src/main/java/org/apache/polaris/core/policy/PolarisPolicyMappingRecord.java: ########## @@ -0,0 +1,215 @@ +/* + * 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; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; + +public class PolarisPolicyMappingRecord { Review Comment: Can we move this to the package `org.apache.polaris.core.entity`? I think it's more suitable there. ########## polaris-core/src/main/java/org/apache/polaris/core/policy/TransactionalPolicyMappingPersistence.java: ########## @@ -0,0 +1,84 @@ +/* + * 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; + +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.List; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.entity.PolarisEntityCore; + +public interface TransactionalPolicyMappingPersistence extends PolicyMappingPersistence { Review Comment: `TransactionalPersistence` itself introduces grant records related interfaces for transactional persistence. We could do the same thing by move all methods here to `TransactionalPersistence`, but I like you did here to let `TransactionalPersistence` extend this. I recommend to move this interface to package `org.apache.polaris.core.persistence.transactional`. Moreover, I don't think this needs to extend `PolicyMappingPersistence`, which is already extended by `TransactionalPersistence` ########## extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkStore.java: ########## @@ -411,6 +413,121 @@ void deletePrincipalSecrets(EntityManager session, String clientId) { session.remove(modelPrincipalSecrets); } + void writeToPolicyMappingRecords( + EntityManager session, PolarisPolicyMappingRecord mappingRecord) { + diagnosticServices.check(session != null, "session_is_null"); + checkInitialized(); + + session.persist(ModelPolicyMappingRecord.fromPolicyMappingRecord(mappingRecord)); Review Comment: Should we check if `ModelPolicyMappingRecord.fromPolicyMappingRecord(mappingRecord)` is null? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java: ########## @@ -111,6 +111,11 @@ public enum ReturnStatus { // error caught while sub-scoping credentials. Error message will be returned SUBSCOPE_CREDS_ERROR(13), Review Comment: nit: add one empty line below? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java: ########## @@ -1821,4 +1824,176 @@ public Map<String, String> getInternalPropertyMap( // return the result return new ResolvedEntityResult(entity, entityVersions.getGrantRecordsVersion(), grantRecords); } + + @Override + public @Nonnull AttachmentResult attachPolicyToEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> targetCatalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy, + Map<String, String> parameters) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolicyType policyType = PolicyType.fromCode(policy.getPolicyTypeCode()); + if (policyType == null) { + // This should never happen + return new AttachmentResult( + BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Unknown policy type"); + } + + // TODO: this may make it necessary to push down this logic to policy persistence layer + if (policyType.isInheritable()) { + // Verify that there is no other mapping of the same type but different entity + List<PolarisPolicyMappingRecord> existingRecordsOfSameType = + ms.loadPoliciesOnTargetByType( + callCtx, target.getCatalogId(), target.getId(), policy.getPolicyTypeCode()); + if (existingRecordsOfSameType.size() > 1) { + return new AttachmentResult( + BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS, null); + } else if (existingRecordsOfSameType.size() == 1) { + PolarisPolicyMappingRecord existingRecord = existingRecordsOfSameType.get(0); + if (existingRecord.getPolicyId() != policy.getId() + || existingRecord.getPolicyCatalogId() != policy.getCatalogId()) { + return new AttachmentResult( + BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS, null); + } + } + } + + PolarisPolicyMappingRecord mappingRecord = + this.persistNewPolicyMappingRecord(callCtx, ms, target, policy, parameters); + return new AttachmentResult(mappingRecord); + } + + @Override + public @Nonnull AttachmentResult detachPolicyFromEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> catalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolarisPolicyMappingRecord mappingRecord = + ms.lookupPolicyMappingRecord( + callCtx, + target.getCatalogId(), + target.getId(), + policy.getPolicyTypeCode(), + policy.getCatalogId(), + policy.getId()); + if (mappingRecord == null) { + return new AttachmentResult(BaseResult.ReturnStatus.POLICY_MAPPING_NOT_FOUND, null); + } + + ms.deleteFromPolicyMappingRecords(callCtx, mappingRecord); + + return new AttachmentResult(mappingRecord); + } + + @Override + public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntity( + @Nonnull PolarisCallContext callCtx, @Nonnull PolarisEntityCore target) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + int grantRecordVersion = + ms.lookupEntityGrantRecordsVersion(callCtx, target.getCatalogId(), target.getId()); Review Comment: A bit confusing here. How does `lookupEntityGrantRecordsVersion` help here? We are trying to find if an entity like namespace exists. Can we just lookup the entity itself? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/PolicyMappingPersistence.java: ########## @@ -0,0 +1,151 @@ +/* + * 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; + +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.List; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.entity.PolarisEntityCore; + +/** + * Interface for interacting with the Polaris persistence backend for Policy Mapping operations. + * This interface provides methods to persist and retrieve policy mapping records, which define the + * relationships between policies and target entities in Polaris. + * + * <p>Note that APIs to the actual persistence store are very basic, often point read or write to + * the underlying data store. The goal is to make it really easy to back this using databases like + * Postgres or simpler KV store. Review Comment: I think it's worth to mention that each method in the class should be an atomic operation. Not a blocker though, we should comment the same thing in `BasePersistence` ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java: ########## @@ -1821,4 +1824,176 @@ public Map<String, String> getInternalPropertyMap( // return the result return new ResolvedEntityResult(entity, entityVersions.getGrantRecordsVersion(), grantRecords); } + + @Override + public @Nonnull AttachmentResult attachPolicyToEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> targetCatalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy, + Map<String, String> parameters) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolicyType policyType = PolicyType.fromCode(policy.getPolicyTypeCode()); + if (policyType == null) { + // This should never happen + return new AttachmentResult( + BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Unknown policy type"); + } + + // TODO: this may make it necessary to push down this logic to policy persistence layer + if (policyType.isInheritable()) { + // Verify that there is no other mapping of the same type but different entity + List<PolarisPolicyMappingRecord> existingRecordsOfSameType = + ms.loadPoliciesOnTargetByType( + callCtx, target.getCatalogId(), target.getId(), policy.getPolicyTypeCode()); + if (existingRecordsOfSameType.size() > 1) { + return new AttachmentResult( + BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS, null); + } else if (existingRecordsOfSameType.size() == 1) { + PolarisPolicyMappingRecord existingRecord = existingRecordsOfSameType.get(0); + if (existingRecord.getPolicyId() != policy.getId() + || existingRecord.getPolicyCatalogId() != policy.getCatalogId()) { + return new AttachmentResult( + BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS, null); + } + } + } + + PolarisPolicyMappingRecord mappingRecord = + this.persistNewPolicyMappingRecord(callCtx, ms, target, policy, parameters); + return new AttachmentResult(mappingRecord); + } + + @Override + public @Nonnull AttachmentResult detachPolicyFromEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> catalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolarisPolicyMappingRecord mappingRecord = + ms.lookupPolicyMappingRecord( + callCtx, + target.getCatalogId(), + target.getId(), + policy.getPolicyTypeCode(), + policy.getCatalogId(), + policy.getId()); + if (mappingRecord == null) { + return new AttachmentResult(BaseResult.ReturnStatus.POLICY_MAPPING_NOT_FOUND, null); + } + + ms.deleteFromPolicyMappingRecords(callCtx, mappingRecord); + + return new AttachmentResult(mappingRecord); + } + + @Override + public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntity( + @Nonnull PolarisCallContext callCtx, @Nonnull PolarisEntityCore target) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + int grantRecordVersion = + ms.lookupEntityGrantRecordsVersion(callCtx, target.getCatalogId(), target.getId()); + if (grantRecordVersion == 0) { + // Target entity does not exists + return new LoadPolicyMappingsResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, null); + } + + final List<PolarisPolicyMappingRecord> policyMappingRecords = + ms.loadAllPoliciesOnTarget(callCtx, target.getCatalogId(), target.getId()); + + List<PolicyEntity> policyEntities = + loadPoliciesFromMappingRecords(callCtx, ms, policyMappingRecords); + return new LoadPolicyMappingsResult(policyMappingRecords, policyEntities); + } + + @Override + public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntityByType( + @Nonnull PolarisCallContext callCtx, + @Nonnull PolarisEntityCore target, + @Nonnull PolicyType policyType) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + int grantRecordVersion = + ms.lookupEntityGrantRecordsVersion(callCtx, target.getCatalogId(), target.getId()); + if (grantRecordVersion == 0) { + // Target entity does not exists + return new LoadPolicyMappingsResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, null); + } + + final List<PolarisPolicyMappingRecord> policyMappingRecords = + ms.loadAllPoliciesOnTarget(callCtx, target.getCatalogId(), target.getId()); + + List<PolicyEntity> policyEntities = + loadPoliciesFromMappingRecords(callCtx, ms, policyMappingRecords); + return new LoadPolicyMappingsResult(policyMappingRecords, policyEntities); + } + + /** + * Create and persist a new policy mapping record + * + * @param callCtx call context + * @param ms meta store in read/write mode + * @param target target + * @param policy policy + * @param parameters optional parameters + * @return new policy mapping record which was created and persisted + */ Review Comment: nit: this java doc may not be necessary as it's a private method, and method name is self descriptive. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java: ########## @@ -41,7 +42,7 @@ * the underlying data store. The goal is to make it really easy to back this using databases like * Postgres or simpler KV store. */ -public interface BasePersistence { +public interface BasePersistence extends PolicyMappingPersistence { Review Comment: Is there any benefit we want BasePersistence to extend `PolicyMappingPersistence`? How about letting `TransactionalPersistence` extend it. This is where DAO model may fit better, we separated the concern by different DAO interfaces organized by data model entities in each layer. The current inheritance hierarchy is a bit confusing. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java: ########## @@ -1821,4 +1824,176 @@ public Map<String, String> getInternalPropertyMap( // return the result return new ResolvedEntityResult(entity, entityVersions.getGrantRecordsVersion(), grantRecords); } + + @Override + public @Nonnull AttachmentResult attachPolicyToEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> targetCatalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy, + Map<String, String> parameters) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolicyType policyType = PolicyType.fromCode(policy.getPolicyTypeCode()); + if (policyType == null) { + // This should never happen + return new AttachmentResult( + BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Unknown policy type"); + } + + // TODO: this may make it necessary to push down this logic to policy persistence layer + if (policyType.isInheritable()) { + // Verify that there is no other mapping of the same type but different entity + List<PolarisPolicyMappingRecord> existingRecordsOfSameType = + ms.loadPoliciesOnTargetByType( + callCtx, target.getCatalogId(), target.getId(), policy.getPolicyTypeCode()); + if (existingRecordsOfSameType.size() > 1) { + return new AttachmentResult( + BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS, null); + } else if (existingRecordsOfSameType.size() == 1) { + PolarisPolicyMappingRecord existingRecord = existingRecordsOfSameType.get(0); + if (existingRecord.getPolicyId() != policy.getId() + || existingRecord.getPolicyCatalogId() != policy.getCatalogId()) { + return new AttachmentResult( + BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS, null); + } + } + } + + PolarisPolicyMappingRecord mappingRecord = + this.persistNewPolicyMappingRecord(callCtx, ms, target, policy, parameters); + return new AttachmentResult(mappingRecord); + } + + @Override + public @Nonnull AttachmentResult detachPolicyFromEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> catalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolarisPolicyMappingRecord mappingRecord = + ms.lookupPolicyMappingRecord( + callCtx, + target.getCatalogId(), + target.getId(), + policy.getPolicyTypeCode(), + policy.getCatalogId(), + policy.getId()); + if (mappingRecord == null) { + return new AttachmentResult(BaseResult.ReturnStatus.POLICY_MAPPING_NOT_FOUND, null); + } + + ms.deleteFromPolicyMappingRecords(callCtx, mappingRecord); + + return new AttachmentResult(mappingRecord); + } + + @Override + public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntity( + @Nonnull PolarisCallContext callCtx, @Nonnull PolarisEntityCore target) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + int grantRecordVersion = + ms.lookupEntityGrantRecordsVersion(callCtx, target.getCatalogId(), target.getId()); + if (grantRecordVersion == 0) { + // Target entity does not exists + return new LoadPolicyMappingsResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, null); + } + + final List<PolarisPolicyMappingRecord> policyMappingRecords = + ms.loadAllPoliciesOnTarget(callCtx, target.getCatalogId(), target.getId()); + + List<PolicyEntity> policyEntities = + loadPoliciesFromMappingRecords(callCtx, ms, policyMappingRecords); + return new LoadPolicyMappingsResult(policyMappingRecords, policyEntities); + } + + @Override + public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntityByType( + @Nonnull PolarisCallContext callCtx, + @Nonnull PolarisEntityCore target, + @Nonnull PolicyType policyType) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + int grantRecordVersion = + ms.lookupEntityGrantRecordsVersion(callCtx, target.getCatalogId(), target.getId()); Review Comment: Same here ########## polaris-core/src/main/java/org/apache/polaris/core/policy/PolarisPolicyMappingManager.java: ########## @@ -0,0 +1,211 @@ +/* + * 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; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonProperty; +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import org.apache.polaris.core.PolarisCallContext; +import org.apache.polaris.core.entity.PolarisEntityCore; +import org.apache.polaris.core.persistence.dao.entity.BaseResult; + +public interface PolarisPolicyMappingManager { + + /** + * Attach a policy to a target entity, for example attach a policy to a table. + * + * <p>For inheritable policy, only one policy of the same type can be attached to the target. For + * non-inheritable policy, multiple policies of the same type can be attached to the target. + * + * @param callCtx call context + * @param targetCatalogPath path to the target entity + * @param target target entity + * @param policyCatalogPath path to the policy entity + * @param policy policy entity + * @param parameters additional parameters for the attachment + * @return The policy mapping record we created for this attachment. Will return ENTITY_NOT_FOUND + * if the specified target or policy does not exist. Will return + * POLICY_OF_SAME_TYPE_ALREADY_ATTACHED if the target already has a policy of the same type + * attached and the policy is inheritable. + */ + @Nonnull + AttachmentResult attachPolicyToEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> targetCatalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy, + Map<String, String> parameters); + + /** + * Detach a policy from a target entity + * + * @param callCtx call context + * @param catalogPath path to the target entity + * @param target target entity + * @param policyCatalogPath path to the policy entity + * @param policy policy entity + * @return The policy mapping record we detached. Will return ENTITY_NOT_FOUND if the specified + * target or policy does not exist. Will return POLICY_MAPPING_NOT_FOUND if the mapping cannot + * be found + */ + @Nonnull + AttachmentResult detachPolicyFromEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> catalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy); + + /** + * Load all policies attached to a target entity + * + * @param callCtx call context + * @param target target entity + * @return the list of policy mapping records on the target entity. Will return ENTITY_NOT_FOUND + * if the specified target does not exist. + */ + @Nonnull + LoadPolicyMappingsResult loadPoliciesOnEntity( + @Nonnull PolarisCallContext callCtx, @Nonnull PolarisEntityCore target); + + /** + * Load all policies of a specific type attached to a target entity + * + * @param callCtx call context + * @param target target entity + * @param policyType the type of policy + * @return the list of policy mapping records on the target entity. Will return ENTITY_NOT_FOUND + * if the specified target does not exist. + */ + @Nonnull + LoadPolicyMappingsResult loadPoliciesOnEntityByType( + @Nonnull PolarisCallContext callCtx, + @Nonnull PolarisEntityCore target, + @Nonnull PolicyType policyType); + + /** result of an attach/detach operation */ + class AttachmentResult extends BaseResult { + // null if not success + private final PolarisPolicyMappingRecord mappingRecord; + + /** + * Constructor for an error + * + * @param errorCode error code, cannot be SUCCESS + * @param extraInformation extra information + */ + public AttachmentResult( + @Nonnull BaseResult.ReturnStatus errorCode, @Nullable String extraInformation) { + super(errorCode, extraInformation); + this.mappingRecord = null; + } + + /** + * Constructor for success + * + * @param mappingRecord policy mapping record being attached/detached + */ + public AttachmentResult(@Nonnull PolarisPolicyMappingRecord mappingRecord) { + super(BaseResult.ReturnStatus.SUCCESS); + this.mappingRecord = mappingRecord; + } + + @JsonCreator + private AttachmentResult( + @JsonProperty("returnStatus") @Nonnull BaseResult.ReturnStatus returnStatus, + @JsonProperty("extraInformation") String extraInformation, + @JsonProperty("policyMappingRecord") PolarisPolicyMappingRecord mappingRecord) { + super(returnStatus, extraInformation); + this.mappingRecord = mappingRecord; + } + + public PolarisPolicyMappingRecord getPolicyMappingRecord() { + return mappingRecord; + } + } + + /** result of a load policy mapping call */ + class LoadPolicyMappingsResult extends BaseResult { Review Comment: Suggest to move results entities to top-level classes in package `org.apache.polaris.core.persistence.dao.entity` ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java: ########## @@ -1821,4 +1824,176 @@ public Map<String, String> getInternalPropertyMap( // return the result return new ResolvedEntityResult(entity, entityVersions.getGrantRecordsVersion(), grantRecords); } + + @Override + public @Nonnull AttachmentResult attachPolicyToEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> targetCatalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy, + Map<String, String> parameters) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolicyType policyType = PolicyType.fromCode(policy.getPolicyTypeCode()); + if (policyType == null) { + // This should never happen + return new AttachmentResult( + BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Unknown policy type"); + } + + // TODO: this may make it necessary to push down this logic to policy persistence layer Review Comment: +1, otherwise, concurrent attaching may cause issues. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java: ########## @@ -1821,4 +1824,176 @@ public Map<String, String> getInternalPropertyMap( // return the result return new ResolvedEntityResult(entity, entityVersions.getGrantRecordsVersion(), grantRecords); } + + @Override + public @Nonnull AttachmentResult attachPolicyToEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> targetCatalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy, + Map<String, String> parameters) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolicyType policyType = PolicyType.fromCode(policy.getPolicyTypeCode()); + if (policyType == null) { + // This should never happen + return new AttachmentResult( + BaseResult.ReturnStatus.UNEXPECTED_ERROR_SIGNALED, "Unknown policy type"); + } + + // TODO: this may make it necessary to push down this logic to policy persistence layer + if (policyType.isInheritable()) { + // Verify that there is no other mapping of the same type but different entity + List<PolarisPolicyMappingRecord> existingRecordsOfSameType = + ms.loadPoliciesOnTargetByType( + callCtx, target.getCatalogId(), target.getId(), policy.getPolicyTypeCode()); + if (existingRecordsOfSameType.size() > 1) { + return new AttachmentResult( + BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS, null); + } else if (existingRecordsOfSameType.size() == 1) { + PolarisPolicyMappingRecord existingRecord = existingRecordsOfSameType.get(0); + if (existingRecord.getPolicyId() != policy.getId() + || existingRecord.getPolicyCatalogId() != policy.getCatalogId()) { + return new AttachmentResult( + BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS, null); + } + } + } + + PolarisPolicyMappingRecord mappingRecord = + this.persistNewPolicyMappingRecord(callCtx, ms, target, policy, parameters); + return new AttachmentResult(mappingRecord); + } + + @Override + public @Nonnull AttachmentResult detachPolicyFromEntity( + @Nonnull PolarisCallContext callCtx, + @Nonnull List<PolarisEntityCore> catalogPath, + @Nonnull PolarisEntityCore target, + @Nonnull List<PolarisEntityCore> policyCatalogPath, + @Nonnull PolicyEntity policy) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + PolarisPolicyMappingRecord mappingRecord = + ms.lookupPolicyMappingRecord( + callCtx, + target.getCatalogId(), + target.getId(), + policy.getPolicyTypeCode(), + policy.getCatalogId(), + policy.getId()); + if (mappingRecord == null) { + return new AttachmentResult(BaseResult.ReturnStatus.POLICY_MAPPING_NOT_FOUND, null); + } + + ms.deleteFromPolicyMappingRecords(callCtx, mappingRecord); + + return new AttachmentResult(mappingRecord); + } + + @Override + public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntity( + @Nonnull PolarisCallContext callCtx, @Nonnull PolarisEntityCore target) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + int grantRecordVersion = + ms.lookupEntityGrantRecordsVersion(callCtx, target.getCatalogId(), target.getId()); + if (grantRecordVersion == 0) { + // Target entity does not exists + return new LoadPolicyMappingsResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, null); + } + + final List<PolarisPolicyMappingRecord> policyMappingRecords = + ms.loadAllPoliciesOnTarget(callCtx, target.getCatalogId(), target.getId()); + + List<PolicyEntity> policyEntities = + loadPoliciesFromMappingRecords(callCtx, ms, policyMappingRecords); + return new LoadPolicyMappingsResult(policyMappingRecords, policyEntities); + } + + @Override + public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntityByType( + @Nonnull PolarisCallContext callCtx, + @Nonnull PolarisEntityCore target, + @Nonnull PolicyType policyType) { + // get metastore we should be using + BasePersistence ms = callCtx.getMetaStore(); + + int grantRecordVersion = + ms.lookupEntityGrantRecordsVersion(callCtx, target.getCatalogId(), target.getId()); + if (grantRecordVersion == 0) { + // Target entity does not exists + return new LoadPolicyMappingsResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, null); + } + + final List<PolarisPolicyMappingRecord> policyMappingRecords = + ms.loadAllPoliciesOnTarget(callCtx, target.getCatalogId(), target.getId()); Review Comment: Looks like `policyType` wasn't used here. How do we filter by type? ########## polaris-core/src/main/java/org/apache/polaris/core/policy/PolarisPolicyMappingRecord.java: ########## @@ -0,0 +1,215 @@ +/* + * 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; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; + +public class PolarisPolicyMappingRecord { + // to serialize/deserialize properties + public static final String EMPTY_MAP_STRING = "{}"; + private static final ObjectMapper MAPPER = new ObjectMapper(); + + // id of the catalog where target entity resides + private long targetCatalogId; + + // id of the target entity + private long targetId; + + // id of the catalog where the policy entity resides + private long policyCatalogId; Review Comment: Sorry, I kept forgetting the reason of catalog ids here. Oh, I think it is needed because `lookupEntities()` requires both -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
