mchades commented on code in PR #12718:
URL: https://github.com/apache/gravitino/pull/12718#discussion_r3901367047
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -261,41 +267,49 @@ public MetadataObject[]
listMetadataObjectsForPolicy(String metalake, String pol
}
@Override
- public PolicyEntity[] listPolicyInfosForMetadataObject(
- String metalake, MetadataObject metadataObject) {
- NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- Entity.EntityType entityType =
MetadataObjectUtil.toEntityType(metadataObject);
-
- MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
+ public RelationalEntity<?>[] listTagAssociationsForPolicy(String metalake,
String policyName) {
+ NameIdentifier policyIdentifier = NameIdentifierUtil.ofPolicy(metalake,
policyName);
checkMetalake(NameIdentifier.of(metalake), entityStore);
-
return TreeLockUtils.doWithTreeLock(
- entityIdent,
+ policyIdentifier,
LockType.READ,
() -> {
+ getPolicyWithoutLock(metalake, policyName);
try {
return entityStore
.relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.POLICY_METADATA_OBJECT_REL,
- entityIdent,
- entityType,
- true /* allFields */)
- .stream()
- .map(entity -> (PolicyEntity) entity)
- .toArray(PolicyEntity[]::new);
- } catch (NoSuchEntityException e) {
- throw new NoSuchMetadataObjectException(
- e,
- "Failed to list policies for metadata object %s due to not
found",
- metadataObject);
+ .batchListEntitiesByRelation(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ Collections.singletonList(policyIdentifier),
+ Entity.EntityType.POLICY)
+ .toArray(new RelationalEntity<?>[0]);
} catch (IOException e) {
- LOG.error("Failed to list policies for metadata object {}",
metadataObject, e);
+ LOG.error(
+ "Failed to list tag associations for policy {} under metalake
{}",
+ policyName,
+ metalake,
+ e);
throw new RuntimeException(e);
}
});
}
+ @Override
+ public PolicyEntity[] listPolicyInfosForMetadataObject(
+ String metalake, MetadataObject metadataObject) {
+ NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
+ Entity.EntityType entityType =
MetadataObjectUtil.toEntityType(metadataObject);
+ MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
+ checkMetalake(NameIdentifier.of(metalake), entityStore);
+
+ Map<Long, PolicyEntity> policiesById = new LinkedHashMap<>();
+ Arrays.stream(listDirectPoliciesForMetadataObject(entityIdent, entityType,
metadataObject))
+ .forEach(policy -> policiesById.putIfAbsent(policy.id(), policy));
+ Arrays.stream(objectPolicyResolver.resolve(metalake, metadataObject))
Review Comment:
`ObjectPolicyResolver.resolve` already walks the requested object and all
parents through `EffectiveTagResolver`, while
[`MetadataObjectPolicyOperations`](https://github.com/apache/gravitino/blob/18ca5608f4a57ebad241fbcde5a881ae8072d167/server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectPolicyOperations.java#L182-L211)
still calls this method once for the object and again for each parent. A child
override such as `data_domain=risk` over a parent `data_domain=finance` is
therefore evaluated correctly here, but the later parent call adds a
`TAG_VALUE("finance")` policy back. Policies selected from inherited tags are
also returned in the first call and marked `inherited=false` by the REST layer.
Please resolve tag-derived policies exactly once for the requested object, keep
any direct-policy compatibility traversal separate, and add an end-to-end
override regression test.
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -227,6 +231,109 @@ public MetadataObject[] listMetadataObjectsForTag(String
metalake, String name)
return listMetadataObjectsForTag(metalake, name, null);
}
+ @Override
+ public RelationalEntity<?>[] listPolicyAssociationsForTag(String metalake,
String name) {
+ NameIdentifier tagIdentifier = NameIdentifierUtil.ofTag(metalake, name);
+ checkMetalake(NameIdentifier.of(metalake), entityStore);
+ return TreeLockUtils.doWithTreeLock(
+ tagIdentifier,
+ LockType.READ,
+ () -> {
+ try {
+ return entityStore
+ .relationOperations()
+ .batchListEntitiesByRelation(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ Collections.singletonList(tagIdentifier),
+ Entity.EntityType.TAG)
+ .toArray(new RelationalEntity<?>[0]);
+ } catch (IOException e) {
+ LOG.error(
+ "Failed to list policy associations for tag {} under metalake
{}",
+ name,
+ metalake,
+ e);
+ throw new RuntimeException(e);
+ }
+ });
+ }
+
+ @Override
+ public void addPolicyForTag(
+ String metalake, String tagName, String policyName,
PolicyAssociationSelector selector) {
+ NameIdentifier tagIdentifier = NameIdentifierUtil.ofTag(metalake, tagName);
+ NameIdentifier policyIdentifier = NameIdentifierUtil.ofPolicy(metalake,
policyName);
+ checkMetalake(NameIdentifier.of(metalake), entityStore);
+ TreeLockUtils.doWithTreeLock(
+ tagIdentifier,
+ LockType.WRITE,
+ () -> {
+ RelationUpdate update =
+ RelationUpdate.of(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ tagIdentifier,
+ Entity.EntityType.TAG,
+ new RelationEdgeTarget[] {
+ RelationEdgeTarget.of(
+ policyIdentifier,
+ Entity.EntityType.POLICY,
+ PolicyAssociationSelectorSerde.serialize(selector))
Review Comment:
This method persists the selector without validating it against the tag's
`TagValueConstraint`. It accepts `TAG_VALUE("engineering")` for an
`ALLOWED_VALUES("finance", "risk")` tag, or any `TAG_VALUE` for a `NO_VALUE`
tag, creating an association that can never match. Please load the tag under
the lock, validate the selector against its constraint, and add negative tests
for unsupported values and constraint types.
##########
core/src/main/java/org/apache/gravitino/tag/TagDispatcher.java:
##########
@@ -117,6 +119,56 @@ default Tag createTag(
*/
MetadataObject[] listMetadataObjectsForTag(String metalake, String name);
+ /**
+ * List policy names directly associated with the specified tag.
+ *
+ * @param metalake The name of the metalake.
+ * @param name The name of the tag.
+ * @return The directly associated policy names.
+ */
+ default String[] listPoliciesForTag(String metalake, String name) {
+ return Arrays.stream(listPolicyAssociationsForTag(metalake, name))
+ .map(association -> association.targetEntity().name())
+ .toArray(String[]::new);
+ }
+
+ /**
+ * List policy associations, including selectors, for the specified tag.
+ *
+ * @param metalake The name of the metalake.
+ * @param name The name of the tag.
+ * @return The policy-to-tag associations.
+ */
+ default RelationalEntity<?>[] listPolicyAssociationsForTag(String metalake,
String name) {
Review Comment:
[`GravitinoEnv`](https://github.com/apache/gravitino/blob/18ca5608f4a57ebad241fbcde5a881ae8072d167/core/src/main/java/org/apache/gravitino/GravitinoEnv.java#L930-L937)
exposes `TagHookDispatcher(TagEventDispatcher(TagManager))`, but neither
wrapper overrides these new default methods, so calls through the production
dispatcher stop at this `UnsupportedOperationException` instead of reaching
`TagManager`. `PolicyHookDispatcher`/`PolicyEventDispatcher` have the same gap
for `listTagAssociationsForPolicy`. Please delegate the new operations through
both wrapper layers and test the composed runtime chain.
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -227,6 +231,109 @@ public MetadataObject[] listMetadataObjectsForTag(String
metalake, String name)
return listMetadataObjectsForTag(metalake, name, null);
}
+ @Override
+ public RelationalEntity<?>[] listPolicyAssociationsForTag(String metalake,
String name) {
+ NameIdentifier tagIdentifier = NameIdentifierUtil.ofTag(metalake, name);
+ checkMetalake(NameIdentifier.of(metalake), entityStore);
+ return TreeLockUtils.doWithTreeLock(
+ tagIdentifier,
+ LockType.READ,
+ () -> {
+ try {
+ return entityStore
+ .relationOperations()
+ .batchListEntitiesByRelation(
Review Comment:
`batchListEntitiesByRelation` returns an empty list when no matching
relation exists and does not prove that the anchor tag exists. Because this
method never loads the tag, a nonexistent tag is indistinguishable from an
existing tag with no policies, so the planned GET endpoint cannot honor its 404
contract. Please check the tag under the read lock before querying and add a
missing-tag test.
--
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]
