opendev/gerrit
Code Issues Proposed changes

Merge "Add explicit factory methods to create AccountsUpdate/GroupsUpdate for serverIdent"

Browse Source
This commit is contained in:
David Pursehouse
2019-02-05 11:05:44 +00:00
committed by Gerrit Code Review
parent 02b53c5a74 4711820e88
commit 3953f4654c
5 changed files with 173 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

74
java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -24,7 +24,6 @@ import com.google.common.base.Throwables;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.Runnables;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.git.LockFailureException;
import com.google.gerrit.git.RefUpdateUtil;
import com.google.gerrit.reviewdb.client.Account;
@@ -44,9 +43,9 @@ import com.google.gerrit.server.update.RetryHelper.Action;
import com.google.gerrit.server.update.RetryHelper.ActionType;
import com.google.gwtorm.server.OrmDuplicateKeyException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.List;
@@ -123,16 +122,28 @@ public class AccountsUpdate {
public interface Factory {
/**
* Creates an {@code AccountsUpdate} which uses the identity of the specified user as author for
* all commits related to accounts. The Gerrit server identity will be used as committer.
* all commits related to accounts. The server identity will be used as committer.
*
* <p><strong>Note</strong>: Please use this method with care and rather consider to use the
* correct annotation on the provider of an {@code AccountsUpdate} instead.
* <p><strong>Note</strong>: Please use this method with care and consider using the {@link
* com.google.gerrit.server.UserInitiated} annotation on the provider of an {@code
* AccountsUpdate} instead.
*
* @param currentUser the user to which modifications should be attributed, or {@code null} if
* the Gerrit server identity should also be used as author
* @param currentUser the user to which modifications should be attributed
* @param externalIdNotesLoader the loader that should be used to load external ID notes
*/
AccountsUpdate create(
@Nullable IdentifiedUser currentUser, ExternalIdNotesLoader externalIdNotesLoader);
AccountsUpdate create(IdentifiedUser currentUser, ExternalIdNotesLoader externalIdNotesLoader);
/**
* Creates an {@code AccountsUpdate} which uses the server identity as author and committer for
* all commits related to accounts.
*
* <p><strong>Note</strong>: Please use this method with care and consider using the {@link
* com.google.gerrit.server.ServerInitiated} annotation on the provider of an {@code
* AccountsUpdate} instead.
*
* @param externalIdNotesLoader the loader that should be used to load external ID notes
*/
AccountsUpdate createWithServerIdent(ExternalIdNotesLoader externalIdNotesLoader);
}
/**
@@ -172,7 +183,7 @@ public class AccountsUpdate {
private final GitRepositoryManager repoManager;
private final GitReferenceUpdated gitRefUpdated;
@Nullable private final IdentifiedUser currentUser;
private final Optional<IdentifiedUser> currentUser;
private final AllUsersName allUsersName;
private final ExternalIds externalIds;
private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
@@ -187,7 +198,7 @@ public class AccountsUpdate {
// Invoked after updating the account but before committing the changes.
private final Runnable beforeCommit;
@Inject
@AssistedInject
AccountsUpdate(
GitRepositoryManager repoManager,
GitReferenceUpdated gitRefUpdated,
@@ -196,19 +207,44 @@ public class AccountsUpdate {
Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
RetryHelper retryHelper,
@GerritPersonIdent PersonIdent serverIdent,
@Assisted @Nullable IdentifiedUser currentUser,
@Assisted ExternalIdNotesLoader extIdNotesLoader) {
this(
repoManager,
gitRefUpdated,
currentUser,
Optional.empty(),
allUsersName,
externalIds,
metaDataUpdateInternalFactory,
retryHelper,
extIdNotesLoader,
serverIdent,
createPersonIdent(serverIdent, currentUser),
createPersonIdent(serverIdent, Optional.empty()),
Runnables.doNothing(),
Runnables.doNothing());
}
@AssistedInject
AccountsUpdate(
GitRepositoryManager repoManager,
GitReferenceUpdated gitRefUpdated,
AllUsersName allUsersName,
ExternalIds externalIds,
Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
RetryHelper retryHelper,
@GerritPersonIdent PersonIdent serverIdent,
@Assisted IdentifiedUser currentUser,
@Assisted ExternalIdNotesLoader extIdNotesLoader) {
this(
repoManager,
gitRefUpdated,
Optional.of(currentUser),
allUsersName,
externalIds,
metaDataUpdateInternalFactory,
retryHelper,
extIdNotesLoader,
serverIdent,
createPersonIdent(serverIdent, Optional.of(currentUser)),
Runnables.doNothing(),
Runnables.doNothing());
}
@@ -217,7 +253,7 @@ public class AccountsUpdate {
public AccountsUpdate(
GitRepositoryManager repoManager,
GitReferenceUpdated gitRefUpdated,
@Nullable IdentifiedUser currentUser,
Optional<IdentifiedUser> currentUser,
AllUsersName allUsersName,
ExternalIds externalIds,
Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
@@ -243,11 +279,11 @@ public class AccountsUpdate {
}
private static PersonIdent createPersonIdent(
PersonIdent serverIdent, @Nullable IdentifiedUser user) {
if (user == null) {
PersonIdent serverIdent, Optional<IdentifiedUser> user) {
if (!user.isPresent()) {
return serverIdent;
}
return user.newCommitterIdent(serverIdent.getWhen(), serverIdent.getTimeZone());
return user.get().newCommitterIdent(serverIdent.getWhen(), serverIdent.getTimeZone());
}
/**
@@ -455,7 +491,7 @@ public class AccountsUpdate {
.updateCaches(accountsThatWillBeReindexByReindexAfterRefUpdate);
gitRefUpdated.fire(
allUsersName, batchRefUpdate, currentUser != null ? currentUser.state() : null);
allUsersName, batchRefUpdate, currentUser.map(user -> user.state()).orElse(null));
}
private static Set<Account.Id> getUpdatedAccounts(BatchRefUpdate batchRefUpdate) {

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

@@ -43,9 +43,9 @@ import com.google.gerrit.server.index.group.GroupIndexer;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gwtorm.server.OrmDuplicateKeyException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.Objects;
@@ -75,13 +75,24 @@ public class GroupsUpdate {
* modifications executed by it. For NoteDb, this identity is used as author and committer for
* all related commits.
*
* <p><strong>Note</strong>: Please use this method with care and rather consider to use the
* correct annotation on the provider of a {@code GroupsUpdate} instead.
* <p><strong>Note</strong>: Please use this method with care and consider using the {@link
* com.google.gerrit.server.UserInitiated} annotation on the provider of a {@code GroupsUpdate}
* instead.
*
* @param currentUser the user to which modifications should be attributed, or {@code null} if
* the Gerrit server identity should be used
* @param currentUser the user to which modifications should be attributed
*/
GroupsUpdate create(@Nullable IdentifiedUser currentUser);
GroupsUpdate create(IdentifiedUser currentUser);
/**
* Creates a {@code GroupsUpdate} which uses the server identity to mark database modifications
* executed by it. For NoteDb, this identity is used as author and committer for all related
* commits.
*
* <p><strong>Note</strong>: Please use this method with care and consider using the {@link
* com.google.gerrit.server.ServerInitiated} annotation on the provider of a {@code
* GroupsUpdate} instead.
*/
GroupsUpdate createWithServerIdent();
}
private final GitRepositoryManager repoManager;
@@ -91,14 +102,48 @@ public class GroupsUpdate {
private final Provider<GroupIndexer> indexer;
private final GroupAuditService groupAuditService;
private final RenameGroupOp.Factory renameGroupOpFactory;
@Nullable private final IdentifiedUser currentUser;
private final Optional<IdentifiedUser> currentUser;
private final AuditLogFormatter auditLogFormatter;
private final PersonIdent authorIdent;
private final MetaDataUpdateFactory metaDataUpdateFactory;
private final GitReferenceUpdated gitRefUpdated;
private final RetryHelper retryHelper;
@Inject
@AssistedInject
GroupsUpdate(
GitRepositoryManager repoManager,
AllUsersName allUsersName,
GroupBackend groupBackend,
GroupCache groupCache,
GroupIncludeCache groupIncludeCache,
Provider<GroupIndexer> indexer,
GroupAuditService auditService,
AccountCache accountCache,
RenameGroupOp.Factory renameGroupOpFactory,
@GerritServerId String serverId,
@GerritPersonIdent PersonIdent serverIdent,
MetaDataUpdate.InternalFactory metaDataUpdateInternalFactory,
GitReferenceUpdated gitRefUpdated,
RetryHelper retryHelper) {
this(
repoManager,
allUsersName,
groupBackend,
groupCache,
groupIncludeCache,
indexer,
auditService,
accountCache,
renameGroupOpFactory,
serverId,
serverIdent,
metaDataUpdateInternalFactory,
gitRefUpdated,
retryHelper,
Optional.empty());
}
@AssistedInject
GroupsUpdate(
GitRepositoryManager repoManager,
AllUsersName allUsersName,
@@ -114,7 +159,41 @@ public class GroupsUpdate {
MetaDataUpdate.InternalFactory metaDataUpdateInternalFactory,
GitReferenceUpdated gitRefUpdated,
RetryHelper retryHelper,
@Assisted @Nullable IdentifiedUser currentUser) {
@Assisted IdentifiedUser currentUser) {
this(
repoManager,
allUsersName,
groupBackend,
groupCache,
groupIncludeCache,
indexer,
auditService,
accountCache,
renameGroupOpFactory,
serverId,
serverIdent,
metaDataUpdateInternalFactory,
gitRefUpdated,
retryHelper,
Optional.of(currentUser));
}
private GroupsUpdate(
GitRepositoryManager repoManager,
AllUsersName allUsersName,
GroupBackend groupBackend,
GroupCache groupCache,
GroupIncludeCache groupIncludeCache,
Provider<GroupIndexer> indexer,
GroupAuditService auditService,
AccountCache accountCache,
RenameGroupOp.Factory renameGroupOpFactory,
@GerritServerId String serverId,
@GerritPersonIdent PersonIdent serverIdent,
MetaDataUpdate.InternalFactory metaDataUpdateInternalFactory,
GitReferenceUpdated gitRefUpdated,
RetryHelper retryHelper,
Optional<IdentifiedUser> currentUser) {
this.repoManager = repoManager;
this.allUsersName = allUsersName;
this.groupCache = groupCache;
@@ -135,7 +214,7 @@ public class GroupsUpdate {
private static MetaDataUpdateFactory getMetaDataUpdateFactory(
MetaDataUpdate.InternalFactory metaDataUpdateInternalFactory,
@Nullable IdentifiedUser currentUser,
Optional<IdentifiedUser> currentUser,
PersonIdent serverIdent,
AuditLogFormatter auditLogFormatter) {
return (projectName, repository, batchRefUpdate) -> {
@@ -143,10 +222,10 @@ public class GroupsUpdate {
metaDataUpdateInternalFactory.create(projectName, repository, batchRefUpdate);
metaDataUpdate.getCommitBuilder().setCommitter(serverIdent);
PersonIdent authorIdent;
if (currentUser != null) {
metaDataUpdate.setAuthor(currentUser);
if (currentUser.isPresent()) {
metaDataUpdate.setAuthor(currentUser.get());
authorIdent =
auditLogFormatter.getParsableAuthorIdent(currentUser.getAccount(), serverIdent);
auditLogFormatter.getParsableAuthorIdent(currentUser.get().getAccount(), serverIdent);
} else {
authorIdent = serverIdent;
}
@@ -156,8 +235,8 @@ public class GroupsUpdate {
}
private static PersonIdent getAuthorIdent(
PersonIdent serverIdent, @Nullable IdentifiedUser currentUser) {
return currentUser != null ? createPersonIdent(serverIdent, currentUser) : serverIdent;
PersonIdent serverIdent, Optional<IdentifiedUser> currentUser) {
return currentUser.map(user -> createPersonIdent(serverIdent, user)).orElse(serverIdent);
}
private static PersonIdent createPersonIdent(PersonIdent ident, IdentifiedUser user) {
@@ -342,7 +421,7 @@ public class GroupsUpdate {
RefUpdateUtil.executeChecked(batchRefUpdate, allUsersRepo);
gitRefUpdated.fire(
allUsersName, batchRefUpdate, currentUser != null ? currentUser.state() : null);
allUsersName, batchRefUpdate, currentUser.map(user -> user.state()).orElse(null));
}
private void updateCachesOnGroupCreation(InternalGroup createdGroup) throws IOException {
@@ -390,20 +469,20 @@ public class GroupsUpdate {
}
private void dispatchAuditEventsOnGroupCreation(InternalGroup createdGroup) {
if (currentUser == null) {
if (!currentUser.isPresent()) {
return;
}
if (!createdGroup.getMembers().isEmpty()) {
groupAuditService.dispatchAddMembers(
currentUser.getAccountId(),
currentUser.get().getAccountId(),
createdGroup.getGroupUUID(),
createdGroup.getMembers(),
createdGroup.getCreatedOn());
}
if (!createdGroup.getSubgroups().isEmpty()) {
groupAuditService.dispatchAddSubgroups(
currentUser.getAccountId(),
currentUser.get().getAccountId(),
createdGroup.getGroupUUID(),
createdGroup.getSubgroups(),
createdGroup.getCreatedOn());
@@ -411,25 +490,34 @@ public class GroupsUpdate {
}
private void dispatchAuditEventsOnGroupUpdate(UpdateResult result, Timestamp updatedOn) {
if (currentUser == null) {
if (!currentUser.isPresent()) {
return;
}
if (!result.getAddedMembers().isEmpty()) {
groupAuditService.dispatchAddMembers(
currentUser.getAccountId(), result.getGroupUuid(), result.getAddedMembers(), updatedOn);
currentUser.get().getAccountId(),
result.getGroupUuid(),
result.getAddedMembers(),
updatedOn);
}
if (!result.getDeletedMembers().isEmpty()) {
groupAuditService.dispatchDeleteMembers(
currentUser.getAccountId(), result.getGroupUuid(), result.getDeletedMembers(), updatedOn);
currentUser.get().getAccountId(),
result.getGroupUuid(),
result.getDeletedMembers(),
updatedOn);
}
if (!result.getAddedSubgroups().isEmpty()) {
groupAuditService.dispatchAddSubgroups(
currentUser.getAccountId(), result.getGroupUuid(), result.getAddedSubgroups(), updatedOn);
currentUser.get().getAccountId(),
result.getGroupUuid(),
result.getAddedSubgroups(),
updatedOn);
}
if (!result.getDeletedSubgroups().isEmpty()) {
groupAuditService.dispatchDeleteSubgroups(
currentUser.getAccountId(),
currentUser.get().getAccountId(),
result.getGroupUuid(),
result.getDeletedSubgroups(),
updatedOn);

2
java/com/google/gerrit/server/restapi/account/Module.java
View File

@@ -118,7 +118,7 @@ public class Module extends RestApiModule {
@ServerInitiated
AccountsUpdate provideServerInitiatedAccountsUpdate(
AccountsUpdate.Factory accountsUpdateFactory, ExternalIdNotes.Factory extIdNotesFactory) {
return accountsUpdateFactory.create(null, extIdNotesFactory);
return accountsUpdateFactory.createWithServerIdent(extIdNotesFactory);
}
@Provides

2
java/com/google/gerrit/server/restapi/group/Module.java
View File

@@ -82,7 +82,7 @@ public class Module extends RestApiModule {
@Provides
@ServerInitiated
GroupsUpdate provideServerInitiatedGroupsUpdate(GroupsUpdate.Factory groupsUpdateFactory) {
return groupsUpdateFactory.create(null);
return groupsUpdateFactory.createWithServerIdent();
}
@Provides

8
javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -2257,7 +2257,7 @@ public class AccountIT extends AbstractDaemonTest {
new AccountsUpdate(
repoManager,
gitReferenceUpdated,
null,
Optional.empty(),
allUsers,
externalIds,
metaDataUpdateInternalFactory,
@@ -2307,7 +2307,7 @@ public class AccountIT extends AbstractDaemonTest {
new AccountsUpdate(
repoManager,
gitReferenceUpdated,
null,
Optional.empty(),
allUsers,
externalIds,
metaDataUpdateInternalFactory,
@@ -2367,7 +2367,7 @@ public class AccountIT extends AbstractDaemonTest {
new AccountsUpdate(
repoManager,
gitReferenceUpdated,
null,
Optional.empty(),
allUsers,
externalIds,
metaDataUpdateInternalFactory,
@@ -2433,7 +2433,7 @@ public class AccountIT extends AbstractDaemonTest {
new AccountsUpdate(
repoManager,
gitReferenceUpdated,
null,
Optional.empty(),
allUsers,
externalIds,
metaDataUpdateInternalFactory,