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 29093d2ecd [#10269] fix(core): prevent fileset privilege removal on
failed drop (#12582)
29093d2ecd is described below
commit 29093d2ecd3d9562335d7f7aa49b7985afd8111e
Author: Saravanan Kandaswamy <[email protected]>
AuthorDate: Mon Sep 21 08:57:52 2026 +0530
[#10269] fix(core): prevent fileset privilege removal on failed drop
(#12582)
[#10269] fix(core): prevent fileset privilege removal on failed drop
### What changes were proposed in this pull request?
Updated `FilesetHookDispatcher.dropFileset` to remove authorization
privileges only when the delegated fileset drop succeeds.
Added a regression test verifying that privileges are not removed when
the delegate returns `false`.
### Why are the changes needed?
When `dropFileset` returned `false`, authorization privileges were still
removed. This could leave authorization state inconsistent with metadata
state when the fileset was not deleted.
Fix: #10269
### Does this PR introduce _any_ user-facing change?
No. This is an internal authorization consistency fix and does not
change any public APIs or configuration properties.
### How was this patch tested?
The following focused tests passed:
```text
./gradlew :core:test --tests
org.apache.gravitino.hook.TestFilesetHookDispatcher -PskipITs
---------
Co-authored-by: Qi Yu <[email protected]>
---
.../gravitino/hook/TestFilesetHookDispatcher.java | 28 ++++++++++++++++++++++
.../gravitino/hook/TestSchemaHookDispatcher.java | 17 +++++++++++++
.../gravitino/hook/TestTopicHookDispatcher.java | 18 ++++++++++++++
3 files changed, 63 insertions(+)
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 ae662040b3..1a47a83025 100644
---
a/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java
+++
b/core/src/test/java/org/apache/gravitino/hook/TestFilesetHookDispatcher.java
@@ -43,9 +43,11 @@ 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.List;
import java.util.Map;
import org.apache.commons.lang3.reflect.FieldUtils;
import org.apache.gravitino.Config;
+import org.apache.gravitino.Entity;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
@@ -260,6 +262,32 @@ public class TestFilesetHookDispatcher extends
TestOperationDispatcher {
});
}
+ @Test
+ public void testDropFilesetShouldNotRemovePrivilegesWhenDropReturnsFalse() {
+ NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "schema",
"fileset");
+ FilesetDispatcher delegate = Mockito.mock(FilesetDispatcher.class);
+ FilesetHookDispatcher hookDispatcher = new FilesetHookDispatcher(delegate);
+ List<String> locations = Lists.newArrayList("/tmp/fileset");
+
+ Mockito.when(delegate.dropFileset(ident)).thenReturn(false);
+
+ try (MockedStatic<AuthorizationUtils> mockedAuthz =
+ Mockito.mockStatic(AuthorizationUtils.class)) {
+ mockedAuthz
+ .when(
+ () -> AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.FILESET))
+ .thenReturn(locations);
+
+ Assertions.assertFalse(hookDispatcher.dropFileset(ident));
+
+ mockedAuthz.verify(
+ () ->
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ ident, Entity.EntityType.FILESET, locations),
+ Mockito.never());
+ }
+ }
+
@Test
public void testRenameAuthorizationPrivilege() {
Namespace filesetNs = Namespace.of(metalake, catalog, "schema1121");
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 478ab3efd5..8c5cfa41c1 100644
--- a/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java
+++ b/core/src/test/java/org/apache/gravitino/hook/TestSchemaHookDispatcher.java
@@ -260,6 +260,23 @@ public class TestSchemaHookDispatcher {
}
}
+ @Test
+ public void testDropSchemaDoesNotRemovePrivilegesWhenSchemaDoesNotExist() {
+ 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)) {
+ boolean dropped = hookDispatcher.dropSchema(ident, false);
+
+ Assertions.assertFalse(dropped);
+ authz.verify(
+ () ->
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ ident, Entity.EntityType.SCHEMA, null),
+ Mockito.never());
+ }
+ }
+
@SuppressWarnings("unchecked")
private List<MetadataObject> captureOwnedObjects() {
ArgumentCaptor<List<MetadataObject>> captor =
ArgumentCaptor.forClass(List.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 96c851aff7..240e3ec71d 100644
--- a/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java
+++ b/core/src/test/java/org/apache/gravitino/hook/TestTopicHookDispatcher.java
@@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.util.Map;
import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.gravitino.Entity;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
@@ -78,6 +79,23 @@ public class TestTopicHookDispatcher extends
TestOperationDispatcher {
Mockito.when(catalog.getAuthorizationPlugin()).thenReturn(authorizationPlugin);
}
+ @Test
+ public void testDropTopicDoesNotRemovePrivilegesWhenTopicDoesNotExist() {
+ TopicDispatcher dispatcher = Mockito.mock(TopicDispatcher.class);
+ TopicHookDispatcher hook = new TopicHookDispatcher(dispatcher);
+ NameIdentifier ident = NameIdentifier.of("test_metalake", "test_catalog",
"topic");
+ Mockito.when(dispatcher.dropTopic(ident)).thenReturn(false);
+
+ try (MockedStatic<AuthorizationUtils> authz =
Mockito.mockStatic(AuthorizationUtils.class)) {
+ Assertions.assertFalse(hook.dropTopic(ident));
+ authz.verify(
+ () ->
+ AuthorizationUtils.authorizationPluginRemovePrivileges(
+ ident, Entity.EntityType.TOPIC, null),
+ Mockito.never());
+ }
+ }
+
@Test
public void testCreateTopicSetsOwnerWithNormalizedIdentifier() throws
Exception {
// Self-contained: use a fresh hook with a directly-mocked TopicDispatcher
and a case-
