This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 06a5a4f323 [#13174] fix(core): Keep plugin privileges when the
external drop returns false (#13196)
06a5a4f323 is described below
commit 06a5a4f323764041bb33974675516ca8b83afece
Author: Qi Yu <[email protected]>
AuthorDate: Wed Sep 16 19:32:47 2026 +0800
[#13174] fix(core): Keep plugin privileges when the external drop returns
false (#13196)
### What changes were proposed in this pull request?
`TableHookDispatcher.dropTable`/`purgeTable`,
`SchemaHookDispatcher.dropSchema`, `FilesetHookDispatcher.dropFileset`
and `TopicHookDispatcher.dropTopic` now call
`AuthorizationUtils.authorizationPluginRemovePrivileges` only when the
inner dispatcher returned `true`. Adds one test per hook that verifies a
`false` result leaves the plugin untouched.
### Why are the changes needed?
#12232 made the dispatchers keep the store registration when the
external drop returns `false` (the object was renamed or dropped out of
band), but the hook layer still removed the authorization-plugin
privileges unconditionally. Under a concurrent rename × drop the entity
alive under its new name lost its Ranger privileges while keeping its
Gravitino registration. `FunctionHookDispatcher` already had the guard;
this aligns the other four.
Fix: #13174
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New unit tests `testDrop*KeepsPrivilegesWhen*ReturnsFalse` in the four
`Test*HookDispatcher` classes; all `*HookDispatcher` tests pass
(`./gradlew :core:test --tests '*HookDispatcher' -PskipITs`).
---
.../ranger/integration/test/RangerBaseE2EIT.java | 20 +++++++++++++---
.../gravitino/hook/FilesetHookDispatcher.java | 7 ++++--
.../gravitino/hook/SchemaHookDispatcher.java | 8 +++++--
.../apache/gravitino/hook/TableHookDispatcher.java | 14 +++++++----
.../apache/gravitino/hook/TopicHookDispatcher.java | 7 ++++--
.../gravitino/hook/TestFilesetHookDispatcher.java | 23 ++++++++++++++++++
.../gravitino/hook/TestSchemaHookDispatcher.java | 22 +++++++++++++++++
.../gravitino/hook/TestTableHookDispatcher.java | 28 ++++++++++++++++++++++
.../gravitino/hook/TestTopicHookDispatcher.java | 23 ++++++++++++++++++
9 files changed, 139 insertions(+), 13 deletions(-)
diff --git
a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java
b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java
index 3c38ba0908..e404e77a36 100644
---
a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java
+++
b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java
@@ -1002,8 +1002,16 @@ public abstract class RangerBaseE2EIT extends BaseIT {
// Owner has all the privileges except for creating table
checkTableAllPrivilegesExceptForCreating();
- // Delete Gravitino's meta data
- catalog.asTableCatalog().purgeTable(NameIdentifier.of(schemaName,
tableName));
+ // The table was dropped externally, so Gravitino keeps its registration
and privileges.
+ Assertions.assertFalse(
+ catalog.asTableCatalog().purgeTable(NameIdentifier.of(schemaName,
tableName)));
+ waitForUpdatingPolicies();
+
+ // The retained owner privileges allow recreating the same table. A
successful Gravitino purge
+ // then removes both its registration and privileges.
+ sparkSession.sql(SQL_CREATE_TABLE);
+ Assertions.assertTrue(
+ catalog.asTableCatalog().purgeTable(NameIdentifier.of(schemaName,
tableName)));
waitForUpdatingPolicies();
checker.checkCreateTable();
@@ -1022,7 +1030,13 @@ public abstract class RangerBaseE2EIT extends BaseIT {
// Succeed to drop schema
sparkSession.sql(SQL_DROP_SCHEMA);
- catalog.asSchemas().dropSchema(schemaName, false);
+ Assertions.assertFalse(catalog.asSchemas().dropSchema(schemaName, false));
+ waitForUpdatingPolicies();
+
+ // The retained owner privileges allow recreating the same schema. A
successful Gravitino drop
+ // then removes both its registration and privileges.
+ sparkSession.sql(SQL_CREATE_SCHEMA);
+ Assertions.assertTrue(catalog.asSchemas().dropSchema(schemaName, false));
waitForUpdatingPolicies();
checker.checkCreateSchema();
diff --git
a/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java
b/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java
index 0e61cf758f..199e8010ab 100644
--- a/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java
+++ b/core/src/main/java/org/apache/gravitino/hook/FilesetHookDispatcher.java
@@ -118,8 +118,11 @@ public class FilesetHookDispatcher implements
FilesetDispatcher {
List<String> locations =
AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.FILESET);
boolean dropped = dispatcher.dropFileset(ident);
- AuthorizationUtils.authorizationPluginRemovePrivileges(
- ident, Entity.EntityType.FILESET, locations);
+ // A false result means the fileset was already gone and the registration
was kept.
+ if (dropped) {
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ ident, Entity.EntityType.FILESET, locations);
+ }
return dropped;
}
diff --git
a/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java
b/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java
index 7a79a9d213..8d2ce75b7b 100644
--- a/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java
+++ b/core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java
@@ -176,8 +176,12 @@ public class SchemaHookDispatcher implements
SchemaDispatcher {
List<String> locations =
AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.SCHEMA);
boolean dropped = dispatcher.dropSchema(ident, cascade);
- AuthorizationUtils.authorizationPluginRemovePrivileges(
- ident, Entity.EntityType.SCHEMA, locations);
+ // Only a schema that was really dropped loses its privileges; a false
result means the
+ // registration was kept.
+ if (dropped) {
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ ident, Entity.EntityType.SCHEMA, locations);
+ }
return dropped;
}
diff --git
a/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java
b/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java
index 9a1037e1ca..768c1e5410 100644
--- a/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java
+++ b/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java
@@ -129,8 +129,12 @@ public class TableHookDispatcher implements
TableDispatcher {
List<String> locations =
AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.TABLE);
boolean dropped = dispatcher.dropTable(ident);
- AuthorizationUtils.authorizationPluginRemovePrivileges(
- ident, Entity.EntityType.TABLE, locations);
+ // A false result means the external table was renamed or dropped out of
band and the
+ // registration was kept; the entity alive under another name must keep
its privileges too.
+ if (dropped) {
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ ident, Entity.EntityType.TABLE, locations);
+ }
return dropped;
}
@@ -139,8 +143,10 @@ public class TableHookDispatcher implements
TableDispatcher {
List<String> locations =
AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.TABLE);
boolean purged = dispatcher.purgeTable(ident);
- AuthorizationUtils.authorizationPluginRemovePrivileges(
- ident, Entity.EntityType.TABLE, locations);
+ if (purged) {
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ ident, Entity.EntityType.TABLE, locations);
+ }
return purged;
}
diff --git
a/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java
b/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java
index ad8440942b..4840cd87ae 100644
--- a/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java
+++ b/core/src/main/java/org/apache/gravitino/hook/TopicHookDispatcher.java
@@ -88,8 +88,11 @@ public class TopicHookDispatcher implements TopicDispatcher {
List<String> locations =
AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.TOPIC);
boolean dropped = dispatcher.dropTopic(ident);
- AuthorizationUtils.authorizationPluginRemovePrivileges(
- ident, Entity.EntityType.TOPIC, locations);
+ // A false result means the topic was dropped out of band and the
registration was kept.
+ if (dropped) {
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ ident, Entity.EntityType.TOPIC, locations);
+ }
return dropped;
}
diff --git
a/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java
b/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java
index a4431e7cbf..ae662040b3 100644
---
a/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java
+++
b/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java
@@ -40,6 +40,7 @@ import static
org.apache.gravitino.Configs.VERSION_RETENTION_COUNT;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import java.util.Map;
@@ -50,6 +51,7 @@ import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.authorization.AccessControlManager;
+import org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.authorization.Owner;
import org.apache.gravitino.authorization.OwnerDispatcher;
import org.apache.gravitino.catalog.CatalogManager;
@@ -69,6 +71,7 @@ import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
+import org.mockito.MockedStatic;
import org.mockito.Mockito;
public class TestFilesetHookDispatcher extends TestOperationDispatcher {
@@ -190,6 +193,26 @@ public class TestFilesetHookDispatcher extends
TestOperationDispatcher {
}
}
+ @Test
+ public void testDropKeepsPrivilegesWhenDropReturnsFalse() {
+ FilesetDispatcher dispatcher = Mockito.mock(FilesetDispatcher.class);
+ FilesetHookDispatcher hook = new FilesetHookDispatcher(dispatcher);
+ NameIdentifier ident = NameIdentifier.of(metalake, catalog, "schema",
"fileset");
+ Mockito.when(dispatcher.dropFileset(ident)).thenReturn(false);
+
+ try (MockedStatic<AuthorizationUtils> authz =
Mockito.mockStatic(AuthorizationUtils.class)) {
+ authz
+ .when(() -> AuthorizationUtils.getMetadataObjectLocation(any(),
any()))
+ .thenReturn(ImmutableList.of("/test"));
+
+ Assertions.assertFalse(hook.dropFileset(ident));
+
+ authz.verify(
+ () -> AuthorizationUtils.authorizationPluginRemovePrivileges(any(),
any(), any()),
+ Mockito.never());
+ }
+ }
+
@Test
public void testDropAuthorizationPrivilege() {
Namespace filesetNs = Namespace.of(metalake, catalog, "schema11212");
diff --git
a/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java
b/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java
index fbf0f43949..478ab3efd5 100644
--- a/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java
+++ b/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java
@@ -213,6 +213,28 @@ public class TestSchemaHookDispatcher {
"Pre-existing ancestor 'A' must keep its owner; only newly-created
schemas are claimed");
}
+ @Test
+ public void testDropSchemaKeepsPrivilegesWhenExternalDropReturnsFalse() {
+ NameIdentifier ident = NameIdentifier.of("test_metalake", "test_catalog",
"A:B:C");
+ when(mockDispatcher.dropSchema(eq(ident), eq(false))).thenReturn(false);
+
+ try (MockedStatic<AuthorizationUtils> authz =
Mockito.mockStatic(AuthorizationUtils.class)) {
+ authz
+ .when(
+ () ->
+ AuthorizationUtils.getMetadataObjectLocation(
+ any(NameIdentifier.class), any(Entity.EntityType.class)))
+ .thenReturn(ImmutableList.of("/test"));
+
+ Assertions.assertFalse(hookDispatcher.dropSchema(ident, false));
+
+ // Nothing was dropped, so the schema that is still registered keeps its
privileges.
+ authz.verify(
+ () -> AuthorizationUtils.authorizationPluginRemovePrivileges(any(),
any(), any()),
+ never());
+ }
+ }
+
@Test
public void testDropSchemaRemovesPrivileges() {
NameIdentifier ident = NameIdentifier.of("test_metalake", "test_catalog",
"A:B:C");
diff --git
a/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java
b/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java
index b1a61795a7..befd3a9dd4 100644
--- a/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java
+++ b/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java
@@ -48,6 +48,7 @@ import
org.apache.gravitino.rel.expressions.distributions.Distributions;
import org.apache.gravitino.rel.expressions.sorts.SortOrder;
import org.apache.gravitino.rel.expressions.transforms.Transform;
import org.apache.gravitino.rel.indexes.Index;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.MockedStatic;
@@ -81,6 +82,33 @@ public class TestTableHookDispatcher {
}
}
+ @Test
+ public void testDropKeepsPrivilegesWhenExternalDropReturnsFalse() {
+ TableDispatcher dispatcher = Mockito.mock(TableDispatcher.class);
+ TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> null);
+ NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "schema",
"table");
+ // false means the table was renamed or dropped out of band: the
registration is kept, so the
+ // plugin privileges of the entity that is still alive under another name
must be kept too.
+ Mockito.when(dispatcher.dropTable(ident)).thenReturn(false);
+ Mockito.when(dispatcher.purgeTable(ident)).thenReturn(false);
+
+ try (MockedStatic<AuthorizationUtils> authorizationUtils =
+ Mockito.mockStatic(AuthorizationUtils.class)) {
+ authorizationUtils
+ .when(() -> AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.TABLE))
+ .thenReturn(ImmutableList.of("/test"));
+
+ Assertions.assertFalse(hook.dropTable(ident));
+ Assertions.assertFalse(hook.purgeTable(ident));
+
+ authorizationUtils.verify(
+ () ->
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ Mockito.any(), Mockito.any(), Mockito.any()),
+ Mockito.never());
+ }
+ }
+
@Test
public void testCreateTableSetsOwnerWithNormalizedIdentifier() throws
Exception {
CatalogManager catalogManager = Mockito.mock(CatalogManager.class);
diff --git
a/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java
b/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java
index e4294b2c17..96c851aff7 100644
--- a/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java
+++ b/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java
@@ -21,6 +21,7 @@ package org.apache.gravitino.hook;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.util.Map;
import org.apache.commons.lang3.reflect.FieldUtils;
@@ -29,6 +30,7 @@ import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.authorization.AccessControlManager;
+import org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.authorization.Owner;
import org.apache.gravitino.authorization.OwnerDispatcher;
import org.apache.gravitino.catalog.CatalogManager;
@@ -46,6 +48,7 @@ import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
+import org.mockito.MockedStatic;
import org.mockito.Mockito;
public class TestTopicHookDispatcher extends TestOperationDispatcher {
@@ -156,6 +159,26 @@ public class TestTopicHookDispatcher extends
TestOperationDispatcher {
}
}
+ @Test
+ public void testDropKeepsPrivilegesWhenExternalDropReturnsFalse() {
+ TopicDispatcher dispatcher = Mockito.mock(TopicDispatcher.class);
+ TopicHookDispatcher hook = new TopicHookDispatcher(dispatcher);
+ NameIdentifier ident = NameIdentifier.of(metalake, catalog, "schema",
"topic");
+ Mockito.when(dispatcher.dropTopic(ident)).thenReturn(false);
+
+ try (MockedStatic<AuthorizationUtils> authz =
Mockito.mockStatic(AuthorizationUtils.class)) {
+ authz
+ .when(() -> AuthorizationUtils.getMetadataObjectLocation(any(),
any()))
+ .thenReturn(ImmutableList.of("/test"));
+
+ Assertions.assertFalse(hook.dropTopic(ident));
+
+ authz.verify(
+ () -> AuthorizationUtils.authorizationPluginRemovePrivileges(any(),
any(), any()),
+ Mockito.never());
+ }
+ }
+
@Test
public void testDropAuthorizationPrivilege() {
Namespace topicNs = Namespace.of(metalake, catalog, "schema1123");
