Copilot commented on code in PR #11882:
URL: https://github.com/apache/gravitino/pull/11882#discussion_r3732983984
##########
core/src/main/java/org/apache/gravitino/listener/AccessControlEventDispatcher.java:
##########
@@ -342,6 +355,43 @@ public User[] listUsers(String metalake) throws
NoSuchMetalakeException {
}
}
+ /** {@inheritDoc} */
+ @Override
+ public PagedResult<User> listUsers(String metalake, int offset, int limit)
+ throws NoSuchMetalakeException {
+ String initiator = PrincipalUtils.getCurrentUserName();
+
+ eventBus.dispatchEvent(new ListUsersPagedPreEvent(initiator, metalake,
offset, limit));
Review Comment:
The PR description says the event dispatcher should be pass-through with “no
new events,” but this path emits new pre/success/failure events and the patch
adds corresponding `OperationType` values and public event classes. Either keep
these methods pass-through as specified, or update the stated scope and justify
the listener API expansion.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java:
##########
@@ -472,4 +473,42 @@ public boolean deleteUserById(String metalake, long
userId) {
userId, Entity.EntityType.USER.name())));
return true;
}
+
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "countUsersByMetalake")
+ public long countUsersByMetalake(String metalakeName) {
+ Long count =
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class, mapper ->
mapper.countUserMetasByMetalakeName(metalakeName));
+ return count == null ? 0L : count;
Review Comment:
The new dispatcher contract says a missing metalake throws
`NoSuchMetalakeException`, but this aggregate returns `0` when the join finds
no metalake. `listUsersByMetalakePaginated` also calls this method and then
returns an empty page, so both user APIs silently treat a nonexistent metalake
as an empty one. Resolve the metalake first (as `listUsersByNamespace` does),
translate the resulting `NoSuchEntityException` in `UserGroupManager`, and
cover this path.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/GroupMetaService.java:
##########
@@ -484,4 +485,42 @@ public boolean deleteGroupById(String metalake, long
groupId) {
groupId, Entity.EntityType.GROUP.name())));
return true;
}
+
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "countGroupsByMetalake")
+ public long countGroupsByMetalake(String metalakeName) {
+ Long count =
+ SessionUtils.getWithoutCommit(
+ GroupMetaMapper.class, mapper ->
mapper.countGroupMetasByMetalakeName(metalakeName));
+ return count == null ? 0L : count;
Review Comment:
The new dispatcher contract says a missing metalake throws
`NoSuchMetalakeException`, but this aggregate returns `0` when the join finds
no metalake. `listGroupsByMetalakePaginated` also calls this method and then
returns an empty page, so both group APIs silently treat a nonexistent metalake
as an empty one. Resolve the metalake first, translate the resulting
`NoSuchEntityException` in `UserGroupManager`, and cover this path.
##########
core/src/main/java/org/apache/gravitino/authorization/PagedResult.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.authorization;
+
+import java.util.Collections;
+import java.util.List;
+
+/** A paginated query result containing the total count and a page of items. */
+public final class PagedResult<T> {
Review Comment:
The PR and linked issue explicitly place `PagedResult<T>` in the `api`
module, but this file is under `core`. Consequently, consumers of the API
artifact cannot reference the promised result type. Move the class to
`api/src/main/java/org/apache/gravitino/authorization` and apply the API
stability conventions used by neighboring authorization types.
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java:
##########
@@ -1100,6 +1101,30 @@ void testGroupExtDel() throws IOException {
assertThrowsExt(NoSuchEntityException.class, () ->
svc.deleteGroup(group.nameIdentifier()));
}
+ @TestTemplate
+ void testGroupPagination() throws IOException {
+ createAndInsertMakeLake(metalakeName);
+ GroupMetaService svc = GroupMetaService.getInstance();
+ svc.insertGroup(
+ createGroupEntity(
+ RandomIdGenerator.INSTANCE.nextId(),
+ AuthorizationUtils.ofGroupNamespace(metalakeName),
+ "g1",
+ AUDIT_INFO),
+ false);
+
+ Assertions.assertEquals(1, svc.countGroupsByMetalake(metalakeName));
+
+ PagedResult<GroupEntity> page =
svc.listGroupsByMetalakePaginated(metalakeName, 0, 10);
Review Comment:
This test never exercises JDBC `OFFSET`: with one row, offset 10 returns
before the paginated SQL runs, while offset 0 cannot detect ignored or unstable
ordering. Add several groups, request a nonzero offset with a smaller limit,
and assert exact IDs/names on repeated calls; also verify role data because the
new query introduces role joins.
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestUserMetaService.java:
##########
@@ -1262,6 +1263,23 @@ void batchGetAuthSubjectsForUser() throws IOException {
assertTrue(none.isEmpty());
}
+ @TestTemplate
+ void testUserPagination() throws IOException {
+ UserMetaService svc = userMetaService();
+ UserEntity user = userWithExtId("u1", "ext-page-1");
+ svc.insertUser(user, false);
+
+ Assertions.assertEquals(1, svc.countUsersByMetalake(metalakeName));
+
+ PagedResult<UserEntity> page =
svc.listUsersByMetalakePaginated(metalakeName, 0, 10);
Review Comment:
This test never exercises JDBC `OFFSET`: with one row, offset 10 returns
before the paginated SQL runs, while offset 0 cannot detect ignored or unstable
ordering. Add several users, request a nonzero offset with a smaller limit, and
assert exact IDs/names on repeated calls; also verify role data because the new
query introduces role joins.
--
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]
