opendev/gerrit
Code Issues Proposed changes

Always use the best possible AuditLogFormatter

Browse Source 
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
This commit is contained in:
Alice Kober-Sotzek
2018-01-09 16:07:17 +01:00
parent 56238b7f4e
commit b8fbda4f13
8 changed files with 71 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
java/com/google/gerrit/pgm/init/GroupsOnInit.java
View File

@@ -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)

69
java/com/google/gerrit/server/group/db/AuditLogFormatter.java
View File

@@ -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<Account.Id, Optional<Account>> accountRetriever;
private final Function<AccountGroup.UUID, Optional<GroupDescription.Basic>> 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<Account.Id, Optional<Account>> accountRetriever,
Function<AccountGroup.UUID, Optional<GroupDescription.Basic>> groupRetriever,
String serverId) {
this.accountRetriever = accountRetriever;
this.groupRetriever = groupRetriever;
this.serverId = serverId;
}
private static Optional<Account> 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<Account> allAccounts,
ImmutableSet<GroupDescription.Basic> allGroups,
String serverId) {
return create(id -> getAccount(allAccounts, id), uuid -> getGroup(allGroups, uuid), serverId);
}
private static Optional<GroupDescription.Basic> getGroup(
ImmutableSet<GroupDescription.Basic> groups, AccountGroup.UUID uuid) {
return groups.stream().filter(group -> group.getGroupUUID().equals(uuid)).findAny();
}
private static Optional<Account> getAccount(ImmutableSet<Account> 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<Account.Id, Optional<Account>> accountRetriever,
Function<AccountGroup.UUID, Optional<GroupDescription.Basic>> groupRetriever,
String serverId) {
return new AuditLogFormatter(accountRetriever, groupRetriever, serverId);
}
private AuditLogFormatter(
Function<Account.Id, Optional<Account>> accountRetriever,
Function<AccountGroup.UUID, Optional<GroupDescription.Basic>> groupRetriever,
String serverId) {
this.accountRetriever = checkNotNull(accountRetriever);
this.groupRetriever = checkNotNull(groupRetriever);
this.serverId = checkNotNull(serverId);
}
private AuditLogFormatter(
Function<Account.Id, Optional<Account>> accountRetriever,
Function<AccountGroup.UUID, Optional<GroupDescription.Basic>> 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;
}

20
java/com/google/gerrit/server/group/db/GroupConfig.java
View File

@@ -77,7 +77,7 @@ public class GroupConfig extends VersionedMetaData {
private Optional<InternalGroup> loadedGroup = Optional.empty();
private Optional<InternalGroupCreation> groupCreation = Optional.empty();
private Optional<InternalGroupUpdate> groupUpdate = Optional.empty();
private Optional<AuditLogFormatter> 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<String> getCommitFootersForMemberModifications(
ImmutableSet<Account.Id> oldMembers, ImmutableSet<Account.Id> newMembers) {
AuditLogFormatter formatter =
auditLogFormatter.orElseThrow(
() -> new IllegalStateException("AuditLogFormatter is necessary but missing"));
Stream<String> removedMembers =
Sets.difference(oldMembers, newMembers)
.stream()
.map(formatter::getParsableAccount)
.map(auditLogFormatter::getParsableAccount)
.map((FOOTER_REMOVE_MEMBER.getName() + ": ")::concat);
Stream<String> 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<String> getCommitFootersForSubgroupModifications(
ImmutableSet<AccountGroup.UUID> oldSubgroups, ImmutableSet<AccountGroup.UUID> newSubgroups) {
AuditLogFormatter formatter =
auditLogFormatter.orElseThrow(
() -> new IllegalStateException("AuditLogFormatter is necessary but missing"));
Stream<String> removedMembers =
Sets.difference(oldSubgroups, newSubgroups)
.stream()
.map(formatter::getParsableGroup)
.map(auditLogFormatter::getParsableGroup)
.map((FOOTER_REMOVE_GROUP.getName() + ": ")::concat);
Stream<String> addedMembers =
Sets.difference(newSubgroups, oldSubgroups)
.stream()
.map(formatter::getParsableGroup)
.map(auditLogFormatter::getParsableGroup)
.map((FOOTER_ADD_GROUP.getName() + ": ")::concat);
return Stream.concat(removedMembers, addedMembers);
}

2
java/com/google/gerrit/server/group/db/GroupRebuilder.java
View File

@@ -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

2
java/com/google/gerrit/server/group/db/GroupsUpdate.java
View File

@@ -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);

6
java/com/google/gerrit/server/schema/SchemaCreator.java
View File

@@ -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);

2
javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java
View File

@@ -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);
}

4
javatests/com/google/gerrit/server/group/db/GroupConfigTest.java
View File

@@ -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 {