Copilot commented on code in PR #9205:
URL: https://github.com/apache/gravitino/pull/9205#discussion_r2562911956
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerIT.java:
##########
@@ -413,4 +414,46 @@ public void testOwnerWithException() {
// Cleanup
client.dropMetalake(metalakeNameE, true);
}
+
+ @Test
+ void testOwnerDrop() {
+ String metalakeNameE = RandomNameUtils.genRandomName("metalakeE");
+ String catalogNameE = RandomNameUtils.genRandomName("catalogE");
+ GravitinoMetalake metalake =
+ client.createMetalake(metalakeNameE, "metalake E comment",
Collections.emptyMap());
+ metalake.createCatalog(
+ catalogNameE, Catalog.Type.FILESET, "hadoop", "comment",
Collections.emptyMap());
+ // Create a user
+ String userName = "ownerUser";
+ metalake.addUser(userName);
+
+ // Now set the owner of catalog to the user
+ MetadataObject catalogObject =
+ MetadataObjects.of(Lists.newArrayList(catalogNameE),
MetadataObject.Type.CATALOG);
+ metalake.setOwner(catalogObject, userName, Owner.Type.USER);
+ Owner owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(userName, owner.name());
+
+ GravitinoClient ordinaryClient =
+ GravitinoClient.builder(serverUri).withMetalake(metalakeNameE).build();
+ // Drop the user
+ ordinaryClient.removeUser(userName);
+ // The owner should be removed as well
+ Owner ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
+ Assertions.assertEquals(null, ownerAfterDrop);
+
+ // create a Group
+ String groupName = "ownerGroup";
+ metalake.addGroup(groupName);
+ // Now set the owner of catalog to the group
+ metalake.setOwner(catalogObject, groupName, Owner.Type.GROUP);
+ owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(groupName, owner.name());
+
+ // Drop the group
+ ordinaryClient.removeGroup(groupName);
+ // The owner should be removed as well
+ ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
+ Assertions.assertEquals(null, ownerAfterDrop);
+ }
Review Comment:
The test `testOwnerDrop` is missing cleanup code. All other tests in this
file clean up by dropping the metalake at the end (e.g.,
`client.dropMetalake(metalakeNameE, true);`). This test should also include
cleanup to prevent resource leaks in the test environment.
Add cleanup at the end of the test:
```java
// Cleanup
client.dropMetalake(metalakeNameE, true);
```
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerIT.java:
##########
@@ -413,4 +414,46 @@ public void testOwnerWithException() {
// Cleanup
client.dropMetalake(metalakeNameE, true);
}
+
+ @Test
+ void testOwnerDrop() {
+ String metalakeNameE = RandomNameUtils.genRandomName("metalakeE");
+ String catalogNameE = RandomNameUtils.genRandomName("catalogE");
+ GravitinoMetalake metalake =
+ client.createMetalake(metalakeNameE, "metalake E comment",
Collections.emptyMap());
+ metalake.createCatalog(
+ catalogNameE, Catalog.Type.FILESET, "hadoop", "comment",
Collections.emptyMap());
+ // Create a user
+ String userName = "ownerUser";
+ metalake.addUser(userName);
+
+ // Now set the owner of catalog to the user
+ MetadataObject catalogObject =
+ MetadataObjects.of(Lists.newArrayList(catalogNameE),
MetadataObject.Type.CATALOG);
+ metalake.setOwner(catalogObject, userName, Owner.Type.USER);
+ Owner owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(userName, owner.name());
+
+ GravitinoClient ordinaryClient =
+ GravitinoClient.builder(serverUri).withMetalake(metalakeNameE).build();
+ // Drop the user
+ ordinaryClient.removeUser(userName);
+ // The owner should be removed as well
+ Owner ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
+ Assertions.assertEquals(null, ownerAfterDrop);
+
+ // create a Group
+ String groupName = "ownerGroup";
+ metalake.addGroup(groupName);
+ // Now set the owner of catalog to the group
+ metalake.setOwner(catalogObject, groupName, Owner.Type.GROUP);
+ owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(groupName, owner.name());
+
+ // Drop the group
+ ordinaryClient.removeGroup(groupName);
+ // The owner should be removed as well
+ ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
+ Assertions.assertEquals(null, ownerAfterDrop);
Review Comment:
Use `Assertions.assertNull(ownerAfterDrop)` instead of
`Assertions.assertEquals(null, ownerAfterDrop)` for better readability and more
informative failure messages. This is the standard JUnit 5 best practice for
checking null values.
```suggestion
Assertions.assertNull(ownerAfterDrop);
```
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerIT.java:
##########
@@ -413,4 +414,46 @@ public void testOwnerWithException() {
// Cleanup
client.dropMetalake(metalakeNameE, true);
}
+
+ @Test
+ void testOwnerDrop() {
+ String metalakeNameE = RandomNameUtils.genRandomName("metalakeE");
+ String catalogNameE = RandomNameUtils.genRandomName("catalogE");
+ GravitinoMetalake metalake =
+ client.createMetalake(metalakeNameE, "metalake E comment",
Collections.emptyMap());
+ metalake.createCatalog(
+ catalogNameE, Catalog.Type.FILESET, "hadoop", "comment",
Collections.emptyMap());
+ // Create a user
+ String userName = "ownerUser";
+ metalake.addUser(userName);
+
+ // Now set the owner of catalog to the user
+ MetadataObject catalogObject =
+ MetadataObjects.of(Lists.newArrayList(catalogNameE),
MetadataObject.Type.CATALOG);
+ metalake.setOwner(catalogObject, userName, Owner.Type.USER);
+ Owner owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(userName, owner.name());
+
+ GravitinoClient ordinaryClient =
+ GravitinoClient.builder(serverUri).withMetalake(metalakeNameE).build();
+ // Drop the user
+ ordinaryClient.removeUser(userName);
+ // The owner should be removed as well
+ Owner ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
+ Assertions.assertEquals(null, ownerAfterDrop);
+
+ // create a Group
+ String groupName = "ownerGroup";
+ metalake.addGroup(groupName);
+ // Now set the owner of catalog to the group
+ metalake.setOwner(catalogObject, groupName, Owner.Type.GROUP);
+ owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(groupName, owner.name());
+
+ // Drop the group
+ ordinaryClient.removeGroup(groupName);
+ // The owner should be removed as well
+ ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
+ Assertions.assertEquals(null, ownerAfterDrop);
Review Comment:
The `GravitinoClient` instance `ordinaryClient` is created but never closed.
Since `GravitinoClient` implements `Closeable`, it should be properly closed to
release resources. Consider using try-with-resources or explicitly calling
`ordinaryClient.close()` at the end of the test.
```suggestion
try (GravitinoClient ordinaryClient =
GravitinoClient.builder(serverUri).withMetalake(metalakeNameE).build()) {
// Drop the user
ordinaryClient.removeUser(userName);
// The owner should be removed as well
Owner ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
Assertions.assertEquals(null, ownerAfterDrop);
// create a Group
String groupName = "ownerGroup";
metalake.addGroup(groupName);
// Now set the owner of catalog to the group
metalake.setOwner(catalogObject, groupName, Owner.Type.GROUP);
owner = metalake.getOwner(catalogObject).get();
Assertions.assertEquals(groupName, owner.name());
// Drop the group
ordinaryClient.removeGroup(groupName);
// The owner should be removed as well
ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
Assertions.assertEquals(null, ownerAfterDrop);
}
```
##########
core/src/main/java/org/apache/gravitino/cache/ReverseIndexRules.java:
##########
@@ -73,8 +84,18 @@ public class ReverseIndexRules {
}
};
+ public static final ReverseIndexCache.ReverseIndexRule
GROUP_OWNERSHIP_REVERSE_RULE =
+ (entity, key, reverseIndexCache) -> {
+ GroupEntity groupEntity = (GroupEntity) entity;
+ // Handle Securable Objects -> Group reverse index, so the key type is
group and the value
+ // type is securable Object.
+ if (key.relationType() == SupportsRelationOperations.Type.OWNER_REL) {
+ reverseIndexCache.put(groupEntity.nameIdentifier(),
EntityType.GROUP, key);
+ }
+ };
+
/** * RoleEntity reverse index processor */
Review Comment:
Extra asterisk in the Javadoc comment. Should be `/** RoleEntity reverse
index processor */` instead of `/** * RoleEntity reverse index processor */`
```suggestion
/** RoleEntity reverse index processor */
```
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/OwnerIT.java:
##########
@@ -413,4 +414,46 @@ public void testOwnerWithException() {
// Cleanup
client.dropMetalake(metalakeNameE, true);
}
+
+ @Test
+ void testOwnerDrop() {
+ String metalakeNameE = RandomNameUtils.genRandomName("metalakeE");
+ String catalogNameE = RandomNameUtils.genRandomName("catalogE");
+ GravitinoMetalake metalake =
+ client.createMetalake(metalakeNameE, "metalake E comment",
Collections.emptyMap());
+ metalake.createCatalog(
+ catalogNameE, Catalog.Type.FILESET, "hadoop", "comment",
Collections.emptyMap());
+ // Create a user
+ String userName = "ownerUser";
+ metalake.addUser(userName);
+
+ // Now set the owner of catalog to the user
+ MetadataObject catalogObject =
+ MetadataObjects.of(Lists.newArrayList(catalogNameE),
MetadataObject.Type.CATALOG);
+ metalake.setOwner(catalogObject, userName, Owner.Type.USER);
+ Owner owner = metalake.getOwner(catalogObject).get();
+ Assertions.assertEquals(userName, owner.name());
+
+ GravitinoClient ordinaryClient =
+ GravitinoClient.builder(serverUri).withMetalake(metalakeNameE).build();
+ // Drop the user
+ ordinaryClient.removeUser(userName);
+ // The owner should be removed as well
+ Owner ownerAfterDrop = metalake.getOwner(catalogObject).orElse(null);
+ Assertions.assertEquals(null, ownerAfterDrop);
Review Comment:
Use `Assertions.assertNull(ownerAfterDrop)` instead of
`Assertions.assertEquals(null, ownerAfterDrop)` for better readability and more
informative failure messages. This is the standard JUnit 5 best practice for
checking null values.
--
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]
