roryqi commented on code in PR #14326:
URL: https://github.com/apache/gravitino/pull/14326#discussion_r4477903378
##########
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:
Fixed in 99859bdfd9. Owner writes (including batches), privilege grants, and
initial grants during role creation now reject persisted hidden tags. Policy
deletion also soft-deletes the hidden tag's owner and securable-object rows in
the same transaction. Added write-rejection and legacy-row cleanup coverage;
the cleanup regression passes on H2, MySQL, and PostgreSQL.
##########
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:
Fixed in 99859bdfd9. Both EntityAlreadyExistsException and
OptimisticLockException now trigger a reload, and binding proceeds only for a
single selector-free link to the expected policy. Added deterministic tests for
the matching-link and no-visible-link branches; the binding suite passes on H2,
MySQL, and PostgreSQL.
##########
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:
Fixed in 99859bdfd9. Persisted hidden tags now raise NoSuchTagException
(404). The visibility check stays outside the missing-entity catch so delete
and association paths cannot swallow the hidden-tag rejection. Updated the
visibility regression tests; existing visible tags with the reserved prefix
remain accessible.
--
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]
