Skip to content

Ensure writeToPolicyMappingRecord update existing record if primary key equals in EclipseLink Persistence Impl #1469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 29, 2025
Merged

Ensure writeToPolicyMappingRecord update existing record if primary key equals in EclipseLink Persistence Impl #1469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c6415a
update PolicyMappingRecord if not exists
HonahX Apr 26, 2025
00ca55d
update test
HonahX Apr 26, 2025
66ec53e
add TODO
HonahX Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion ...va/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,22 @@ void writeToPolicyMappingRecords(
diagnosticServices.check(session != null, "session_is_null");
checkInitialized();

session.persist(ModelPolicyMappingRecord.fromPolicyMappingRecord(mappingRecord));
// TODO: combine existence check and write into one statement
ModelPolicyMappingRecord model =
lookupPolicyMappingRecord(
session,
mappingRecord.getTargetCatalogId(),
mappingRecord.getTargetId(),
mappingRecord.getPolicyTypeCode(),
mappingRecord.getPolicyCatalogId(),
mappingRecord.getPolicyId());
if (model != null) {
model.update(mappingRecord);
Copy link
Contributor

@eric-maynard eric-maynard Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this adding another roundtrip to the metastore? Can that be avoided?

Copy link
Contributor Author

@HonahX HonahX Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not find useful things in EclipseLink to do existence check + upsert in one command. So I followed the existing pattern in writeEntities

polaris/extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkStore.java

Lines 87 to 97 in a84b5c4

ModelEntity model =
lookupEntity(session, entity.getCatalogId(), entity.getId(), entity.getTypeCode());
if (model != null) {
// Update if the same entity already exists
model.update(entity);
} else {
model = ModelEntity.fromEntity(entity);
}
session.persist(model);
}
to do this.

Shall we add a "TODO" here to note that we are looking for ways to improve this to one execution?

Copy link
Contributor

@eric-maynard eric-maynard Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah with a TODO, this LGTM

Copy link
Contributor

@eric-maynard eric-maynard Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For EclipseLink, see:
https://eclipse.dev/eclipselink/documentation/2.4/jpa/extensions/a_existencechecking.htm

Copy link
Contributor Author

@HonahX HonahX Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a TODO. Yeah, I did several attempts to use the existence checking annotation and merge but it did not work out-of-box. We probably still need to query the database to get the "version" for existing row for optimistic locking. Will try this approach or other approach later when I got some time : )

Copy link
Contributor

@singhpk234 singhpk234 Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i tried merge too when i thinking of re:using the eclipselink for AtomicMetaStoreManager, but i think the issue is that when we run hand crafted queries (i.e not model.update() / .merge()) the entity manager goes out of sync and hence the two queries are un-avoidable here even though this is a atomic operation :/ , atleast to the best of my understanding. It would be really awesome if we can work around this or get this into one query !

} else {
model = ModelPolicyMappingRecord.fromPolicyMappingRecord(mappingRecord);
}

session.persist(model);
}

void deleteFromPolicyMappingRecords(
Expand Down
9 changes: 9 additions & 0 deletions ...tence/jpa-model/src/main/java/org/apache/polaris/jpa/models/ModelPolicyMappingRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,15 @@ public ModelPolicyMappingRecord build() {
}
}

public void update(PolarisPolicyMappingRecord record) {
this.targetCatalogId = record.getTargetCatalogId();
this.targetId = record.getTargetId();
this.policyTypeCode = record.getPolicyTypeCode();
this.policyCatalogId = record.getPolicyCatalogId();
this.policyId = record.getPolicyId();
this.parameters = record.getParameters();
}

public static ModelPolicyMappingRecord fromPolicyMappingRecord(
PolarisPolicyMappingRecord record) {
if (record == null) return null;
Expand Down
57 changes: 50 additions & 7 deletions ...rc/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -995,9 +995,19 @@ void attachPolicyToTarget(
PolarisBaseEntity target,
List<PolarisEntityCore> policyCatalogPath,
PolicyEntity policy) {
attachPolicyToTarget(targetCatalogPath, target, policyCatalogPath, policy, null);
}

void attachPolicyToTarget(
List<PolarisEntityCore> targetCatalogPath,
PolarisBaseEntity target,
List<PolarisEntityCore> policyCatalogPath,
PolicyEntity policy,
Map<String, String> parameters) {
polarisMetaStoreManager.attachPolicyToEntity(
polarisCallContext, targetCatalogPath, target, policyCatalogPath, policy, null);
ensurePolicyMappingRecordExists(target, policy);
polarisCallContext, targetCatalogPath, target, policyCatalogPath, policy, parameters);

ensurePolicyMappingRecordExists(target, policy, parameters);
}

/** detach a policy from a target */
Expand All @@ -1016,8 +1026,10 @@ void detachPolicyFromTarget(
*
* @param target the target
* @param policy the policy
* @param parameters the parameters
*/
void ensurePolicyMappingRecordExists(PolarisBaseEntity target, PolicyEntity policy) {
void ensurePolicyMappingRecordExists(
PolarisBaseEntity target, PolicyEntity policy, Map<String, String> parameters) {
target =
polarisMetaStoreManager
.loadEntity(
Expand All @@ -1042,15 +1054,15 @@ void ensurePolicyMappingRecordExists(PolarisBaseEntity target, PolicyEntity poli
validateLoadedPolicyMappings(loadPolicyMappingsResult);

checkPolicyMappingRecordExists(
loadPolicyMappingsResult.getPolicyMappingRecords(), target, policy);
loadPolicyMappingsResult.getPolicyMappingRecords(), target, policy, parameters);

// also try load by specific type
LoadPolicyMappingsResult loadPolicyMappingsResultByType =
polarisMetaStoreManager.loadPoliciesOnEntityByType(
this.polarisCallContext, target, policy.getPolicyType());
validateLoadedPolicyMappings(loadPolicyMappingsResultByType);
checkPolicyMappingRecordExists(
loadPolicyMappingsResultByType.getPolicyMappingRecords(), target, policy);
loadPolicyMappingsResultByType.getPolicyMappingRecords(), target, policy, parameters);
}

/**
Expand Down Expand Up @@ -1134,8 +1146,9 @@ void validateLoadedPolicyMappings(LoadPolicyMappingsResult loadPolicyMappingReco
void checkPolicyMappingRecordExists(
List<PolarisPolicyMappingRecord> policyMappingRecords,
PolarisBaseEntity target,
PolicyEntity policy) {
boolean exists = isPolicyMappingRecordExists(policyMappingRecords, target, policy);
PolicyEntity policy,
Map<String, String> parameters) {
boolean exists = isPolicyMappingRecordExists(policyMappingRecords, target, policy, parameters);
Assertions.assertThat(exists).isTrue();
}

Expand Down Expand Up @@ -1178,6 +1191,32 @@ record ->
return policyMappingCount == 1;
}

/**
* Check if the policy mapping record exists and verify if the parameters also equals
*
* @param policyMappingRecords list of policy mapping records
* @param target the target
* @param policy the policy
* @param parameters the parameters
*/
boolean isPolicyMappingRecordExists(
List<PolarisPolicyMappingRecord> policyMappingRecords,
PolarisBaseEntity target,
PolicyEntity policy,
Map<String, String> parameters) {
PolarisPolicyMappingRecord expected =
new PolarisPolicyMappingRecord(
target.getCatalogId(),
target.getId(),
policy.getCatalogId(),
policy.getId(),
policy.getPolicyTypeCode(),
parameters);
long policyMappingCount =
policyMappingRecords.stream().filter(record -> expected.equals(record)).count();
return policyMappingCount == 1;
}

/**
* Create a test catalog. This is a new catalog which will have the following objects (N is for a
* namespace, T for a table, V for a view, R for a role, P for a principal, POL for a policy):
Expand Down Expand Up @@ -2775,6 +2814,10 @@ void testPolicyMapping() {
Assertions.assertThat(policyAttachmentResult.getReturnStatus())
.isEqualTo(BaseResult.ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS);

// Attach the same policy to same target again should succeed and replace the existing one
attachPolicyToTarget(
List.of(catalog, N1, N1_N2), N1_N2_T1, List.of(catalog, N1), N1_P1, Map.of("test", "test"));

LoadPolicyMappingsResult loadPolicyMappingsResult =
polarisMetaStoreManager.loadPoliciesOnEntityByType(
polarisCallContext, N1_N2_T1, PredefinedPolicyTypes.DATA_COMPACTION);
Expand Down
Loading