This is an automated email from the ASF dual-hosted git repository.
roryqi 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 584890d261 [#10411] feat(core): Extend metadata object owner to
support group (#10848)
584890d261 is described below
commit 584890d261ed46ec31c843182d0dd38c1f766f59
Author: Bharath Krishna <[email protected]>
AuthorDate: Thu Apr 23 23:26:58 2026 -0700
[#10411] feat(core): Extend metadata object owner to support group (#10848)
### What changes were proposed in this pull request?
Remove the USER-only restriction in `OwnerManager.setOwner()` and add
GROUP support to `notifyOwnerChange()`, enabling groups to be set as
metadata object owners.
Changes:
- Remove `Preconditions.checkArgument(ownerType == Owner.Type.USER)`
guard in `OwnerManager.setOwner()`
- Extend `notifyOwnerChange()` to resolve group entity IDs
- Fix `GroupEntity` namespace in test (was using `ofUserNamespace`,
corrected to `ofGroupNamespace`)
- Replace `testGroupTypeOwnerNotSupported` with tests that verify group
ownership works
### Why are the changes needed?
The owner field was implicitly assumed to point to a User. In practice,
the logical owner of a resource (Schema, Table, etc.) is often a team
(Group) rather than an individual. The downstream layers (REST API,
DTOs, relational store) already support GROUP — only `OwnerManager` had
a guard blocking it.
Fix: #10411
### Does this PR introduce _any_ user-facing change?
Yes. The `setOwner` API now accepts `Owner.Type.GROUP` in addition to
`Owner.Type.USER`.
### How was this patch tested?
Unit tests in `TestOwnerManager`:
- `testGroupTypeOwner`: Sets group as owner, verifies, then replaces
with user owner (exercises `notifyOwnerChange` GROUP path)
- `testSetNonExistentGroupAsOwner`: Verifies `NotFoundException` for
non-existent group
---
.../integration/test/authorization/OwnerIT.java | 10 +++--
.../gravitino/authorization/OwnerManager.java | 41 ++++++++++++--------
.../gravitino/authorization/TestOwnerManager.java | 44 +++++++++++++++++-----
3 files changed, 66 insertions(+), 29 deletions(-)
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerIT.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerIT.java
index e76356f1e6..4427dcaf53 100644
---
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerIT.java
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerIT.java
@@ -447,12 +447,16 @@ public class OwnerIT extends BaseIT {
metalake.addGroup(groupName);
// Now set the owner of catalog to the group
- Assertions.assertThrows(
- IllegalArgumentException.class,
- () -> metalake.setOwner(catalogObject, groupName, Owner.Type.GROUP));
+ metalake.setOwner(catalogObject, groupName, Owner.Type.GROUP);
+ Owner groupOwner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(groupName, groupOwner.name());
+ Assertions.assertEquals(Owner.Type.GROUP, groupOwner.type());
// Drop the group
ordinaryClient.removeGroup(groupName);
+ // The owner should be removed as well
+ Owner ownerAfterGroupDrop =
metalake.getOwner(catalogObject).orElse(null);
+ Assertions.assertNull(ownerAfterGroupDrop);
}
// Cleanup
client.disableMetalake(metalakeNameE);
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
b/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
index 5b7ce28c98..70e10f1488 100644
--- a/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
+++ b/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java
@@ -18,7 +18,6 @@
*/
package org.apache.gravitino.authorization;
-import com.google.common.base.Preconditions;
import java.io.IOException;
import java.util.List;
import java.util.Optional;
@@ -65,9 +64,6 @@ public class OwnerManager implements OwnerDispatcher {
@Override
public void setOwner(
String metalake, MetadataObject metadataObject, String ownerName,
Owner.Type ownerType) {
- // TODO(ISSUE-9375): Support GROUP type owner in the future.
- Preconditions.checkArgument(
- ownerType == Owner.Type.USER, "Only USER type is supported as owner
currently.");
NameIdentifier objectIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
try {
Optional<Owner> originOwner = getOwner(metalake, metadataObject);
@@ -113,6 +109,8 @@ public class OwnerManager implements OwnerDispatcher {
newOwner.name = ownerName;
newOwner.type = Owner.Type.GROUP;
+ } else {
+ throw new IllegalArgumentException("Unsupported owner type: " +
ownerType);
}
AuthorizationUtils.callAuthorizationPluginForMetadataObject(
@@ -139,8 +137,9 @@ public class OwnerManager implements OwnerDispatcher {
private void notifyOwnerChange(Owner oldOwner, String metalake,
MetadataObject metadataObject) {
GravitinoAuthorizer gravitinoAuthorizer =
GravitinoEnv.getInstance().gravitinoAuthorizer();
if (gravitinoAuthorizer != null) {
- if (oldOwner.type() == Owner.Type.USER) {
- try {
+ try {
+ Long oldOwnerId;
+ if (oldOwner.type() == Owner.Type.USER) {
UserEntity userEntity =
GravitinoEnv.getInstance()
.entityStore()
@@ -148,17 +147,27 @@ public class OwnerManager implements OwnerDispatcher {
NameIdentifierUtil.ofUser(metalake, oldOwner.name()),
Entity.EntityType.USER,
UserEntity.class);
- gravitinoAuthorizer.handleMetadataOwnerChange(
- metalake,
- userEntity.id(),
- MetadataObjectUtil.toEntityIdent(metalake, metadataObject),
- Entity.EntityType.valueOf(metadataObject.type().name()));
- } catch (IOException e) {
- LOG.warn(e.getMessage(), e);
+ oldOwnerId = userEntity.id();
+ } else if (oldOwner.type() == Owner.Type.GROUP) {
+ GroupEntity groupEntity =
+ GravitinoEnv.getInstance()
+ .entityStore()
+ .get(
+ NameIdentifierUtil.ofGroup(metalake, oldOwner.name()),
+ Entity.EntityType.GROUP,
+ GroupEntity.class);
+ oldOwnerId = groupEntity.id();
+ } else {
+ LOG.warn("Unsupported owner type: {}", oldOwner.type());
+ return;
}
- } else {
- throw new UnsupportedOperationException(
- "Notification for Group Owner is not supported yet.");
+ gravitinoAuthorizer.handleMetadataOwnerChange(
+ metalake,
+ oldOwnerId,
+ MetadataObjectUtil.toEntityIdent(metalake, metadataObject),
+ Entity.EntityType.valueOf(metadataObject.type().name()));
+ } catch (IOException e) {
+ LOG.warn(e.getMessage(), e);
}
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/authorization/TestOwnerManager.java
b/core/src/test/java/org/apache/gravitino/authorization/TestOwnerManager.java
index 1840a44251..56dd78f797 100644
---
a/core/src/test/java/org/apache/gravitino/authorization/TestOwnerManager.java
+++
b/core/src/test/java/org/apache/gravitino/authorization/TestOwnerManager.java
@@ -67,9 +67,13 @@ import org.apache.gravitino.storage.RandomIdGenerator;
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.Test;
+import org.junit.jupiter.api.TestMethodOrder;
import org.mockito.Mockito;
+@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
public class TestOwnerManager {
private static final String JDBC_STORE_PATH =
"/tmp/gravitino_jdbc_entityStore_" +
UUID.randomUUID().toString().replace("-", "");
@@ -152,7 +156,7 @@ public class TestOwnerManager {
.withName(GROUP)
.withRoleNames(Collections.emptyList())
.withRoleIds(Collections.emptyList())
- .withNamespace(AuthorizationUtils.ofUserNamespace(METALAKE))
+ .withNamespace(AuthorizationUtils.ofGroupNamespace(METALAKE))
.withAuditInfo(audit)
.build();
entityStore.put(groupEntity, false /* overwritten*/);
@@ -176,6 +180,7 @@ public class TestOwnerManager {
}
@Test
+ @Order(1)
public void testOwner() {
// Test no owner
MetadataObject metalakeObject =
@@ -205,18 +210,37 @@ public class TestOwnerManager {
}
@Test
- public void testGroupTypeOwnerNotSupported() {
- // Test that GROUP type owner is not supported and throws
IllegalArgumentException
+ @Order(2)
+ public void testGroupTypeOwner() {
MetadataObject metalakeObject =
MetadataObjects.of(Lists.newArrayList(METALAKE),
MetadataObject.Type.METALAKE);
- // Verify that setting GROUP type owner throws IllegalArgumentException
- IllegalArgumentException exception =
- Assertions.assertThrows(
- IllegalArgumentException.class,
- () -> ownerManager.setOwner(METALAKE, metalakeObject, GROUP,
Owner.Type.GROUP));
+ // Test to set the group as the owner
+ ownerManager.setOwner(METALAKE, metalakeObject, GROUP, Owner.Type.GROUP);
+ Mockito.verify(authorizationPlugin, Mockito.atLeastOnce())
+ .onOwnerSet(Mockito.any(), Mockito.any(), Mockito.any());
+
+ Owner owner = ownerManager.getOwner(METALAKE, metalakeObject).get();
+ Assertions.assertEquals(GROUP, owner.name());
+ Assertions.assertEquals(Owner.Type.GROUP, owner.type());
+
+ // Test replacing group owner with user owner
+ ownerManager.setOwner(METALAKE, metalakeObject, USER, Owner.Type.USER);
+ Owner replacedOwner = ownerManager.getOwner(METALAKE,
metalakeObject).get();
+ Assertions.assertEquals(USER, replacedOwner.name());
+ Assertions.assertEquals(Owner.Type.USER, replacedOwner.type());
+ }
- Assertions.assertEquals(
- "Only USER type is supported as owner currently.",
exception.getMessage());
+ @Test
+ @Order(3)
+ public void testSetNonExistentGroupAsOwner() {
+ MetadataObject metalakeObject =
+ MetadataObjects.of(Lists.newArrayList(METALAKE),
MetadataObject.Type.METALAKE);
+
+ Assertions.assertThrows(
+ NotFoundException.class,
+ () ->
+ ownerManager.setOwner(
+ METALAKE, metalakeObject, "non-existent-group",
Owner.Type.GROUP));
}
}
