roryqi commented on code in PR #13355:
URL: https://github.com/apache/gravitino/pull/13355#discussion_r4060135414
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -302,150 +252,7 @@ public PolicyEntity[] listPolicyInfosForMetadataObject(
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
checkMetalake(NameIdentifier.of(metalake), entityStore);
- Map<Long, PolicyEntity> policiesById = new LinkedHashMap<>();
- Arrays.stream(listDirectPoliciesForMetadataObject(metalake,
metadataObject, false))
- .forEach(policy -> policiesById.put(policy.id(), policy));
-
- Arrays.stream(objectPolicyResolver.resolve(metalake, metadataObject))
- .forEach(
- policy -> {
- if (policy.inherited().orElse(false)) {
- policiesById.putIfAbsent(policy.id(), policy);
- } else {
- policiesById.put(policy.id(), policy);
- }
- });
-
- for (MetadataObject parent :
MetadataObjectUtil.getParentMetadataObjects(metadataObject)) {
- Arrays.stream(listDirectPoliciesForMetadataObject(metalake, parent,
true))
- .forEach(policy -> policiesById.putIfAbsent(policy.id(), policy));
- }
- return policiesById.values().stream()
- .filter(PolicyEntity::enabled)
- .toArray(PolicyEntity[]::new);
- }
-
- @Override
- public String[] associatePoliciesForMetadataObject(
- String metalake,
- MetadataObject metadataObject,
- String[] policiesToAdd,
- String[] policiesToRemove) {
- Preconditions.checkArgument(
-
SUPPORTED_METADATA_OBJECT_TYPES_FOR_POLICIES.contains(metadataObject.type()),
- "Cannot associate policies for unsupported metadata object type %s",
- metadataObject.type());
-
- NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- Entity.EntityType entityType =
MetadataObjectUtil.toEntityType(metadataObject);
-
- MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
-
- // Remove all the policies that are both set to add and remove
- Set<String> policiesToAddSet =
- policiesToAdd == null ? Sets.newHashSet() :
Sets.newHashSet(policiesToAdd);
- Set<String> policiesToRemoveSet =
- policiesToRemove == null ? Sets.newHashSet() :
Sets.newHashSet(policiesToRemove);
- Set<String> common = Sets.intersection(policiesToAddSet,
policiesToRemoveSet).immutableCopy();
- policiesToAddSet.removeAll(common);
- policiesToRemoveSet.removeAll(common);
-
- NameIdentifier[] policiesToAddIdent =
- policiesToAddSet.stream()
- .map(p -> NameIdentifierUtil.ofPolicy(metalake, p))
- .toArray(NameIdentifier[]::new);
- NameIdentifier[] policiesToRemoveIdent =
- policiesToRemoveSet.stream()
- .map(p -> NameIdentifierUtil.ofPolicy(metalake, p))
- .toArray(NameIdentifier[]::new);
-
- checkMetalake(NameIdentifier.of(metalake), entityStore);
- return TreeLockUtils.doWithTreeLock(
- entityIdent,
- LockType.READ,
- () ->
- TreeLockUtils.doWithTreeLock(
- NameIdentifier.of(NamespaceUtil.ofPolicy(metalake).levels()),
- LockType.WRITE,
- () -> {
- try {
- List<PolicyEntity> updatedPolicies =
- entityStore
- .relationOperations()
- .updateEntityRelations(
-
SupportsRelationOperations.Type.POLICY_METADATA_OBJECT_REL,
- entityIdent,
- entityType,
- policiesToAddIdent,
- policiesToRemoveIdent);
- return
updatedPolicies.stream().map(PolicyEntity::name).toArray(String[]::new);
- } catch (NoSuchEntityException e) {
- throw new NoSuchMetadataObjectException(
- e,
- "Failed to associate policies for metadata object %s
due to not found",
- metadataObject);
- } catch (EntityAlreadyExistsException e) {
- throw new PolicyAlreadyAssociatedException(
- e,
- "Failed to associate policies for metadata object due
to some policies %s already "
- + "associated to the metadata object %s",
- Arrays.toString(policiesToAdd),
- metadataObject);
- } catch (IOException e) {
- LOG.error(
- "Failed to associate policies for metadata object {}",
metadataObject, e);
- throw new RuntimeException(e);
- }
- }));
- }
-
- @Override
- public PolicyEntity getPolicyForMetadataObject(
- String metalake, MetadataObject metadataObject, String policyName) {
- try {
- return Arrays.stream(listPolicyInfosForMetadataObject(metalake,
metadataObject))
- .filter(policy -> policy.name().equals(policyName))
- .findFirst()
- .orElseThrow(
- () ->
- new NoSuchPolicyException(
- "Policy %s does not exist for metadata object %s",
- policyName, metadataObject));
- } catch (NoSuchMetadataObjectException e) {
- throw new NoSuchMetadataObjectException(
- e, "Failed to get policy for metadata object %s due to not found",
metadataObject);
- }
- }
-
- private PolicyEntity[] listDirectPoliciesForMetadataObject(
- String metalake, MetadataObject metadataObject, boolean inherited) {
- NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- Entity.EntityType entityType =
MetadataObjectUtil.toEntityType(metadataObject);
- return TreeLockUtils.doWithTreeLock(
- entityIdent,
- LockType.READ,
- () -> {
- try {
- return entityStore
- .relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.POLICY_METADATA_OBJECT_REL,
- entityIdent,
- entityType,
- true /* allFields */)
- .stream()
- .map(entity -> ((PolicyEntity)
entity).copyWithInherited(inherited))
- .toArray(PolicyEntity[]::new);
- } catch (NoSuchEntityException e) {
- throw new NoSuchMetadataObjectException(
- e,
- "Failed to list policies for metadata object %s due to not
found",
- metadataObject);
- } catch (IOException e) {
- LOG.error("Failed to list policies for metadata object {}",
metadataObject, e);
- throw new RuntimeException(e);
- }
- });
+ return objectPolicyResolver.resolve(metalake, metadataObject);
Review Comment:
Agreed. Tag-derived policy reads are best-effort across the object and its
ancestors. I added a comment in `PolicyManager` to document the accepted
behavior in 1d8ff2b358.
##########
docs/manage-policies-in-gravitino.md:
##########
@@ -217,42 +217,15 @@ client.deletePolicy("retention_30d");
## Object Operations
-### Attach and Detach Policies
-
-Both happen in one request, and either list can be omitted. Catalogs, schemas,
tables, filesets,
-topics, models, views, and functions can carry a policy.
-
-<Tabs groupId='language' queryString>
-<TabItem value="shell" label="REST">
-
-```shell
-curl -X POST -H "Accept: application/vnd.gravitino.v1+json" \
- -H "Content-Type: application/json" -d '{
- "policiesToAdd": ["retention_30d"],
- "policiesToRemove": ["retention_7d"]
-}' http://localhost:8090/api/metalakes/test/objects/catalog/catalog1/policies
-```
-
-</TabItem>
-<TabItem value="java" label="Java">
-
-```java
-Catalog catalog = client.loadCatalog("catalog1");
-catalog.supportsPolicies().associatePolicies(
- new String[] {"retention_30d"},
- new String[] {"retention_7d"});
-
-Schema schema = catalog.asSchemas().loadSchema("schema1");
-schema.supportsPolicies().associatePolicies(new String[] {"retention_30d"},
null);
-```
-
-</TabItem>
-</Tabs>
+Object policies are read-only results derived from effective tags. To change
the policies that apply
+to an object, associate a policy with a tag and then assign or remove that tag
on the object or one
+of its ancestors. See [Manage tags in
Gravitino](./manage-tags-in-gravitino.md) for tag assignment
+operations.
### List Policies on an Object
-The response includes policies inherited from ancestors. With `details=true`
each policy carries an
-`inherited` field, which a plain name listing does not.
+The response includes policies derived from effective tags assigned to the
object or its ancestors.
+With `details=true`, the response returns full policy objects instead of
policy names.
Review Comment:
Restored the `inherited` field explanation for `details=true` in 1d8ff2b358.
Thanks for catching this.
##########
maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/integration/test/AbstractGravitinoOptimizerEnvIT.java:
##########
@@ -115,17 +116,24 @@ protected void createPolicy(String policyName,
Map<String, Object> rules, String
GravitinoStrategy.JOB_TEMPLATE_NAME_KEY,
"template-name"));
metalakeClient.createPolicy(policyName, "custom", "comment", true,
content);
+ String tagName = policyTagName(policyName);
+ metalakeClient.createTag(tagName, "comment", Map.of());
+ metalakeClient.addPolicyForTag(tagName, policyName,
AllValuesSelector.get());
Review Comment:
Split tag creation and policy binding into `createTagForPolicy()` and
renamed the association helpers to `associatePolicyTagToTable/Schema()`,
updating their callers in 1d8ff2b358. `createPolicy()` now creates only the
policy.
--
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]
