jerryshao commented on code in PR #14326:
URL: https://github.com/apache/gravitino/pull/14326#discussion_r4476465470
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:
##########
@@ -499,6 +560,35 @@ private void updatePolicyRootWithVersion(
() -> policyWriteFailure(identifier, oldPolicyPO));
}
+ private void deleteHiddenTagWithoutCommit(PolicyPO policy, String metalake) {
+ TagPO tag =
+ SessionUtils.getWithoutCommit(
+ TagMetaMapper.class,
+ mapper ->
+ mapper.selectTagMetaByMetalakeIdAndNameForUpdate(
+ policy.getMetalakeId(),
HiddenPolicyTag.name(policy.getPolicyId())));
+ if (tag == null || !tag.isHidden()) {
+ return;
+ }
+ int deleted =
+ SessionUtils.getWithoutCommit(
+ TagMetaMapper.class,
+ mapper ->
+ mapper.softDeleteTagMetaByIdAndVersion(tag.getTagId(),
tag.getCurrentVersion()));
+ Preconditions.checkState(
+ deleted == 1,
+ "Failed to delete hidden tag %s for policy %s",
+ tag.getTagName(),
+ policy.getPolicyId());
+ SessionUtils.doWithoutCommit(
Review Comment:
[Minor] `deleteHiddenTagWithoutCommit` soft-deletes the tag and its
`tag_relation_meta` rows, but not the owner or securable-object rows that
`TagMetaService.deleteTag` clears (`TagMetaService.java:704-715`). Also,
`OwnerManager.setOwner` and `PermissionManager.grantPrivilegesToRole` never
check `hidden`. So a grant on `reserved_{policyId}` is stored on the role,
shows up in privilege listings, and survives policy deletion. Could we reject
hidden tags on those writes and soft-delete the same rows here?
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -592,10 +619,21 @@ private TagEntity updateTagEntity(TagEntity tagEntity,
TagChange... changes) {
.build();
}
+ private void checkVisibleTagIfExists(String metalake, String name) {
+ try {
+ loadTagEntity(metalake, name);
+ } catch (NoSuchTagException e) {
+ // Missing names retain the existing idempotent delete and association
behavior.
+ }
+ }
+
private TagEntity loadTagEntity(String metalake, String name) {
try {
- return entityStore.get(
- NameIdentifierUtil.ofTag(metalake, name), Entity.EntityType.TAG,
TagEntity.class);
+ TagEntity tag =
+ entityStore.get(
+ NameIdentifierUtil.ofTag(metalake, name), Entity.EntityType.TAG,
TagEntity.class);
+ HiddenPolicyTag.checkVisible(tag);
Review Comment:
[Nit] A hidden tag throws `IllegalArgumentException` here (400), while a
missing tag is `NoSuchTagException` (404). That lets callers probe whether
`reserved_{id}` exists through get, alter, delete, or associate. Since listing
already filters hidden tags, consider returning not-found for hidden tags too.
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -73,6 +78,114 @@ public PolicyManager(IdGenerator idGenerator, EntityStore
entityStore) {
this.objectPolicyResolver = new ObjectPolicyResolver(entityStore);
}
+ /**
+ * Applies a policy to an object through its hidden tag using the entity
store.
+ *
+ * <p>This internal operation is idempotent. Migration callers can group it
with legacy cleanup in
+ * their storage transaction. It is not exposed through the policy
dispatcher or client APIs.
+ *
+ * @param metalake The policy and object's metalake.
+ * @param policyName The policy name.
+ * @param object The metadata object to associate.
+ * @param auditInfo The audit information to preserve on newly created
relations.
+ * @return The policy's hidden tag ID.
+ * @throws IOException If an entity or relation cannot be persisted.
+ */
+ public long addPolicyToObject(
+ String metalake, String policyName, MetadataObject object, Audit
auditInfo)
+ throws IOException {
+ Preconditions.checkNotNull(auditInfo, "Policy binding audit information is
required");
+ Preconditions.checkArgument(
+ SUPPORTED_METADATA_OBJECT_TYPES_FOR_TAGS.contains(object.type()),
+ "Unsupported policy binding object type: %s",
+ object.type());
+ checkMetalake(NameIdentifier.of(metalake), entityStore);
+ NameIdentifier policyIdent = NameIdentifierUtil.ofPolicy(metalake,
policyName);
+ PolicyEntity policy =
+ entityStore.get(policyIdent, Entity.EntityType.POLICY,
PolicyEntity.class);
+ NameIdentifier objectIdent = MetadataObjectUtil.toEntityIdent(metalake,
object);
+ Entity.EntityType objectType = MetadataObjectUtil.toEntityType(object);
+ // Relation lookup validates the endpoint, including columns stored as
part of a table.
+ entityStore
+ .relationOperations()
+ .listEntitiesByRelation(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
objectIdent, objectType);
+ NameIdentifier tagIdent = NameIdentifierUtil.ofTag(metalake,
HiddenPolicyTag.name(policy.id()));
+ TagEntity tag;
+ try {
+ tag = entityStore.get(tagIdent, Entity.EntityType.TAG, TagEntity.class);
+ } catch (NoSuchEntityException e) {
+ tag = HiddenPolicyTag.create(policy, idGenerator.nextId());
+ try {
+ entityStore.put(tag, false);
+ } catch (EntityAlreadyExistsException concurrentCreate) {
+ tag = entityStore.get(tagIdent, Entity.EntityType.TAG,
TagEntity.class);
+ }
+ }
+ Preconditions.checkArgument(
+ tag.hidden()
+ && tag.valueConstraint().allowedValues() != null
+ && tag.valueConstraint().allowedValues().length == 0
+ && HiddenPolicyTag.create(policy,
tag.id()).properties().equals(tag.properties()),
+ "Reserved tag name collision for policy %s",
+ policy.id());
+ List<RelationalEntity<?>> policies =
+ entityStore
+ .relationOperations()
+ .batchListEntitiesByRelation(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ Collections.singletonList(tagIdent),
+ Entity.EntityType.TAG);
+ if (policies.isEmpty()) {
+ try {
+ entityStore
+ .relationOperations()
+ .updateEntityRelations(
+ RelationUpdate.of(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ tagIdent,
+ Entity.EntityType.TAG,
+ new RelationEdgeTarget[] {
+ RelationEdgeTarget.of(policyIdent,
Entity.EntityType.POLICY, null)
+ },
+ null)
+ .withAuditInfo(auditInfo));
+ } catch (EntityAlreadyExistsException concurrentCreate) {
Review Comment:
[Minor] `PolicyTagRelService.insertIfAbsent` throws
`OptimisticLockException` (not `EntityAlreadyExistsException`) when the insert
affects zero rows and the follow-up read doesn't see the competing row yet
(`PolicyTagRelService.java:308-316`). This catch misses that case, so the call
fails before associating the object. A retry succeeds, and the new concurrency
test doesn't cover this branch. Could we handle `OptimisticLockException` the
same way: reload, accept a single selector-free link to this policy, then
associate the object?
--
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]
