You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "dennishuo (via GitHub)" <gi...@apache.org> on 2025/04/01 04:18:25 UTC
Re: [PR] Policy Store: PolicyMappingRecord with Persistence Impl [polaris]
dennishuo commented on code in PR #1104:
URL: https://github.com/apache/polaris/pull/1104#discussion_r2022099571
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java:
##########
@@ -2316,4 +2321,227 @@ public Map<String, String> getInternalPropertyMap(
entityCatalogId,
entityId));
}
+
+ /** {@inheritDoc} */
+ @Override
+ public @Nonnull PolicyAttachmentResult 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
+ TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore());
+
+ return ms.runInTransaction(
+ callCtx,
+ () ->
+ this.doAttachPolicyToEntity(
+ callCtx, ms, targetCatalogPath, target, policyCatalogPath, policy, parameters));
+ }
+
+ /**
+ * See {@link #attachPolicyToEntity(PolarisCallContext, List, PolarisEntityCore, List,
+ * PolicyEntity, Map)}
+ */
+ private @Nonnull PolicyAttachmentResult doAttachPolicyToEntity(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull TransactionalPersistence ms,
+ @Nonnull List<PolarisEntityCore> targetCatalogPath,
+ @Nonnull PolarisEntityCore target,
+ @Nonnull List<PolarisEntityCore> policyCatalogPath,
+ @Nonnull PolicyEntity policy,
+ Map<String, String> parameters) {
+ PolarisEntityResolver targetResolver =
+ new PolarisEntityResolver(callCtx, ms, targetCatalogPath, target);
+ PolarisEntityResolver policyResolver =
+ new PolarisEntityResolver(callCtx, ms, policyCatalogPath, policy);
+ if (targetResolver.isFailure() || policyResolver.isFailure()) {
+ return new PolicyAttachmentResult(BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED, null);
+ }
+
+ return this.persistNewPolicyMappingRecord(callCtx, ms, target, policy, parameters);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public @Nonnull PolicyAttachmentResult detachPolicyFromEntity(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull List<PolarisEntityCore> targetCatalogPath,
+ @Nonnull PolarisEntityCore target,
+ @Nonnull List<PolarisEntityCore> policyCatalogPath,
+ @Nonnull PolicyEntity policy) {
+ TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore());
+ return ms.runInTransaction(
+ callCtx,
+ () ->
+ this.doDetachPolicyFromEntity(
+ callCtx, ms, targetCatalogPath, target, policyCatalogPath, policy));
+ }
+
+ /**
+ * See {@link #detachPolicyFromEntity(PolarisCallContext, List, PolarisEntityCore,
+ * List,PolicyEntity)}
+ */
+ private PolicyAttachmentResult doDetachPolicyFromEntity(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull TransactionalPersistence ms,
+ @Nonnull List<PolarisEntityCore> targetCatalogPath,
+ @Nonnull PolarisEntityCore target,
+ @Nonnull List<PolarisEntityCore> policyCatalogPath,
+ @Nonnull PolicyEntity policy) {
+ PolarisEntityResolver targetResolver =
+ new PolarisEntityResolver(callCtx, ms, targetCatalogPath, target);
+ PolarisEntityResolver policyResolver =
+ new PolarisEntityResolver(callCtx, ms, policyCatalogPath, policy);
+ if (targetResolver.isFailure() || policyResolver.isFailure()) {
+ return new PolicyAttachmentResult(BaseResult.ReturnStatus.ENTITY_CANNOT_BE_RESOLVED, null);
+ }
+
+ PolarisPolicyMappingRecord mappingRecord =
+ ms.lookupPolicyMappingRecordInCurrentTxn(
+ callCtx,
+ target.getCatalogId(),
+ target.getId(),
+ policy.getPolicyTypeCode(),
+ policy.getCatalogId(),
+ policy.getId());
+ if (mappingRecord == null) {
+ return new PolicyAttachmentResult(BaseResult.ReturnStatus.POLICY_MAPPING_NOT_FOUND, null);
+ }
+
+ ms.deleteFromPolicyMappingRecordsInCurrentTxn(callCtx, mappingRecord);
+
+ return new PolicyAttachmentResult(mappingRecord);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntity(
+ @Nonnull PolarisCallContext callCtx, @Nonnull PolarisEntityCore target) {
+ TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore());
+ return ms.runInReadTransaction(callCtx, () -> this.doLoadPoliciesOnEntity(callCtx, ms, target));
+ }
+
+ /** See {@link #loadPoliciesOnEntity(PolarisCallContext, PolarisEntityCore)} */
+ private LoadPolicyMappingsResult doLoadPoliciesOnEntity(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull TransactionalPersistence ms,
+ @Nonnull PolarisEntityCore target) {
+ PolarisBaseEntity entity =
+ ms.lookupEntityInCurrentTxn(
+ callCtx, target.getCatalogId(), target.getId(), target.getTypeCode());
+ if (entity == null) {
+ // Target entity does not exists
+ return new LoadPolicyMappingsResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, null);
+ }
+
+ final List<PolarisPolicyMappingRecord> policyMappingRecords =
+ ms.loadAllPoliciesOnTargetInCurrentTxn(callCtx, target.getCatalogId(), target.getId());
+
+ List<PolarisBaseEntity> policyEntities =
+ loadPoliciesFromMappingRecords(callCtx, ms, policyMappingRecords);
+ return new LoadPolicyMappingsResult(policyMappingRecords, policyEntities);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public @Nonnull LoadPolicyMappingsResult loadPoliciesOnEntityByType(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull PolarisEntityCore target,
+ @Nonnull PolicyType policyType) {
+ TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore());
+ return ms.runInReadTransaction(
+ callCtx, () -> this.doLoadPoliciesOnEntityByType(callCtx, ms, target, policyType));
+ }
+
+ /** See {@link #loadPoliciesOnEntityByType(PolarisCallContext, PolarisEntityCore, PolicyType)} */
+ public LoadPolicyMappingsResult doLoadPoliciesOnEntityByType(
+ @Nonnull PolarisCallContext callCtx,
+ @Nonnull TransactionalPersistence ms,
+ @Nonnull PolarisEntityCore target,
+ @Nonnull PolicyType policyType) {
+ PolarisBaseEntity entity =
+ ms.lookupEntityInCurrentTxn(
+ callCtx, target.getCatalogId(), target.getId(), target.getTypeCode());
+ if (entity == null) {
+ // Target entity does not exists
+ return new LoadPolicyMappingsResult(BaseResult.ReturnStatus.ENTITY_NOT_FOUND, null);
+ }
+
+ final List<PolarisPolicyMappingRecord> policyMappingRecords =
+ ms.loadPoliciesOnTargetByTypeInCurrentTxn(
+ callCtx, target.getCatalogId(), target.getId(), policyType.getCode());
+ List<PolarisBaseEntity> policyEntities =
+ loadPoliciesFromMappingRecords(callCtx, ms, policyMappingRecords);
Review Comment:
Since it looks like this PR doesn't modify `dropEntityIfExists` (which is probably good, for now), we should think about how we want to deal with "inactivated" PolicyMappingRecords.
We could either:
1. Require no more mappings exist on a target entity before allowing deletion of the policy, and I guess also no mappings for a target anymore before deleting a target entity
2. Proactively delete all policy mapping records associated with either a targetEntity or a polictyEntity when deleting either one
3. Prune out mapping records on-read by validating ids, have some async garbage-collection of orphaned mapping records
In particular, since we're implementing the uniqueness constraint in `checkConditionsForWriteToPolicyMappingRecordsInCurrentTxn` by calling this `loadPoliciesOnTargetByTypeInCurrentTxn`, either the caller or this method needs to prune out mapping records where either the `policyId` or `targetId` no longer exist via `lookupEntity`.
This is already *somewhat* handled for grantRecords in both ways:
1. Callsites (mostly) gracefully handle failed lookups when resolving grant records to their entities whenever they need entities
4. Things that don't need the actual entities in grantRecords are "safe" to just interpret the grantRecord at face value
5. The persistence impls all try to delete all the grant records associated with either end of an entity when deleting the entity -- the Transactional impls are strictly correct for these, while the AtomicOperation one is "best-effort".
calling `BasePersistence::lookupEntityVersions` is the efficient way to basically test for the existence of a whole batch of entityIds in one shot.
For the current implementation, pruning using that would at least make things logically work if someone deletes the PolicyEntity before detaching it.
##########
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. Each API in this interface need to be atomic.
+ */
+public interface PolicyMappingPersistence {
+
+ /**
+ * Write the specified policyMappingRecord to the policy_mapping_records table. If there is a
+ * conflict (existing record with the same PK), all attributes of the new record will replace the
Review Comment:
Would be helpful to give details on the PK here; looking at the code I guess this one refers to the entire 5-tuple of `targetCatalogId, targetId, policyTypeCode, policyCatalogId, policyId`, so it's only the `parameters` that are the payload outside of the PK?
If we're going to try to enforce the semantic of "inheritable type codes must also be unique per target" we should document that expectation here in the javadoc as well. Otherwise that intent is kind of hidden in `TransactionalPolicyMappingPersistence`, even if we don't have a prescribed implementation for how to do in other atomic backends.
I guess one way we could do it in NoSQL databases that do support UNIQUE constraints on secondary indexes would be to have an additional redundant field like `typeUniquenessCode` that we come up with when writing the PolicyMappingRecord.
Like MongoDB can have:
createIndex( { "targetCatalogId": 1, "targetId": 1, "typeUniquenessCode": 1 }, { unique: true })
And the persistence impl could say
// Assume we pre-enforce that entityIds draw from a different set of possible numbers than typeCodes
long typeUniquenessCode = isInheritable(typeCode) ? typeCode : policyId;
In fact, this could allow implementing `oneof` semantics as well someday if needed, where we could have some typeCodes that are like `groupTypeCodes` that indicate "oneof" a certain set of other types, and use that in this index.
##########
extension/persistence/jpa-model/src/main/java/org/apache/polaris/jpa/models/ModelPolicyMappingRecord.java:
##########
@@ -0,0 +1,153 @@
+/*
+ * 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.jpa.models;
+
+import jakarta.persistence.Entity;
+import jakarta.persistence.Id;
+import jakarta.persistence.Index;
+import jakarta.persistence.Table;
+import jakarta.persistence.Version;
+import org.apache.polaris.core.policy.PolarisPolicyMappingRecord;
+
+@Entity
+@Table(
+ name = "POLICY_MAPPING_RECORDS",
+ indexes = {
+ @Index(
+ name = "POLICY_MAPPING_RECORDS_BY_POLICY_INDEX",
+ columnList = "policyCatalogId,policyId,targetCatalogId,targetId")
+ })
+public class ModelPolicyMappingRecord {
+ // id of the catalog where target entity resides
+ @Id private long targetCatalogId;
Review Comment:
Before this schema becomes sticky anywhere, we should do a quick check that the index is in the right "order", since the ordering of columns in defining the covering index matters when we need to use the prefix of the index (`targetCatalogId, targetId, policyTypeCode`) as our predicate (https://www.postgresql.org/docs/current/indexes-multicolumn.html)
While investigating https://github.com/apache/polaris/issues/1123#issuecomment-2744951321 I discovered that EclipseLink appear *not* to respect the ordering of the columns in order of their appearance in the member declarations using the `@Id` annotation. Sometimes we just got lucky and it happened to define the primary key index with the right ordering, but I noticed it sometimes got scrambled.
It was hard to find EclipseLink docs on this problem, but there are related shortcomings in Hibernate too: https://stackoverflow.com/questions/8139437/how-to-set-the-column-order-of-a-composite-primary-key-using-jpa-hibernate
You can follow the steps in the github issue to run `docker compose -f getting-started/eclipselink/docker-compose.yml up` to try to run your new policy stuff on EclipseLink and then `docker exec -it 6b104a740efc bash` into your Postgres container and peek at your table:
psql -U postgres -d POLARIS
\d policy_mapping_records;
Either way, it might be best to use explicit definitions instead of the `@Id` annotation. I did some trial-and-error and found this syntax seemed to work:
import jakarta.persistence.Column;
import org.eclipse.persistence.annotations.PrimaryKey;
@Entity
@Table(name = "ENTITIES_ACTIVE")
@PrimaryKey(columns = {@Column(name="CATALOGID"), @Column(name="PARENTID"), @Column(name="TYPECODE"), @Column(name="NAME")})
I can't remember if I could get it to work with indexes defined inside the `@Table`; maybe you know of better ways.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org