HonahX commented on code in PR #1468:
URL: https://github.com/apache/polaris/pull/1468#discussion_r2061136265
##########
scripts/postgres/schema-v1-postgresql.sql:
##########
@@ -94,3 +94,17 @@ CREATE TABLE IF NOT EXISTS principal_authentication_data (
);
COMMENT ON TABLE principal_authentication_data IS 'authentication data for
client';
+
+DROP TABLE IF EXISTS policy_mapping_record;
+CREATE TABLE IF NOT EXISTS policy_mapping_record (
+ realm_id TEXT NOT NULL,
+ target_catalog_id BIGINT NOT NULL,
+ target_id BIGINT NOT NULL,
+ policy_type_code INTEGER NOT NULL,
+ policy_catalog_id BIGINT NOT NULL,
+ policy_id BIGINT NOT NULL,
+ parameters JSONB NOT NULL DEFAULT '{}'::JSONB,
+ PRIMARY KEY (realm_id, target_catalog_id, target_id, policy_type_code,
policy_catalog_id, policy_id)
+);
+
+CREATE INDEX IF NOT EXISTS idx_policy_mapping_record ON policy_mapping_record
(realm_id, policy_catalog_id, policy_id, target_catalog_id, target_id);
Review Comment:
```suggestion
CREATE INDEX IF NOT EXISTS idx_policy_mapping_record ON
policy_mapping_record (realm_id, policy_type_code, policy_catalog_id,
policy_id, target_catalog_id, target_id);
```
Would you mind helping add `policy_type_code` to the index as well? I
realize this field is not currently indexed in the EclipseLink implementation,
but including it could help support future features, such as efficiently
retrieving all entities with a specific type of policy (e.g., data compaction
policies).
cc @flyrain
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -727,6 +732,164 @@ public void deletePrincipalSecrets(
}
}
+ @Override
+ public void writeToPolicyMappingRecords(
+ @Nonnull PolarisCallContext callCtx, @Nonnull PolarisPolicyMappingRecord
record) {
+ try {
+ datasourceOperations.runWithinTransaction(
+ statement -> {
+ PolicyType policyType =
PolicyType.fromCode(record.getPolicyTypeCode());
+ Preconditions.checkArgument(
+ policyType != null, "Invalid policy type code: %s",
record.getPolicyTypeCode());
+
+ if (policyType.isInheritable()) {
+ List<PolarisPolicyMappingRecord> existingRecords =
+ loadPoliciesOnTargetByType(
+ callCtx,
+ record.getTargetCatalogId(),
+ record.getTargetId(),
+ record.getPolicyTypeCode());
+ if (existingRecords.size() > 1) {
+ throw new
PolicyMappingAlreadyExistsException(existingRecords.get(0));
+ } else if (existingRecords.size() == 1) {
+ PolarisPolicyMappingRecord existingRecord =
existingRecords.get(0);
+ if (existingRecord.getPolicyCatalogId() !=
record.getPolicyCatalogId()
+ || existingRecord.getPolicyId() != record.getPolicyId()) {
+ throw new
PolicyMappingAlreadyExistsException(existingRecord);
+ }
Review Comment:
In the case where there is an existing record with same target and same
policy (i.e., same primary key), `writeToPolicyMappingRecord`, all attributes
of the new record should replace the existing one. Hence I think we need an
update statement in this case.
I just realized I made this mistake in EclipseLink Persistence
Implementation which result in a primary key violation, I've opened a fix for
that: https://github.com/apache/polaris/pull/1469
--
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
