Merge "Include view name into log that records internal server errors"
diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java
index 77437b3..56fb748 100644
--- a/java/com/google/gerrit/entities/Change.java
+++ b/java/com/google/gerrit/entities/Change.java
@@ -17,7 +17,6 @@
import static com.google.gerrit.entities.RefNames.REFS_CHANGES;
import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableMap;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.ChangeStatus;
@@ -493,9 +492,6 @@
/** References the source change and patchset that this change was cherry-picked from. */
@Nullable private PatchSet.Id cherryPickOf;
- /** Custom keyed values that were provided during change creation. */
- @Nullable private ImmutableMap<String, String> customKeyedValues;
-
Change() {}
public Change(
@@ -527,7 +523,6 @@
reviewStarted = other.reviewStarted;
revertOf = other.revertOf;
cherryPickOf = other.cherryPickOf;
- customKeyedValues = other.customKeyedValues;
}
/** 32 bit integer identity for a change. */
@@ -718,14 +713,6 @@
this.cherryPickOf = cherryPickOf;
}
- public void setCustomKeyedValues(ImmutableMap<String, String> customKeyedValues) {
- this.customKeyedValues = customKeyedValues;
- }
-
- public ImmutableMap<String, String> getCustomKeyedValues() {
- return customKeyedValues;
- }
-
@Override
public String toString() {
return new StringBuilder(getClass().getSimpleName())
diff --git a/java/com/google/gerrit/server/account/AccountConfig.java b/java/com/google/gerrit/server/account/AccountConfig.java
index f2331e3..e4fa718 100644
--- a/java/com/google/gerrit/server/account/AccountConfig.java
+++ b/java/com/google/gerrit/server/account/AccountConfig.java
@@ -95,7 +95,7 @@
}
@Override
- protected String getRefName() {
+ public String getRefName() {
return ref;
}
@@ -186,7 +186,7 @@
* @return the new account
* @throws DuplicateKeyException if the user branch already exists
*/
- Account getNewAccount(Instant registeredOn) throws DuplicateKeyException {
+ public Account getNewAccount(Instant registeredOn) throws DuplicateKeyException {
checkLoaded();
if (revision != null) {
throw new DuplicateKeyException(String.format("account %s already exists", accountId));
diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java
index e801b14..2080e93 100644
--- a/java/com/google/gerrit/server/account/AccountsUpdate.java
+++ b/java/com/google/gerrit/server/account/AccountsUpdate.java
@@ -14,123 +14,39 @@
package com.google.gerrit.server.account;
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkState;
-import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.ACCOUNTS_UPDATE;
-import static java.util.Objects.requireNonNull;
-import static java.util.stream.Collectors.toList;
-import static java.util.stream.Collectors.toSet;
+import static java.lang.annotation.ElementType.FIELD;
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Strings;
-import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
-import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.exceptions.DuplicateKeyException;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.git.LockFailureException;
-import com.google.gerrit.git.RefUpdateUtil;
-import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.externalids.ExternalId;
-import com.google.gerrit.server.account.externalids.ExternalIdNotes;
-import com.google.gerrit.server.account.externalids.ExternalIdNotes.ExternalIdNotesLoader;
-import com.google.gerrit.server.account.externalids.ExternalIds;
-import com.google.gerrit.server.account.storage.notedb.AccountsNoteDbImpl;
-import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.config.CachedPreferences;
-import com.google.gerrit.server.config.VersionedDefaultPreferences;
-import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
-import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.meta.MetaDataUpdate;
-import com.google.gerrit.server.index.change.ReindexAfterRefUpdate;
import com.google.gerrit.server.notedb.Sequences;
-import com.google.gerrit.server.update.RetryHelper;
-import com.google.gerrit.server.update.RetryableAction.Action;
-import com.google.gerrit.server.update.context.RefUpdateContext;
-import com.google.inject.Provider;
-import com.google.inject.assistedinject.Assisted;
-import com.google.inject.assistedinject.AssistedInject;
+import com.google.inject.BindingAnnotation;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.HashSet;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
import java.util.List;
-import java.util.Objects;
import java.util.Optional;
-import java.util.Set;
import java.util.function.Consumer;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.BatchRefUpdate;
-import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.PersonIdent;
-import org.eclipse.jgit.lib.RefUpdate;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.transport.ReceiveCommand;
/**
* Creates and updates accounts.
*
- * <p>This class should be used for all account updates. See {@link AccountDelta} for what can be
- * updated.
- *
- * <p>Batch updates of multiple different accounts can be performed atomically, see {@link
- * #updateBatch(List)}. Batch creation is not supported.
- *
- * <p>For any account update the caller must provide a commit message, the account ID and an {@link
- * ConfigureDeltaFromState}. The account updater reads the current {@link AccountState} and prepares
- * updates to the account by calling setters on the provided {@link AccountDelta.Builder}. If the
- * current account state is of no interest the caller may also provide a {@link Consumer} for {@link
- * AccountDelta.Builder} instead of the account updater.
- *
- * <p>The provided commit message is used for the update of the user branch. Using a precise and
- * unique commit message allows to identify the code from which an update was made when looking at a
- * commit in the user branch, and thus help debugging.
+ * <p>This interface should be used for all account updates. See {@link AccountDelta} for what can
+ * be updated.
*
* <p>For creating a new account a new account ID can be retrieved from {@link
* Sequences#nextAccountId()}.
*
- * <p>The account updates are written to NoteDb. In NoteDb accounts are represented as user branches
- * in the {@code All-Users} repository. Optionally a user branch can contain a 'account.config' file
- * that stores account properties, such as full name, display name, preferred email, status and the
- * active flag. The timestamp of the first commit on a user branch denotes the registration date.
- * The initial commit on the user branch may be empty (since having an 'account.config' is
- * optional). See {@link AccountConfig} for details of the 'account.config' file format. In addition
- * the user branch can contain a 'preferences.config' config file to store preferences (see {@link
- * StoredPreferences}) and a 'watch.config' config file to store project watches (see {@link
- * ProjectWatches}). External IDs are stored separately in the {@code refs/meta/external-ids} notes
- * branch (see {@link ExternalIdNotes}).
- *
- * <p>On updating an account the account is evicted from the account cache and reindexed. The
- * eviction from the account cache and the reindexing is done by the {@link ReindexAfterRefUpdate}
- * class which receives the event about updating the user branch that is triggered by this class.
- *
- * <p>If external IDs are updated, the ExternalIdCache is automatically updated by {@link
- * ExternalIdNotes}. In addition {@link ExternalIdNotes} takes care about evicting and reindexing
- * corresponding accounts. This is needed because external ID updates don't touch the user branches.
- * Hence in this case the accounts are not evicted and reindexed via {@link ReindexAfterRefUpdate}.
- *
- * <p>Reindexing and flushing accounts from the account cache can be disabled by
- *
- * <ul>
- * <li>binding {@link GitReferenceUpdated#DISABLED} and
- * <li>passing an {@link
- * com.google.gerrit.server.account.externalids.ExternalIdNotes.FactoryNoReindex} factory as
- * parameter of {@link AccountsUpdate.Factory#create(IdentifiedUser,
- * ExternalIdNotes.ExternalIdNotesLoader)}
- * </ul>
- *
- * <p>If there are concurrent account updates updating the user branch in NoteDb may fail with
- * {@link LockFailureException}. In this case the account update is automatically retried and the
- * account updater is invoked once more with the updated account state. This means the whole
- * read-modify-write sequence is atomic. Retrying is limited by a timeout. If the timeout is
- * exceeded the account update can still fail with {@link LockFailureException}.
+ * <p>See the implementing classes for more information.
*/
-public class AccountsUpdate {
- public interface Factory {
+public interface AccountsUpdate {
+ abstract class AccountsUpdateLoader {
/**
* Creates an {@code AccountsUpdate} which uses the identity of the specified user as author for
* all commits related to accounts. The server identity will be used as committer.
@@ -140,9 +56,8 @@
* AccountsUpdate} instead.
*
* @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(IdentifiedUser currentUser, ExternalIdNotesLoader externalIdNotesLoader);
+ public abstract AccountsUpdate create(IdentifiedUser currentUser);
/**
* Creates an {@code AccountsUpdate} which uses the server identity as author and committer for
@@ -151,10 +66,34 @@
* <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);
+ public abstract AccountsUpdate createWithServerIdent();
+
+ @BindingAnnotation
+ @Target({FIELD, PARAMETER, METHOD})
+ @Retention(RUNTIME)
+ public @interface WithReindex {}
+
+ @BindingAnnotation
+ @Target({FIELD, PARAMETER, METHOD})
+ @Retention(RUNTIME)
+ public @interface NoReindex {}
+ }
+
+ /** Data holder for the set of arguments required to update an account. Used for batch updates. */
+ class UpdateArguments {
+ public final String message;
+ public final Account.Id accountId;
+ public final AccountsUpdate.ConfigureDeltaFromState configureDeltaFromState;
+
+ public UpdateArguments(
+ String message,
+ Account.Id accountId,
+ AccountsUpdate.ConfigureDeltaFromState configureDeltaFromState) {
+ this.message = message;
+ this.accountId = accountId;
+ this.configureDeltaFromState = configureDeltaFromState;
+ }
}
/**
@@ -162,13 +101,13 @@
* delta to be applied to it in a later step. This is done by implementing this interface.
*
* <p>If the current account state is not needed, use a {@link Consumer} of {@link
- * AccountDelta.Builder} instead.
+ * com.google.gerrit.server.account.AccountDelta.Builder} instead.
*/
@FunctionalInterface
- public interface ConfigureDeltaFromState {
+ interface ConfigureDeltaFromState {
/**
* Receives the current {@link AccountState} (which is immutable) and configures an {@link
- * AccountDelta.Builder} with changes to the account.
+ * com.google.gerrit.server.account.AccountDelta.Builder} with changes to the account.
*
* @param accountState the state of the account that is being updated
* @param delta the changes to be applied
@@ -176,145 +115,18 @@
void configure(AccountState accountState, AccountDelta.Builder delta) throws IOException;
}
- /** Data holder for the set of arguments required to update an account. Used for batch updates. */
- public static class UpdateArguments {
- private final String message;
- private final Account.Id accountId;
- private final ConfigureDeltaFromState configureDeltaFromState;
-
- public UpdateArguments(
- String message, Account.Id accountId, ConfigureDeltaFromState configureDeltaFromState) {
- this.message = message;
- this.accountId = accountId;
- this.configureDeltaFromState = configureDeltaFromState;
- }
- }
-
- private final GitRepositoryManager repoManager;
- private final GitReferenceUpdated gitRefUpdated;
- private final Optional<IdentifiedUser> currentUser;
- private final AllUsersName allUsersName;
- private final ExternalIds externalIds;
- private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
- private final RetryHelper retryHelper;
- private final ExternalIdNotesLoader extIdNotesLoader;
- private final PersonIdent committerIdent;
- private final PersonIdent authorIdent;
-
- /** Invoked after reading the account config. */
- private final Runnable afterReadRevision;
-
- /** Invoked after updating the account but before committing the changes. */
- private final Runnable beforeCommit;
-
- /** Single instance that accumulates updates from the batch. */
- @Nullable private ExternalIdNotes externalIdNotes;
-
- @AssistedInject
- AccountsUpdate(
- GitRepositoryManager repoManager,
- GitReferenceUpdated gitRefUpdated,
- AllUsersName allUsersName,
- ExternalIds externalIds,
- Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
- RetryHelper retryHelper,
- @GerritPersonIdent PersonIdent serverIdent,
- @Assisted ExternalIdNotesLoader extIdNotesLoader) {
- this(
- repoManager,
- gitRefUpdated,
- Optional.empty(),
- allUsersName,
- externalIds,
- metaDataUpdateInternalFactory,
- retryHelper,
- extIdNotesLoader,
- serverIdent,
- createPersonIdent(serverIdent, Optional.empty()),
- AccountsUpdate::doNothing,
- AccountsUpdate::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)),
- AccountsUpdate::doNothing,
- AccountsUpdate::doNothing);
- }
-
- @VisibleForTesting
- public AccountsUpdate(
- GitRepositoryManager repoManager,
- GitReferenceUpdated gitRefUpdated,
- Optional<IdentifiedUser> currentUser,
- AllUsersName allUsersName,
- ExternalIds externalIds,
- Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
- RetryHelper retryHelper,
- ExternalIdNotesLoader extIdNotesLoader,
- PersonIdent committerIdent,
- PersonIdent authorIdent,
- Runnable afterReadRevision,
- Runnable beforeCommit) {
- this.repoManager = requireNonNull(repoManager, "repoManager");
- this.gitRefUpdated = requireNonNull(gitRefUpdated, "gitRefUpdated");
- this.currentUser = currentUser;
- this.allUsersName = requireNonNull(allUsersName, "allUsersName");
- this.externalIds = requireNonNull(externalIds, "externalIds");
- this.metaDataUpdateInternalFactory =
- requireNonNull(metaDataUpdateInternalFactory, "metaDataUpdateInternalFactory");
- this.retryHelper = requireNonNull(retryHelper, "retryHelper");
- this.extIdNotesLoader = requireNonNull(extIdNotesLoader, "extIdNotesLoader");
- this.committerIdent = requireNonNull(committerIdent, "committerIdent");
- this.authorIdent = requireNonNull(authorIdent, "authorIdent");
- this.afterReadRevision = requireNonNull(afterReadRevision, "afterReadRevision");
- this.beforeCommit = requireNonNull(beforeCommit, "beforeCommit");
- }
-
/** Returns an instance that runs all specified consumers. */
- public static ConfigureDeltaFromState joinConsumers(
- List<Consumer<AccountDelta.Builder>> consumers) {
+ static ConfigureDeltaFromState joinConsumers(List<Consumer<AccountDelta.Builder>> consumers) {
return (accountStateIgnored, update) -> consumers.forEach(c -> c.accept(update));
}
- private static ConfigureDeltaFromState fromConsumer(Consumer<AccountDelta.Builder> consumer) {
- return (a, u) -> consumer.accept(u);
- }
-
- private static PersonIdent createPersonIdent(
- PersonIdent serverIdent, Optional<IdentifiedUser> user) {
- return user.isPresent() ? user.get().newCommitterIdent(serverIdent) : serverIdent;
- }
-
/**
* Like {@link #insert(String, Account.Id, ConfigureDeltaFromState)}, but using a {@link Consumer}
* instead, i.e. the update does not depend on the current account state (which, for insertion,
* would only contain the account ID).
*/
- public AccountState insert(
- String message, Account.Id accountId, Consumer<AccountDelta.Builder> init)
- throws IOException, ConfigInvalidException {
- return insert(message, accountId, fromConsumer(init));
- }
+ AccountState insert(String message, Account.Id accountId, Consumer<AccountDelta.Builder> init)
+ throws IOException, ConfigInvalidException;
/**
* Inserts a new account.
@@ -327,41 +139,16 @@
* @throws IOException if creating the user branch fails due to an IO error
* @throws ConfigInvalidException if any of the account fields has an invalid value
*/
- public AccountState insert(String message, Account.Id accountId, ConfigureDeltaFromState init)
- throws IOException, ConfigInvalidException {
- return execute(
- ImmutableList.of(
- repo -> {
- AccountConfig accountConfig = read(repo, accountId);
- Account account = accountConfig.getNewAccount(committerIdent.getWhenAsInstant());
- AccountState accountState = AccountState.forAccount(account);
- AccountDelta.Builder deltaBuilder = AccountDelta.builder();
- init.configure(accountState, deltaBuilder);
-
- AccountDelta accountDelta = deltaBuilder.build();
- accountConfig.setAccountDelta(accountDelta);
- externalIdNotes =
- createExternalIdNotes(
- repo, accountConfig.getExternalIdsRev(), accountId, accountDelta);
- CachedPreferences defaultPreferences =
- CachedPreferences.fromConfig(
- VersionedDefaultPreferences.get(repo, allUsersName));
-
- return new UpdatedAccount(message, accountConfig, defaultPreferences, true);
- }))
- .get(0)
- .get();
- }
+ AccountState insert(String message, Account.Id accountId, ConfigureDeltaFromState init)
+ throws IOException, ConfigInvalidException;
/**
* Like {@link #update(String, Account.Id, ConfigureDeltaFromState)}, but using a {@link Consumer}
* instead, i.e. the update does not depend on the current account state.
*/
- public Optional<AccountState> update(
+ Optional<AccountState> update(
String message, Account.Id accountId, Consumer<AccountDelta.Builder> update)
- throws IOException, ConfigInvalidException {
- return update(message, accountId, fromConsumer(update));
- }
+ throws IOException, ConfigInvalidException;
/**
* Gets the account and updates it atomically.
@@ -378,76 +165,9 @@
* after the retry timeout exceeded
* @throws ConfigInvalidException if any of the account fields has an invalid value
*/
- public Optional<AccountState> update(
+ Optional<AccountState> update(
String message, Account.Id accountId, ConfigureDeltaFromState configureDeltaFromState)
- throws LockFailureException, IOException, ConfigInvalidException {
- return updateBatch(
- ImmutableList.of(new UpdateArguments(message, accountId, configureDeltaFromState)))
- .get(0);
- }
-
- /**
- * Deletes all the account state data.
- *
- * @param message commit message for the account update, must not be {@code null or empty}
- * @param accountId ID of the account
- * @throws IOException if updating the user branch fails due to an IO error
- * @throws ConfigInvalidException if any of the account fields has an invalid value
- */
- public void delete(String message, Account.Id accountId)
- throws IOException, ConfigInvalidException {
- ImmutableSet<ExternalId> accountExternalIds = externalIds.byAccount(accountId);
- Consumer<AccountDelta.Builder> delta =
- deltaBuilder -> deltaBuilder.deleteAccount(accountExternalIds);
- update(message, accountId, delta);
- }
-
- private ExecutableUpdate createExecutableUpdate(UpdateArguments updateArguments) {
- return repo -> {
- AccountConfig accountConfig = read(repo, updateArguments.accountId);
- CachedPreferences defaultPreferences =
- CachedPreferences.fromConfig(VersionedDefaultPreferences.get(repo, allUsersName));
- Optional<AccountState> accountState =
- AccountsNoteDbImpl.getFromAccountConfig(externalIds, accountConfig, defaultPreferences);
- if (!accountState.isPresent()) {
- return null;
- }
-
- AccountDelta.Builder deltaBuilder = AccountDelta.builder();
- updateArguments.configureDeltaFromState.configure(accountState.get(), deltaBuilder);
-
- AccountDelta delta = deltaBuilder.build();
-
- ExternalIdNotes.checkSameAccount(
- Iterables.concat(
- delta.getCreatedExternalIds(),
- delta.getUpdatedExternalIds(),
- delta.getDeletedExternalIds()),
- updateArguments.accountId);
-
- if (delta.hasExternalIdUpdates()) {
- // Only load the externalIds if they are going to be updated
- // This makes e.g. preferences updates faster.
- if (externalIdNotes == null) {
- externalIdNotes =
- extIdNotesLoader.load(
- repo, accountConfig.getExternalIdsRev().orElse(ObjectId.zeroId()));
- }
- externalIdNotes.replace(delta.getDeletedExternalIds(), delta.getCreatedExternalIds());
- externalIdNotes.upsert(delta.getUpdatedExternalIds());
- }
-
- if (delta.getShouldDeleteAccount().orElse(false)) {
- return new DeletedAccount(updateArguments.message, accountConfig.getRefName());
- }
-
- accountConfig.setAccountDelta(delta);
- CachedPreferences cachedDefaultPreferences =
- CachedPreferences.fromConfig(VersionedDefaultPreferences.get(repo, allUsersName));
- return new UpdatedAccount(
- updateArguments.message, accountConfig, cachedDefaultPreferences, false);
- };
- }
+ throws LockFailureException, IOException, ConfigInvalidException;
/**
* Updates multiple different accounts atomically. This will only store a single new value (aka
@@ -459,238 +179,16 @@
* the error. Callers should be aware that a single "update of death" (or a set of updates that
* together have this property) will always prevent the entire batch from being executed.
*/
- public ImmutableList<Optional<AccountState>> updateBatch(List<UpdateArguments> updates)
- throws IOException, ConfigInvalidException {
- checkArgument(
- updates.stream().map(u -> u.accountId.get()).distinct().count() == updates.size(),
- "updates must all be for different accounts");
- return execute(updates.stream().map(this::createExecutableUpdate).collect(toList()));
- }
+ ImmutableList<Optional<AccountState>> updateBatch(List<UpdateArguments> updates)
+ throws IOException, ConfigInvalidException;
- private AccountConfig read(Repository allUsersRepo, Account.Id accountId)
- throws IOException, ConfigInvalidException {
- AccountConfig accountConfig = new AccountConfig(accountId, allUsersName, allUsersRepo).load();
- afterReadRevision.run();
- return accountConfig;
- }
-
- private ImmutableList<Optional<AccountState>> execute(List<ExecutableUpdate> executableUpdates)
- throws IOException, ConfigInvalidException {
- try (RefUpdateContext ctx = RefUpdateContext.open(ACCOUNTS_UPDATE)) {
- List<Optional<AccountState>> accountState = new ArrayList<>();
- List<UpdatedAccount> updatedAccounts = new ArrayList<>();
- executeWithRetry(
- () -> {
-
- // Reset state for retry.
- externalIdNotes = null;
- accountState.clear();
- updatedAccounts.clear();
- try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
- for (ExecutableUpdate executableUpdate : executableUpdates) {
- updatedAccounts.add(executableUpdate.execute(allUsersRepo));
- }
- commit(
- allUsersRepo,
- updatedAccounts.stream().filter(Objects::nonNull).collect(toList()));
- for (UpdatedAccount ua : updatedAccounts) {
- accountState.add(
- ua == null || ua.deleted ? Optional.empty() : ua.getAccountState());
- }
- }
- return null;
- });
-
- return ImmutableList.copyOf(accountState);
- }
- }
-
- private void executeWithRetry(Action<Void> action) throws IOException, ConfigInvalidException {
- try {
- retryHelper.accountUpdate("updateAccount", action).call();
- } catch (Exception e) {
- Throwables.throwIfUnchecked(e);
- Throwables.throwIfInstanceOf(e, IOException.class);
- Throwables.throwIfInstanceOf(e, ConfigInvalidException.class);
- throw new StorageException(e);
- }
- }
-
- private ExternalIdNotes createExternalIdNotes(
- Repository allUsersRepo, Optional<ObjectId> rev, Account.Id accountId, AccountDelta update)
- throws IOException, ConfigInvalidException, DuplicateKeyException {
- ExternalIdNotes.checkSameAccount(
- Iterables.concat(
- update.getCreatedExternalIds(),
- update.getUpdatedExternalIds(),
- update.getDeletedExternalIds()),
- accountId);
-
- ExternalIdNotes extIdNotes = extIdNotesLoader.load(allUsersRepo, rev.orElse(ObjectId.zeroId()));
- extIdNotes.replace(update.getDeletedExternalIds(), update.getCreatedExternalIds());
- extIdNotes.upsert(update.getUpdatedExternalIds());
- return extIdNotes;
- }
-
- private void commit(Repository allUsersRepo, List<UpdatedAccount> updatedAccounts)
- throws IOException {
- if (updatedAccounts.isEmpty()) {
- return;
- }
-
- beforeCommit.run();
-
- BatchRefUpdate batchRefUpdate = allUsersRepo.getRefDatabase().newBatchUpdate();
- Set<Account.Id> accountsToSkipForReindex = new HashSet<>();
- // External ids may be not updated if:
- // * externalIdNotes is not loaded (there were no externalId updates in the delta)
- // * new revCommit is identical to the previous externalId tip
- boolean externalIdsUpdated = false;
- if (externalIdNotes != null) {
- String externalIdUpdateMessage =
- updatedAccounts.size() == 1
- ? Iterables.getOnlyElement(updatedAccounts).message
- : "Batch update for " + updatedAccounts.size() + " accounts";
- ObjectId oldExternalIdsRevision = externalIdNotes.getRevision();
- // These update the same ref, so they need to be stacked on top of one another using the same
- // ExternalIdNotes instance.
- RevCommit revCommit =
- commitExternalIdUpdates(externalIdUpdateMessage, allUsersRepo, batchRefUpdate);
- externalIdsUpdated = !Objects.equals(revCommit.getId(), oldExternalIdsRevision);
- }
- for (UpdatedAccount updatedAccount : updatedAccounts) {
- if (updatedAccount.deleted) {
- RefUpdate ru = RefUpdateUtil.deleteChecked(allUsersRepo, updatedAccount.refName);
- gitRefUpdated.fire(allUsersName, ru, ReceiveCommand.Type.DELETE, null);
- accountsToSkipForReindex.add(Account.Id.fromRef(updatedAccount.refName));
- continue;
- }
- // These updates are all for different refs (because batches never update the same account
- // more than once), so there can be multiple commits in the same batch, all with the same base
- // revision in their AccountConfig.
- // We allow empty commits:
- // 1) When creating a new account, so that the user branch gets created with an empty commit
- // when no account properties are set and hence no
- // 'account.config' file will be created.
- // 2) When updating "refs/meta/external-ids", so that refs/users/* meta ref is updated too.
- // This allows to schedule reindexing of account transactionally on refs/users/* meta
- // updates.
- boolean allowEmptyCommit = externalIdsUpdated || updatedAccount.created;
- commitAccountConfig(
- updatedAccount.message,
- allUsersRepo,
- batchRefUpdate,
- updatedAccount.accountConfig,
- allowEmptyCommit);
- }
-
- RefUpdateUtil.executeChecked(batchRefUpdate, allUsersRepo);
-
- if (externalIdsUpdated) {
- accountsToSkipForReindex.addAll(getUpdatedAccountIds(batchRefUpdate));
- extIdNotesLoader.updateExternalIdCacheAndMaybeReindexAccounts(
- externalIdNotes, accountsToSkipForReindex);
- }
-
- gitRefUpdated.fire(
- allUsersName, batchRefUpdate, currentUser.map(IdentifiedUser::state).orElse(null));
- }
-
- private static Set<Account.Id> getUpdatedAccountIds(BatchRefUpdate batchRefUpdate) {
- return batchRefUpdate.getCommands().stream()
- .map(c -> Account.Id.fromRef(c.getRefName()))
- .filter(Objects::nonNull)
- .collect(toSet());
- }
-
- private void commitAccountConfig(
- String message,
- Repository allUsersRepo,
- BatchRefUpdate batchRefUpdate,
- AccountConfig accountConfig,
- boolean allowEmptyCommit)
- throws IOException {
- try (MetaDataUpdate md = createMetaDataUpdate(message, allUsersRepo, batchRefUpdate)) {
- md.setAllowEmpty(allowEmptyCommit);
- accountConfig.commit(md);
- }
- }
-
- private RevCommit commitExternalIdUpdates(
- String message, Repository allUsersRepo, BatchRefUpdate batchRefUpdate) throws IOException {
- try (MetaDataUpdate md = createMetaDataUpdate(message, allUsersRepo, batchRefUpdate)) {
- return externalIdNotes.commit(md);
- }
- }
-
- private MetaDataUpdate createMetaDataUpdate(
- String message, Repository allUsersRepo, BatchRefUpdate batchRefUpdate) {
- MetaDataUpdate metaDataUpdate =
- metaDataUpdateInternalFactory.get().create(allUsersName, allUsersRepo, batchRefUpdate);
- if (!message.endsWith("\n")) {
- message = message + "\n";
- }
-
- metaDataUpdate.getCommitBuilder().setMessage(message);
- metaDataUpdate.getCommitBuilder().setCommitter(committerIdent);
- metaDataUpdate.getCommitBuilder().setAuthor(authorIdent);
- return metaDataUpdate;
- }
-
- private static void doNothing() {}
-
- @FunctionalInterface
- private interface ExecutableUpdate {
- UpdatedAccount execute(Repository allUsersRepo) throws IOException, ConfigInvalidException;
- }
-
- private class UpdatedAccount {
- final String message;
- final AccountConfig accountConfig;
- final CachedPreferences defaultPreferences;
- final String refName;
- final boolean created;
- final boolean deleted;
-
- UpdatedAccount(
- String message,
- AccountConfig accountConfig,
- CachedPreferences defaultPreferences,
- boolean created) {
- this(
- message,
- requireNonNull(accountConfig),
- defaultPreferences,
- accountConfig.getRefName(),
- created,
- false);
- }
-
- protected UpdatedAccount(
- String message,
- AccountConfig accountConfig,
- CachedPreferences defaultPreferences,
- String refName,
- boolean created,
- boolean deleted) {
- checkState(!Strings.isNullOrEmpty(message), "message for account update must be set");
- this.message = requireNonNull(message);
- this.accountConfig = accountConfig;
- this.defaultPreferences = defaultPreferences;
- this.refName = refName;
- this.created = created;
- this.deleted = deleted;
- }
-
- Optional<AccountState> getAccountState() throws IOException {
- return AccountsNoteDbImpl.getFromAccountConfig(
- externalIds, accountConfig, externalIdNotes, defaultPreferences);
- }
- }
-
- private class DeletedAccount extends UpdatedAccount {
- DeletedAccount(String message, String refName) {
- super(message, null, null, refName, false, true);
- }
- }
+ /**
+ * Deletes all the account state data.
+ *
+ * @param message commit message for the account update, must not be {@code null or empty}
+ * @param accountId ID of the account
+ * @throws IOException if updating the user branch fails due to an IO error
+ * @throws ConfigInvalidException if any of the account fields has an invalid value
+ */
+ void delete(String message, Account.Id accountId) throws IOException, ConfigInvalidException;
}
diff --git a/java/com/google/gerrit/server/account/storage/notedb/AccountNoteDbStorageModule.java b/java/com/google/gerrit/server/account/storage/notedb/AccountNoteDbStorageModule.java
index 4cbe48f..51f6b86 100644
--- a/java/com/google/gerrit/server/account/storage/notedb/AccountNoteDbStorageModule.java
+++ b/java/com/google/gerrit/server/account/storage/notedb/AccountNoteDbStorageModule.java
@@ -14,13 +14,25 @@
package com.google.gerrit.server.account.storage.notedb;
+import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.server.account.Accounts;
+import com.google.gerrit.server.account.AccountsUpdate;
+import com.google.gerrit.server.account.externalids.ExternalIdUpsertPreprocessor;
import com.google.inject.AbstractModule;
import com.google.inject.Singleton;
public class AccountNoteDbStorageModule extends AbstractModule {
@Override
protected void configure() {
- binder().bind(Accounts.class).to(AccountsNoteDbImpl.class).in(Singleton.class);
+ bind(Accounts.class).to(AccountsNoteDbImpl.class).in(Singleton.class);
+
+ DynamicMap.mapOf(binder(), ExternalIdUpsertPreprocessor.class);
+
+ bind(AccountsUpdate.AccountsUpdateLoader.class)
+ .annotatedWith(AccountsUpdate.AccountsUpdateLoader.WithReindex.class)
+ .to(AccountsUpdateNoteDbImpl.Factory.class);
+ bind(AccountsUpdate.AccountsUpdateLoader.class)
+ .annotatedWith(AccountsUpdate.AccountsUpdateLoader.NoReindex.class)
+ .to(AccountsUpdateNoteDbImpl.FactoryNoReindex.class);
}
}
diff --git a/java/com/google/gerrit/server/account/storage/notedb/AccountsNoteDbImpl.java b/java/com/google/gerrit/server/account/storage/notedb/AccountsNoteDbImpl.java
index ce8cd4d..9fb7917 100644
--- a/java/com/google/gerrit/server/account/storage/notedb/AccountsNoteDbImpl.java
+++ b/java/com/google/gerrit/server/account/storage/notedb/AccountsNoteDbImpl.java
@@ -141,7 +141,7 @@
* @return the account state, {@link Optional#empty()} if the account doesn't exist
* @throws IOException if accessing the external IDs fails
*/
- public static Optional<AccountState> getFromAccountConfig(
+ static Optional<AccountState> getFromAccountConfig(
ExternalIds externalIds, AccountConfig accountConfig, CachedPreferences defaultPreferences)
throws IOException {
return getFromAccountConfig(externalIds, accountConfig, null, defaultPreferences);
@@ -164,7 +164,7 @@
* @return the account state, {@link Optional#empty()} if the account doesn't exist
* @throws IOException if accessing the external IDs fails
*/
- public static Optional<AccountState> getFromAccountConfig(
+ static Optional<AccountState> getFromAccountConfig(
ExternalIds externalIds,
AccountConfig accountConfig,
@Nullable ExternalIdNotes extIdNotes,
diff --git a/java/com/google/gerrit/server/account/storage/notedb/AccountsUpdateNoteDbImpl.java b/java/com/google/gerrit/server/account/storage/notedb/AccountsUpdateNoteDbImpl.java
new file mode 100644
index 0000000..cc8ef14
--- /dev/null
+++ b/java/com/google/gerrit/server/account/storage/notedb/AccountsUpdateNoteDbImpl.java
@@ -0,0 +1,642 @@
+// 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.account.storage.notedb;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.ACCOUNTS_UPDATE;
+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toSet;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.exceptions.DuplicateKeyException;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.git.LockFailureException;
+import com.google.gerrit.git.RefUpdateUtil;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountConfig;
+import com.google.gerrit.server.account.AccountDelta;
+import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.account.AccountsUpdate;
+import com.google.gerrit.server.account.ProjectWatches;
+import com.google.gerrit.server.account.StoredPreferences;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdNotes;
+import com.google.gerrit.server.account.externalids.ExternalIds;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.CachedPreferences;
+import com.google.gerrit.server.config.VersionedDefaultPreferences;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.index.change.ReindexAfterRefUpdate;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryableAction.Action;
+import com.google.gerrit.server.update.context.RefUpdateContext;
+import com.google.inject.Provider;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Consumer;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+/**
+ * Creates and updates accounts which are stored in All-Users NoteDB repository.
+ *
+ * <p>Batch updates of multiple different accounts can be performed atomically, see {@link
+ * #updateBatch(List)}. Batch creation is not supported.
+ *
+ * <p>For any account update the caller must provide a commit message, the account ID and an {@link
+ * com.google.gerrit.server.account.AccountsUpdate.ConfigureDeltaFromState}. The account updater
+ * reads the current {@link AccountState} and prepares updates to the account by calling setters on
+ * the provided {@link com.google.gerrit.server.account.AccountDelta.Builder}. If the current
+ * account state is of no interest the caller may also provide a {@link Consumer} for {@link
+ * com.google.gerrit.server.account.AccountDelta.Builder} instead of the account updater.
+ *
+ * <p>The provided commit message is used for the update of the user branch. Using a precise and
+ * unique commit message allows to identify the code from which an update was made when looking at a
+ * commit in the user branch, and thus help debugging.
+ *
+ * <p>The account updates are written to NoteDb. In NoteDb accounts are represented as user branches
+ * in the {@code All-Users} repository. Optionally a user branch can contain a 'account.config' file
+ * that stores account properties, such as full name, display name, preferred email, status and the
+ * active flag. The timestamp of the first commit on a user branch denotes the registration date.
+ * The initial commit on the user branch may be empty (since having an 'account.config' is
+ * optional). See {@link AccountConfig} for details of the 'account.config' file format. In addition
+ * the user branch can contain a 'preferences.config' config file to store preferences (see {@link
+ * StoredPreferences}) and a 'watch.config' config file to store project watches (see {@link
+ * ProjectWatches}). External IDs are stored separately in the {@code refs/meta/external-ids} notes
+ * branch (see {@link ExternalIdNotes}).
+ *
+ * <p>On updating an account the account is evicted from the account cache and reindexed. The
+ * eviction from the account cache and the reindexing is done by the {@link ReindexAfterRefUpdate}
+ * class which receives the event about updating the user branch that is triggered by this class.
+ *
+ * <p>If external IDs are updated, the ExternalIdCache is automatically updated by {@link
+ * ExternalIdNotes}. In addition {@link ExternalIdNotes} takes care about evicting and reindexing
+ * corresponding accounts. This is needed because external ID updates don't touch the user branches.
+ * Hence in this case the accounts are not evicted and reindexed via {@link ReindexAfterRefUpdate}.
+ *
+ * <p>Reindexing and flushing accounts from the account cache can be disabled by
+ *
+ * <ul>
+ * <li>using {@link
+ * com.google.gerrit.server.account.storage.notedb.AccountsUpdateNoteDbImpl.FactoryNoReindex}
+ * and
+ * <li>binding {@link GitReferenceUpdated#DISABLED}
+ * </ul>
+ *
+ * <p>If there are concurrent account updates which updating the user branch in NoteDb may fail with
+ * {@link LockFailureException}. In this case the account update is automatically retried and the
+ * account updater is invoked once more with the updated account state. This means the whole
+ * read-modify-write sequence is atomic. Retrying is limited by a timeout. If the timeout is
+ * exceeded the account update can still fail with {@link LockFailureException}.
+ */
+public class AccountsUpdateNoteDbImpl implements AccountsUpdate {
+ private static class AbstractFactory extends AccountsUpdateLoader {
+ GitRepositoryManager repoManager;
+ GitReferenceUpdated gitRefUpdated;
+ AllUsersName allUsersName;
+ ExternalIds externalIds;
+ ExternalIdNotes.ExternalIdNotesLoader extIdNotesFactory;
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
+ RetryHelper retryHelper;
+ PersonIdent serverIdent;
+
+ private AbstractFactory(
+ GitRepositoryManager repoManager,
+ GitReferenceUpdated gitRefUpdated,
+ AllUsersName allUsersName,
+ ExternalIds externalIds,
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
+ RetryHelper retryHelper,
+ @GerritPersonIdent PersonIdent serverIdent,
+ ExternalIdNotes.ExternalIdNotesLoader extIdNotesFactory) {
+ this.repoManager = repoManager;
+ this.gitRefUpdated = gitRefUpdated;
+ this.allUsersName = allUsersName;
+ this.externalIds = externalIds;
+ this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
+ this.retryHelper = retryHelper;
+ this.serverIdent = serverIdent;
+ this.extIdNotesFactory = extIdNotesFactory;
+ }
+
+ @Override
+ public AccountsUpdate create(IdentifiedUser currentUser) {
+ return new AccountsUpdateNoteDbImpl(
+ repoManager,
+ gitRefUpdated,
+ Optional.of(currentUser),
+ allUsersName,
+ externalIds,
+ extIdNotesFactory,
+ metaDataUpdateInternalFactory,
+ retryHelper,
+ serverIdent,
+ createPersonIdent(serverIdent, Optional.of(currentUser)),
+ AccountsUpdateNoteDbImpl::doNothing,
+ AccountsUpdateNoteDbImpl::doNothing);
+ }
+
+ @Override
+ public AccountsUpdate createWithServerIdent() {
+ return new AccountsUpdateNoteDbImpl(
+ repoManager,
+ gitRefUpdated,
+ Optional.empty(),
+ allUsersName,
+ externalIds,
+ extIdNotesFactory,
+ metaDataUpdateInternalFactory,
+ retryHelper,
+ serverIdent,
+ createPersonIdent(serverIdent, Optional.empty()),
+ AccountsUpdateNoteDbImpl::doNothing,
+ AccountsUpdateNoteDbImpl::doNothing);
+ }
+ }
+
+ @Singleton
+ public static class Factory extends AbstractFactory {
+ @Inject
+ Factory(
+ GitRepositoryManager repoManager,
+ GitReferenceUpdated gitRefUpdated,
+ AllUsersName allUsersName,
+ ExternalIds externalIds,
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
+ RetryHelper retryHelper,
+ @GerritPersonIdent PersonIdent serverIdent,
+ ExternalIdNotes.Factory extIdNotesFactory) {
+ super(
+ repoManager,
+ gitRefUpdated,
+ allUsersName,
+ externalIds,
+ metaDataUpdateInternalFactory,
+ retryHelper,
+ serverIdent,
+ extIdNotesFactory);
+ }
+ }
+
+ @Singleton
+ public static class FactoryNoReindex extends AbstractFactory {
+ @Inject
+ FactoryNoReindex(
+ GitRepositoryManager repoManager,
+ GitReferenceUpdated gitRefUpdated,
+ AllUsersName allUsersName,
+ ExternalIds externalIds,
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
+ RetryHelper retryHelper,
+ @GerritPersonIdent PersonIdent serverIdent,
+ ExternalIdNotes.FactoryNoReindex extIdNotesFactory) {
+ super(
+ repoManager,
+ gitRefUpdated,
+ allUsersName,
+ externalIds,
+ metaDataUpdateInternalFactory,
+ retryHelper,
+ serverIdent,
+ extIdNotesFactory);
+ }
+ }
+
+ private final GitRepositoryManager repoManager;
+ private final GitReferenceUpdated gitRefUpdated;
+ private final Optional<IdentifiedUser> currentUser;
+ private final AllUsersName allUsersName;
+ private final ExternalIds externalIds;
+
+ private final ExternalIdNotes.ExternalIdNotesLoader extIdNotesFactory;
+ private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
+ private final RetryHelper retryHelper;
+ private final PersonIdent committerIdent;
+ private final PersonIdent authorIdent;
+
+ /** Invoked after reading the account config. */
+ private final Runnable afterReadRevision;
+
+ /** Invoked after updating the account but before committing the changes. */
+ private final Runnable beforeCommit;
+
+ /** Single instance that accumulates updates from the batch. */
+ @Nullable private ExternalIdNotes externalIdNotes;
+
+ @VisibleForTesting
+ public AccountsUpdateNoteDbImpl(
+ GitRepositoryManager repoManager,
+ GitReferenceUpdated gitRefUpdated,
+ Optional<IdentifiedUser> currentUser,
+ AllUsersName allUsersName,
+ ExternalIds externalIds,
+ ExternalIdNotes.ExternalIdNotesLoader extIdNotesFactory,
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
+ RetryHelper retryHelper,
+ PersonIdent committerIdent,
+ PersonIdent authorIdent,
+ Runnable afterReadRevision,
+ Runnable beforeCommit) {
+ this.repoManager = requireNonNull(repoManager, "repoManager");
+ this.gitRefUpdated = requireNonNull(gitRefUpdated, "gitRefUpdated");
+ this.currentUser = currentUser;
+ this.allUsersName = requireNonNull(allUsersName, "allUsersName");
+ this.externalIds = requireNonNull(externalIds, "externalIds");
+ this.extIdNotesFactory = extIdNotesFactory;
+ this.metaDataUpdateInternalFactory =
+ requireNonNull(metaDataUpdateInternalFactory, "metaDataUpdateInternalFactory");
+ this.retryHelper = requireNonNull(retryHelper, "retryHelper");
+ this.committerIdent = requireNonNull(committerIdent, "committerIdent");
+ this.authorIdent = requireNonNull(authorIdent, "authorIdent");
+ this.afterReadRevision = requireNonNull(afterReadRevision, "afterReadRevision");
+ this.beforeCommit = requireNonNull(beforeCommit, "beforeCommit");
+ }
+
+ private static ConfigureDeltaFromState fromConsumer(Consumer<AccountDelta.Builder> consumer) {
+ return (a, u) -> consumer.accept(u);
+ }
+
+ private static PersonIdent createPersonIdent(
+ PersonIdent serverIdent, Optional<IdentifiedUser> user) {
+ return user.isPresent() ? user.get().newCommitterIdent(serverIdent) : serverIdent;
+ }
+
+ @Override
+ public AccountState insert(
+ String message, Account.Id accountId, Consumer<AccountDelta.Builder> init)
+ throws IOException, ConfigInvalidException {
+ return insert(message, accountId, fromConsumer(init));
+ }
+
+ @Override
+ public AccountState insert(String message, Account.Id accountId, ConfigureDeltaFromState init)
+ throws IOException, ConfigInvalidException {
+ return execute(
+ ImmutableList.of(
+ repo -> {
+ AccountConfig accountConfig = read(repo, accountId);
+ Account account = accountConfig.getNewAccount(committerIdent.getWhenAsInstant());
+ AccountState accountState = AccountState.forAccount(account);
+ AccountDelta.Builder deltaBuilder = AccountDelta.builder();
+ init.configure(accountState, deltaBuilder);
+
+ AccountDelta accountDelta = deltaBuilder.build();
+ accountConfig.setAccountDelta(accountDelta);
+ externalIdNotes =
+ createExternalIdNotes(
+ repo, accountConfig.getExternalIdsRev(), accountId, accountDelta);
+ CachedPreferences defaultPreferences =
+ CachedPreferences.fromConfig(
+ VersionedDefaultPreferences.get(repo, allUsersName));
+
+ return new UpdatedAccount(message, accountConfig, defaultPreferences, true);
+ }))
+ .get(0)
+ .get();
+ }
+
+ @Override
+ public Optional<AccountState> update(
+ String message, Account.Id accountId, Consumer<AccountDelta.Builder> update)
+ throws IOException, ConfigInvalidException {
+ return update(message, accountId, fromConsumer(update));
+ }
+
+ @Override
+ public Optional<AccountState> update(
+ String message, Account.Id accountId, ConfigureDeltaFromState configureDeltaFromState)
+ throws LockFailureException, IOException, ConfigInvalidException {
+ return updateBatch(
+ ImmutableList.of(new UpdateArguments(message, accountId, configureDeltaFromState)))
+ .get(0);
+ }
+
+ @Override
+ public void delete(String message, Account.Id accountId)
+ throws IOException, ConfigInvalidException {
+ ImmutableSet<ExternalId> accountExternalIds = externalIds.byAccount(accountId);
+ Consumer<AccountDelta.Builder> delta =
+ deltaBuilder -> deltaBuilder.deleteAccount(accountExternalIds);
+ update(message, accountId, delta);
+ }
+
+ private ExecutableUpdate createExecutableUpdate(UpdateArguments updateArguments) {
+ return repo -> {
+ AccountConfig accountConfig = read(repo, updateArguments.accountId);
+ CachedPreferences defaultPreferences =
+ CachedPreferences.fromConfig(VersionedDefaultPreferences.get(repo, allUsersName));
+ Optional<AccountState> accountState =
+ AccountsNoteDbImpl.getFromAccountConfig(externalIds, accountConfig, defaultPreferences);
+ if (!accountState.isPresent()) {
+ return null;
+ }
+
+ AccountDelta.Builder deltaBuilder = AccountDelta.builder();
+ updateArguments.configureDeltaFromState.configure(accountState.get(), deltaBuilder);
+
+ AccountDelta delta = deltaBuilder.build();
+ ExternalIdNotes.checkSameAccount(
+ Iterables.concat(
+ delta.getCreatedExternalIds(),
+ delta.getUpdatedExternalIds(),
+ delta.getDeletedExternalIds()),
+ updateArguments.accountId);
+
+ if (delta.hasExternalIdUpdates()) {
+ // Only load the externalIds if they are going to be updated
+ // This makes e.g. preferences updates faster.
+ if (externalIdNotes == null) {
+ externalIdNotes =
+ extIdNotesFactory.load(
+ repo, accountConfig.getExternalIdsRev().orElse(ObjectId.zeroId()));
+ }
+ externalIdNotes.replace(delta.getDeletedExternalIds(), delta.getCreatedExternalIds());
+ externalIdNotes.upsert(delta.getUpdatedExternalIds());
+ }
+
+ if (delta.getShouldDeleteAccount().orElse(false)) {
+ return new DeletedAccount(updateArguments.message, accountConfig.getRefName());
+ }
+
+ accountConfig.setAccountDelta(delta);
+ CachedPreferences cachedDefaultPreferences =
+ CachedPreferences.fromConfig(VersionedDefaultPreferences.get(repo, allUsersName));
+ return new UpdatedAccount(
+ updateArguments.message, accountConfig, cachedDefaultPreferences, false);
+ };
+ }
+
+ @Override
+ public ImmutableList<Optional<AccountState>> updateBatch(List<UpdateArguments> updates)
+ throws IOException, ConfigInvalidException {
+ checkArgument(
+ updates.stream().map(u -> u.accountId.get()).distinct().count() == updates.size(),
+ "updates must all be for different accounts");
+ return execute(updates.stream().map(this::createExecutableUpdate).collect(toList()));
+ }
+
+ private AccountConfig read(Repository allUsersRepo, Account.Id accountId)
+ throws IOException, ConfigInvalidException {
+ AccountConfig accountConfig = new AccountConfig(accountId, allUsersName, allUsersRepo).load();
+ afterReadRevision.run();
+ return accountConfig;
+ }
+
+ private ImmutableList<Optional<AccountState>> execute(List<ExecutableUpdate> executableUpdates)
+ throws IOException, ConfigInvalidException {
+ try (RefUpdateContext ctx = RefUpdateContext.open(ACCOUNTS_UPDATE)) {
+ List<Optional<AccountState>> accountState = new ArrayList<>();
+ List<UpdatedAccount> updatedAccounts = new ArrayList<>();
+ executeWithRetry(
+ () -> {
+
+ // Reset state for retry.
+ externalIdNotes = null;
+ accountState.clear();
+ updatedAccounts.clear();
+ try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
+ for (ExecutableUpdate executableUpdate : executableUpdates) {
+ updatedAccounts.add(executableUpdate.execute(allUsersRepo));
+ }
+ commit(
+ allUsersRepo,
+ updatedAccounts.stream().filter(Objects::nonNull).collect(toList()));
+ for (UpdatedAccount ua : updatedAccounts) {
+ accountState.add(
+ ua == null || ua.deleted ? Optional.empty() : ua.getAccountState());
+ }
+ }
+ return null;
+ });
+
+ return ImmutableList.copyOf(accountState);
+ }
+ }
+
+ private void executeWithRetry(Action<Void> action) throws IOException, ConfigInvalidException {
+ try {
+ retryHelper.accountUpdate("updateAccount", action).call();
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ Throwables.throwIfInstanceOf(e, IOException.class);
+ Throwables.throwIfInstanceOf(e, ConfigInvalidException.class);
+ throw new StorageException(e);
+ }
+ }
+
+ private ExternalIdNotes createExternalIdNotes(
+ Repository allUsersRepo, Optional<ObjectId> rev, Account.Id accountId, AccountDelta update)
+ throws IOException, ConfigInvalidException, DuplicateKeyException {
+ ExternalIdNotes.checkSameAccount(
+ Iterables.concat(
+ update.getCreatedExternalIds(),
+ update.getUpdatedExternalIds(),
+ update.getDeletedExternalIds()),
+ accountId);
+
+ ExternalIdNotes extIdNotes =
+ extIdNotesFactory.load(allUsersRepo, rev.orElse(ObjectId.zeroId()));
+ extIdNotes.replace(update.getDeletedExternalIds(), update.getCreatedExternalIds());
+ extIdNotes.upsert(update.getUpdatedExternalIds());
+ return extIdNotes;
+ }
+
+ private void commit(Repository allUsersRepo, List<UpdatedAccount> updatedAccounts)
+ throws IOException {
+ if (updatedAccounts.isEmpty()) {
+ return;
+ }
+
+ beforeCommit.run();
+
+ BatchRefUpdate batchRefUpdate = allUsersRepo.getRefDatabase().newBatchUpdate();
+ Set<Account.Id> accountsToSkipForReindex = new HashSet<>();
+ // External ids may be not updated if:
+ // * externalIdNotes is not loaded (there were no externalId updates in the delta)
+ // * new revCommit is identical to the previous externalId tip
+ boolean externalIdsUpdated = false;
+ if (externalIdNotes != null) {
+ String externalIdUpdateMessage =
+ updatedAccounts.size() == 1
+ ? Iterables.getOnlyElement(updatedAccounts).message
+ : "Batch update for " + updatedAccounts.size() + " accounts";
+ ObjectId oldExternalIdsRevision = externalIdNotes.getRevision();
+ // These update the same ref, so they need to be stacked on top of one another using the same
+ // ExternalIdNotes instance.
+ RevCommit revCommit =
+ commitExternalIdUpdates(externalIdUpdateMessage, allUsersRepo, batchRefUpdate);
+ externalIdsUpdated = !Objects.equals(revCommit.getId(), oldExternalIdsRevision);
+ }
+ for (UpdatedAccount updatedAccount : updatedAccounts) {
+ if (updatedAccount.deleted) {
+ RefUpdate ru = RefUpdateUtil.deleteChecked(allUsersRepo, updatedAccount.refName);
+ gitRefUpdated.fire(allUsersName, ru, ReceiveCommand.Type.DELETE, null);
+ accountsToSkipForReindex.add(Account.Id.fromRef(updatedAccount.refName));
+ continue;
+ }
+ // These updates are all for different refs (because batches never update the same account
+ // more than once), so there can be multiple commits in the same batch, all with the same base
+ // revision in their AccountConfig.
+ // We allow empty commits:
+ // 1) When creating a new account, so that the user branch gets created with an empty commit
+ // when no account properties are set and hence no
+ // 'account.config' file will be created.
+ // 2) When updating "refs/meta/external-ids", so that refs/users/* meta ref is updated too.
+ // This allows to schedule reindexing of account transactionally on refs/users/* meta
+ // updates.
+ boolean allowEmptyCommit = externalIdsUpdated || updatedAccount.created;
+ commitAccountConfig(
+ updatedAccount.message,
+ allUsersRepo,
+ batchRefUpdate,
+ updatedAccount.accountConfig,
+ allowEmptyCommit);
+ }
+
+ RefUpdateUtil.executeChecked(batchRefUpdate, allUsersRepo);
+
+ if (externalIdsUpdated) {
+ accountsToSkipForReindex.addAll(getUpdatedAccountIds(batchRefUpdate));
+ extIdNotesFactory.updateExternalIdCacheAndMaybeReindexAccounts(
+ externalIdNotes, accountsToSkipForReindex);
+ }
+
+ gitRefUpdated.fire(
+ allUsersName, batchRefUpdate, currentUser.map(IdentifiedUser::state).orElse(null));
+ }
+
+ private static Set<Account.Id> getUpdatedAccountIds(BatchRefUpdate batchRefUpdate) {
+ return batchRefUpdate.getCommands().stream()
+ .map(c -> Account.Id.fromRef(c.getRefName()))
+ .filter(Objects::nonNull)
+ .collect(toSet());
+ }
+
+ private void commitAccountConfig(
+ String message,
+ Repository allUsersRepo,
+ BatchRefUpdate batchRefUpdate,
+ AccountConfig accountConfig,
+ boolean allowEmptyCommit)
+ throws IOException {
+ try (MetaDataUpdate md = createMetaDataUpdate(message, allUsersRepo, batchRefUpdate)) {
+ md.setAllowEmpty(allowEmptyCommit);
+ accountConfig.commit(md);
+ }
+ }
+
+ private RevCommit commitExternalIdUpdates(
+ String message, Repository allUsersRepo, BatchRefUpdate batchRefUpdate) throws IOException {
+ try (MetaDataUpdate md = createMetaDataUpdate(message, allUsersRepo, batchRefUpdate)) {
+ return externalIdNotes.commit(md);
+ }
+ }
+
+ private MetaDataUpdate createMetaDataUpdate(
+ String message, Repository allUsersRepo, BatchRefUpdate batchRefUpdate) {
+ MetaDataUpdate metaDataUpdate =
+ metaDataUpdateInternalFactory.get().create(allUsersName, allUsersRepo, batchRefUpdate);
+ if (!message.endsWith("\n")) {
+ message = message + "\n";
+ }
+
+ metaDataUpdate.getCommitBuilder().setMessage(message);
+ metaDataUpdate.getCommitBuilder().setCommitter(committerIdent);
+ metaDataUpdate.getCommitBuilder().setAuthor(authorIdent);
+ return metaDataUpdate;
+ }
+
+ private static void doNothing() {}
+
+ @FunctionalInterface
+ private interface ExecutableUpdate {
+ UpdatedAccount execute(Repository allUsersRepo) throws IOException, ConfigInvalidException;
+ }
+
+ private class UpdatedAccount {
+ final String message;
+ final AccountConfig accountConfig;
+ final CachedPreferences defaultPreferences;
+ final String refName;
+ final boolean created;
+ final boolean deleted;
+
+ UpdatedAccount(
+ String message,
+ AccountConfig accountConfig,
+ CachedPreferences defaultPreferences,
+ boolean created) {
+ this(
+ message,
+ requireNonNull(accountConfig),
+ defaultPreferences,
+ accountConfig.getRefName(),
+ created,
+ false);
+ }
+
+ protected UpdatedAccount(
+ String message,
+ AccountConfig accountConfig,
+ CachedPreferences defaultPreferences,
+ String refName,
+ boolean created,
+ boolean deleted) {
+ checkState(!Strings.isNullOrEmpty(message), "message for account update must be set");
+ this.message = requireNonNull(message);
+ this.accountConfig = accountConfig;
+ this.defaultPreferences = defaultPreferences;
+ this.refName = refName;
+ this.created = created;
+ this.deleted = deleted;
+ }
+
+ Optional<AccountState> getAccountState() throws IOException {
+ return AccountsNoteDbImpl.getFromAccountConfig(
+ externalIds, accountConfig, externalIdNotes, defaultPreferences);
+ }
+ }
+
+ private class DeletedAccount extends UpdatedAccount {
+ DeletedAccount(String message, String refName) {
+ super(message, null, null, refName, false, true);
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index 83b7565..fb450ea 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -223,7 +223,6 @@
change.setWorkInProgress(workInProgress);
change.setReviewStarted(!workInProgress);
change.setRevertOf(revertOf);
- change.setCustomKeyedValues(customKeyedValues);
return change;
}
@@ -475,12 +474,12 @@
} catch (ValidationException ex) {
throw new BadRequestException(ex.getMessage());
}
- if (change.getCustomKeyedValues() != null) {
+ if (customKeyedValues != null) {
try {
- if (change.getCustomKeyedValues().entrySet().size() > MAX_CUSTOM_KEYED_VALUES) {
+ if (customKeyedValues.entrySet().size() > MAX_CUSTOM_KEYED_VALUES) {
throw new ValidationException("Too many custom keyed values");
}
- for (Map.Entry<String, String> entry : change.getCustomKeyedValues().entrySet()) {
+ for (Map.Entry<String, String> entry : customKeyedValues.entrySet()) {
update.addCustomKeyedValue(entry.getKey(), entry.getValue());
}
} catch (ValidationException ex) {
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index d54d0f2..cb321e1 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -108,7 +108,6 @@
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.ExternalIdCacheModule;
import com.google.gerrit.server.account.externalids.ExternalIdModule;
-import com.google.gerrit.server.account.externalids.ExternalIdUpsertPreprocessor;
import com.google.gerrit.server.account.storage.notedb.AccountNoteDbStorageModule;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.auth.AuthBackend;
@@ -511,7 +510,6 @@
bind(ReloadPluginListener.class)
.annotatedWith(UniqueAnnotations.create())
.to(PluginConfigFactory.class);
- DynamicMap.mapOf(binder(), ExternalIdUpsertPreprocessor.class);
bind(AttentionSetObserver.class);
}
diff --git a/java/com/google/gerrit/server/restapi/account/AccountRestApiModule.java b/java/com/google/gerrit/server/restapi/account/AccountRestApiModule.java
index af93b8a..d0b4470 100644
--- a/java/com/google/gerrit/server/restapi/account/AccountRestApiModule.java
+++ b/java/com/google/gerrit/server/restapi/account/AccountRestApiModule.java
@@ -29,7 +29,6 @@
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.UserInitiated;
import com.google.gerrit.server.account.AccountsUpdate;
-import com.google.gerrit.server.account.externalids.ExternalIdNotes;
import com.google.inject.Provides;
import com.google.inject.multibindings.OptionalBinder;
@@ -112,23 +111,22 @@
post(ACCOUNT_KIND, "drafts:delete").to(DeleteDraftComments.class);
// The gpgkeys REST endpoints are bound via GpgApiModule.
-
- factory(AccountsUpdate.Factory.class);
}
@Provides
@ServerInitiated
AccountsUpdate provideServerInitiatedAccountsUpdate(
- AccountsUpdate.Factory accountsUpdateFactory, ExternalIdNotes.Factory extIdNotesFactory) {
- return accountsUpdateFactory.createWithServerIdent(extIdNotesFactory);
+ @AccountsUpdate.AccountsUpdateLoader.WithReindex
+ AccountsUpdate.AccountsUpdateLoader accountsUpdateFactory) {
+ return accountsUpdateFactory.createWithServerIdent();
}
@Provides
@UserInitiated
AccountsUpdate provideUserInitiatedAccountsUpdate(
- AccountsUpdate.Factory accountsUpdateFactory,
- IdentifiedUser currentUser,
- ExternalIdNotes.Factory extIdNotesFactory) {
- return accountsUpdateFactory.create(currentUser, extIdNotesFactory);
+ @AccountsUpdate.AccountsUpdateLoader.WithReindex
+ AccountsUpdate.AccountsUpdateLoader accountsUpdateFactory,
+ IdentifiedUser currentUser) {
+ return accountsUpdateFactory.create(currentUser);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 2c65019..3fff964 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -146,6 +146,7 @@
import com.google.gerrit.server.account.externalids.ExternalIdKeyFactory;
import com.google.gerrit.server.account.externalids.ExternalIdNotes;
import com.google.gerrit.server.account.externalids.ExternalIds;
+import com.google.gerrit.server.account.storage.notedb.AccountsUpdateNoteDbImpl;
import com.google.gerrit.server.change.AccountPatchReviewStore;
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
@@ -2170,26 +2171,8 @@
String status = "happy";
String fullName = "Foo";
AtomicBoolean doneBgUpdate = new AtomicBoolean(false);
- PersonIdent ident = serverIdent.get();
AccountsUpdate update =
- new AccountsUpdate(
- repoManager,
- gitReferenceUpdated,
- Optional.empty(),
- allUsers,
- externalIds,
- metaDataUpdateInternalFactory,
- new RetryHelper(
- cfg,
- retryMetrics,
- null,
- null,
- null,
- exceptionHooks,
- r -> r.withBlockStrategy(noSleepBlockStrategy)),
- extIdNotesFactory,
- ident,
- ident,
+ getAccountsUpdate(
() -> {
if (!doneBgUpdate.getAndSet(true)) {
try {
@@ -2226,28 +2209,8 @@
List<String> status = ImmutableList.of("foo", "bar", "baz");
String fullName = "Foo";
AtomicInteger bgCounter = new AtomicInteger(0);
- PersonIdent ident = serverIdent.get();
AccountsUpdate update =
- new AccountsUpdate(
- repoManager,
- gitReferenceUpdated,
- Optional.empty(),
- allUsers,
- externalIds,
- metaDataUpdateInternalFactory,
- new RetryHelper(
- cfg,
- retryMetrics,
- null,
- null,
- null,
- exceptionHooks,
- r ->
- r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size()))
- .withBlockStrategy(noSleepBlockStrategy)),
- extIdNotesFactory,
- ident,
- ident,
+ getAccountsUpdate(
() -> {
try {
accountsUpdateProvider
@@ -2260,7 +2223,17 @@
// Ignore, the expected exception is asserted later
}
},
- Runnables.doNothing());
+ Runnables.doNothing(),
+ new RetryHelper(
+ cfg,
+ retryMetrics,
+ null,
+ null,
+ null,
+ exceptionHooks,
+ r ->
+ r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size()))
+ .withBlockStrategy(noSleepBlockStrategy)));
assertThat(bgCounter.get()).isEqualTo(0);
AccountInfo accountInfo = gApi.accounts().id(admin.id().get()).get();
assertThat(accountInfo.status).isNull();
@@ -2286,26 +2259,8 @@
AtomicInteger bgCounterA1 = new AtomicInteger(0);
AtomicInteger bgCounterA2 = new AtomicInteger(0);
- PersonIdent ident = serverIdent.get();
AccountsUpdate update =
- new AccountsUpdate(
- repoManager,
- gitReferenceUpdated,
- Optional.empty(),
- allUsers,
- externalIds,
- metaDataUpdateInternalFactory,
- new RetryHelper(
- cfg,
- retryMetrics,
- null,
- null,
- null,
- exceptionHooks,
- r -> r.withBlockStrategy(noSleepBlockStrategy)),
- extIdNotesFactory,
- ident,
- ident,
+ getAccountsUpdate(
Runnables.doNothing(),
() -> {
try {
@@ -2360,27 +2315,9 @@
AtomicInteger bgCounterA1 = new AtomicInteger(0);
AtomicInteger bgCounterA2 = new AtomicInteger(0);
- PersonIdent ident = serverIdent.get();
ExternalId extIdA2 = externalIdFactory.create("foo", "A-2", accountId);
AccountsUpdate update =
- new AccountsUpdate(
- repoManager,
- gitReferenceUpdated,
- Optional.empty(),
- allUsers,
- externalIds,
- metaDataUpdateInternalFactory,
- new RetryHelper(
- cfg,
- retryMetrics,
- null,
- null,
- null,
- exceptionHooks,
- r -> r.withBlockStrategy(noSleepBlockStrategy)),
- extIdNotesFactory,
- ident,
- ident,
+ getAccountsUpdate(
Runnables.doNothing(),
() -> {
try {
@@ -3681,6 +3618,37 @@
"login?account_id=" + accountId, HttpServletResponse.SC_MOVED_TEMPORARILY);
}
+ private AccountsUpdate getAccountsUpdate(Runnable afterReadRevision, Runnable beforeCommit) {
+ return getAccountsUpdate(
+ afterReadRevision,
+ beforeCommit,
+ new RetryHelper(
+ cfg,
+ retryMetrics,
+ null,
+ null,
+ null,
+ exceptionHooks,
+ r -> r.withBlockStrategy(noSleepBlockStrategy)));
+ }
+
+ private AccountsUpdate getAccountsUpdate(
+ Runnable afterReadRevision, Runnable beforeCommit, RetryHelper retryHelper) {
+ return new AccountsUpdateNoteDbImpl(
+ repoManager,
+ gitReferenceUpdated,
+ Optional.empty(),
+ allUsers,
+ externalIds,
+ extIdNotesFactory,
+ metaDataUpdateInternalFactory,
+ retryHelper,
+ serverIdent.get(),
+ serverIdent.get(),
+ afterReadRevision,
+ beforeCommit);
+ }
+
private void httpGetAndAssertStatus(String urlPath, int expectedHttpStatus)
throws ClientProtocolException, IOException {
HttpGet httpGet = new HttpGet(canonicalWebUrl.get() + urlPath);
diff --git a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
index 296d801..bbf10bd 100644
--- a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
+++ b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
@@ -26,7 +26,6 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.proto.Entities;
import com.google.gerrit.proto.testing.SerializedClassSubject;
-import com.google.inject.TypeLiteral;
import java.lang.reflect.Type;
import java.time.Instant;
import org.junit.Test;
@@ -285,9 +284,6 @@
.put("currentPatchSetId", int.class)
.put("subject", String.class)
.put("topic", String.class)
- .put(
- "customKeyedValues",
- new TypeLiteral<ImmutableMap<String, String>>() {}.getType())
.put("originalSubject", String.class)
.put("submissionId", String.class)
.put("isPrivate", boolean.class)
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
index 8d72a97..1f32073 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
@@ -327,7 +327,7 @@
${this.renderCCs()} ${this.renderProjectBranch()} ${this.renderParent()}
${this.renderMergedAs()} ${this.renderShowRevertCreatedAs()}
${this.renderTopic()} ${this.renderCherryPickOf()}
- ${this.renderStrategy()} ${this.renderHashTags()}
+ ${this.renderRevertOf()} ${this.renderStrategy()} ${this.renderHashTags()}
${this.renderSubmitRequirements()} ${this.renderWeblinks()}
<gr-endpoint-decorator name="change-metadata-item">
<gr-endpoint-param
@@ -687,6 +687,23 @@
</section>`;
}
+ private renderRevertOf() {
+ if (!this.change?.revert_of) return nothing;
+ return html` <section class=${this.computeDisplayState(Metadata.REVERT_OF)}>
+ <span class="title">Revert of</span>
+ <span class="value">
+ <a
+ href=${createChangeUrl({
+ changeNum: this.change.revert_of,
+ repo: this.change.project,
+ usp: 'metadata',
+ })}
+ >${this.change.revert_of}</a
+ >
+ </span>
+ </section>`;
+ }
+
private renderStrategy() {
if (!changeIsOpen(this.change)) return nothing;
return html`<section
diff --git a/polygerrit-ui/app/elements/shared/gr-change-status/gr-change-status.ts b/polygerrit-ui/app/elements/shared/gr-change-status/gr-change-status.ts
index c93cc97..97fa267 100644
--- a/polygerrit-ui/app/elements/shared/gr-change-status/gr-change-status.ts
+++ b/polygerrit-ui/app/elements/shared/gr-change-status/gr-change-status.ts
@@ -87,6 +87,10 @@
background-color: var(--status-ready);
color: var(--status-ready);
}
+ :host(.revert) .chip {
+ background-color: var(--status-revert);
+ color: var(--status-revert);
+ }
:host(.revert-created) .chip {
background-color: var(--status-revert-created);
color: var(--status-revert-created);
diff --git a/polygerrit-ui/app/embed/diff-old/gr-diff-builder/gr-diff-row.ts b/polygerrit-ui/app/embed/diff-old/gr-diff-builder/gr-diff-row.ts
index f2742c7..1486154 100644
--- a/polygerrit-ui/app/embed/diff-old/gr-diff-builder/gr-diff-row.ts
+++ b/polygerrit-ui/app/embed/diff-old/gr-diff-builder/gr-diff-row.ts
@@ -315,8 +315,11 @@
// For unified diff, this method will be called with number set to 0 for
// the empty line number column for added/removed lines. This should not
// be announced to the screenreader.
- if (lineNumber === LOST || lineNumber <= 0) return undefined;
-
+ if (
+ lineNumber === LOST ||
+ (typeof lineNumber === 'number' && lineNumber <= 0)
+ )
+ return undefined;
switch (line.type) {
case GrDiffLineType.REMOVE:
return `${lineNumber} removed`;
diff --git a/polygerrit-ui/app/embed/diff-old/gr-diff-selection/gr-diff-selection.ts b/polygerrit-ui/app/embed/diff-old/gr-diff-selection/gr-diff-selection.ts
index a9ec6a2..407b403 100644
--- a/polygerrit-ui/app/embed/diff-old/gr-diff-selection/gr-diff-selection.ts
+++ b/polygerrit-ui/app/embed/diff-old/gr-diff-selection/gr-diff-selection.ts
@@ -18,6 +18,7 @@
getSideByLineEl,
isThreadEl,
} from '../../diff/gr-diff/gr-diff-utils';
+import {getContentFromDiff} from '../../../utils/diff-util';
/**
* Possible CSS classes indicating the state of selection. Dynamically added/
@@ -34,15 +35,6 @@
return side === Side.LEFT ? SelectionClass.LEFT : SelectionClass.RIGHT;
}
-interface LinesCache {
- left: string[] | null;
- right: string[] | null;
-}
-
-function getNewCache(): LinesCache {
- return {left: null, right: null};
-}
-
export class GrDiffSelection {
// visible for testing
diff?: DiffInfo;
@@ -50,9 +42,6 @@
// visible for testing
diffTable?: HTMLElement;
- // visible for testing
- linesCache: LinesCache = getNewCache();
-
init(diff: DiffInfo, diffTable: HTMLElement) {
this.cleanup();
this.diff = diff;
@@ -60,7 +49,6 @@
this.diffTable.classList.add(SelectionClass.RIGHT);
this.diffTable.addEventListener('copy', this.handleCopy);
this.diffTable.addEventListener('mousedown', this.handleDown);
- this.linesCache = getNewCache();
}
cleanup() {
@@ -161,6 +149,7 @@
* @return The selected text.
*/
getSelectedText(side: Side) {
+ if (!this.diff) return '';
const sel = this.getSelection();
if (!sel || sel.rangeCount !== 1) {
return ''; // No multi-select support yet.
@@ -188,7 +177,8 @@
if (endLineDataValue) endLineNum = Number(endLineDataValue);
}
- return this.getRangeFromDiff(
+ return getContentFromDiff(
+ this.diff,
startLineNum,
range.startOffset,
endLineNum,
@@ -196,52 +186,4 @@
side
);
}
-
- /**
- * Query the diff object for the selected lines.
- */
- getRangeFromDiff(
- startLineNum: number,
- startOffset: number,
- endLineNum: number | undefined,
- endOffset: number,
- side: Side
- ) {
- const skipChunk = this.diff?.content.find(chunk => chunk.skip);
- if (skipChunk) {
- startLineNum -= skipChunk.skip!;
- if (endLineNum) endLineNum -= skipChunk.skip!;
- }
- const lines = this.getDiffLines(side).slice(startLineNum - 1, endLineNum);
- if (lines.length) {
- lines[lines.length - 1] = lines[lines.length - 1].substring(0, endOffset);
- lines[0] = lines[0].substring(startOffset);
- }
- return lines.join('\n');
- }
-
- /**
- * Query the diff object for the lines from a particular side.
- *
- * @param side The side that is currently selected.
- * @return An array of strings indexed by line number.
- */
- getDiffLines(side: Side): string[] {
- if (this.linesCache[side]) {
- return this.linesCache[side]!;
- }
- if (!this.diff) return [];
- let lines: string[] = [];
- for (const chunk of this.diff.content) {
- if (chunk.ab) {
- lines = lines.concat(chunk.ab);
- } else if (side === Side.LEFT && chunk.a) {
- lines = lines.concat(chunk.a);
- } else if (side === Side.RIGHT && chunk.b) {
- lines = lines.concat(chunk.b);
- }
- }
- this.linesCache[side] = lines;
- return lines;
- }
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
index bfd6a0d..0ac7516 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-row.ts
@@ -312,7 +312,11 @@
// For unified diff, this method will be called with number set to 0 for
// the empty line number column for added/removed lines. This should not
// be announced to the screenreader.
- if (lineNumber === LOST || lineNumber <= 0) return undefined;
+ if (
+ lineNumber === LOST ||
+ (typeof lineNumber === 'number' && lineNumber <= 0)
+ )
+ return undefined;
switch (line.type) {
case GrDiffLineType.REMOVE:
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-selection/gr-diff-selection.ts b/polygerrit-ui/app/embed/diff/gr-diff-selection/gr-diff-selection.ts
index a790736..f403ef9 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-selection/gr-diff-selection.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-selection/gr-diff-selection.ts
@@ -18,6 +18,7 @@
getSideByLineEl,
isThreadEl,
} from '../gr-diff/gr-diff-utils';
+import {getContentFromDiff} from '../../../utils/diff-util';
/**
* Possible CSS classes indicating the state of selection. Dynamically added/
@@ -34,15 +35,6 @@
return side === Side.LEFT ? SelectionClass.LEFT : SelectionClass.RIGHT;
}
-interface LinesCache {
- left: string[] | null;
- right: string[] | null;
-}
-
-function getNewCache(): LinesCache {
- return {left: null, right: null};
-}
-
export class GrDiffSelection {
// visible for testing
diff?: DiffInfo;
@@ -50,9 +42,6 @@
// visible for testing
diffTable?: HTMLElement;
- // visible for testing
- linesCache: LinesCache = getNewCache();
-
init(diff: DiffInfo, diffTable: HTMLElement) {
this.cleanup();
this.diff = diff;
@@ -60,7 +49,6 @@
this.diffTable.classList.add(SelectionClass.RIGHT);
this.diffTable.addEventListener('copy', this.handleCopy);
this.diffTable.addEventListener('mousedown', this.handleDown);
- this.linesCache = getNewCache();
}
cleanup() {
@@ -161,6 +149,7 @@
* @return The selected text.
*/
getSelectedText(side: Side) {
+ if (!this.diff) return '';
const sel = this.getSelection();
if (!sel || sel.rangeCount !== 1) {
return ''; // No multi-select support yet.
@@ -188,7 +177,8 @@
if (endLineDataValue) endLineNum = Number(endLineDataValue);
}
- return this.getRangeFromDiff(
+ return getContentFromDiff(
+ this.diff,
startLineNum,
range.startOffset,
endLineNum,
@@ -196,52 +186,4 @@
side
);
}
-
- /**
- * Query the diff object for the selected lines.
- */
- getRangeFromDiff(
- startLineNum: number,
- startOffset: number,
- endLineNum: number | undefined,
- endOffset: number,
- side: Side
- ) {
- const skipChunk = this.diff?.content.find(chunk => chunk.skip);
- if (skipChunk) {
- startLineNum -= skipChunk.skip!;
- if (endLineNum) endLineNum -= skipChunk.skip!;
- }
- const lines = this.getDiffLines(side).slice(startLineNum - 1, endLineNum);
- if (lines.length) {
- lines[lines.length - 1] = lines[lines.length - 1].substring(0, endOffset);
- lines[0] = lines[0].substring(startOffset);
- }
- return lines.join('\n');
- }
-
- /**
- * Query the diff object for the lines from a particular side.
- *
- * @param side The side that is currently selected.
- * @return An array of strings indexed by line number.
- */
- getDiffLines(side: Side): string[] {
- if (this.linesCache[side]) {
- return this.linesCache[side]!;
- }
- if (!this.diff) return [];
- let lines: string[] = [];
- for (const chunk of this.diff.content) {
- if (chunk.ab) {
- lines = lines.concat(chunk.ab);
- } else if (side === Side.LEFT && chunk.a) {
- lines = lines.concat(chunk.a);
- } else if (side === Side.RIGHT && chunk.b) {
- lines = lines.concat(chunk.b);
- }
- }
- this.linesCache[side] = lines;
- return lines;
- }
}
diff --git a/polygerrit-ui/app/styles/themes/app-theme.ts b/polygerrit-ui/app/styles/themes/app-theme.ts
index 0503e4c..7742b1f 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.ts
+++ b/polygerrit-ui/app/styles/themes/app-theme.ts
@@ -323,6 +323,7 @@
--status-wip: #795548;
--status-private: var(--purple-500);
--status-conflict: var(--red-600);
+ --status-revert: var(--gray-900);
--status-revert-created: #e64a19;
--status-active: var(--blue-700);
--status-ready: var(--pink-800);
diff --git a/polygerrit-ui/app/styles/themes/dark-theme.ts b/polygerrit-ui/app/styles/themes/dark-theme.ts
index dc3d4e9..574e6c2 100644
--- a/polygerrit-ui/app/styles/themes/dark-theme.ts
+++ b/polygerrit-ui/app/styles/themes/dark-theme.ts
@@ -181,6 +181,7 @@
--status-wip: #bcaaa4;
--status-private: var(--purple-200);
--status-conflict: var(--red-300);
+ --status-revert: var(--gray-200);
--status-revert-created: #ff8a65;
--status-active: var(--blue-400);
--status-ready: var(--pink-500);
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 7a6610b..766b407 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -692,7 +692,11 @@
MERGED = 'Merged',
PRIVATE = 'Private',
READY_TO_SUBMIT = 'Ready to submit',
+ /** This change is a revert of another change. */
+ REVERT = 'Revert',
+ /** A revert of this change was created. */
REVERT_CREATED = 'Revert Created',
+ /** A revert of this change was submitted. */
REVERT_SUBMITTED = 'Revert Submitted',
WIP = 'WIP',
}
diff --git a/polygerrit-ui/app/utils/change-metadata-util.ts b/polygerrit-ui/app/utils/change-metadata-util.ts
index 9d3106d..8b208e4 100644
--- a/polygerrit-ui/app/utils/change-metadata-util.ts
+++ b/polygerrit-ui/app/utils/change-metadata-util.ts
@@ -22,6 +22,7 @@
AUTHOR = 'Author',
COMMITTER = 'Committer',
CHERRY_PICK_OF = 'Cherry pick of',
+ REVERT_OF = 'Revert of',
}
export const DisplayRules = {
@@ -39,6 +40,7 @@
Metadata.AUTHOR,
Metadata.COMMITTER,
Metadata.CHERRY_PICK_OF,
+ Metadata.REVERT_OF,
],
ALWAYS_HIDE: [
Metadata.PARENT,
diff --git a/polygerrit-ui/app/utils/change-util.ts b/polygerrit-ui/app/utils/change-util.ts
index 7de5e7e..584a4b1 100644
--- a/polygerrit-ui/app/utils/change-util.ts
+++ b/polygerrit-ui/app/utils/change-util.ts
@@ -157,6 +157,9 @@
options?: ChangeStatusesOptions
): ChangeStates[] {
const states = [];
+ if (change.revert_of) {
+ states.push(ChangeStates.REVERT);
+ }
if (change.status === ChangeStatus.MERGED) {
if (options?.revertingChangeStatus === ChangeStatus.MERGED) {
return [ChangeStates.MERGED, ChangeStates.REVERT_SUBMITTED];
diff --git a/polygerrit-ui/app/utils/change-util_test.ts b/polygerrit-ui/app/utils/change-util_test.ts
index 1782d80..1d209f9 100644
--- a/polygerrit-ui/app/utils/change-util_test.ts
+++ b/polygerrit-ui/app/utils/change-util_test.ts
@@ -163,6 +163,19 @@
assert.deepEqual(changeStatuses(change), [ChangeStates.ABANDONED]);
});
+ test('Revert status', () => {
+ const change = {
+ ...createChange(),
+ revert_of: 123 as NumericChangeId,
+ };
+ assert.deepEqual(changeStatuses(change), [ChangeStates.REVERT]);
+ change.is_private = true;
+ assert.deepEqual(changeStatuses(change), [
+ ChangeStates.REVERT,
+ ChangeStates.PRIVATE,
+ ]);
+ });
+
test('Open status with private and wip', () => {
const change = {
...createChange(),
diff --git a/polygerrit-ui/app/utils/diff-util.ts b/polygerrit-ui/app/utils/diff-util.ts
index 1e5abf9..28c22e5 100644
--- a/polygerrit-ui/app/utils/diff-util.ts
+++ b/polygerrit-ui/app/utils/diff-util.ts
@@ -18,6 +18,38 @@
}, 0);
}
+function getDiffLines(diff: DiffInfo, side: Side): string[] {
+ let lines: string[] = [];
+ for (const chunk of diff.content) {
+ if (chunk.skip) {
+ lines = lines.concat(Array(chunk.skip).fill(''));
+ } else if (chunk.ab) {
+ lines = lines.concat(chunk.ab);
+ } else if (side === Side.LEFT && chunk.a) {
+ lines = lines.concat(chunk.a);
+ } else if (side === Side.RIGHT && chunk.b) {
+ lines = lines.concat(chunk.b);
+ }
+ }
+ return lines;
+}
+
+export function getContentFromDiff(
+ diff: DiffInfo,
+ startLineNum: number,
+ startOffset: number,
+ endLineNum: number | undefined,
+ endOffset: number,
+ side: Side
+) {
+ const lines = getDiffLines(diff, side).slice(startLineNum - 1, endLineNum);
+ if (lines.length) {
+ lines[lines.length - 1] = lines[lines.length - 1].substring(0, endOffset);
+ lines[0] = lines[0].substring(startOffset);
+ }
+ return lines.join('\n');
+}
+
export function isFileUnchanged(diff: DiffInfo) {
return !diff.content.some(
content => (content.a && !content.common) || (content.b && !content.common)
diff --git a/polygerrit-ui/app/utils/diff-util_test.ts b/polygerrit-ui/app/utils/diff-util_test.ts
index dbab76d..2829209 100644
--- a/polygerrit-ui/app/utils/diff-util_test.ts
+++ b/polygerrit-ui/app/utils/diff-util_test.ts
@@ -4,10 +4,10 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {assert} from '@open-wc/testing';
-import {DiffInfo} from '../api/diff';
+import {DiffInfo, Side} from '../api/diff';
import '../test/common-test-setup';
import {createDiff} from '../test/test-data-generators';
-import {isFileUnchanged} from './diff-util';
+import {getContentFromDiff, isFileUnchanged} from './diff-util';
suite('diff-util tests', () => {
test('isFileUnchanged', () => {
@@ -41,4 +41,78 @@
};
assert.equal(isFileUnchanged(diff), true);
});
+
+ suite('getContentFromDiff', () => {
+ test('one changed line', () => {
+ const diff: DiffInfo = {
+ ...createDiff(),
+ content: [{a: ['abcd']}, {b: ['wxyz']}],
+ };
+ assert.equal(getContentFromDiff(diff, 1, 1, 1, 3, Side.LEFT), 'bc');
+ assert.equal(getContentFromDiff(diff, 1, 1, 1, 3, Side.RIGHT), 'xy');
+ });
+
+ test('one common line', () => {
+ const diff: DiffInfo = {
+ ...createDiff(),
+ content: [{ab: ['abcd']}],
+ };
+ assert.equal(getContentFromDiff(diff, 1, 1, 1, 3, Side.LEFT), 'bc');
+ assert.equal(getContentFromDiff(diff, 1, 1, 1, 3, Side.RIGHT), 'bc');
+ });
+
+ test('multiple lines', () => {
+ const diff: DiffInfo = {
+ ...createDiff(),
+ content: [
+ {a: ['l1-asdf', 'l2-asdf']},
+ {b: ['r1-wxyz']},
+ {ab: ['l3-r2-qwer', 'l4-r3-uiop']},
+ {b: ['r4-hjkl']},
+ {ab: ['l5-r5-bnm,']},
+ ],
+ };
+ assert.equal(
+ getContentFromDiff(diff, 1, 0, 5, 10, Side.LEFT),
+ 'l1-asdf\nl2-asdf\nl3-r2-qwer\nl4-r3-uiop\nl5-r5-bnm,'
+ );
+ assert.equal(
+ getContentFromDiff(diff, 1, 0, 5, 10, Side.RIGHT),
+ 'r1-wxyz\nl3-r2-qwer\nl4-r3-uiop\nr4-hjkl\nl5-r5-bnm,'
+ );
+ });
+
+ test('one skip chunk', () => {
+ const diff: DiffInfo = {
+ ...createDiff(),
+ content: [{skip: 5}, {ab: ['abcd']}],
+ };
+ assert.equal(getContentFromDiff(diff, 1, 1, 1, 3, Side.LEFT), '');
+ assert.equal(getContentFromDiff(diff, 1, 1, 1, 3, Side.RIGHT), '');
+ assert.equal(getContentFromDiff(diff, 6, 1, 6, 3, Side.LEFT), 'bc');
+ assert.equal(getContentFromDiff(diff, 6, 1, 6, 3, Side.RIGHT), 'bc');
+ });
+
+ test('multiple skip chunks', () => {
+ const diff: DiffInfo = {
+ ...createDiff(),
+ content: [
+ {skip: 5},
+ {ab: ['abcd']},
+ {skip: 5},
+ {ab: ['qwer']},
+ {skip: 5},
+ {ab: ['zxcv']},
+ ],
+ };
+ assert.equal(getContentFromDiff(diff, 1, 1, 1, 3, Side.LEFT), '');
+ assert.equal(getContentFromDiff(diff, 1, 1, 1, 3, Side.RIGHT), '');
+ assert.equal(getContentFromDiff(diff, 6, 1, 6, 3, Side.LEFT), 'bc');
+ assert.equal(getContentFromDiff(diff, 6, 1, 6, 3, Side.RIGHT), 'bc');
+ assert.equal(getContentFromDiff(diff, 12, 1, 12, 3, Side.LEFT), 'we');
+ assert.equal(getContentFromDiff(diff, 12, 1, 12, 3, Side.RIGHT), 'we');
+ assert.equal(getContentFromDiff(diff, 18, 1, 18, 3, Side.LEFT), 'xc');
+ assert.equal(getContentFromDiff(diff, 18, 1, 18, 3, Side.RIGHT), 'xc');
+ });
+ });
});
