From f80d44ccffc9259c5c23d4ae1859f625f595e8d8 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 8 Nov 2017 14:28:58 -0500 Subject: [PATCH 1/4] Implement group audit log in NoteDb In the GetAuditLog endpoint, AccountGroupMemberAudit logs are read and processed first followed by AccountGroupByIdAud logs. In the end, logs are ordered from new -> old. Currently, we only return logs created by real users and logs created by server are skipped. The creator of a log is decided by the Author of the commit. We may revisit this behavior after groups are fully migrated to NoteDb. Change-Id: I08e335931cefa0e99a82917e1902794980cc3d07 --- .../google/gerrit/server/git/CommitUtil.java | 12 +- .../gerrit/server/group/GetAuditLog.java | 10 +- .../server/group/db/AuditLogReader.java | 274 +++++++++++++ .../gerrit/server/group/db/GroupConfig.java | 15 +- .../google/gerrit/server/group/db/Groups.java | 33 +- .../group/db/testing/GroupTestUtil.java | 13 +- .../gerrit/server/project/GetCommit.java | 3 +- .../gerrit/acceptance/api/group/GroupsIT.java | 23 +- .../server/group/db/AbstractGroupTest.java | 143 +++++++ .../server/group/db/AuditLogReaderTest.java | 381 ++++++++++++++++++ .../server/group/db/GroupRebuilderTest.java | 74 +--- 11 files changed, 906 insertions(+), 75 deletions(-) create mode 100644 java/com/google/gerrit/server/group/db/AuditLogReader.java create mode 100644 javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java create mode 100644 javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java index fd512a5079..b0f10f2bc8 100644 --- a/java/com/google/gerrit/server/git/CommitUtil.java +++ b/java/com/google/gerrit/server/git/CommitUtil.java @@ -14,14 +14,22 @@ package com.google.gerrit.server.git; +import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.common.CommitInfo; import com.google.gerrit.server.CommonConverters; +import java.io.IOException; import java.util.ArrayList; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; /** Static utilities for working with {@link RevCommit}s. */ public class CommitUtil { - public static CommitInfo toCommitInfo(RevCommit commit) { + public static CommitInfo toCommitInfo(RevCommit commit) throws IOException { + return toCommitInfo(commit, null); + } + + public static CommitInfo toCommitInfo(RevCommit commit, @Nullable RevWalk walk) + throws IOException { CommitInfo info = new CommitInfo(); info.commit = commit.getName(); info.author = CommonConverters.toGitPerson(commit.getAuthorIdent()); @@ -30,7 +38,7 @@ public class CommitUtil { info.message = commit.getFullMessage(); info.parents = new ArrayList<>(commit.getParentCount()); for (int i = 0; i < commit.getParentCount(); i++) { - RevCommit p = commit.getParent(i); + RevCommit p = walk == null ? commit.getParent(i) : walk.parseCommit(commit.getParent(i)); CommitInfo parentInfo = new CommitInfo(); parentInfo.commit = p.getName(); parentInfo.subject = p.getShortMessage(); diff --git a/java/com/google/gerrit/server/group/GetAuditLog.java b/java/com/google/gerrit/server/group/GetAuditLog.java index ebada0b092..58a057be8c 100644 --- a/java/com/google/gerrit/server/group/GetAuditLog.java +++ b/java/com/google/gerrit/server/group/GetAuditLog.java @@ -36,10 +36,12 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Optional; +import org.eclipse.jgit.errors.ConfigInvalidException; @Singleton public class GetAuditLog implements RestReadView { @@ -68,7 +70,8 @@ public class GetAuditLog implements RestReadView { @Override public List apply(GroupResource rsrc) - throws AuthException, MethodNotAllowedException, OrmException { + throws AuthException, MethodNotAllowedException, OrmException, IOException, + ConfigInvalidException { GroupDescription.Internal group = rsrc.asInternalGroup().orElseThrow(MethodNotAllowedException::new); if (!rsrc.getControl().isOwner()) { @@ -123,8 +126,9 @@ public class GetAuditLog implements RestReadView { accountLoader.fill(); - // sort by date in reverse order so that the newest audit event comes first - Collections.sort(auditEvents, comparing((GroupAuditEventInfo a) -> a.date).reversed()); + // sort by date and then reverse so that the newest audit event comes first + Collections.sort(auditEvents, comparing((GroupAuditEventInfo a) -> a.date)); + Collections.reverse(auditEvents); return auditEvents; } diff --git a/java/com/google/gerrit/server/group/db/AuditLogReader.java b/java/com/google/gerrit/server/group/db/AuditLogReader.java new file mode 100644 index 0000000000..3ab91dde4f --- /dev/null +++ b/java/com/google/gerrit/server/group/db/AuditLogReader.java @@ -0,0 +1,274 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed 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 com.google.gerrit.server.group.db; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.MultimapBuilder; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; +import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.config.GerritServerId; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.notedb.NoteDbUtil; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.io.IOException; +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.FooterLine; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevSort; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.util.RawParseUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** NoteDb reader for group audit log. */ +@Singleton +class AuditLogReader { + private static final Logger log = LoggerFactory.getLogger(AuditLogReader.class); + + private final String serverId; + private final GitRepositoryManager repoManager; + private final AllUsersName allUsers; + + @Inject + AuditLogReader( + @GerritServerId String serverId, GitRepositoryManager repoManager, AllUsersName allUsers) { + this.serverId = serverId; + this.repoManager = repoManager; + this.allUsers = allUsers; + } + + // Having separate methods for reading the two types of audit records mirrors the split in + // ReviewDb. Once ReviewDb is gone, the audit record interface becomes more flexible and we can + // revisit this, e.g. to do only a single walk, or even change the record types. + + ImmutableList getMembersAudit(AccountGroup.UUID uuid) + throws IOException, ConfigInvalidException { + return new MembersAuditLogParser().parseAuditLog(uuid); + } + + ImmutableList getSubgroupsAudit(AccountGroup.UUID uuid) + throws IOException, ConfigInvalidException { + return new SubgroupsAuditLogParser().parseAuditLog(uuid); + } + + private Optional parse(AccountGroup.UUID uuid, RevCommit c) { + Optional authorId = NoteDbUtil.parseIdent(c.getAuthorIdent(), serverId); + if (!authorId.isPresent()) { + // Only report audit events from identified users, since this is a non-nullable field in + // ReviewDb. May be revisited after groups are fully migrated to NoteDb. + return Optional.empty(); + } + + List addedMembers = new ArrayList<>(); + List addedSubgroups = new ArrayList<>(); + List removedMembers = new ArrayList<>(); + List removedSubgroups = new ArrayList<>(); + + for (FooterLine line : c.getFooterLines()) { + if (line.matches(GroupConfig.FOOTER_ADD_MEMBER)) { + parseAccount(uuid, c, line).ifPresent(addedMembers::add); + } else if (line.matches(GroupConfig.FOOTER_REMOVE_MEMBER)) { + parseAccount(uuid, c, line).ifPresent(removedMembers::add); + } else if (line.matches(GroupConfig.FOOTER_ADD_GROUP)) { + parseGroup(uuid, c, line).ifPresent(addedSubgroups::add); + } else if (line.matches(GroupConfig.FOOTER_REMOVE_GROUP)) { + parseGroup(uuid, c, line).ifPresent(removedSubgroups::add); + } + } + return Optional.of( + new AutoValue_AuditLogReader_ParsedCommit( + authorId.get(), + new Timestamp(c.getAuthorIdent().getWhen().getTime()), + ImmutableList.copyOf(addedMembers), + ImmutableList.copyOf(removedMembers), + ImmutableList.copyOf(addedSubgroups), + ImmutableList.copyOf(removedSubgroups))); + } + + private Optional parseAccount(AccountGroup.UUID uuid, RevCommit c, FooterLine line) { + Optional result = + Optional.ofNullable(RawParseUtils.parsePersonIdent(line.getValue())) + .flatMap(ident -> NoteDbUtil.parseIdent(ident, serverId)); + if (!result.isPresent()) { + logInvalid(uuid, c, line); + } + return result; + } + + private static Optional parseGroup( + AccountGroup.UUID uuid, RevCommit c, FooterLine line) { + PersonIdent ident = RawParseUtils.parsePersonIdent(line.getValue()); + if (ident == null) { + logInvalid(uuid, c, line); + return Optional.empty(); + } + return Optional.of(new AccountGroup.UUID(ident.getEmailAddress())); + } + + private static void logInvalid(AccountGroup.UUID uuid, RevCommit c, FooterLine line) { + log.debug( + "Invalid footer line in commit {} while parsing audit log for group {}: {}", + c.name(), + uuid, + line); + } + + private abstract class AuditLogParser { + final ImmutableList parseAuditLog(AccountGroup.UUID uuid) + throws IOException, ConfigInvalidException { + try (Repository repo = repoManager.openRepository(allUsers); + RevWalk rw = new RevWalk(repo)) { + Ref ref = repo.exactRef(RefNames.refsGroups(uuid)); + if (ref == null) { + return ImmutableList.of(); + } + + // TODO(dborowitz): This re-walks all commits just to find createdOn, which we don't need. + AccountGroup.Id groupId = + GroupConfig.loadForGroup(repo, uuid).getLoadedGroup().get().getId(); + + rw.reset(); + rw.markStart(rw.parseCommit(ref.getObjectId())); + rw.setRetainBody(true); + rw.sort(RevSort.COMMIT_TIME_DESC, true); + rw.sort(RevSort.REVERSE, true); + + ImmutableList.Builder result = ImmutableList.builder(); + RevCommit c; + while ((c = rw.next()) != null) { + parse(uuid, c).ifPresent(pc -> visit(groupId, pc, result)); + } + return result.build(); + } + } + + protected abstract void visit( + AccountGroup.Id groupId, ParsedCommit pc, ImmutableList.Builder result); + } + + private class MembersAuditLogParser extends AuditLogParser { + private ListMultimap audits = + MultimapBuilder.hashKeys().linkedListValues().build(); + + @Override + protected void visit( + AccountGroup.Id groupId, + ParsedCommit pc, + ImmutableList.Builder result) { + for (Account.Id id : pc.addedMembers()) { + MemberKey key = MemberKey.create(groupId, id); + AccountGroupMemberAudit audit = + new AccountGroupMemberAudit( + new AccountGroupMemberAudit.Key(id, groupId, pc.when()), pc.authorId()); + audits.put(key, audit); + result.add(audit); + } + for (Account.Id id : pc.removedMembers()) { + List adds = audits.get(MemberKey.create(groupId, id)); + if (!adds.isEmpty()) { + AccountGroupMemberAudit audit = adds.remove(0); + audit.removed(pc.authorId(), pc.when()); + } else { + // Match old behavior of DbGroupMemberAuditListener and add a "legacy" add/remove pair. + AccountGroupMemberAudit audit = + new AccountGroupMemberAudit( + new AccountGroupMemberAudit.Key(id, groupId, pc.when()), pc.authorId()); + audit.removedLegacy(); + result.add(audit); + } + } + } + } + + private class SubgroupsAuditLogParser extends AuditLogParser { + private ListMultimap audits = + MultimapBuilder.hashKeys().linkedListValues().build(); + + @Override + protected void visit( + AccountGroup.Id groupId, + ParsedCommit pc, + ImmutableList.Builder result) { + for (AccountGroup.UUID uuid : pc.addedSubgroups()) { + SubgroupKey key = SubgroupKey.create(groupId, uuid); + AccountGroupByIdAud audit = + new AccountGroupByIdAud( + new AccountGroupByIdAud.Key(groupId, uuid, pc.when()), pc.authorId()); + audits.put(key, audit); + result.add(audit); + } + for (AccountGroup.UUID uuid : pc.removedSubgroups()) { + List adds = audits.get(SubgroupKey.create(groupId, uuid)); + if (!adds.isEmpty()) { + AccountGroupByIdAud audit = adds.remove(0); + audit.removed(pc.authorId(), pc.when()); + } else { + // Unlike members, DbGroupMemberAuditListener didn't insert an add/remove pair here. + } + } + } + } + + @AutoValue + abstract static class MemberKey { + static MemberKey create(AccountGroup.Id groupId, Account.Id memberId) { + return new AutoValue_AuditLogReader_MemberKey(groupId, memberId); + } + + abstract AccountGroup.Id groupId(); + + abstract Account.Id memberId(); + } + + @AutoValue + abstract static class SubgroupKey { + static SubgroupKey create(AccountGroup.Id groupId, AccountGroup.UUID subgroupUuid) { + return new AutoValue_AuditLogReader_SubgroupKey(groupId, subgroupUuid); + } + + abstract AccountGroup.Id groupId(); + + abstract AccountGroup.UUID subgroupUuid(); + } + + @AutoValue + abstract static class ParsedCommit { + abstract Account.Id authorId(); + + abstract Timestamp when(); + + abstract ImmutableList addedMembers(); + + abstract ImmutableList removedMembers(); + + abstract ImmutableList addedSubgroups(); + + abstract ImmutableList removedSubgroups(); + } +} diff --git a/java/com/google/gerrit/server/group/db/GroupConfig.java b/java/com/google/gerrit/server/group/db/GroupConfig.java index 17ab34468b..c23dc09600 100644 --- a/java/com/google/gerrit/server/group/db/GroupConfig.java +++ b/java/com/google/gerrit/server/group/db/GroupConfig.java @@ -44,12 +44,19 @@ import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevSort; // TODO(aliceks): Add Javadoc descriptions to this file. public class GroupConfig extends VersionedMetaData { public static final String GROUP_CONFIG_FILE = "group.config"; + + static final FooterKey FOOTER_ADD_MEMBER = new FooterKey("Add"); + static final FooterKey FOOTER_REMOVE_MEMBER = new FooterKey("Remove"); + static final FooterKey FOOTER_ADD_GROUP = new FooterKey("Add-group"); + static final FooterKey FOOTER_REMOVE_GROUP = new FooterKey("Remove-group"); + private static final String MEMBERS_FILE = "members"; private static final String SUBGROUPS_FILE = "subgroups"; private static final Pattern LINE_SEPARATOR_PATTERN = Pattern.compile("\\R"); @@ -332,12 +339,12 @@ public class GroupConfig extends VersionedMetaData { Sets.difference(oldMembers, newMembers) .stream() .map(accountNameEmailRetriever) - .map("Remove: "::concat); + .map((FOOTER_REMOVE_MEMBER.getName() + ": ")::concat); Stream addedMembers = Sets.difference(newMembers, oldMembers) .stream() .map(accountNameEmailRetriever) - .map("Add: "::concat); + .map((FOOTER_ADD_MEMBER.getName() + ": ")::concat); return Stream.concat(removedMembers, addedMembers); } @@ -347,12 +354,12 @@ public class GroupConfig extends VersionedMetaData { Sets.difference(oldSubgroups, newSubgroups) .stream() .map(groupNameRetriever) - .map("Remove-group: "::concat); + .map((FOOTER_REMOVE_GROUP.getName() + ": ")::concat); Stream addedMembers = Sets.difference(newSubgroups, oldSubgroups) .stream() .map(groupNameRetriever) - .map("Add-group: "::concat); + .map((FOOTER_ADD_GROUP.getName() + ": ")::concat); return Stream.concat(removedMembers, addedMembers); } } diff --git a/java/com/google/gerrit/server/group/db/Groups.java b/java/com/google/gerrit/server/group/db/Groups.java index e1bd4abefc..7f63021f9c 100644 --- a/java/com/google/gerrit/server/group/db/Groups.java +++ b/java/com/google/gerrit/server/group/db/Groups.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.group.db; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static java.util.Comparator.comparing; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -39,6 +40,7 @@ import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.stream.Stream; @@ -63,15 +65,18 @@ public class Groups { private final GroupsMigration groupsMigration; private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; + private final AuditLogReader auditLogReader; @Inject public Groups( GroupsMigration groupsMigration, GitRepositoryManager repoManager, - AllUsersName allUsersName) { + AllUsersName allUsersName, + AuditLogReader auditLogReader) { this.groupsMigration = groupsMigration; this.repoManager = repoManager; this.allUsersName = allUsersName; + this.auditLogReader = auditLogReader; } /** @@ -311,18 +316,23 @@ public class Groups { * @param groupUuid the UUID of the group * @return the audit records, in arbitrary order; empty if the group does not exist * @throws OrmException if an error occurs while reading from ReviewDb + * @throws IOException if an error occurs while reading from NoteDb + * @throws ConfigInvalidException if the group couldn't be retrieved from NoteDb */ public List getMembersAudit(ReviewDb db, AccountGroup.UUID groupUuid) - throws OrmException { + throws OrmException, IOException, ConfigInvalidException { if (groupsMigration.readFromNoteDb()) { - // TODO(dborowitz): Implement. - throw new OrmException("Audit logs not yet implemented in NoteDb"); + return auditLogReader.getMembersAudit(groupUuid); } Optional group = getGroupFromReviewDb(db, groupUuid); if (!group.isPresent()) { return ImmutableList.of(); } - return db.accountGroupMembersAudit().byGroup(group.get().getId()).toList(); + + List audits = + db.accountGroupMembersAudit().byGroup(group.get().getId()).toList(); + Collections.sort(audits, comparing((AccountGroupMemberAudit a) -> a.getAddedOn())); + return audits; } /** @@ -332,17 +342,22 @@ public class Groups { * @param groupUuid the UUID of the group * @return the audit records, in arbitrary order; empty if the group does not exist * @throws OrmException if an error occurs while reading from ReviewDb + * @throws IOException if an error occurs while reading from NoteDb + * @throws ConfigInvalidException if the group couldn't be retrieved from NoteDb */ public List getSubgroupsAudit(ReviewDb db, AccountGroup.UUID groupUuid) - throws OrmException { + throws OrmException, IOException, ConfigInvalidException { if (groupsMigration.readFromNoteDb()) { - // TODO(dborowitz): Implement. - throw new OrmException("Audit logs not yet implemented in NoteDb"); + return auditLogReader.getSubgroupsAudit(groupUuid); } Optional group = getGroupFromReviewDb(db, groupUuid); if (!group.isPresent()) { return ImmutableList.of(); } - return db.accountGroupByIdAud().byGroup(group.get().getId()).toList(); + + List audits = + db.accountGroupByIdAud().byGroup(group.get().getId()).toList(); + Collections.sort(audits, comparing((AccountGroupByIdAud a) -> a.getAddedOn())); + return audits; } } diff --git a/java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java b/java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java index 7378b15302..a71f4172ef 100644 --- a/java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java +++ b/java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java @@ -24,6 +24,7 @@ import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.extensions.common.CommitInfo; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.git.CommitUtil; +import java.io.IOException; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.notes.Note; @@ -55,7 +56,17 @@ public class GroupTestUtil { if (ref != null) { rw.sort(RevSort.REVERSE); rw.markStart(rw.parseCommit(ref.getObjectId())); - return Streams.stream(rw).map(CommitUtil::toCommitInfo).collect(toImmutableList()); + return Streams.stream(rw) + .map( + c -> { + try { + return CommitUtil.toCommitInfo(c); + } catch (IOException e) { + throw new IllegalStateException( + "unexpected state when converting commit " + c.getName(), e); + } + }) + .collect(toImmutableList()); } } return ImmutableList.of(); diff --git a/java/com/google/gerrit/server/project/GetCommit.java b/java/com/google/gerrit/server/project/GetCommit.java index 2afeb074c5..d8fc5b65ec 100644 --- a/java/com/google/gerrit/server/project/GetCommit.java +++ b/java/com/google/gerrit/server/project/GetCommit.java @@ -18,12 +18,13 @@ import com.google.gerrit.extensions.common.CommitInfo; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.server.git.CommitUtil; import com.google.inject.Singleton; +import java.io.IOException; @Singleton public class GetCommit implements RestReadView { @Override - public CommitInfo apply(CommitResource rsrc) { + public CommitInfo apply(CommitResource rsrc) throws IOException { return CommitUtil.toCommitInfo(rsrc.getCommit()); } } diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index e52c9a6ca7..ba92835f28 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -778,7 +778,6 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void getAuditLog() throws Exception { - assume().that(cfg.getBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, false)).isFalse(); GroupApi g = gApi.groups().create(name("group")); List auditEvents = g.auditLog(); assertThat(auditEvents).hasSize(1); @@ -806,10 +805,30 @@ public class GroupsIT extends AbstractDaemonTest { assertThat(auditEvents).hasSize(5); assertAuditEvent(auditEvents.get(0), Type.REMOVE_GROUP, admin.id, otherGroup); + /** + * Make sure the new commit is created in a different second. This is added for NoteDb since the + * resolution of Timestamp is 1s there. Adding here is enough because the sort used in {@code + * GetAuditLog} is stable and we process {@code AccountGroupMemberAudit} before {@code + * AccountGroupByIdAud}. + */ + Thread.sleep(1000); + + // 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); + + // 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); + Timestamp lastDate = null; for (GroupAuditEventInfo auditEvent : auditEvents) { if (lastDate != null) { - assertThat(lastDate).isGreaterThan(auditEvent.date); + assertThat(lastDate).isAtLeast(auditEvent.date); } lastDate = auditEvent.date; } diff --git a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java new file mode 100644 index 0000000000..70d5bf6482 --- /dev/null +++ b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java @@ -0,0 +1,143 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed 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 com.google.gerrit.server.group.db; + +import static com.google.gerrit.extensions.common.testing.CommitInfoSubject.assertThat; + +import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.common.CommitInfo; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.config.AllUsersNameProvider; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.CommitUtil; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.testing.GerritBaseTests; +import com.google.gerrit.testing.InMemoryRepositoryManager; +import java.sql.Timestamp; +import java.util.TimeZone; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevWalk; +import org.junit.After; +import org.junit.Before; +import org.junit.Ignore; + +@Ignore +public class AbstractGroupTest extends GerritBaseTests { + protected static final TimeZone TZ = TimeZone.getTimeZone("America/Los_Angeles"); + protected static final String SERVER_ID = "server-id"; + protected static final String SERVER_NAME = "Gerrit Server"; + protected static final String SERVER_EMAIL = "noreply@gerritcodereview.com"; + protected static final int SERVER_ACCOUNT_NUMBER = 100000; + protected static final int USER_ACCOUNT_NUMBER = 100001; + + protected AllUsersName allUsersName; + protected InMemoryRepositoryManager repoManager; + protected Repository allUsersRepo; + protected Account.Id serverAccountId; + protected PersonIdent serverIdent; + protected Account.Id userId; + protected PersonIdent userIdent; + + @Before + public void abstractGroupTestSetUp() throws Exception { + allUsersName = new AllUsersName(AllUsersNameProvider.DEFAULT); + repoManager = new InMemoryRepositoryManager(); + allUsersRepo = repoManager.createRepository(allUsersName); + serverAccountId = new Account.Id(SERVER_ACCOUNT_NUMBER); + serverIdent = new PersonIdent(SERVER_NAME, SERVER_EMAIL, TimeUtil.nowTs(), TZ); + userId = new Account.Id(USER_ACCOUNT_NUMBER); + userIdent = newPersonIdent(userId, serverIdent); + } + + @After + public void abstractGroupTestTearDown() throws Exception { + allUsersRepo.close(); + } + + protected Timestamp getTipTimestamp(AccountGroup.UUID uuid) throws Exception { + try (RevWalk rw = new RevWalk(allUsersRepo)) { + Ref ref = allUsersRepo.exactRef(RefNames.refsGroups(uuid)); + return ref == null + ? null + : new Timestamp(rw.parseCommit(ref.getObjectId()).getAuthorIdent().getWhen().getTime()); + } + } + + protected void assertTipCommit(AccountGroup.UUID uuid, String expectedMessage) throws Exception { + try (RevWalk rw = new RevWalk(allUsersRepo)) { + Ref ref = allUsersRepo.exactRef(RefNames.refsGroups(uuid)); + assertCommit( + CommitUtil.toCommitInfo(rw.parseCommit(ref.getObjectId()), rw), + expectedMessage, + getAccountName(userId), + getAccountEmail(userId)); + } + } + + protected static void assertServerCommit(CommitInfo commitInfo, String expectedMessage) { + assertCommit(commitInfo, expectedMessage, SERVER_NAME, SERVER_EMAIL); + } + + protected static void assertCommit( + CommitInfo commitInfo, String expectedMessage, String expectedName, String expectedEmail) { + assertThat(commitInfo).message().isEqualTo(expectedMessage); + assertThat(commitInfo).author().name().isEqualTo(expectedName); + assertThat(commitInfo).author().email().isEqualTo(expectedEmail); + + // Committer should always be the server, regardless of author. + assertThat(commitInfo).committer().name().isEqualTo(SERVER_NAME); + assertThat(commitInfo).committer().email().isEqualTo(SERVER_EMAIL); + assertThat(commitInfo).committer().date().isEqualTo(commitInfo.author.date); + assertThat(commitInfo).committer().tz().isEqualTo(commitInfo.author.tz); + } + + protected MetaDataUpdate createMetaDataUpdate(PersonIdent authorIdent) { + MetaDataUpdate md = + new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, allUsersRepo); + md.getCommitBuilder().setAuthor(authorIdent); + md.getCommitBuilder().setCommitter(serverIdent); // Committer is always the server identity. + return md; + } + + protected static PersonIdent newPersonIdent() { + return new PersonIdent(SERVER_NAME, SERVER_EMAIL, TimeUtil.nowTs(), TZ); + } + + protected static PersonIdent newPersonIdent(Account.Id id, PersonIdent ident) { + return new PersonIdent( + getAccountName(id), getAccountEmail(id), ident.getWhen(), ident.getTimeZone()); + } + + protected static String getAccountNameEmail(Account.Id id) { + return String.format("%s <%s>", getAccountName(id), getAccountEmail(id)); + } + + protected static String getGroupName(AccountGroup.UUID uuid) { + return String.format("Group <%s>", uuid); + } + + protected static String getAccountName(Account.Id id) { + return "Account " + id; + } + + protected static String getAccountEmail(Account.Id id) { + return String.format("%s@%s", id, SERVER_ID); + } +} diff --git a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java new file mode 100644 index 0000000000..321580c4e8 --- /dev/null +++ b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java @@ -0,0 +1,381 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed 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 com.google.gerrit.server.group.db; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; +import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; +import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; +import com.google.gerrit.server.account.GroupUUID; +import com.google.gerrit.server.git.CommitUtil; +import com.google.gerrit.server.group.InternalGroup; +import java.sql.Timestamp; +import java.util.Set; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.revwalk.RevCommit; +import org.junit.Before; +import org.junit.Test; + +/** Unit tests for {@link AuditLogReader}. */ +public final class AuditLogReaderTest extends AbstractGroupTest { + + private AuditLogReader auditLogReader; + + @Before + public void setUp() throws Exception { + auditLogReader = new AuditLogReader(SERVER_ID, repoManager, allUsersName); + } + + @Test + public void createGroupAsUserIdent() throws Exception { + InternalGroup group = createGroupAsUser(1, "test-group"); + AccountGroup.UUID uuid = group.getGroupUUID(); + + AccountGroupMemberAudit expAudit = + createExpMemberAudit(group.getId(), userId, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getMembersAudit(uuid)).containsExactly(expAudit); + } + + @Test + public void createGroupAsServerIdent() throws Exception { + InternalGroup group = createGroup(1, "test-group", serverIdent, null); + assertThat(auditLogReader.getMembersAudit(group.getGroupUUID())).hasSize(0); + } + + @Test + public void addAndRemoveMember() throws Exception { + InternalGroup group = createGroupAsUser(1, "test-group"); + AccountGroup.UUID uuid = group.getGroupUUID(); + + AccountGroupMemberAudit expAudit1 = + createExpMemberAudit(group.getId(), userId, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getMembersAudit(uuid)).containsExactly(expAudit1); + + // User adds account 100002 to the group. + Account.Id id = new Account.Id(100002); + addMembers(uuid, ImmutableSet.of(id)); + + AccountGroupMemberAudit expAudit2 = + createExpMemberAudit(group.getId(), id, userId, getTipTimestamp(uuid)); + assertTipCommit(uuid, "Update group\n\nAdd: Account 100002 <100002@server-id>"); + assertThat(auditLogReader.getMembersAudit(uuid)) + .containsExactly(expAudit1, expAudit2) + .inOrder(); + + // User removes account 100002 from the group. + removeMembers(uuid, ImmutableSet.of(id)); + assertTipCommit(uuid, "Update group\n\nRemove: Account 100002 <100002@server-id>"); + + expAudit2.removed(userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getMembersAudit(uuid)) + .containsExactly(expAudit1, expAudit2) + .inOrder(); + } + + @Test + public void addMultiMembers() throws Exception { + InternalGroup group = createGroupAsUser(1, "test-group"); + AccountGroup.Id groupId = group.getId(); + AccountGroup.UUID uuid = group.getGroupUUID(); + + AccountGroupMemberAudit expAudit1 = + createExpMemberAudit(groupId, userId, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getMembersAudit(uuid)).containsExactly(expAudit1); + + Account.Id id1 = new Account.Id(100002); + Account.Id id2 = new Account.Id(100003); + addMembers(uuid, ImmutableSet.of(id1, id2)); + + AccountGroupMemberAudit expAudit2 = + createExpMemberAudit(groupId, id1, userId, getTipTimestamp(uuid)); + AccountGroupMemberAudit expAudit3 = + createExpMemberAudit(groupId, id2, userId, getTipTimestamp(uuid)); + + assertTipCommit( + uuid, + "Update group\n" + + "\n" + + "Add: Account 100002 <100002@server-id>\n" + + "Add: Account 100003 <100003@server-id>"); + assertThat(auditLogReader.getMembersAudit(uuid)) + .containsExactly(expAudit1, expAudit2, expAudit3) + .inOrder(); + } + + @Test + public void addAndRemoveSubgroups() throws Exception { + InternalGroup group = createGroupAsUser(1, "test-group"); + AccountGroup.UUID uuid = group.getGroupUUID(); + + InternalGroup subgroup = createGroupAsUser(2, "test-group-2"); + AccountGroup.UUID subgroupUuid = subgroup.getGroupUUID(); + + addSubgroups(uuid, ImmutableSet.of(subgroupUuid)); + assertTipCommit(uuid, String.format("Update group\n\nAdd-group: Group <%s>", subgroupUuid)); + + AccountGroupByIdAud expAudit = + createExpGroupAudit(group.getId(), subgroupUuid, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getSubgroupsAudit(uuid)).containsExactly(expAudit); + + removeSubgroups(uuid, ImmutableSet.of(subgroupUuid)); + assertTipCommit(uuid, String.format("Update group\n\nRemove-group: Group <%s>", subgroupUuid)); + + expAudit.removed(userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getSubgroupsAudit(uuid)).containsExactly(expAudit); + } + + @Test + public void addMultiSubgroups() throws Exception { + InternalGroup group = createGroupAsUser(1, "test-group"); + AccountGroup.UUID uuid = group.getGroupUUID(); + + InternalGroup subgroup1 = createGroupAsUser(2, "test-group-2"); + InternalGroup subgroup2 = createGroupAsUser(3, "test-group-3"); + AccountGroup.UUID subgroupUuid1 = subgroup1.getGroupUUID(); + AccountGroup.UUID subgroupUuid2 = subgroup2.getGroupUUID(); + + addSubgroups(uuid, ImmutableSet.of(subgroupUuid1, subgroupUuid2)); + + assertTipCommit( + uuid, + "Update group\n" + + "\n" + + String.format("Add-group: Group <%s>\n", subgroupUuid1) + + String.format("Add-group: Group <%s>", subgroupUuid2)); + + AccountGroupByIdAud expAudit1 = + createExpGroupAudit(group.getId(), subgroupUuid1, userId, getTipTimestamp(uuid)); + AccountGroupByIdAud expAudit2 = + createExpGroupAudit(group.getId(), subgroupUuid2, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getSubgroupsAudit(uuid)) + .containsExactly(expAudit1, expAudit2) + .inOrder(); + } + + @Test + public void addAndRemoveMembersAndSubgroups() throws Exception { + InternalGroup group = createGroupAsUser(1, "test-group"); + AccountGroup.Id groupId = group.getId(); + AccountGroup.UUID uuid = group.getGroupUUID(); + AccountGroupMemberAudit expMemberAudit = + createExpMemberAudit(groupId, userId, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getMembersAudit(uuid)).containsExactly(expMemberAudit); + + Account.Id id1 = new Account.Id(100002); + Account.Id id2 = new Account.Id(100003); + Account.Id id3 = new Account.Id(100004); + InternalGroup subgroup1 = createGroupAsUser(2, "test-group-2"); + InternalGroup subgroup2 = createGroupAsUser(3, "test-group-3"); + InternalGroup subgroup3 = createGroupAsUser(4, "test-group-4"); + AccountGroup.UUID subgroupUuid1 = subgroup1.getGroupUUID(); + AccountGroup.UUID subgroupUuid2 = subgroup2.getGroupUUID(); + AccountGroup.UUID subgroupUuid3 = subgroup3.getGroupUUID(); + + // Add two accounts. + addMembers(uuid, ImmutableSet.of(id1, id2)); + assertTipCommit( + uuid, + "Update group\n" + + "\n" + + String.format("Add: Account %s <%s@server-id>\n", id1, id1) + + String.format("Add: Account %s <%s@server-id>", id2, id2)); + AccountGroupMemberAudit expMemberAudit1 = + createExpMemberAudit(groupId, id1, userId, getTipTimestamp(uuid)); + AccountGroupMemberAudit expMemberAudit2 = + createExpMemberAudit(groupId, id2, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getMembersAudit(uuid)) + .containsExactly(expMemberAudit, expMemberAudit1, expMemberAudit2) + .inOrder(); + + // Add one subgroup. + addSubgroups(uuid, ImmutableSet.of(subgroupUuid1)); + assertTipCommit(uuid, String.format("Update group\n\nAdd-group: Group <%s>", subgroupUuid1)); + AccountGroupByIdAud expGroupAudit1 = + createExpGroupAudit(group.getId(), subgroupUuid1, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getSubgroupsAudit(uuid)).containsExactly(expGroupAudit1); + + // Remove one account. + removeMembers(uuid, ImmutableSet.of(id2)); + assertTipCommit( + uuid, String.format("Update group\n\nRemove: Account %s <%s@server-id>", id2, id2)); + expMemberAudit2.removed(userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getMembersAudit(uuid)) + .containsExactly(expMemberAudit, expMemberAudit1, expMemberAudit2) + .inOrder(); + + // Add two subgroups. + addSubgroups(uuid, ImmutableSet.of(subgroupUuid2, subgroupUuid3)); + assertTipCommit( + uuid, + "Update group\n" + + "\n" + + String.format("Add-group: Group <%s>\n", subgroupUuid2) + + String.format("Add-group: Group <%s>", subgroupUuid3)); + AccountGroupByIdAud expGroupAudit2 = + createExpGroupAudit(group.getId(), subgroupUuid2, userId, getTipTimestamp(uuid)); + AccountGroupByIdAud expGroupAudit3 = + createExpGroupAudit(group.getId(), subgroupUuid3, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getSubgroupsAudit(uuid)) + .containsExactly(expGroupAudit1, expGroupAudit2, expGroupAudit3) + .inOrder(); + + // Add two account, including a removed account. + addMembers(uuid, ImmutableSet.of(id2, id3)); + assertTipCommit( + uuid, + "Update group\n" + + "\n" + + String.format("Add: Account %s <%s@server-id>\n", id2, id2) + + String.format("Add: Account %s <%s@server-id>", id3, id3)); + AccountGroupMemberAudit expMemberAudit4 = + createExpMemberAudit(groupId, id2, userId, getTipTimestamp(uuid)); + AccountGroupMemberAudit expMemberAudit3 = + createExpMemberAudit(groupId, id3, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getMembersAudit(uuid)) + .containsExactly( + expMemberAudit, expMemberAudit1, expMemberAudit2, expMemberAudit4, expMemberAudit3) + .inOrder(); + + // Remove two subgroups. + removeSubgroups(uuid, ImmutableSet.of(subgroupUuid1, subgroupUuid3)); + assertTipCommit( + uuid, + "Update group\n" + + "\n" + + String.format("Remove-group: Group <%s>\n", subgroupUuid1) + + String.format("Remove-group: Group <%s>", subgroupUuid3)); + expGroupAudit1.removed(userId, getTipTimestamp(uuid)); + expGroupAudit3.removed(userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getSubgroupsAudit(uuid)) + .containsExactly(expGroupAudit1, expGroupAudit2, expGroupAudit3) + .inOrder(); + + // Add back one removed subgroup. + addSubgroups(uuid, ImmutableSet.of(subgroupUuid1)); + AccountGroupByIdAud expGroupAudit4 = + createExpGroupAudit(group.getId(), subgroupUuid1, userId, getTipTimestamp(uuid)); + assertThat(auditLogReader.getSubgroupsAudit(uuid)) + .containsExactly(expGroupAudit1, expGroupAudit2, expGroupAudit3, expGroupAudit4) + .inOrder(); + } + + private InternalGroup createGroupAsUser(int next, String groupName) throws Exception { + return createGroup(next, groupName, userIdent, userId); + } + + private InternalGroup createGroup( + int next, String groupName, PersonIdent authorIdent, Account.Id authorId) throws Exception { + InternalGroupCreation groupCreation = + InternalGroupCreation.builder() + .setGroupUUID(GroupUUID.make(groupName, serverIdent)) + .setNameKey(new AccountGroup.NameKey(groupName)) + .setId(new AccountGroup.Id(next)) + .setCreatedOn(TimeUtil.nowTs()) + .build(); + InternalGroupUpdate groupUpdate = + authorIdent.equals(serverIdent) + ? InternalGroupUpdate.builder().setDescription("Groups").build() + : InternalGroupUpdate.builder() + .setDescription("Groups") + .setMemberModification(members -> ImmutableSet.of(authorId)) + .build(); + + GroupConfig groupConfig = GroupConfig.createForNewGroup(allUsersRepo, groupCreation); + groupConfig.setGroupUpdate( + groupUpdate, AbstractGroupTest::getAccountNameEmail, AbstractGroupTest::getGroupName); + + RevCommit commit = groupConfig.commit(createMetaDataUpdate(authorIdent)); + assertCreateGroup(authorIdent, commit); + return groupConfig + .getLoadedGroup() + .orElseThrow(() -> new IllegalStateException("create group failed")); + } + + private void assertCreateGroup(PersonIdent authorIdent, RevCommit commit) throws Exception { + if (authorIdent.equals(serverIdent)) { + assertServerCommit(CommitUtil.toCommitInfo(commit), "Create group"); + } else { + assertCommit( + CommitUtil.toCommitInfo(commit), + String.format("Create group\n\nAdd: Account %s <%s@%s>", userId, userId, SERVER_ID), + getAccountName(userId), + getAccountEmail(userId)); + } + } + + private InternalGroup updateGroup(AccountGroup.UUID uuid, InternalGroupUpdate groupUpdate) + throws Exception { + GroupConfig groupConfig = GroupConfig.loadForGroup(allUsersRepo, uuid); + groupConfig.setGroupUpdate( + groupUpdate, AbstractGroupTest::getAccountNameEmail, AbstractGroupTest::getGroupName); + + groupConfig.commit(createMetaDataUpdate(userIdent)); + return groupConfig + .getLoadedGroup() + .orElseThrow(() -> new IllegalStateException("updated group failed")); + } + + private InternalGroup addMembers(AccountGroup.UUID groupUuid, Set ids) + throws Exception { + InternalGroupUpdate update = + InternalGroupUpdate.builder() + .setMemberModification(memberIds -> Sets.union(memberIds, ids)) + .build(); + return updateGroup(groupUuid, update); + } + + private InternalGroup removeMembers(AccountGroup.UUID groupUuid, Set ids) + throws Exception { + InternalGroupUpdate update = + InternalGroupUpdate.builder() + .setMemberModification(memberIds -> Sets.difference(memberIds, ids)) + .build(); + return updateGroup(groupUuid, update); + } + + private InternalGroup addSubgroups(AccountGroup.UUID groupUuid, Set uuids) + throws Exception { + InternalGroupUpdate update = + InternalGroupUpdate.builder() + .setSubgroupModification(memberIds -> Sets.union(memberIds, uuids)) + .build(); + return updateGroup(groupUuid, update); + } + + private InternalGroup removeSubgroups(AccountGroup.UUID groupUuid, Set uuids) + throws Exception { + InternalGroupUpdate update = + InternalGroupUpdate.builder() + .setSubgroupModification(memberIds -> Sets.difference(memberIds, uuids)) + .build(); + return updateGroup(groupUuid, update); + } + + private AccountGroupMemberAudit createExpMemberAudit( + AccountGroup.Id groupId, Account.Id id, Account.Id addedBy, Timestamp addedOn) { + return new AccountGroupMemberAudit( + new AccountGroupMemberAudit.Key(id, groupId, addedOn), addedBy); + } + + private AccountGroupByIdAud createExpGroupAudit( + AccountGroup.Id groupId, AccountGroup.UUID uuid, Account.Id addedBy, Timestamp addedOn) { + return new AccountGroupByIdAud(new AccountGroupByIdAud.Key(groupId, uuid, addedOn), addedBy); + } +} diff --git a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java index 4e9fc7ffd5..8efb8a8d20 100644 --- a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java +++ b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java @@ -16,7 +16,6 @@ package com.google.gerrit.server.group.db; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.extensions.common.testing.CommitInfoSubject.assertThat; import static com.google.gerrit.reviewdb.client.RefNames.REFS_GROUPNAMES; import static com.google.gerrit.server.group.db.GroupBundle.builder; @@ -31,17 +30,14 @@ import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; import com.google.gerrit.reviewdb.client.AccountGroupMember; import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; import com.google.gerrit.reviewdb.client.RefNames; -import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.AllUsersNameProvider; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.db.testing.GroupTestUtil; import com.google.gerrit.server.update.RefUpdateUtil; -import com.google.gerrit.testing.GerritBaseTests; import com.google.gerrit.testing.TestTimeUtil; import java.sql.Timestamp; -import java.util.TimeZone; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.IntStream; @@ -49,40 +45,33 @@ import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.ObjectInserter; -import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.junit.After; import org.junit.Before; import org.junit.Test; -public class GroupRebuilderTest extends GerritBaseTests { - private static final String SERVER_NAME = "Gerrit Server"; - private static final String SERVER_EMAIL = "noreply@gerritcodereview.com"; - private static final TimeZone TZ = TimeZone.getTimeZone("America/Los_Angeles"); - +public class GroupRebuilderTest extends AbstractGroupTest { private AtomicInteger idCounter; private Repository repo; private GroupRebuilder rebuilder; @Before - public void setUp() { + public void setUp() throws Exception { TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS); idCounter = new AtomicInteger(); repo = new InMemoryRepository(new DfsRepositoryDescription(AllUsersNameProvider.DEFAULT)); rebuilder = new GroupRebuilder( GroupRebuilderTest::newPersonIdent, - new AllUsersName(AllUsersNameProvider.DEFAULT), + allUsersName, (project, repo, batch) -> new MetaDataUpdate(GitReferenceUpdated.DISABLED, project, repo, batch), // Note that the expected name/email values in tests are not necessarily realistic, // since they use these trivial name/email functions. GroupRebuilderIT checks the actual // values. - (id, ident) -> - new PersonIdent( - "Account " + id, id + "@server-id", ident.getWhen(), ident.getTimeZone()), - id -> String.format("Account %s <%s@server-id>", id, id), - uuid -> "Group " + uuid); + AbstractGroupTest::newPersonIdent, + AbstractGroupTest::getAccountNameEmail, + AbstractGroupTest::getGroupName); } @After @@ -142,8 +131,8 @@ public class GroupRebuilderTest extends GerritBaseTests { + "\n" + "Add: Account 1 <1@server-id>\n" + "Add: Account 2 <2@server-id>\n" - + "Add-group: Group x\n" - + "Add-group: Group y"); + + "Add-group: Group \n" + + "Add-group: Group "); } @Test @@ -240,9 +229,9 @@ public class GroupRebuilderTest extends GerritBaseTests { ImmutableList log = log(g); assertThat(log).hasSize(4); assertServerCommit(log.get(0), "Create group"); - assertCommit(log.get(1), "Update group\n\nAdd-group: Group y", "Account 8", "8@server-id"); - assertCommit(log.get(2), "Update group\n\nAdd-group: Group x", "Account 8", "8@server-id"); - assertCommit(log.get(3), "Update group\n\nRemove-group: Group y", "Account 9", "9@server-id"); + assertCommit(log.get(1), "Update group\n\nAdd-group: Group ", "Account 8", "8@server-id"); + assertCommit(log.get(2), "Update group\n\nAdd-group: Group ", "Account 8", "8@server-id"); + assertCommit(log.get(3), "Update group\n\nRemove-group: Group ", "Account 9", "9@server-id"); } @Test @@ -261,8 +250,8 @@ public class GroupRebuilderTest extends GerritBaseTests { ImmutableList log = log(g); assertThat(log).hasSize(3); assertServerCommit(log.get(0), "Create group"); - assertCommit(log.get(1), "Update group\n\nAdd-group: Group x", "Account 8", "8@server-id"); - assertServerCommit(log.get(2), "Update group\n\nAdd-group: Group y\nAdd-group: Group z"); + assertCommit(log.get(1), "Update group\n\nAdd-group: Group ", "Account 8", "8@server-id"); + assertServerCommit(log.get(2), "Update group\n\nAdd-group: Group \nAdd-group: Group "); } @Test @@ -306,12 +295,12 @@ public class GroupRebuilderTest extends GerritBaseTests { log.get(3), "Update group\n" + "\n" - + "Add-group: Group x\n" - + "Add-group: Group y\n" - + "Add-group: Group z", + + "Add-group: Group \n" + + "Add-group: Group \n" + + "Add-group: Group ", "Account 8", "8@server-id"); - assertCommit(log.get(4), "Update group\n\nRemove-group: Group z", "Account 8", "8@server-id"); + assertCommit(log.get(4), "Update group\n\nRemove-group: Group ", "Account 8", "8@server-id"); } @Test @@ -345,12 +334,12 @@ public class GroupRebuilderTest extends GerritBaseTests { "8@server-id"); assertCommit( log.get(2), - "Update group\n\nAdd-group: Group x\nAdd-group: Group z", + "Update group\n\nAdd-group: Group \nAdd-group: Group ", "Account 8", "8@server-id"); assertCommit( log.get(3), "Update group\n\nAdd: Account 2 <2@server-id>", "Account 9", "9@server-id"); - assertCommit(log.get(4), "Update group\n\nAdd-group: Group y", "Account 9", "9@server-id"); + assertCommit(log.get(4), "Update group\n\nAdd-group: Group ", "Account 9", "9@server-id"); } @Test @@ -373,8 +362,8 @@ public class GroupRebuilderTest extends GerritBaseTests { ImmutableList log = log(g); assertThat(log).hasSize(3); assertServerCommit(log.get(0), "Create group"); - assertCommit(log.get(1), "Update group\n\nAdd-group: Group x", "Account 8", "8@server-id"); - assertServerCommit(log.get(2), "Update group\n\nAdd-group: Group y\nAdd-group: Group z"); + assertCommit(log.get(1), "Update group\n\nAdd-group: Group ", "Account 8", "8@server-id"); + assertServerCommit(log.get(2), "Update group\n\nAdd-group: Group \nAdd-group: Group "); assertThat(log.stream().map(c -> c.committer.date).collect(toImmutableList())) .named("%s", log) @@ -487,28 +476,7 @@ public class GroupRebuilderTest extends GerritBaseTests { return GroupTestUtil.log(repo, REFS_GROUPNAMES); } - private static void assertServerCommit(CommitInfo commitInfo, String expectedMessage) { - assertCommit(commitInfo, expectedMessage, SERVER_NAME, SERVER_EMAIL); - } - - private static void assertCommit( - CommitInfo commitInfo, String expectedMessage, String expectedName, String expectedEmail) { - assertThat(commitInfo).message().isEqualTo(expectedMessage); - assertThat(commitInfo).author().name().isEqualTo(expectedName); - assertThat(commitInfo).author().email().isEqualTo(expectedEmail); - - // Committer should always be the server, regardless of author. - assertThat(commitInfo).committer().name().isEqualTo(SERVER_NAME); - assertThat(commitInfo).committer().email().isEqualTo(SERVER_EMAIL); - assertThat(commitInfo).committer().date().isEqualTo(commitInfo.author.date); - assertThat(commitInfo).committer().tz().isEqualTo(commitInfo.author.tz); - } - private static InternalGroup removeRefState(InternalGroup group) throws Exception { return group.toBuilder().setRefState(null).build(); } - - private static PersonIdent newPersonIdent() { - return new PersonIdent(SERVER_NAME, SERVER_EMAIL, TimeUtil.nowTs(), TZ); - } } From 24ca2011909ee3417ba305cf33652eef2d9b825d Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 16 Nov 2017 10:27:18 -0500 Subject: [PATCH 2/4] GroupBundle: Check columns like we do for ChangeBundle Change-Id: I76efb02924efd873a576acda113ec6382cb64cc6 --- .../gerrit/reviewdb/server/ReviewDbUtil.java | 25 +++++++++++++++++++ .../gerrit/server/group/db/GroupBundle.java | 22 ++++++++++++++++ .../gerrit/server/notedb/ChangeBundle.java | 20 +-------------- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java b/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java index bb31b1cae0..e6f02701da 100644 --- a/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java +++ b/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java @@ -14,8 +14,16 @@ package com.google.gerrit.reviewdb.server; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.collect.Ordering; +import com.google.common.collect.Sets; +import com.google.gwtorm.client.Column; import com.google.gwtorm.client.IntKey; +import java.lang.reflect.Field; +import java.util.Arrays; +import java.util.Set; +import java.util.TreeSet; /** Static utilities for ReviewDb types. */ public class ReviewDbUtil { @@ -48,5 +56,22 @@ public class ReviewDbUtil { return db; } + public static void checkColumns(Class clazz, Integer... expected) { + Set ids = new TreeSet<>(); + for (Field f : clazz.getDeclaredFields()) { + Column col = f.getAnnotation(Column.class); + if (col != null) { + ids.add(col.id()); + } + } + Set expectedIds = Sets.newTreeSet(Arrays.asList(expected)); + checkState( + ids.equals(expectedIds), + "Unexpected column set for %s: %s != %s", + clazz.getSimpleName(), + ids, + expectedIds); + } + private ReviewDbUtil() {} } diff --git a/java/com/google/gerrit/server/group/db/GroupBundle.java b/java/com/google/gerrit/server/group/db/GroupBundle.java index f83f0942ce..7dd76b8240 100644 --- a/java/com/google/gerrit/server/group/db/GroupBundle.java +++ b/java/com/google/gerrit/server/group/db/GroupBundle.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.group.db; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.gerrit.reviewdb.server.ReviewDbUtil.checkColumns; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; @@ -37,6 +38,27 @@ import com.google.gwtorm.server.OrmException; */ @AutoValue public abstract class GroupBundle { + static { + // Initialization-time checks that the column set hasn't changed since the + // last time this file was updated. + checkColumns(AccountGroup.NameKey.class, 1); + checkColumns(AccountGroup.UUID.class, 1); + checkColumns(AccountGroup.Id.class, 1); + checkColumns(AccountGroup.class, 1, 2, 4, 7, 9, 10, 11); + + checkColumns(AccountGroupById.Key.class, 1, 2); + checkColumns(AccountGroupById.class, 1); + + checkColumns(AccountGroupByIdAud.Key.class, 1, 2, 3); + checkColumns(AccountGroupByIdAud.class, 1, 2, 3, 4); + + checkColumns(AccountGroupMember.Key.class, 1, 2); + checkColumns(AccountGroupMember.class, 1); + + checkColumns(AccountGroupMemberAudit.Key.class, 1, 2, 3); + checkColumns(AccountGroupMemberAudit.class, 1, 2, 3, 4); + } + public static GroupBundle fromReviewDb(ReviewDb db, AccountGroup.Id id) throws OrmException { AccountGroup group = db.accountGroups().get(id); if (group == null) { diff --git a/java/com/google/gerrit/server/notedb/ChangeBundle.java b/java/com/google/gerrit/server/notedb/ChangeBundle.java index a9663c735e..221252ca70 100644 --- a/java/com/google/gerrit/server/notedb/ChangeBundle.java +++ b/java/com/google/gerrit/server/notedb/ChangeBundle.java @@ -17,8 +17,8 @@ package com.google.gerrit.server.notedb; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.common.TimeUtil.roundToSecond; +import static com.google.gerrit.reviewdb.server.ReviewDbUtil.checkColumns; import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering; import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB; import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB; @@ -71,7 +71,6 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.TreeMap; -import java.util.TreeSet; /** * A bundle of all entities rooted at a single {@link Change} entity. @@ -212,23 +211,6 @@ public class ChangeBundle { .compare(a.get(), b.get()); } - private static void checkColumns(Class clazz, Integer... expected) { - Set ids = new TreeSet<>(); - for (Field f : clazz.getDeclaredFields()) { - Column col = f.getAnnotation(Column.class); - if (col != null) { - ids.add(col.id()); - } - } - Set expectedIds = Sets.newTreeSet(Arrays.asList(expected)); - checkState( - ids.equals(expectedIds), - "Unexpected column set for %s: %s != %s", - clazz.getSimpleName(), - ids, - expectedIds); - } - static { // Initialization-time checks that the column set hasn't changed since the // last time this file was updated. From 4910d807c4f6656650a5629777e040b3f73cc18e Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 16 Nov 2017 10:29:03 -0500 Subject: [PATCH 3/4] GroupBundle: Store sets instead of lists Bundles with the same set of members and audit records should compare equal regardless of ordering; the relevant types already implement hashCode and equals. Change-Id: I15a68a9557cdbd4066574e21d9f7f97e1f77376f --- .../gerrit/server/group/db/GroupBundle.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/java/com/google/gerrit/server/group/db/GroupBundle.java b/java/com/google/gerrit/server/group/db/GroupBundle.java index 7dd76b8240..80d083f84a 100644 --- a/java/com/google/gerrit/server/group/db/GroupBundle.java +++ b/java/com/google/gerrit/server/group/db/GroupBundle.java @@ -14,12 +14,11 @@ package com.google.gerrit.server.group.db; -import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.gerrit.reviewdb.server.ReviewDbUtil.checkColumns; import com.google.auto.value.AutoValue; -import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupById; @@ -101,13 +100,13 @@ public abstract class GroupBundle { public abstract AccountGroup group(); - public abstract ImmutableList members(); + public abstract ImmutableSet members(); - public abstract ImmutableList memberAudit(); + public abstract ImmutableSet memberAudit(); - public abstract ImmutableList byId(); + public abstract ImmutableSet byId(); - public abstract ImmutableList byIdAudit(); + public abstract ImmutableSet byIdAudit(); public abstract Builder toBuilder(); @@ -119,8 +118,8 @@ public abstract class GroupBundle { return toBuilder() .group(newGroup) .memberAudit( - memberAudit().stream().map(GroupBundle::roundToSecond).collect(toImmutableList())) - .byIdAudit(byIdAudit().stream().map(GroupBundle::roundToSecond).collect(toImmutableList())) + memberAudit().stream().map(GroupBundle::roundToSecond).collect(toImmutableSet())) + .byIdAudit(byIdAudit().stream().map(GroupBundle::roundToSecond).collect(toImmutableSet())) .build(); } From 3d332e19cc0d89ee197b1edd35f50343a322767f Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 16 Nov 2017 10:30:42 -0500 Subject: [PATCH 4/4] Compare full GroupBundles in GroupRebuilder tests Now that we can read the audit log from NoteDb, we can compile a full bundle from NoteDb and compare it directly to the original ReviewDb bundle using simple equality comparison. Convert GroupBundle to be constructed with a factory, so we can inject AuditLogReader, which is package-private in the db class. This is necessary so GroupRebuilderIT can read NoteDb bundles, and is preferable to exposing the AuditLogReader unnecessary. Add some toString methods to the ReviewDb types to make the failure messages readable. Change-Id: I10e17e091292b160afc46b44f89a523584a2e0e1 --- .../gerrit/reviewdb/client/AccountGroup.java | 21 ++++++ .../reviewdb/client/AccountGroupById.java | 5 ++ .../reviewdb/client/AccountGroupByIdAud.java | 15 ++++ .../reviewdb/client/AccountGroupMember.java | 5 ++ .../client/AccountGroupMemberAudit.java | 15 ++++ .../gerrit/server/group/db/GroupBundle.java | 71 ++++++++++++++++--- .../api/group/GroupRebuilderIT.java | 59 +++++---------- .../server/group/db/GroupRebuilderTest.java | 38 +++++----- 8 files changed, 158 insertions(+), 71 deletions(-) diff --git a/java/com/google/gerrit/reviewdb/client/AccountGroup.java b/java/com/google/gerrit/reviewdb/client/AccountGroup.java index 2dd5b36ad9..c7dc420fe8 100644 --- a/java/com/google/gerrit/reviewdb/client/AccountGroup.java +++ b/java/com/google/gerrit/reviewdb/client/AccountGroup.java @@ -286,4 +286,25 @@ public final class AccountGroup { return Objects.hash( name, groupId, description, visibleToAll, groupUUID, ownerGroupUUID, createdOn); } + + @Override + public String toString() { + return getClass().getSimpleName() + + "{" + + "name=" + + name + + ", groupId=" + + groupId + + ", description=" + + description + + ", visibleToAll=" + + visibleToAll + + ", groupUUID=" + + groupUUID + + ", ownerGroupUUID=" + + ownerGroupUUID + + ", createdOn=" + + createdOn + + "}"; + } } diff --git a/java/com/google/gerrit/reviewdb/client/AccountGroupById.java b/java/com/google/gerrit/reviewdb/client/AccountGroupById.java index 30ca38fb2d..17a205e1fc 100644 --- a/java/com/google/gerrit/reviewdb/client/AccountGroupById.java +++ b/java/com/google/gerrit/reviewdb/client/AccountGroupById.java @@ -88,4 +88,9 @@ public final class AccountGroupById { public int hashCode() { return key.hashCode(); } + + @Override + public String toString() { + return getClass().getSimpleName() + "{key=" + key + "}"; + } } diff --git a/java/com/google/gerrit/reviewdb/client/AccountGroupByIdAud.java b/java/com/google/gerrit/reviewdb/client/AccountGroupByIdAud.java index 33955c4dc5..5246d729a0 100644 --- a/java/com/google/gerrit/reviewdb/client/AccountGroupByIdAud.java +++ b/java/com/google/gerrit/reviewdb/client/AccountGroupByIdAud.java @@ -142,4 +142,19 @@ public final class AccountGroupByIdAud { public int hashCode() { return Objects.hash(key, addedBy, removedBy, removedOn); } + + @Override + public String toString() { + return getClass().getSimpleName() + + "{" + + "key=" + + key + + ", addedBy=" + + addedBy + + ", removedBy=" + + removedBy + + ", removedOn=" + + removedOn + + "}"; + } } diff --git a/java/com/google/gerrit/reviewdb/client/AccountGroupMember.java b/java/com/google/gerrit/reviewdb/client/AccountGroupMember.java index ea46366a29..e1e075414d 100644 --- a/java/com/google/gerrit/reviewdb/client/AccountGroupMember.java +++ b/java/com/google/gerrit/reviewdb/client/AccountGroupMember.java @@ -84,4 +84,9 @@ public final class AccountGroupMember { public int hashCode() { return key.hashCode(); } + + @Override + public String toString() { + return getClass().getSimpleName() + "{key=" + key + "}"; + } } diff --git a/java/com/google/gerrit/reviewdb/client/AccountGroupMemberAudit.java b/java/com/google/gerrit/reviewdb/client/AccountGroupMemberAudit.java index 9968b7dd1a..4ea19d20c6 100644 --- a/java/com/google/gerrit/reviewdb/client/AccountGroupMemberAudit.java +++ b/java/com/google/gerrit/reviewdb/client/AccountGroupMemberAudit.java @@ -147,4 +147,19 @@ public final class AccountGroupMemberAudit { public int hashCode() { return Objects.hash(key, addedBy, removedBy, removedOn); } + + @Override + public String toString() { + return getClass().getSimpleName() + + "{" + + "key=" + + key + + ", addedBy=" + + addedBy + + ", removedBy=" + + removedBy + + ", removedOn=" + + removedOn + + "}"; + } } diff --git a/java/com/google/gerrit/server/group/db/GroupBundle.java b/java/com/google/gerrit/server/group/db/GroupBundle.java index 80d083f84a..2f3b1189c0 100644 --- a/java/com/google/gerrit/server/group/db/GroupBundle.java +++ b/java/com/google/gerrit/server/group/db/GroupBundle.java @@ -28,6 +28,11 @@ import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.group.InternalGroup; import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Repository; /** * A bundle of all entities rooted at a single {@link AccountGroup} entity. @@ -58,17 +63,63 @@ public abstract class GroupBundle { checkColumns(AccountGroupMemberAudit.class, 1, 2, 3, 4); } - public static GroupBundle fromReviewDb(ReviewDb db, AccountGroup.Id id) throws OrmException { - AccountGroup group = db.accountGroups().get(id); - if (group == null) { - throw new OrmException("Group " + id + " not found"); + @Singleton + public static class Factory { + private final AuditLogReader auditLogReader; + + @Inject + Factory(AuditLogReader auditLogReader) { + this.auditLogReader = auditLogReader; + } + + public GroupBundle fromReviewDb(ReviewDb db, AccountGroup.Id id) throws OrmException { + AccountGroup group = db.accountGroups().get(id); + if (group == null) { + throw new OrmException("Group " + id + " not found"); + } + return create( + group, + db.accountGroupMembers().byGroup(id), + db.accountGroupMembersAudit().byGroup(id), + db.accountGroupById().byGroup(id), + db.accountGroupByIdAud().byGroup(id)); + } + + public GroupBundle fromNoteDb(Repository repo, AccountGroup.UUID uuid) + throws ConfigInvalidException, IOException { + GroupConfig groupConfig = GroupConfig.loadForGroup(repo, uuid); + InternalGroup internalGroup = groupConfig.getLoadedGroup().get(); + AccountGroup.Id groupId = internalGroup.getId(); + + AccountGroup accountGroup = + new AccountGroup( + internalGroup.getNameKey(), + internalGroup.getId(), + internalGroup.getGroupUUID(), + internalGroup.getCreatedOn()); + accountGroup.setDescription(internalGroup.getDescription()); + accountGroup.setOwnerGroupUUID(internalGroup.getOwnerGroupUUID()); + accountGroup.setVisibleToAll(internalGroup.isVisibleToAll()); + + return create( + accountGroup, + internalGroup + .getMembers() + .stream() + .map( + accountId -> + new AccountGroupMember(new AccountGroupMember.Key(accountId, groupId))) + .collect(toImmutableSet()), + auditLogReader.getMembersAudit(uuid), + internalGroup + .getSubgroups() + .stream() + .map( + subgroupUuid -> + new AccountGroupById(new AccountGroupById.Key(groupId, subgroupUuid))) + .collect(toImmutableSet()), + auditLogReader.getSubgroupsAudit(uuid)); } - return create( - group, - db.accountGroupMembers().byGroup(id), - db.accountGroupMembersAudit().byGroup(id), - db.accountGroupById().byGroup(id), - db.accountGroupByIdAud().byGroup(id)); } public static GroupBundle create( diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java index 4c0ab943fe..9f9889560c 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java @@ -15,7 +15,6 @@ package com.google.gerrit.acceptance.api.group; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static com.google.gerrit.extensions.common.testing.CommitInfoSubject.assertThat; import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS; import static com.google.gerrit.server.notedb.NotesMigration.READ; @@ -25,19 +24,15 @@ import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.TestAccount; -import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.common.CommitInfo; import com.google.gerrit.extensions.common.GroupInfo; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.config.GerritServerId; import com.google.gerrit.server.git.CommitUtil; -import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.ServerInitiated; import com.google.gerrit.server.group.db.GroupBundle; -import com.google.gerrit.server.group.db.GroupConfig; import com.google.gerrit.server.group.db.GroupRebuilder; -import com.google.gerrit.server.group.db.Groups; import com.google.gerrit.server.group.db.GroupsUpdate; import com.google.gerrit.testing.ConfigSuite; import com.google.gerrit.testing.TestTimeUtil; @@ -47,7 +42,6 @@ import com.google.inject.Provider; import java.util.ArrayList; import java.util.Date; import java.util.List; -import java.util.Optional; import java.util.concurrent.TimeUnit; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; @@ -74,8 +68,8 @@ public class GroupRebuilderIT extends AbstractDaemonTest { @Inject @GerritServerId private String serverId; @Inject @ServerInitiated private Provider groupsUpdate; + @Inject private GroupBundle.Factory bundleFactory; @Inject private GroupRebuilder rebuilder; - @Inject private Groups groups; @Before public void setTimeForTesting() { @@ -91,10 +85,11 @@ public class GroupRebuilderIT extends AbstractDaemonTest { public void basicGroupProperties() throws Exception { GroupInfo createdGroup = gApi.groups().create(name("group")).get(); try (BlockReviewDbUpdatesForGroups ctx = new BlockReviewDbUpdatesForGroups()) { - InternalGroup reviewDbGroup = - groups.getGroup(db, new AccountGroup.UUID(createdGroup.id)).get(); - deleteGroupRefs(reviewDbGroup); - assertThat(removeRefState(rebuild(reviewDbGroup))).isEqualTo(roundToSecond(reviewDbGroup)); + GroupBundle reviewDbBundle = + bundleFactory.fromReviewDb(db, new AccountGroup.Id(createdGroup.groupId)); + deleteGroupRefs(reviewDbBundle); + + assertThat(rebuild(reviewDbBundle)).isEqualTo(reviewDbBundle.roundToSecond()); } } @@ -111,11 +106,12 @@ public class GroupRebuilderIT extends AbstractDaemonTest { gApi.groups().id(group1.id).addGroups(group2.id); try (BlockReviewDbUpdatesForGroups ctx = new BlockReviewDbUpdatesForGroups()) { - InternalGroup reviewDbGroup = groups.getGroup(db, new AccountGroup.UUID(group1.id)).get(); - deleteGroupRefs(reviewDbGroup); + GroupBundle reviewDbBundle = + bundleFactory.fromReviewDb(db, new AccountGroup.Id(group1.groupId)); + deleteGroupRefs(reviewDbBundle); - InternalGroup noteDbGroup = rebuild(reviewDbGroup); - assertThat(removeRefState(noteDbGroup)).isEqualTo(roundToSecond(reviewDbGroup)); + GroupBundle noteDbBundle = rebuild(reviewDbBundle); + assertThat(noteDbBundle).isEqualTo(reviewDbBundle.roundToSecond()); ImmutableList log = log(group1); assertThat(log).hasSize(4); @@ -123,7 +119,7 @@ public class GroupRebuilderIT extends AbstractDaemonTest { assertThat(log.get(0)).message().isEqualTo("Create group"); assertThat(log.get(0)).author().name().isEqualTo(serverIdent.get().getName()); assertThat(log.get(0)).author().email().isEqualTo(serverIdent.get().getEmailAddress()); - assertThat(log.get(0)).author().date().isEqualTo(noteDbGroup.getCreatedOn()); + assertThat(log.get(0)).author().date().isEqualTo(noteDbBundle.group().getCreatedOn()); assertThat(log.get(0)).author().tz().isEqualTo(serverIdent.get().getTimeZoneOffset()); assertThat(log.get(0)).committer().isEqualTo(log.get(0).author); @@ -154,13 +150,9 @@ public class GroupRebuilderIT extends AbstractDaemonTest { } } - private static InternalGroup removeRefState(InternalGroup group) throws Exception { - return group.toBuilder().setRefState(null).build(); - } - - private void deleteGroupRefs(InternalGroup group) throws Exception { + private void deleteGroupRefs(GroupBundle bundle) throws Exception { try (Repository repo = repoManager.openRepository(allUsers)) { - String refName = RefNames.refsGroups(group.getGroupUUID()); + String refName = RefNames.refsGroups(bundle.uuid()); RefUpdate ru = repo.updateRef(refName); ru.setForceUpdate(true); Ref oldRef = repo.exactRef(refName); @@ -173,30 +165,13 @@ public class GroupRebuilderIT extends AbstractDaemonTest { } } - private InternalGroup rebuild(InternalGroup group) throws Exception { + private GroupBundle rebuild(GroupBundle reviewDbBundle) throws Exception { try (Repository repo = repoManager.openRepository(allUsers)) { - rebuilder.rebuild(repo, GroupBundle.fromReviewDb(db, group.getId()), null); - GroupConfig groupConfig = GroupConfig.loadForGroup(repo, group.getGroupUUID()); - Optional result = groupConfig.getLoadedGroup(); - assertThat(result).isPresent(); - return result.get(); + rebuilder.rebuild(repo, reviewDbBundle, null); + return bundleFactory.fromNoteDb(repo, reviewDbBundle.uuid()); } } - private InternalGroup roundToSecond(InternalGroup g) { - return InternalGroup.builder() - .setId(g.getId()) - .setNameKey(g.getNameKey()) - .setDescription(g.getDescription()) - .setOwnerGroupUUID(g.getOwnerGroupUUID()) - .setVisibleToAll(g.isVisibleToAll()) - .setGroupUUID(g.getGroupUUID()) - .setCreatedOn(TimeUtil.roundToSecond(g.getCreatedOn())) - .setMembers(g.getMembers()) - .setSubgroups(g.getSubgroups()) - .build(); - } - private ImmutableList log(GroupInfo g) throws Exception { ImmutableList.Builder result = ImmutableList.builder(); List commitDates = new ArrayList<>(); diff --git a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java index 8efb8a8d20..e2177e257f 100644 --- a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java +++ b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java @@ -30,7 +30,6 @@ import com.google.gerrit.reviewdb.client.AccountGroupByIdAud; import com.google.gerrit.reviewdb.client.AccountGroupMember; import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; import com.google.gerrit.reviewdb.client.RefNames; -import com.google.gerrit.server.config.AllUsersNameProvider; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.group.InternalGroup; @@ -41,8 +40,6 @@ import java.sql.Timestamp; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.IntStream; -import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription; -import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.Repository; @@ -54,12 +51,13 @@ public class GroupRebuilderTest extends AbstractGroupTest { private AtomicInteger idCounter; private Repository repo; private GroupRebuilder rebuilder; + private GroupBundle.Factory bundleFactory; @Before public void setUp() throws Exception { TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS); idCounter = new AtomicInteger(); - repo = new InMemoryRepository(new DfsRepositoryDescription(AllUsersNameProvider.DEFAULT)); + repo = repoManager.createRepository(allUsersName); rebuilder = new GroupRebuilder( GroupRebuilderTest::newPersonIdent, @@ -72,6 +70,8 @@ public class GroupRebuilderTest extends AbstractGroupTest { AbstractGroupTest::newPersonIdent, AbstractGroupTest::getAccountNameEmail, AbstractGroupTest::getGroupName); + bundleFactory = + new GroupBundle.Factory(new AuditLogReader(SERVER_ID, repoManager, allUsersName)); } @After @@ -86,7 +86,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(1); assertCommit(log.get(0), "Create group", SERVER_NAME, SERVER_EMAIL); @@ -103,7 +103,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(1); assertServerCommit(log.get(0), "Create group"); @@ -121,7 +121,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(2); assertServerCommit(log.get(0), "Create group"); @@ -150,7 +150,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(4); assertServerCommit(log.get(0), "Create group"); @@ -176,7 +176,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(4); assertServerCommit(log.get(0), "Create group"); @@ -200,7 +200,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(3); assertServerCommit(log.get(0), "Create group"); @@ -225,7 +225,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(4); assertServerCommit(log.get(0), "Create group"); @@ -246,7 +246,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(3); assertServerCommit(log.get(0), "Create group"); @@ -276,7 +276,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(5); assertServerCommit(log.get(0), "Create group"); @@ -323,7 +323,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(5); assertServerCommit(log.get(0), "Create group"); @@ -358,7 +358,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { rebuilder.rebuild(repo, b, null); - assertThat(reload(g)).isEqualTo(b.toInternalGroup()); + assertThat(reload(g)).isEqualTo(b); ImmutableList log = log(g); assertThat(log).hasSize(3); assertServerCommit(log.get(0), "Create group"); @@ -399,14 +399,14 @@ public class GroupRebuilderTest extends AbstractGroupTest { assertThat(log(g1)).hasSize(1); assertThat(log(g2)).hasSize(1); assertThat(logGroupNames()).hasSize(1); - assertThat(reload(g1)).isEqualTo(b1.toInternalGroup()); - assertThat(reload(g2)).isEqualTo(b2.toInternalGroup()); + assertThat(reload(g1)).isEqualTo(b1); + assertThat(reload(g2)).isEqualTo(b2); assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2"); } - private InternalGroup reload(AccountGroup g) throws Exception { - return removeRefState(GroupConfig.loadForGroup(repo, g.getGroupUUID()).getLoadedGroup().get()); + private GroupBundle reload(AccountGroup g) throws Exception { + return bundleFactory.fromNoteDb(repo, g.getGroupUUID()); } private AccountGroup newGroup(String name) {