From b8fbda4f13d38c28320daedb0d98746c1ac58f2a Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Tue, 9 Jan 2018 16:07:17 +0100 Subject: [PATCH] Always use the best possible AuditLogFormatter In order to have more robust code, we never work without an instance of AuditLogFormatter. In fact, for all real code paths, it's always possible to specify a valid AuditLogFormatter. As fallback, we can provide an instance which should work for most cases and which simply omits cosmetic details. Instead of directly failing when we have to use the fallback (as previously), we can at least give the fallback a try. That situation should never occur with the current code but who knows which future code changes could result in calling the fallback. Due to the tests added by Id3810e02fa, we have the guarantee that the fallback isn't used for all foreseen cases. Change-Id: I5466e586ee14bfa2d6b732619f0ec693d7312e4a --- .../google/gerrit/pgm/init/GroupsOnInit.java | 6 +- .../server/group/db/AuditLogFormatter.java | 69 +++++++++++++++---- .../gerrit/server/group/db/GroupConfig.java | 20 ++---- .../server/group/db/GroupRebuilder.java | 2 +- .../gerrit/server/group/db/GroupsUpdate.java | 2 +- .../gerrit/server/schema/SchemaCreator.java | 6 +- .../server/group/db/AbstractGroupTest.java | 2 +- .../server/group/db/GroupConfigTest.java | 4 +- 8 files changed, 71 insertions(+), 40 deletions(-) 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 {