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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
