diff --git a/Documentation/rest-api-groups.txt b/Documentation/rest-api-groups.txt index a3bcab1078..49d5ee5e39 100644 --- a/Documentation/rest-api-groups.txt +++ b/Documentation/rest-api-groups.txt @@ -1518,7 +1518,9 @@ The group member that is added/removed. If `type` is `ADD_USER` or `REMOVE_USER` the member is returned as detailed link:rest-api-accounts.html#account-info[AccountInfo] entity, if `type` is `ADD_GROUP` or `REMOVE_GROUP` the member is returned as -link:#group-info[GroupInfo] entity. +link:#group-info[GroupInfo] entity. Note that the `name` in +link:#group-info[GroupInfo] will not be set if the member group is not +available. |`type` | The event type, can be: `ADD_USER`, `REMOVE_USER`, `ADD_GROUP` or `REMOVE_GROUP`. diff --git a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java index 2c583c6d62..51fffbbcad 100644 --- a/java/com/google/gerrit/server/restapi/group/GetAuditLog.java +++ b/java/com/google/gerrit/server/restapi/group/GetAuditLog.java @@ -117,10 +117,12 @@ public class GetAuditLog implements RestReadView { if (includedGroup.isPresent()) { member = groupJson.format(new InternalGroupDescription(includedGroup.get())); } else { - GroupDescription.Basic groupDescription = groupBackend.get(includedGroupUUID); member = new GroupInfo(); member.id = Url.encode(includedGroupUUID.get()); - member.name = groupDescription.getName(); + GroupDescription.Basic groupDescription = groupBackend.get(includedGroupUUID); + if (groupDescription != null) { + member.name = groupDescription.getName(); + } } auditEvents.add( diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index 32dc5ddbe7..8aa03a4f2a 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -280,7 +280,7 @@ public class GroupsIT extends AbstractDaemonTest { List auditEvents = gApi.groups().id(g).auditLog(); assertThat(auditEvents).hasSize(1); - assertAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, "Registered Users"); + assertSubgroupAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, "Registered Users"); } @Test @@ -857,41 +857,41 @@ public class GroupsIT extends AbstractDaemonTest { GroupApi g = gApi.groups().create(name("group")); List auditEvents = g.auditLog(); assertThat(auditEvents).hasSize(1); - assertAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, admin.id); + assertMemberAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, admin.id); g.addMembers(user.username); auditEvents = g.auditLog(); assertThat(auditEvents).hasSize(2); - assertAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id); + assertMemberAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id); g.removeMembers(user.username); auditEvents = g.auditLog(); assertThat(auditEvents).hasSize(3); - assertAuditEvent(auditEvents.get(0), Type.REMOVE_USER, admin.id, user.id); + assertMemberAuditEvent(auditEvents.get(0), Type.REMOVE_USER, admin.id, user.id); String otherGroup = name("otherGroup"); gApi.groups().create(otherGroup); g.addGroups(otherGroup); auditEvents = g.auditLog(); assertThat(auditEvents).hasSize(4); - assertAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup); + assertSubgroupAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup); g.removeGroups(otherGroup); auditEvents = g.auditLog(); assertThat(auditEvents).hasSize(5); - assertAuditEvent(auditEvents.get(0), Type.REMOVE_GROUP, admin.id, otherGroup); + assertSubgroupAuditEvent(auditEvents.get(0), Type.REMOVE_GROUP, admin.id, otherGroup); // Add a removed member back again. g.addMembers(user.username); auditEvents = g.auditLog(); assertThat(auditEvents).hasSize(6); - assertAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id); + assertMemberAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id); // Add a removed group back again. g.addGroups(otherGroup); auditEvents = g.auditLog(); assertThat(auditEvents).hasSize(7); - assertAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup); + assertSubgroupAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup); Timestamp lastDate = null; for (GroupAuditEventInfo auditEvent : auditEvents) { @@ -902,6 +902,52 @@ public class GroupsIT extends AbstractDaemonTest { } } + /** + * @Sandboxed is used by this test because it deletes a group reference which introduces an + * inconsistency for the group storage. Once group deletion is supported, this test should be + * updated to use the API instead. + */ + @Test + @Sandboxed + @IgnoreGroupInconsistencies + public void getAuditLogAfterDeletingASubgroup() throws Exception { + assume().that(readGroupsFromNoteDb()).isTrue(); + + GroupInfo parentGroup = gApi.groups().create(name("parent-group")).get(); + + // Creates a subgroup and adds it to "parent-group" as a subgroup. + GroupInfo subgroup = gApi.groups().create(name("sub-group")).get(); + gApi.groups().id(parentGroup.id).addGroups(subgroup.id); + + // Deletes the subgroup. + deleteGroupRef(subgroup.id); + + List auditEvents = gApi.groups().id(parentGroup.id).auditLog(); + assertThat(auditEvents).hasSize(2); + // Verify the unavailable subgroup's name is null. + assertSubgroupAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, null); + } + + private void deleteGroupRef(String groupId) throws Exception { + AccountGroup.UUID uuid = new AccountGroup.UUID(groupId); + try (Repository repo = repoManager.openRepository(allUsers)) { + RefUpdate ru = repo.updateRef(RefNames.refsGroups(uuid)); + ru.setForceUpdate(true); + ru.setNewObjectId(ObjectId.zeroId()); + assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED); + } + + // Reindex the group. + gApi.groups().id(uuid.get()).index(); + + // Verify "sub-group" has been deleted. + try { + gApi.groups().id(uuid.get()).get(); + fail(); + } catch (ResourceNotFoundException e) { + } + } + // reindex is tested by {@link AbstractQueryGroupsTest#reindex} @Test public void reindexPermissions() throws Exception { @@ -1411,7 +1457,7 @@ public class GroupsIT extends AbstractDaemonTest { } } - private void assertAuditEvent( + private void assertMemberAuditEvent( GroupAuditEventInfo info, Type expectedType, Account.Id expectedUser, @@ -1422,7 +1468,7 @@ public class GroupsIT extends AbstractDaemonTest { assertThat(((UserMemberAuditEventInfo) info).member._accountId).isEqualTo(expectedMember.get()); } - private void assertAuditEvent( + private void assertSubgroupAuditEvent( GroupAuditEventInfo info, Type expectedType, Account.Id expectedUser,