Copilot commented on code in PR #12718:
URL: https://github.com/apache/gravitino/pull/12718#discussion_r3893385749
##########
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:
`listPoliciesForTag` uses `Arrays.stream(...)` but `java.util.Arrays` is not
imported in this file, which should fail compilation. Add the missing import
(or avoid `Arrays.stream` by iterating directly).
##########
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:
`listPoliciesForTag` uses `Arrays.stream(...)` but `java.util.Arrays` is not
imported in this file, which should fail compilation. Add the missing import
(or avoid `Arrays.stream` by iterating directly).
##########
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:
The POLICY_TAG relation is created as (source=TAG, target=POLICY) in
`TagManager.addPolicyForTag`, but this method queries it as if (source=POLICY).
That likely returns no results (or incorrect ones), so `listTagsForPolicy` /
`listTagAssociationsForPolicy` will be broken. A concrete fix is to query
POLICY_TAG relations by *target policy* (e.g., via a relation query API that
filters by target), or store a reverse edge (POLICY→TAG) at association time;
then ensure the returned associations actually contain tags (not policies) for
callers.
##########
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:
`relation.relationValue().orElseThrow()` will throw if a POLICY_TAG relation
exists without a selector payload (e.g., older persisted associations or
partially-migrated data). To keep policy-on-tag usable during transition, treat
a missing relation value as a safe default (typically
`AllValuesSelector.get()`), or fail with a domain-specific error that clearly
indicates the association is invalid and needs migration.
##########
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:
This deserializer will NPE if `type` is missing/null (or if `value` is
missing for TAG_VALUE). Please validate required fields and throw a
`JsonMappingException` with a clear message (e.g., missing `type`, missing
`value` for TAG_VALUE) instead of relying on `node.get(...).asText()`.
--
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]
