Copilot commented on code in PR #9252:
URL: https://github.com/apache/gravitino/pull/9252#discussion_r2567009714
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/annotations/AuthorizationExpression.java:
##########
@@ -32,7 +32,7 @@
@Retention(RetentionPolicy.RUNTIME)
public @interface AuthorizationExpression {
/**
- * The expression to evaluate for authorization, which represents multiple
privileges.
+ * The expression to evaluate for authorization, which represents multiple
privileges. g
Review Comment:
Corrected incomplete sentence ending with 'g' to 'privileges.'
```suggestion
* The expression to evaluate for authorization, which represents multiple
privileges.
```
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/PolicyAuthorizationIT.java:
##########
@@ -0,0 +1,385 @@
+/*
+ * 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.client.integration.test.authorization;
+
+import static org.junit.Assert.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.gravitino.Catalog;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.MetadataObjects;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.authorization.Privileges;
+import org.apache.gravitino.authorization.SecurableObject;
+import org.apache.gravitino.authorization.SecurableObjects;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.integration.test.container.ContainerSuite;
+import org.apache.gravitino.integration.test.container.HiveContainer;
+import org.apache.gravitino.policy.PolicyChange;
+import org.apache.gravitino.policy.PolicyContents;
+import org.apache.gravitino.policy.SupportsPolicies;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.TableCatalog;
+import org.apache.gravitino.rel.types.Types;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.MethodOrderer;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestMethodOrder;
+
+@Tag("gravitino-docker-test")
+@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
+public class PolicyAuthorizationIT extends BaseRestApiAuthorizationIT {
+
+ private static final String CATALOG = "catalog";
+ private static final String SCHEMA = "schema";
+ private static final ContainerSuite containerSuite =
ContainerSuite.getInstance();
+ private static String hmsUri;
+ private static String role = "role";
+
+ @BeforeAll
+ public void startIntegrationTest() throws Exception {
+ containerSuite.startHiveContainer();
+ super.startIntegrationTest();
+ hmsUri =
+ String.format(
+ "thrift://%s:%d",
+ containerSuite.getHiveContainer().getContainerIpAddress(),
+ HiveContainer.HIVE_METASTORE_PORT);
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put("metastore.uris", hmsUri);
+ client
+ .loadMetalake(METALAKE)
+ .createCatalog(CATALOG, Catalog.Type.RELATIONAL, "hive", "comment",
properties)
+ .asSchemas()
+ .createSchema(SCHEMA, "test", new HashMap<>());
+ // try to load the schema as normal user, expect failure
+ assertThrows(
+ "Can not access metadata.{" + CATALOG + "." + SCHEMA + "}.",
+ RuntimeException.class,
+ () -> {
+ normalUserClient
+ .loadMetalake(METALAKE)
+ .loadCatalog(CATALOG)
+ .asSchemas()
+ .loadSchema(SCHEMA);
+ });
+ // grant tester privilege
+ List<SecurableObject> securableObjects = new ArrayList<>();
+ GravitinoMetalake gravitinoMetalake = client.loadMetalake(METALAKE);
+ SecurableObject catalogObject =
+ SecurableObjects.ofCatalog(CATALOG,
ImmutableList.of(Privileges.UseCatalog.allow()));
+ securableObjects.add(catalogObject);
+ gravitinoMetalake.createRole(role, new HashMap<>(), securableObjects);
+ gravitinoMetalake.grantRolesToUser(ImmutableList.of(role), NORMAL_USER);
+ // normal user can load the catalog but not the schema
+ Catalog catalogLoadByNormalUser =
normalUserClient.loadMetalake(METALAKE).loadCatalog(CATALOG);
+ assertEquals(CATALOG, catalogLoadByNormalUser.name());
+ assertThrows(
+ "Can not access metadata.{" + CATALOG + "." + SCHEMA + "}.",
+ ForbiddenException.class,
+ () -> {
+ catalogLoadByNormalUser.asSchemas().loadSchema(SCHEMA);
+ });
+ TableCatalog tableCatalog =
client.loadMetalake(METALAKE).loadCatalog(CATALOG).asTableCatalog();
+ tableCatalog.createTable(
+ NameIdentifier.of(SCHEMA, "table1"), createColumns(), "test", new
HashMap<>());
+ }
+
+ private Column[] createColumns() {
+ return new Column[] {Column.of("col1", Types.StringType.get())};
+ }
+
+ @AfterAll
+ public void stopIntegrationTest() throws IOException, InterruptedException {
Review Comment:
The method `stopIntegrationTest` should be annotated with `@AfterAll` but is
not static. JUnit 5 requires `@AfterAll` methods to be static unless the test
class uses `@TestInstance(Lifecycle.PER_CLASS)`. Either make this method static
or add the appropriate test instance lifecycle annotation.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:
##########
@@ -461,4 +461,18 @@ private List<PolicyPO> getPolicyPOsByMetalakeAndNames(
PolicyMetaMapper.class,
mapper -> mapper.listPolicyPOsByMetalakeAndPolicyNames(metalakeName,
policyNames));
}
+
+ public long getPolicyIdByPolicyName(long metalakeId, String policyName) {
+ PolicyPO policyPO =
+ SessionUtils.getWithoutCommit(
+ PolicyMetaMapper.class,
+ mapper -> mapper.selectPolicyMetaByMetalakeIdAndName(metalakeId,
policyName));
+ if (policyPO == null) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.POLICY.name().toLowerCase(),
+ policyName);
+ }
+ return policyPO.getPolicyId();
Review Comment:
The `getPolicyIdByPolicyName` method lacks Javadoc documentation. It should
describe the method's purpose, parameters, return value, and the exception it
throws.
##########
core/src/main/java/org/apache/gravitino/hook/PolicyHookDispatcher.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.hook;
+
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.authorization.AuthorizationUtils;
+import org.apache.gravitino.authorization.Owner;
+import org.apache.gravitino.authorization.OwnerDispatcher;
+import org.apache.gravitino.exceptions.NoSuchPolicyException;
+import org.apache.gravitino.exceptions.PolicyAlreadyExistsException;
+import org.apache.gravitino.meta.PolicyEntity;
+import org.apache.gravitino.policy.Policy;
+import org.apache.gravitino.policy.PolicyChange;
+import org.apache.gravitino.policy.PolicyContent;
+import org.apache.gravitino.policy.PolicyDispatcher;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+import org.apache.gravitino.utils.PrincipalUtils;
+
+public class PolicyHookDispatcher implements PolicyDispatcher {
+
+ private final PolicyDispatcher dispatcher;
+
+ public PolicyHookDispatcher(PolicyDispatcher dispatcher) {
+ this.dispatcher = dispatcher;
+ }
+
+ @Override
+ public String[] listPolicies(String metalake) {
+ return dispatcher.listPolicies(metalake);
+ }
+
+ @Override
+ public PolicyEntity[] listPolicyInfos(String metalake) {
+ return dispatcher.listPolicyInfos(metalake);
+ }
+
+ @Override
+ public PolicyEntity getPolicy(String metalake, String policyName) throws
NoSuchPolicyException {
+ return dispatcher.getPolicy(metalake, policyName);
+ }
+
+ @Override
+ public PolicyEntity createPolicy(
+ String metalake,
+ String name,
+ Policy.BuiltInType type,
+ String comment,
+ boolean enabled,
+ PolicyContent content)
+ throws PolicyAlreadyExistsException {
+ AuthorizationUtils.checkCurrentUser(metalake,
PrincipalUtils.getCurrentUserName());
+ PolicyEntity policy = dispatcher.createPolicy(metalake, name, type,
comment, enabled, content);
+
+ // Set the creator as the owner of the catalog.
Review Comment:
The comment incorrectly states 'catalog' when the code is setting the owner
for a policy. Update to 'Set the creator as the owner of the policy.'
```suggestion
// Set the creator as the owner of the policy.
```
##########
core/src/main/java/org/apache/gravitino/hook/PolicyHookDispatcher.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.hook;
+
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.authorization.AuthorizationUtils;
+import org.apache.gravitino.authorization.Owner;
+import org.apache.gravitino.authorization.OwnerDispatcher;
+import org.apache.gravitino.exceptions.NoSuchPolicyException;
+import org.apache.gravitino.exceptions.PolicyAlreadyExistsException;
+import org.apache.gravitino.meta.PolicyEntity;
+import org.apache.gravitino.policy.Policy;
+import org.apache.gravitino.policy.PolicyChange;
+import org.apache.gravitino.policy.PolicyContent;
+import org.apache.gravitino.policy.PolicyDispatcher;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+import org.apache.gravitino.utils.PrincipalUtils;
+
+public class PolicyHookDispatcher implements PolicyDispatcher {
+
+ private final PolicyDispatcher dispatcher;
+
+ public PolicyHookDispatcher(PolicyDispatcher dispatcher) {
+ this.dispatcher = dispatcher;
+ }
+
+ @Override
+ public String[] listPolicies(String metalake) {
+ return dispatcher.listPolicies(metalake);
+ }
+
+ @Override
+ public PolicyEntity[] listPolicyInfos(String metalake) {
+ return dispatcher.listPolicyInfos(metalake);
+ }
+
+ @Override
+ public PolicyEntity getPolicy(String metalake, String policyName) throws
NoSuchPolicyException {
+ return dispatcher.getPolicy(metalake, policyName);
+ }
+
+ @Override
+ public PolicyEntity createPolicy(
+ String metalake,
+ String name,
+ Policy.BuiltInType type,
+ String comment,
+ boolean enabled,
+ PolicyContent content)
+ throws PolicyAlreadyExistsException {
+ AuthorizationUtils.checkCurrentUser(metalake,
PrincipalUtils.getCurrentUserName());
+ PolicyEntity policy = dispatcher.createPolicy(metalake, name, type,
comment, enabled, content);
+
+ // Set the creator as the owner of the catalog.
+ OwnerDispatcher ownerDispatcher =
GravitinoEnv.getInstance().ownerDispatcher();
+ if (ownerDispatcher != null) {
+ ownerDispatcher.setOwner(
+ metalake,
+ NameIdentifierUtil.toMetadataObject(
+ NameIdentifierUtil.ofPolicy(metalake, name),
Entity.EntityType.POLICY),
+ PrincipalUtils.getCurrentUserName(),
+ Owner.Type.USER);
+ }
+ return policy;
+ }
+
+ @Override
+ public PolicyEntity alterPolicy(String metalake, String policyName,
PolicyChange... changes) {
+ return dispatcher.alterPolicy(metalake, policyName, changes);
Review Comment:
The `alterPolicy` method lacks authorization checks for modifying policies.
Unlike `createPolicy` which validates the current user, this method delegates
directly to the dispatcher without checking if the user has permission to alter
the policy.
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/PolicyAuthorizationIT.java:
##########
@@ -0,0 +1,385 @@
+/*
+ * 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.client.integration.test.authorization;
+
+import static org.junit.Assert.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.gravitino.Catalog;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.MetadataObjects;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.authorization.Privileges;
+import org.apache.gravitino.authorization.SecurableObject;
+import org.apache.gravitino.authorization.SecurableObjects;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.integration.test.container.ContainerSuite;
+import org.apache.gravitino.integration.test.container.HiveContainer;
+import org.apache.gravitino.policy.PolicyChange;
+import org.apache.gravitino.policy.PolicyContents;
+import org.apache.gravitino.policy.SupportsPolicies;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.TableCatalog;
+import org.apache.gravitino.rel.types.Types;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.MethodOrderer;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestMethodOrder;
+
+@Tag("gravitino-docker-test")
+@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
+public class PolicyAuthorizationIT extends BaseRestApiAuthorizationIT {
+
+ private static final String CATALOG = "catalog";
+ private static final String SCHEMA = "schema";
+ private static final ContainerSuite containerSuite =
ContainerSuite.getInstance();
+ private static String hmsUri;
+ private static String role = "role";
+
+ @BeforeAll
+ public void startIntegrationTest() throws Exception {
Review Comment:
The method `startIntegrationTest` should be annotated with `@BeforeAll` but
is not static. JUnit 5 requires `@BeforeAll` methods to be static unless the
test class uses `@TestInstance(Lifecycle.PER_CLASS)`. Either make this method
static or add the appropriate test instance lifecycle annotation.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/MetadataObjectService.java:
##########
@@ -84,7 +85,24 @@ public class MetadataObjectService {
MetadataObject.Type.COLUMN,
MetadataObjectService::getColumnObjectsFullName,
MetadataObject.Type.TAG,
- MetadataObjectService::getTagObjectsFullName);
+ MetadataObjectService::getTagObjectsFullName,
+ MetadataObject.Type.POLICY,
+ MetadataObjectService::getPolicyObjectsFullName);
+
+ private static Map<Long, String> getPolicyObjectsFullName(List<Long>
policyIds) {
+ if (policyIds == null || policyIds.isEmpty()) {
+ return Map.of();
+ }
+ return policyIds.stream()
+ .collect(
+ Collectors.toMap(
+ policyId -> policyId,
+ policyId ->
+ SessionUtils.getWithoutCommit(
+ PolicyMetaMapper.class,
+ policyMetaMapper ->
+
policyMetaMapper.selectPolicyByPolicyId(policyId).getPolicyName())));
Review Comment:
This method performs a database query for each policy ID inside the stream
collector, resulting in N+1 query pattern. Consider fetching all policies in a
single batch query by passing the list of policy IDs to a batch select method.
```suggestion
// Batch fetch all policies in a single query
return SessionUtils.getWithoutCommit(
PolicyMetaMapper.class,
policyMetaMapper -> {
List<org.apache.gravitino.storage.relational.po.PolicyPO> policies
=
policyMetaMapper.selectPoliciesByPolicyIds(policyIds);
// Map policyId to policyName
return policies.stream()
.collect(Collectors.toMap(
org.apache.gravitino.storage.relational.po.PolicyPO::getPolicyId,
org.apache.gravitino.storage.relational.po.PolicyPO::getPolicyName
));
}
);
```
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -95,7 +97,10 @@ public Filter getDescriptorFilter() {
StatisticOperations.class.getName(),
PartitionOperations.class.getName(),
MetadataObjectTagOperations.class.getName(),
- TagOperations.class.getName()));
+ TagOperations.class.getName(),
+ MetadataObjectTagOperations.class.getName(),
Review Comment:
`MetadataObjectTagOperations.class.getName()` is duplicated on lines 99 and
101. Remove the duplicate entry on line 101.
```suggestion
```
##########
core/src/main/java/org/apache/gravitino/hook/PolicyHookDispatcher.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.hook;
+
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.authorization.AuthorizationUtils;
+import org.apache.gravitino.authorization.Owner;
+import org.apache.gravitino.authorization.OwnerDispatcher;
+import org.apache.gravitino.exceptions.NoSuchPolicyException;
+import org.apache.gravitino.exceptions.PolicyAlreadyExistsException;
+import org.apache.gravitino.meta.PolicyEntity;
+import org.apache.gravitino.policy.Policy;
+import org.apache.gravitino.policy.PolicyChange;
+import org.apache.gravitino.policy.PolicyContent;
+import org.apache.gravitino.policy.PolicyDispatcher;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+import org.apache.gravitino.utils.PrincipalUtils;
+
+public class PolicyHookDispatcher implements PolicyDispatcher {
+
+ private final PolicyDispatcher dispatcher;
+
+ public PolicyHookDispatcher(PolicyDispatcher dispatcher) {
+ this.dispatcher = dispatcher;
+ }
+
+ @Override
+ public String[] listPolicies(String metalake) {
+ return dispatcher.listPolicies(metalake);
+ }
+
+ @Override
+ public PolicyEntity[] listPolicyInfos(String metalake) {
+ return dispatcher.listPolicyInfos(metalake);
+ }
+
+ @Override
+ public PolicyEntity getPolicy(String metalake, String policyName) throws
NoSuchPolicyException {
+ return dispatcher.getPolicy(metalake, policyName);
+ }
+
+ @Override
+ public PolicyEntity createPolicy(
+ String metalake,
+ String name,
+ Policy.BuiltInType type,
+ String comment,
+ boolean enabled,
+ PolicyContent content)
+ throws PolicyAlreadyExistsException {
+ AuthorizationUtils.checkCurrentUser(metalake,
PrincipalUtils.getCurrentUserName());
+ PolicyEntity policy = dispatcher.createPolicy(metalake, name, type,
comment, enabled, content);
+
+ // Set the creator as the owner of the catalog.
+ OwnerDispatcher ownerDispatcher =
GravitinoEnv.getInstance().ownerDispatcher();
+ if (ownerDispatcher != null) {
+ ownerDispatcher.setOwner(
+ metalake,
+ NameIdentifierUtil.toMetadataObject(
+ NameIdentifierUtil.ofPolicy(metalake, name),
Entity.EntityType.POLICY),
+ PrincipalUtils.getCurrentUserName(),
+ Owner.Type.USER);
+ }
+ return policy;
+ }
+
+ @Override
+ public PolicyEntity alterPolicy(String metalake, String policyName,
PolicyChange... changes) {
+ return dispatcher.alterPolicy(metalake, policyName, changes);
+ }
+
+ @Override
+ public void enablePolicy(String metalake, String policyName) throws
NoSuchPolicyException {
+ dispatcher.enablePolicy(metalake, policyName);
+ }
+
+ @Override
+ public void disablePolicy(String metalake, String policyName) throws
NoSuchPolicyException {
+ dispatcher.disablePolicy(metalake, policyName);
+ }
+
+ @Override
+ public boolean deletePolicy(String metalake, String policyName) {
+ return dispatcher.deletePolicy(metalake, policyName);
Review Comment:
The `deletePolicy` method does not remove the owner record when a policy is
deleted. This can lead to orphaned owner entries in the system. Consider adding
logic to clean up the owner association before or after deletion.
```suggestion
boolean deleted = dispatcher.deletePolicy(metalake, policyName);
if (deleted) {
OwnerDispatcher ownerDispatcher =
GravitinoEnv.getInstance().ownerDispatcher();
if (ownerDispatcher != null) {
ownerDispatcher.removeOwner(
metalake,
NameIdentifierUtil.toMetadataObject(
NameIdentifierUtil.ofPolicy(metalake, policyName),
Entity.EntityType.POLICY)
);
}
}
return deleted;
```
##########
core/src/main/java/org/apache/gravitino/cache/ReverseIndexRules.java:
##########
@@ -157,6 +157,14 @@ public class ReverseIndexRules {
nsFileset.level(0),
nsFileset.level(1));
break;
+ case TAG:
+ entityType = Entity.EntityType.TAG;
+ namespace =
NamespaceUtil.ofTag(roleEntity.namespace().level(0));
+ break;
+ case POLICY:
+ entityType = Entity.EntityType.POLICY;
+ namespace =
NamespaceUtil.ofPolicy(roleEntity.namespace().level(0));
+ break;
Review Comment:
[nitpick] The TAG and POLICY case blocks set `entityType` and `namespace`
variables, but the broader context of how these variables are used is not clear
from this diff alone. Consider adding inline comments explaining the purpose of
this mapping, especially since this appears to be part of a cache invalidation
or index maintenance logic.
--
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]
