roryqi commented on code in PR #12718:
URL: https://github.com/apache/gravitino/pull/12718#discussion_r3901993746
##########
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);
+ }
Review Comment:
Thanks. This duplicates the other import comment. java.util.Arrays is
already imported in the current head, so no change is needed here.
##########
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:
Thanks. The REST entry points and the corresponding Tag/Policy Event and
Hook dispatcher delegation will be added together in the follow-up REST
integration PR. This core runtime PR intentionally does not expose these
operations through the composed production chain yet.
##########
core/src/main/java/org/apache/gravitino/tag/TagDispatcher.java:
##########
@@ -22,8 +22,10 @@
import java.util.Map;
import javax.annotation.Nullable;
import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.RelationalEntity;
import org.apache.gravitino.exceptions.NoSuchTagException;
import org.apache.gravitino.exceptions.TagAlreadyExistsException;
+import org.apache.gravitino.policy.PolicyAssociationSelector;
Review Comment:
Thanks. java.util.Arrays is already imported at line 21 in the current head,
and the build passes, so no change is needed here.
##########
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:
Thanks. Policy-tag association create, update, and delete operations enter
through REST, so selector validation against TagValueConstraint will be
implemented in the follow-up REST PR. TagValueConstraint is immutable after tag
creation, and this keeps semantic request validation in the REST layer instead
of duplicating it in core.
##########
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:
Thanks. We agree this must be addressed together with the planned
object-policy semantic change in the follow-up REST PR. That change will
separate direct-policy compatibility traversal from effective tag-derived
resolution and define inherited flags there. We are intentionally not changing
the existing REST traversal in this core runtime PR.
##########
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]);
Review Comment:
Thanks. batchListEntitiesByRelation supports either endpoint for
POLICY_TAG_REL. With a POLICY anchor, PolicyTagRelService.listRelations calls
listByPolicyNames and then tagTargets, so the returned target entities are
tags. The current implementation is correct and no change is needed here.
##########
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:
Fixed in e533d2de3. listPolicyAssociationsForTag now loads the anchor tag
under the read lock before querying relations, and the regression test verifies
that a missing tag raises NoSuchTagException.
##########
common/src/main/java/org/apache/gravitino/json/PolicyAssociationSelectorSerde.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.json;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.JsonSerializer;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import java.io.IOException;
+import org.apache.gravitino.policy.AllValuesSelector;
+import org.apache.gravitino.policy.PolicyAssociationSelector;
+import org.apache.gravitino.policy.TagValueSelector;
+
+/** JSON serializer and deserializer for policy-to-tag selectors. */
+public final class PolicyAssociationSelectorSerde {
+
+ private static final String TYPE = "type";
+ private static final String VALUE = "value";
+
+ private static final ObjectMapper MAPPER =
+ JsonUtils.anyFieldMapper()
+ .copy()
+ .registerModule(
+ new SimpleModule()
+ .addSerializer(PolicyAssociationSelector.class, new
Serializer())
+ .addDeserializer(PolicyAssociationSelector.class, new
Deserializer()));
+
+ private PolicyAssociationSelectorSerde() {}
+
+ /**
+ * Serializes a selector.
+ *
+ * @param selector The selector to serialize.
+ * @return The selector JSON.
+ */
+ public static String serialize(PolicyAssociationSelector selector) {
+ try {
+ return
MAPPER.writerFor(PolicyAssociationSelector.class).writeValueAsString(selector);
+ } catch (JsonProcessingException e) {
+ throw new IllegalArgumentException("Failed to serialize policy
association selector", e);
+ }
+ }
+
+ /**
+ * Deserializes a selector.
+ *
+ * @param json The selector JSON.
+ * @return The selector.
+ */
+ public static PolicyAssociationSelector deserialize(String json) {
+ try {
+ return MAPPER.readValue(json, PolicyAssociationSelector.class);
+ } catch (JsonProcessingException e) {
+ throw new IllegalArgumentException("Failed to deserialize policy
association selector", e);
+ }
+ }
+
+ /** Serializes a policy association selector. */
+ public static final class Serializer extends
JsonSerializer<PolicyAssociationSelector> {
+
+ @Override
+ public void serialize(
+ PolicyAssociationSelector selector,
+ JsonGenerator generator,
+ SerializerProvider serializerProvider)
+ throws IOException {
+ if (!(selector instanceof AllValuesSelector) && !(selector instanceof
TagValueSelector)) {
+ throw JsonMappingException.from(
+ generator, "Unsupported policy association selector: " + selector);
+ }
+
+ generator.writeStartObject();
+ generator.writeStringField(TYPE, selector.type());
+ if (selector instanceof TagValueSelector) {
+ generator.writeStringField(VALUE, ((TagValueSelector)
selector).value());
+ }
+ generator.writeEndObject();
+ }
+ }
+
+ /** Deserializes a policy association selector. */
+ public static final class Deserializer extends
JsonDeserializer<PolicyAssociationSelector> {
+
+ @Override
+ public PolicyAssociationSelector deserialize(
+ JsonParser parser, DeserializationContext deserializationContext)
throws IOException {
+ JsonNode node = parser.getCodec().readTree(parser);
+ String type = node.get(TYPE).asText();
+ if (AllValuesSelector.TYPE.equals(type)) {
+ return AllValuesSelector.get();
+ }
+ if (TagValueSelector.TYPE.equals(type)) {
+ return TagValueSelector.of(node.get(VALUE).asText());
+ }
+ throw JsonMappingException.from(
+ parser, "Unsupported policy association selector type: " + type);
+ }
Review Comment:
Fixed in e533d2de3. The deserializer now validates the required textual type
and value fields and reports a clear JsonMappingException through the public
deserialize helper. Tests cover missing and null fields.
##########
core/src/main/java/org/apache/gravitino/policy/ObjectPolicyResolver.java:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.policy;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.EntityStore;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.RelationalEntity;
+import org.apache.gravitino.SupportsRelationOperations;
+import org.apache.gravitino.json.PolicyAssociationSelectorSerde;
+import org.apache.gravitino.meta.PolicyEntity;
+import org.apache.gravitino.meta.TagEntity;
+import org.apache.gravitino.tag.EffectiveTagResolver;
+import org.apache.gravitino.tag.TagAssignment;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Resolves policies for a metadata object from its effective tag assignments.
+ *
+ * <p>The resolver evaluates relation selectors, rejects mixed match results
for the same policy,
+ * filters disabled policies, and deduplicates repeated matches by policy
entity ID.
+ */
+public class ObjectPolicyResolver {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ObjectPolicyResolver.class);
+
+ private final EntityStore entityStore;
+ private final EffectiveTagResolver effectiveTagResolver;
+
+ /**
+ * Creates an object policy resolver.
+ *
+ * @param entityStore The entity store used to read policy-to-tag relations.
+ */
+ public ObjectPolicyResolver(EntityStore entityStore) {
+ this(entityStore, new EffectiveTagResolver(entityStore));
+ }
+
+ ObjectPolicyResolver(EntityStore entityStore, EffectiveTagResolver
effectiveTagResolver) {
+ this.entityStore = entityStore;
+ this.effectiveTagResolver = effectiveTagResolver;
+ }
+
+ /**
+ * Resolves enabled policies for a metadata object.
+ *
+ * @param metalake The metalake name.
+ * @param metadataObject The metadata object.
+ * @return Enabled policies selected by the object's effective tags.
+ */
+ public PolicyEntity[] resolve(String metalake, MetadataObject
metadataObject) {
+ TagEntity[] effectiveTags = effectiveTagResolver.resolve(metalake,
metadataObject);
+ if (effectiveTags.length == 0) {
+ return new PolicyEntity[0];
+ }
+
+ Map<String, TagEntity> tagsByName =
+ Arrays.stream(effectiveTags)
+ .collect(
+ Collectors.toMap(
+ TagEntity::name, tag -> tag, (left, right) -> left,
LinkedHashMap::new));
+ List<NameIdentifier> tagIdentifiers =
+ tagsByName.keySet().stream()
+ .map(tagName -> NameIdentifierUtil.ofTag(metalake, tagName))
+ .collect(Collectors.toList());
+
+ List<RelationalEntity<?>> relations;
+ try {
+ relations =
+ entityStore
+ .relationOperations()
+ .batchListEntitiesByRelation(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ tagIdentifiers,
+ Entity.EntityType.TAG);
+ } catch (IOException e) {
+ LOG.error("Failed to resolve policies for metadata object {}",
metadataObject, e);
+ throw new RuntimeException(e);
+ }
+
+ Map<Long, MatchState> matchStates = new LinkedHashMap<>();
+ for (RelationalEntity<?> relation : relations) {
+ TagEntity tag = tagsByName.get(relation.source().name());
+ if (tag == null) {
+ continue;
+ }
+ PolicyEntity policy = (PolicyEntity) relation.targetEntity();
+ PolicyAssociationSelector selector =
+
PolicyAssociationSelectorSerde.deserialize(relation.relationValue().orElseThrow());
+ TagAssignment assignment =
tag.assignment().orElseGet(TagAssignment::noValue);
+ boolean matches = matches(selector, assignment);
Review Comment:
Fixed in e533d2de3. A missing relation payload is now treated as
AllValuesSelector, preserving the tag-presence compatibility behavior, and a
regression test was added.
--
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]
