jerryshao commented on code in PR #4055:
URL: https://github.com/apache/gravitino/pull/4055#discussion_r1759624250
##########
core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java:
##########
@@ -109,6 +113,25 @@ User getUser(String metalake, String user) throws
NoSuchUserException {
}
}
+ String[] listUserNames(String metalake) {
+ return
Arrays.stream(listUsers(metalake)).map(User::name).toArray(String[]::new);
+ }
+
+ User[] listUsers(String metalake) {
+ try {
+ Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake);
+ return store.list(namespace, UserEntity.class,
Entity.EntityType.USER).stream()
+ .map(entity -> (User) entity)
Review Comment:
So you only get the user list here, right? Will you also fetch the roles for
each user, I saw that `User` and `UserDTO` have `roles` fields, will this be
empty?
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java:
##########
@@ -294,4 +296,103 @@ public void testRemoveUser() {
Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE,
errorResponse.getCode());
Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResponse.getType());
}
+
+ @Test
+ public void testListUsernames() {
+ when(manager.listUserNames(any())).thenReturn(new String[] {"user"});
+
+ Response resp =
+ target("/metalakes/metalake1/users/")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .get();
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+
+ NameListResponse listResponse = resp.readEntity(NameListResponse.class);
+ Assertions.assertEquals(0, listResponse.getCode());
+
+ Assertions.assertEquals(1, listResponse.getNames().length);
+ Assertions.assertEquals("user", listResponse.getNames()[0]);
+
+ // Test to throw NoSuchMetalakeException
+ doThrow(new NoSuchMetalakeException("mock
error")).when(manager).listUserNames(any());
+ Response resp1 =
+ target("/metalakes/metalake1/users/")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .get();
+
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp1.getStatus());
+
+ ErrorResponse errorResponse = resp1.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE,
errorResponse.getCode());
+ Assertions.assertEquals(NoSuchMetalakeException.class.getSimpleName(),
errorResponse.getType());
+
+ // Test to throw internal RuntimeException
+ doThrow(new RuntimeException("mock
error")).when(manager).listUserNames(any());
+ Response resp3 =
+ target("/metalakes/metalake1/users")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .get();
+
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp3.getStatus());
+
+ ErrorResponse errorResponse2 = resp3.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE,
errorResponse2.getCode());
+ Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResponse2.getType());
+ }
+
+ @Test
+ public void testListUsers() {
+ User user = buildUser("user");
+ when(manager.listUsers(any())).thenReturn(new User[] {user});
+
+ Response resp =
+ target("/metalakes/metalake1/users/")
+ .queryParam("details", "true")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .get();
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+
+ UserListResponse listResponse = resp.readEntity(UserListResponse.class);
+ Assertions.assertEquals(0, listResponse.getCode());
+
+ Assertions.assertEquals(1, listResponse.getUsers().length);
+ Assertions.assertEquals(user.name(), listResponse.getUsers()[0].name());
+ Assertions.assertEquals(user.roles(), listResponse.getUsers()[0].roles());
+
+ // Test to throw NoSuchMetalakeException
+ doThrow(new NoSuchMetalakeException("mock
error")).when(manager).listUsers(any());
+ Response resp1 =
+ target("/metalakes/metalake1/users/")
+ .queryParam("details", "true")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .get();
+
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp1.getStatus());
+
+ ErrorResponse errorResponse = resp1.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE,
errorResponse.getCode());
+ Assertions.assertEquals(NoSuchMetalakeException.class.getSimpleName(),
errorResponse.getType());
+
+ // Test to throw internal RuntimeException
+ doThrow(new RuntimeException("mock error")).when(manager).listUsers(any());
+ Response resp3 =
+ target("/metalakes/metalake1/users")
+ .queryParam("details", "true")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .get();
+
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp3.getStatus());
+
+ ErrorResponse errorResponse2 = resp3.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE,
errorResponse2.getCode());
+ Assertions.assertEquals(RuntimeException.class.getSimpleName(),
errorResponse2.getType());
+ }
Review Comment:
I think you also need to add integration tests for this.
##########
core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java:
##########
@@ -109,6 +113,25 @@ User getUser(String metalake, String user) throws
NoSuchUserException {
}
}
+ String[] listUserNames(String metalake) {
+ return
Arrays.stream(listUsers(metalake)).map(User::name).toArray(String[]::new);
+ }
+
+ User[] listUsers(String metalake) {
+ try {
+ Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake);
+ return store.list(namespace, UserEntity.class,
Entity.EntityType.USER).stream()
+ .map(entity -> (User) entity)
Review Comment:
Why do we need this line?
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java:
##########
@@ -138,6 +138,21 @@ public String listUsersByRoleId(@Param("roleId") Long
roleId) {
+ " AND us.deleted_at = 0 AND re.deleted_at = 0";
}
+ public String listUserPOsByMetalake(@Param("metalakeName") String
metalakeName) {
+ return "SELECT ut.user_id as userId, ut.user_name as userName,"
+ + " ut.metalake_id as metalakeId,"
+ + " ut.audit_info as auditInfo,"
+ + " ut.current_version as currentVersion, ut.last_version as
lastVersion,"
+ + " ut.deleted_at as deletedAt"
+ + " FROM "
+ + USER_TABLE_NAME
+ + " ut JOIN "
+ + MetalakeMetaMapper.TABLE_NAME
+ + " mt ON ut.metalake_id = mt.metalake_id"
+ + " WHERE mt.metalake_name = #{metalakeName}"
+ + " AND ut.deleted_at = 0 AND mt.deleted_at = 0";
+ }
Review Comment:
This SQL will not throw an exception when metalake is not existed, it will
only return an empty list. In your definition, you have
`NoSuchMetalakeException` defined, to throw an such exception, you have to add
one more check to see if metalake is existed or not.
--
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]
