Copilot commented on code in PR #9359:
URL: https://github.com/apache/gravitino/pull/9359#discussion_r2580840591
##########
core/src/test/java/org/apache/gravitino/authorization/TestOwnerManager.java:
##########
@@ -198,18 +198,50 @@ public void testOwner() {
Assertions.assertEquals(USER, owner.name());
Assertions.assertEquals(Owner.Type.USER, owner.type());
- // Test to set the group as the owner
- Mockito.reset(authorizationPlugin);
- ownerManager.setOwner(METALAKE, metalakeObject, GROUP, Owner.Type.GROUP);
- Mockito.verify(authorizationPlugin).onOwnerSet(Mockito.any(),
Mockito.any(), Mockito.any());
-
- // Test not-existed metadata object
+ // Test not-existed metadata object when setting owner
Assertions.assertThrows(
NotFoundException.class,
- () -> ownerManager.setOwner(METALAKE, notExistObject, GROUP,
Owner.Type.GROUP));
+ () -> ownerManager.setOwner(METALAKE, notExistObject, USER,
Owner.Type.USER));
+ }
+
+ @Test
+ public void testGroupTypeOwnerNotSupported() {
+ // Test that GROUP type owner is not supported and throws
IllegalArgumentException
+ 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));
- owner = ownerManager.getOwner(METALAKE, metalakeObject).get();
- Assertions.assertEquals(GROUP, owner.name());
- Assertions.assertEquals(Owner.Type.GROUP, owner.type());
+ Assertions.assertEquals(
+ "Only USER type is supported as owner currently.",
exception.getMessage());
+
+ // Verify that the authorization plugin was never called since validation
failed
+ Mockito.verify(authorizationPlugin, Mockito.never())
+ .onOwnerSet(Mockito.any(), Mockito.any(), Mockito.any());
+
+ // Verify that no owner was set for the metadata object
+ Assertions.assertFalse(ownerManager.getOwner(METALAKE,
metalakeObject).isPresent());
+ }
+
+ @Test
+ public void testUserTypeOwnerStillWorks() {
Review Comment:
[nitpick] The test name 'testUserTypeOwnerStillWorks' uses informal language
('StillWorks'). Consider renaming to 'testUserTypeOwnerIsSupported' or
'testSetUserTypeOwner' to follow conventional test naming patterns.
```suggestion
public void testSetUserTypeOwner() {
```
##########
core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java:
##########
@@ -66,6 +67,9 @@ public void setOwner(
String metalake, MetadataObject metadataObject, String ownerName,
Owner.Type ownerType) {
NameIdentifier objectIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
try {
+ // TODO: Support GROUP type owner in the future.
+ Preconditions.checkArgument(
+ ownerType == Owner.Type.USER, "Only USER type is supported as owner
currently.");
Review Comment:
The validation check should occur before calling
`MetadataObjectUtil.toEntityIdent()` on line 68. Moving the precondition to the
beginning of the method would fail fast and avoid unnecessary computation when
an invalid owner type is provided.
```suggestion
// TODO: 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 {
```
--
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]
