diff --git a/java/com/google/gerrit/pgm/init/GroupsOnInit.java b/java/com/google/gerrit/pgm/init/GroupsOnInit.java index 85a5de16fb..4b752e6c0b 100644 --- a/java/com/google/gerrit/pgm/init/GroupsOnInit.java +++ b/java/com/google/gerrit/pgm/init/GroupsOnInit.java @@ -52,7 +52,6 @@ import java.io.IOException; import java.nio.file.Path; import java.sql.Timestamp; import java.util.List; -import java.util.Optional; import java.util.stream.Stream; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.internal.storage.file.FileRepository; @@ -258,10 +257,7 @@ public class GroupsOnInit { private AuditLogFormatter getAuditLogFormatter(Account account) throws IOException, ConfigInvalidException { String serverId = new GerritServerIdProvider(flags.cfg, site).get(); - return new AuditLogFormatter( - accountId -> account.getId().equals(accountId) ? Optional.of(account) : Optional.empty(), - uuid -> Optional.empty(), - serverId); + return AuditLogFormatter.createBackedBy(ImmutableSet.of(account), ImmutableSet.of(), serverId); } private void commit(Repository repository, GroupConfig groupConfig, Timestamp groupCreatedOn) diff --git a/java/com/google/gerrit/server/group/db/AuditLogFormatter.java b/java/com/google/gerrit/server/group/db/AuditLogFormatter.java index 64097e5465..fa438dc33c 100644 --- a/java/com/google/gerrit/server/group/db/AuditLogFormatter.java +++ b/java/com/google/gerrit/server/group/db/AuditLogFormatter.java @@ -14,6 +14,11 @@ package com.google.gerrit.server.group.db; +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; + +import com.google.common.collect.ImmutableSet; +import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; @@ -35,24 +40,16 @@ import org.eclipse.jgit.lib.PersonIdent; public class AuditLogFormatter { private final Function> accountRetriever; private final Function> groupRetriever; - private final String serverId; + @Nullable private final String serverId; - public AuditLogFormatter(AccountCache accountCache, GroupBackend groupBackend, String serverId) { - this( + public static AuditLogFormatter createBackedBy( + AccountCache accountCache, GroupBackend groupBackend, String serverId) { + return create( accountId -> getAccount(accountCache, accountId), groupUuid -> getGroup(groupBackend, groupUuid), serverId); } - public AuditLogFormatter( - Function> accountRetriever, - Function> groupRetriever, - String serverId) { - this.accountRetriever = accountRetriever; - this.groupRetriever = groupRetriever; - this.serverId = serverId; - } - private static Optional getAccount(AccountCache accountCache, Account.Id accountId) { return accountCache.maybeGet(accountId).map(AccountState::getAccount); } @@ -62,6 +59,50 @@ public class AuditLogFormatter { return Optional.ofNullable(groupBackend.get(groupUuid)); } + public static AuditLogFormatter createBackedBy( + ImmutableSet allAccounts, + ImmutableSet allGroups, + String serverId) { + return create(id -> getAccount(allAccounts, id), uuid -> getGroup(allGroups, uuid), serverId); + } + + private static Optional getGroup( + ImmutableSet groups, AccountGroup.UUID uuid) { + return groups.stream().filter(group -> group.getGroupUUID().equals(uuid)).findAny(); + } + + private static Optional getAccount(ImmutableSet accounts, Account.Id id) { + return accounts.stream().filter(account -> account.getId().equals(id)).findAny(); + } + + public static AuditLogFormatter createPartiallyWorkingFallBack() { + return new AuditLogFormatter(id -> Optional.empty(), uuid -> Optional.empty()); + } + + public static AuditLogFormatter create( + Function> accountRetriever, + Function> groupRetriever, + String serverId) { + return new AuditLogFormatter(accountRetriever, groupRetriever, serverId); + } + + private AuditLogFormatter( + Function> accountRetriever, + Function> groupRetriever, + String serverId) { + this.accountRetriever = checkNotNull(accountRetriever); + this.groupRetriever = checkNotNull(groupRetriever); + this.serverId = checkNotNull(serverId); + } + + private AuditLogFormatter( + Function> accountRetriever, + Function> groupRetriever) { + this.accountRetriever = checkNotNull(accountRetriever); + this.groupRetriever = checkNotNull(groupRetriever); + serverId = null; + } + /** * Creates a parsable {@code PersonIdent} for commits which are used as an audit log. * @@ -139,6 +180,10 @@ public class AuditLogFormatter { } private String getEmailForAuditLog(Account.Id accountId) { + // If we ever switch to UUIDs for accounts, consider to remove the serverId and to use a similar + // approach as for group UUIDs. + checkState( + serverId != null, "serverId must be defined; fall-back AuditLogFormatter isn't sufficient"); return accountId.get() + "@" + serverId; } diff --git a/java/com/google/gerrit/server/group/db/GroupConfig.java b/java/com/google/gerrit/server/group/db/GroupConfig.java index 7a825e181c..dc249cf413 100644 --- a/java/com/google/gerrit/server/group/db/GroupConfig.java +++ b/java/com/google/gerrit/server/group/db/GroupConfig.java @@ -77,7 +77,7 @@ public class GroupConfig extends VersionedMetaData { private Optional loadedGroup = Optional.empty(); private Optional groupCreation = Optional.empty(); private Optional groupUpdate = Optional.empty(); - private Optional auditLogFormatter = Optional.empty(); + private AuditLogFormatter auditLogFormatter = AuditLogFormatter.createPartiallyWorkingFallBack(); private boolean isLoaded = false; private boolean allowSaveEmptyName; @@ -131,7 +131,7 @@ public class GroupConfig extends VersionedMetaData { public void setGroupUpdate(InternalGroupUpdate groupUpdate, AuditLogFormatter auditLogFormatter) { this.groupUpdate = Optional.of(groupUpdate); - this.auditLogFormatter = Optional.of(auditLogFormatter); + this.auditLogFormatter = auditLogFormatter; } @Override @@ -368,38 +368,30 @@ public class GroupConfig extends VersionedMetaData { private Stream getCommitFootersForMemberModifications( ImmutableSet oldMembers, ImmutableSet newMembers) { - AuditLogFormatter formatter = - auditLogFormatter.orElseThrow( - () -> new IllegalStateException("AuditLogFormatter is necessary but missing")); - Stream removedMembers = Sets.difference(oldMembers, newMembers) .stream() - .map(formatter::getParsableAccount) + .map(auditLogFormatter::getParsableAccount) .map((FOOTER_REMOVE_MEMBER.getName() + ": ")::concat); Stream addedMembers = Sets.difference(newMembers, oldMembers) .stream() - .map(formatter::getParsableAccount) + .map(auditLogFormatter::getParsableAccount) .map((FOOTER_ADD_MEMBER.getName() + ": ")::concat); return Stream.concat(removedMembers, addedMembers); } private Stream getCommitFootersForSubgroupModifications( ImmutableSet oldSubgroups, ImmutableSet newSubgroups) { - AuditLogFormatter formatter = - auditLogFormatter.orElseThrow( - () -> new IllegalStateException("AuditLogFormatter is necessary but missing")); - Stream removedMembers = Sets.difference(oldSubgroups, newSubgroups) .stream() - .map(formatter::getParsableGroup) + .map(auditLogFormatter::getParsableGroup) .map((FOOTER_REMOVE_GROUP.getName() + ": ")::concat); Stream addedMembers = Sets.difference(newSubgroups, oldSubgroups) .stream() - .map(formatter::getParsableGroup) + .map(auditLogFormatter::getParsableGroup) .map((FOOTER_ADD_GROUP.getName() + ": ")::concat); return Stream.concat(removedMembers, addedMembers); } diff --git a/java/com/google/gerrit/server/group/db/GroupRebuilder.java b/java/com/google/gerrit/server/group/db/GroupRebuilder.java index 8f34d41617..b1d4682cfa 100644 --- a/java/com/google/gerrit/server/group/db/GroupRebuilder.java +++ b/java/com/google/gerrit/server/group/db/GroupRebuilder.java @@ -82,7 +82,7 @@ public class GroupRebuilder { metaDataUpdateFactory, // TODO(dborowitz): These probably won't work during init. - new AuditLogFormatter(accountCache, groupBackend, serverId)); + AuditLogFormatter.createBackedBy(accountCache, groupBackend, serverId)); } @VisibleForTesting diff --git a/java/com/google/gerrit/server/group/db/GroupsUpdate.java b/java/com/google/gerrit/server/group/db/GroupsUpdate.java index 130b0b2b8e..bc5df79641 100644 --- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java +++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java @@ -142,7 +142,7 @@ public class GroupsUpdate { this.retryHelper = retryHelper; this.currentUser = currentUser; - auditLogFormatter = new AuditLogFormatter(accountCache, groupBackend, serverId); + auditLogFormatter = AuditLogFormatter.createBackedBy(accountCache, groupBackend, serverId); metaDataUpdateFactory = getMetaDataUpdateFactory( metaDataUpdateInternalFactory, currentUser, serverIdent, auditLogFormatter); diff --git a/java/com/google/gerrit/server/schema/SchemaCreator.java b/java/com/google/gerrit/server/schema/SchemaCreator.java index 3daf67d6f0..ba68b2cca9 100644 --- a/java/com/google/gerrit/server/schema/SchemaCreator.java +++ b/java/com/google/gerrit/server/schema/SchemaCreator.java @@ -56,7 +56,6 @@ import com.google.inject.Inject; import java.io.IOException; import java.nio.file.Path; import java.util.Collections; -import java.util.Optional; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.Config; @@ -240,10 +239,9 @@ public class SchemaCreator { private InternalGroup createGroupInNoteDb( Repository allUsersRepo, InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) throws ConfigInvalidException, IOException, OrmDuplicateKeyException { - // We don't add any initial members or subgroups and hence the provided functions should never - // be called. + // This method is only executed on a new server which doesn't have any accounts or groups. AuditLogFormatter auditLogFormatter = - new AuditLogFormatter(id -> Optional.empty(), uuid -> Optional.empty(), serverId); + AuditLogFormatter.createBackedBy(ImmutableSet.of(), ImmutableSet.of(), serverId); GroupConfig groupConfig = GroupConfig.createForNewGroup(allUsersRepo, groupCreation); groupConfig.setGroupUpdate(groupUpdate, auditLogFormatter); diff --git a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java index 80a48087f2..50e1e066a7 100644 --- a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java +++ b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java @@ -131,7 +131,7 @@ public class AbstractGroupTest extends GerritBaseTests { } protected static AuditLogFormatter getAuditLogFormatter() { - return new AuditLogFormatter( + return AuditLogFormatter.create( AbstractGroupTest::getAccount, AbstractGroupTest::getGroup, SERVER_ID); } diff --git a/javatests/com/google/gerrit/server/group/db/GroupConfigTest.java b/javatests/com/google/gerrit/server/group/db/GroupConfigTest.java index ed3d689f83..3fc1a00bd0 100644 --- a/javatests/com/google/gerrit/server/group/db/GroupConfigTest.java +++ b/javatests/com/google/gerrit/server/group/db/GroupConfigTest.java @@ -17,13 +17,13 @@ package com.google.gerrit.server.group.db; import static com.google.common.truth.Truth.assertThat; import static org.hamcrest.CoreMatchers.instanceOf; +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.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.MetaDataUpdate; -import java.util.Optional; import java.util.TimeZone; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription; @@ -45,7 +45,7 @@ public class GroupConfigTest { private final AccountGroup.NameKey groupName = new AccountGroup.NameKey("users"); private final AccountGroup.Id groupId = new AccountGroup.Id(123); private final AuditLogFormatter auditLogFormatter = - new AuditLogFormatter(id -> Optional.empty(), uuid -> Optional.empty(), "server-id"); + AuditLogFormatter.createBackedBy(ImmutableSet.of(), ImmutableSet.of(), "server-id"); @Before public void setUp() throws Exception {