Policy Store: PolicyMappingRecord with Persistence Impl

[HonahX]

Depends on #1133

Policy Store in Polaris allow user to attach a policy to a target entity (e.g. table). To represent and persist this information, we introduces PolicyMappingRecords which is similar to GrantRecords that represent a single attachment from a policy to a target entity.

This PR introduces new persistence interfaces and impl for PolicyMappingRecords for supporting Policy Store in Polaris, as discussed in apache/polaris#1059.

Key Additions:

• PolarisPolicyMappingRecord – Stores the mapping between a policy and a target entity. For example, when a user attaches a policy to a table, a new record will be created and persisted.
• PolicyMappingPersistence: Interface for interacting with the Polaris persistence backend for Policy Mapping operations
• PolarisPolicyMappingManager: Interfaces for managing policy mapping in Polaris (attach/detach/lookup mappings)

Persistence Implementation:

• InMemory: PolarisTreeMapStoreSessionImpl
• SQL: PolarisEclipseLinkMetaStoreSessionImpl

cc: @flyrain

[snazy]

Given the existing and recently created GH issues around persistence and the fact that this demands another unnecessarily complication to #1189, required for MongoDB (#650) and DynamoDB (#844), a much better approach IMHO is to model necessary policy APIs that are truly persistence implementation agnostic and have implementations that to satisfy both the persistence model for BaseMetaStoreManager (aka Eclipselink) and for #1189. This means an API that does not depend on the dictated long IDs but names/name tuples.

Especially #1202 and previously voiced doubts about using serializable isolation-level raise concerns. I'd like especially new work like this one to be more agnostic and open to alternatives.

[HonahX]

@snazy Thank you for your feedback on the persistence model for PolicyMappingRecord. Our goal in designing PolicyMappingRecord was to maintain consistency by aligning it with the existing data model, particularly the structure and persistence APIs of GrantRecords, which manage privilege relationships between securable and grantee entities.

I agree that it's worthwhile to continue discussions about persistence refactoring and better persistence interface. For example, there have been discussions initiated around persisting entity relationships, such as GrantRecords and PolicyMappingRecords, in a more generic manner. However, I believe pursuing broader refactoring should not delay the implementation of new features. Ideally, any refactoring efforts would address both PolicyMappingRecords and GrantRecords simultaneously to ensure coherence. IMHO, it makes more sense to develop new features based on the existing model and perform refactoring later, once we have a well-defined persistence model agreed upon.

This means an API that does not depend on the dictated long IDs but names/name tuples.

Additionally, I'd appreciate if you could provide more details about why the current PolicyMappingRecord persistence approach would not be compatible with NoSQL persistence. As a reference, the current lookupGrantRecords API already relies on IDs, could you please elaborate more on this part?

PolarisGrantRecord lookupGrantRecord(
      @Nonnull PolarisCallContext callCtx,
      long securableCatalogId,
      long securableId,
      long granteeCatalogId,
      long granteeId,
      int privilegeCode);

[dennishuo]

Generally LGTM! The two things that should probably be followed up quickly if not directly in this PR are mentioned in my comments:

1. Make sure the ordering of columns in the generated primary key index for EclipseLink is actually what we expect -- it appears that order of appearance in the class declaration doesn't generally guarantee the ordering in the generated index. And this one we'll want to make sure to fix before anyone starts getting any sticky Postgres schemas.
2. Pruning out mapping records where either the policy or target have been deleted

As long as we have the overall feature still inactive and default-gated-off by feature configuration though, I'm okay with fixing those in follow-up PRs.

[dennishuo]

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.

[dennishuo]

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
2. Things that don't need the actual entities in grantRecords are "safe" to just interpret the grantRecord at face value
3. 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.

[dennishuo]

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 #1123 (comment) 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.

Thanks for the feedback.
The comments were addressed by #1104 (comment). And I believe this PR is compatible with Non-SQL backend.

Feel free to open separate PRs or post in dev mail list if there were additional concerns.