From 1df95271ad8c7db57fa5d2bc95d9c7f2f2abc375 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 3 Jan 2018 13:02:14 +0100 Subject: [PATCH 1/7] Allow to update accounts and project watches atomically AccountsUpdate now also supports to update project watches. This means account properties and project watches can now be updated in the same transaction. WatchConfig.Accessor is removed so that project watch updates can only be done through AccountsUpdate. For reading project watches a new method is added to Accounts. In the future we would rather like to change the Accounts#get(Account.Id) method to return AccountState instead of Account so that this get method can be used to retrieve the account as well as the project watches. Once this is done the getProjectWatches method will be removed again. Reading and writing project watches is now done via AccountConfig. On load we always read the watch.config file, but parsing it is done lazily when the project watches are accessed for the first time. Most callers that load accounts are not interested in project watches and this avoids unneeded parsing effort for them. Change-Id: I8cd441eee3f65817a3653610c5d3bb62b0bfea5e Signed-off-by: Edwin Kempin --- .../gerrit/pgm/init/AccountsOnInit.java | 2 +- .../server/account/AccountCacheImpl.java | 8 +- .../gerrit/server/account/AccountConfig.java | 103 ++++++++++- .../gerrit/server/account/Accounts.java | 12 ++ .../server/account/InternalAccountUpdate.java | 110 ++++++++++- .../gerrit/server/account/WatchConfig.java | 174 ++---------------- .../git/validators/AccountValidator.java | 22 ++- .../git/validators/CommitValidators.java | 29 --- .../account/DeleteWatchedProjects.java | 32 ++-- .../restapi/account/GetWatchedProjects.java | 12 +- .../restapi/account/PostWatchedProjects.java | 22 +-- .../gerrit/server/schema/Schema_139.java | 15 +- .../acceptance/api/accounts/AccountIT.java | 2 +- .../server/project/ProjectWatchIT.java | 37 ---- 14 files changed, 294 insertions(+), 286 deletions(-) diff --git a/java/com/google/gerrit/pgm/init/AccountsOnInit.java b/java/com/google/gerrit/pgm/init/AccountsOnInit.java index fbe9b6289d..0a94b42992 100644 --- a/java/com/google/gerrit/pgm/init/AccountsOnInit.java +++ b/java/com/google/gerrit/pgm/init/AccountsOnInit.java @@ -70,7 +70,7 @@ public class AccountsOnInit { new GerritPersonIdentProvider(flags.cfg).get(), account.getRegisteredOn()); Config accountConfig = new Config(); - AccountConfig.writeToConfig( + AccountConfig.writeToAccountConfig( InternalAccountUpdate.builder() .setActive(account.isActive()) .setFullName(account.getFullName()) diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java index 9894751994..5342710bc7 100644 --- a/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java @@ -137,7 +137,6 @@ public class AccountCacheImpl implements AccountCache { private final AllUsersName allUsersName; private final Accounts accounts; private final GeneralPreferencesLoader loader; - private final Provider watchConfig; private final ExternalIds externalIds; @Inject @@ -145,12 +144,10 @@ public class AccountCacheImpl implements AccountCache { AllUsersName allUsersName, Accounts accounts, GeneralPreferencesLoader loader, - Provider watchConfig, ExternalIds externalIds) { this.allUsersName = allUsersName; this.accounts = accounts; this.loader = loader; - this.watchConfig = watchConfig; this.externalIds = externalIds; } @@ -170,10 +167,7 @@ public class AccountCacheImpl implements AccountCache { return Optional.of( new AccountState( - allUsersName, - account, - externalIds.byAccount(who), - watchConfig.get().getProjectWatches(who))); + allUsersName, account, externalIds.byAccount(who), accounts.getProjectWatches(who))); } } } diff --git a/java/com/google/gerrit/server/account/AccountConfig.java b/java/com/google/gerrit/server/account/AccountConfig.java index 830f2798f1..b634961556 100644 --- a/java/com/google/gerrit/server/account/AccountConfig.java +++ b/java/com/google/gerrit/server/account/AccountConfig.java @@ -18,15 +18,23 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.account.WatchConfig.NotifyType; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.git.VersionedMetaData; import com.google.gwtorm.server.OrmDuplicateKeyException; import java.io.IOException; import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; import java.util.Optional; +import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; @@ -61,7 +69,7 @@ import org.eclipse.jgit.revwalk.RevSort; * account. The first commit may be an empty commit (if no properties were set and 'account.config' * doesn't exist). */ -public class AccountConfig extends VersionedMetaData { +public class AccountConfig extends VersionedMetaData implements ValidationError.Sink { public static final String ACCOUNT_CONFIG = "account.config"; public static final String ACCOUNT = "account"; public static final String KEY_ACTIVE = "active"; @@ -73,14 +81,32 @@ public class AccountConfig extends VersionedMetaData { private final String ref; private Optional loadedAccount; + private WatchConfig watchConfig; private Optional accountUpdate = Optional.empty(); private Timestamp registeredOn; + private boolean eagerParsing; + private List validationErrors; public AccountConfig(Account.Id accountId) { this.accountId = checkNotNull(accountId); this.ref = RefNames.refsUsers(accountId); } + /** + * Sets whether all account data should be eagerly parsed. + * + *

Eager parsing should only be used if the caller is interested in validation errors for all + * account data (see {@link #getValidationErrors()}. + * + * @param eagerParsing whether all account data should be eagerly parsed + * @return this AccountConfig instance for chaining + */ + public AccountConfig setEagerParsing(boolean eagerParsing) { + checkState(loadedAccount == null, "Account %s already loaded", accountId.get()); + this.eagerParsing = eagerParsing; + return this; + } + @Override protected String getRefName() { return ref; @@ -98,6 +124,16 @@ public class AccountConfig extends VersionedMetaData { return loadedAccount; } + /** + * Get the project watches of the loaded account. + * + * @return the project watches of the loaded account + */ + public Map> getProjectWatches() { + checkLoaded(); + return watchConfig.getProjectWatches(); + } + /** * Sets the account. This means the loaded account will be overwritten with the given account. * @@ -158,9 +194,13 @@ public class AccountConfig extends VersionedMetaData { rw.sort(RevSort.REVERSE); registeredOn = new Timestamp(rw.next().getCommitTime() * 1000L); - Config cfg = readConfig(ACCOUNT_CONFIG); + Config accountConfig = readConfig(ACCOUNT_CONFIG); + loadedAccount = Optional.of(parse(accountConfig, revision.name())); - loadedAccount = Optional.of(parse(cfg, revision.name())); + watchConfig = new WatchConfig(accountId, readConfig(WatchConfig.WATCH_CONFIG), this); + if (eagerParsing) { + watchConfig.parse(); + } } else { loadedAccount = Optional.empty(); } @@ -207,21 +247,27 @@ public class AccountConfig extends VersionedMetaData { commit.setCommitter(new PersonIdent(commit.getCommitter(), registeredOn)); } - Config cfg = readConfig(ACCOUNT_CONFIG); - if (accountUpdate.isPresent()) { - writeToConfig(accountUpdate.get(), cfg); - } - saveConfig(ACCOUNT_CONFIG, cfg); + Config accountConfig = saveAccount(); + saveProjectWatches(); // metaId is set in the commit(MetaDataUpdate) method after the commit is created - loadedAccount = Optional.of(parse(cfg, null)); + loadedAccount = Optional.of(parse(accountConfig, null)); accountUpdate = Optional.empty(); return true; } - public static void writeToConfig(InternalAccountUpdate accountUpdate, Config cfg) { + private Config saveAccount() throws IOException, ConfigInvalidException { + Config accountConfig = readConfig(ACCOUNT_CONFIG); + if (accountUpdate.isPresent()) { + writeToAccountConfig(accountUpdate.get(), accountConfig); + } + saveConfig(ACCOUNT_CONFIG, accountConfig); + return accountConfig; + } + + public static void writeToAccountConfig(InternalAccountUpdate accountUpdate, Config cfg) { accountUpdate.getActive().ifPresent(active -> setActive(cfg, active)); accountUpdate.getFullName().ifPresent(fullName -> set(cfg, KEY_FULL_NAME, fullName)); accountUpdate @@ -230,6 +276,20 @@ public class AccountConfig extends VersionedMetaData { accountUpdate.getStatus().ifPresent(status -> set(cfg, KEY_STATUS, status)); } + private void saveProjectWatches() throws IOException { + if (accountUpdate.isPresent() + && (!accountUpdate.get().getDeletedProjectWatches().isEmpty() + || !accountUpdate.get().getUpdatedProjectWatches().isEmpty())) { + Map> projectWatches = watchConfig.getProjectWatches(); + accountUpdate.get().getDeletedProjectWatches().forEach(pw -> projectWatches.remove(pw)); + accountUpdate + .get() + .getUpdatedProjectWatches() + .forEach((pw, nt) -> projectWatches.put(pw, nt)); + saveConfig(WatchConfig.WATCH_CONFIG, watchConfig.save(projectWatches)); + } + } + /** * Sets/Unsets {@code account.active} in the given config. * @@ -282,4 +342,27 @@ public class AccountConfig extends VersionedMetaData { private void checkLoaded() { checkState(loadedAccount != null, "Account %s not loaded yet", accountId.get()); } + + /** + * Get the validation errors, if any were discovered during parsing the account data. + * + *

To get validation errors for all account data request eager parsing before loading the + * account (see {@link #setEagerParsing(boolean)}). + * + * @return list of errors; empty list if there are no errors. + */ + public List getValidationErrors() { + if (validationErrors != null) { + return ImmutableList.copyOf(validationErrors); + } + return ImmutableList.of(); + } + + @Override + public void error(ValidationError error) { + if (validationErrors == null) { + validationErrors = new ArrayList<>(4); + } + validationErrors.add(error); + } } diff --git a/java/com/google/gerrit/server/account/Accounts.java b/java/com/google/gerrit/server/account/Accounts.java index 5292a2f8ec..258f5a981c 100644 --- a/java/com/google/gerrit/server/account/Accounts.java +++ b/java/com/google/gerrit/server/account/Accounts.java @@ -21,6 +21,8 @@ import static java.util.stream.Collectors.toSet; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.account.WatchConfig.NotifyType; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.inject.Inject; @@ -29,6 +31,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -124,6 +127,15 @@ public class Accounts { return readUserRefs(repo).findAny().isPresent(); } + public Map> getProjectWatches(Account.Id accountId) + throws IOException, ConfigInvalidException { + try (Repository repo = repoManager.openRepository(allUsersName)) { + AccountConfig accountConfig = new AccountConfig(accountId); + accountConfig.load(repo); + return accountConfig.getProjectWatches(); + } + } + private Stream readUserRefs() throws IOException { try (Repository repo = repoManager.openRepository(allUsersName)) { return readUserRefs(repo); diff --git a/java/com/google/gerrit/server/account/InternalAccountUpdate.java b/java/com/google/gerrit/server/account/InternalAccountUpdate.java index ea778ca49a..bede7b6902 100644 --- a/java/com/google/gerrit/server/account/InternalAccountUpdate.java +++ b/java/com/google/gerrit/server/account/InternalAccountUpdate.java @@ -16,12 +16,17 @@ package com.google.gerrit.server.account; import com.google.auto.value.AutoValue; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.server.account.WatchConfig.NotifyType; +import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.account.externalids.DuplicateExternalIdKeyException; import com.google.gerrit.server.account.externalids.ExternalId; import java.util.Collection; +import java.util.Map; import java.util.Optional; +import java.util.Set; /** * Class to prepare updates to an account. @@ -92,6 +97,20 @@ public abstract class InternalAccountUpdate { */ public abstract ImmutableSet getDeletedExternalIds(); + /** + * Returns external IDs that should be updated for the account. + * + * @return external IDs that should be updated for the account + */ + public abstract ImmutableMap> getUpdatedProjectWatches(); + + /** + * Returns project watches that should be deleted for the account. + * + * @return project watches that should be deleted for the account + */ + public abstract ImmutableSet getDeletedProjectWatches(); + /** * Class to build an account update. * @@ -239,7 +258,7 @@ public abstract class InternalAccountUpdate { } /** - * Delete external IDs for the account. + * Deletes external IDs for the account. * *

The account IDs of the external IDs must match the account ID of the account that is * updated. @@ -277,6 +296,73 @@ public abstract class InternalAccountUpdate { return deleteExternalIds(extIdsToDelete).addExternalIds(extIdsToAdd); } + /** + * Returns a builder for the map of updated project watches. + * + * @return builder for the map of updated project watches. + */ + abstract ImmutableMap.Builder> updatedProjectWatchesBuilder(); + + /** + * Updates a project watch for the account. + * + *

If no project watch with the key exists the project watch is created. + * + * @param projectWatchKey key of the project watch that should be updated + * @param notifyTypes the notify types that should be set for the project watch + * @return the builder + */ + public Builder updateProjectWatch( + ProjectWatchKey projectWatchKey, Set notifyTypes) { + return updateProjectWatches(ImmutableMap.of(projectWatchKey, notifyTypes)); + } + + /** + * Updates project watches for the account. + * + *

If any of the project watches already exists, it is overwritten. New project watches are + * inserted. + * + * @param projectWatches project watches that should be updated + * @return the builder + */ + public Builder updateProjectWatches(Map> projectWatches) { + updatedProjectWatchesBuilder().putAll(projectWatches); + return this; + } + + /** + * Returns a builder for the set of deleted project watches. + * + * @return builder for the set of deleted project watches. + */ + abstract ImmutableSet.Builder deletedProjectWatchesBuilder(); + + /** + * Deletes a project watch for the account. + * + *

If no project watch with the ID exists this is a no-op. + * + * @param projectWatch project watch that should be deleted + * @return the builder + */ + public Builder deleteProjectWatch(ProjectWatchKey projectWatch) { + return deleteProjectWatches(ImmutableSet.of(projectWatch)); + } + + /** + * Deletes project watches for the account. + * + *

For non-existing project watches this is a no-op. + * + * @param projectWatches project watches that should be deleted + * @return the builder + */ + public Builder deleteProjectWatches(Collection projectWatches) { + deletedProjectWatchesBuilder().addAll(projectWatches); + return this; + } + /** * Builds the account update. * @@ -385,6 +471,28 @@ public abstract class InternalAccountUpdate { delegate.deleteExternalIds(extIds); return this; } + + @Override + ImmutableMap.Builder> updatedProjectWatchesBuilder() { + return delegate.updatedProjectWatchesBuilder(); + } + + @Override + public Builder updateProjectWatches(Map> projectWatches) { + delegate.updateProjectWatches(projectWatches); + return this; + } + + @Override + ImmutableSet.Builder deletedProjectWatchesBuilder() { + return delegate.deletedProjectWatchesBuilder(); + } + + @Override + public Builder deleteProjectWatches(Collection projectWatches) { + delegate.deleteProjectWatches(projectWatches); + return this; + } } } } diff --git a/java/com/google/gerrit/server/account/WatchConfig.java b/java/com/google/gerrit/server/account/WatchConfig.java index 667ca37642..7adadf7b6b 100644 --- a/java/com/google/gerrit/server/account/WatchConfig.java +++ b/java/com/google/gerrit/server/account/WatchConfig.java @@ -15,7 +15,7 @@ package com.google.gerrit.server.account; import static com.google.common.base.MoreObjects.firstNonNull; -import static com.google.common.base.Preconditions.checkState; +import static com.google.common.base.Preconditions.checkNotNull; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; @@ -23,7 +23,6 @@ import com.google.common.base.Enums; import com.google.common.base.Joiner; import com.google.common.base.Splitter; import com.google.common.base.Strings; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; import com.google.common.collect.MultimapBuilder; @@ -31,17 +30,7 @@ import com.google.common.collect.Sets; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.reviewdb.client.RefNames; -import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ValidationError; -import com.google.gerrit.server.git.VersionedMetaData; -import com.google.inject.Inject; -import com.google.inject.Provider; -import com.google.inject.Singleton; -import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.EnumSet; @@ -49,10 +38,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.Repository; /** * ‘watch.config’ file in the user branch in the All-Users repository that contains the watch @@ -85,91 +71,7 @@ import org.eclipse.jgit.lib.Repository; * *

Unknown notify types are ignored and removed on save. */ -public class WatchConfig extends VersionedMetaData implements ValidationError.Sink { - @Singleton - public static class Accessor { - private final GitRepositoryManager repoManager; - private final AllUsersName allUsersName; - private final Provider metaDataUpdateFactory; - private final IdentifiedUser.GenericFactory userFactory; - - @Inject - Accessor( - GitRepositoryManager repoManager, - AllUsersName allUsersName, - Provider metaDataUpdateFactory, - IdentifiedUser.GenericFactory userFactory) { - this.repoManager = repoManager; - this.allUsersName = allUsersName; - this.metaDataUpdateFactory = metaDataUpdateFactory; - this.userFactory = userFactory; - } - - public Map> getProjectWatches(Account.Id accountId) - throws IOException, ConfigInvalidException { - try (Repository git = repoManager.openRepository(allUsersName)) { - WatchConfig watchConfig = new WatchConfig(accountId); - watchConfig.load(git); - return watchConfig.getProjectWatches(); - } - } - - public synchronized void upsertProjectWatches( - Account.Id accountId, Map> newProjectWatches) - throws IOException, ConfigInvalidException { - WatchConfig watchConfig = read(accountId); - Map> projectWatches = watchConfig.getProjectWatches(); - projectWatches.putAll(newProjectWatches); - commit(watchConfig); - } - - public synchronized void deleteProjectWatches( - Account.Id accountId, Collection projectWatchKeys) - throws IOException, ConfigInvalidException { - WatchConfig watchConfig = read(accountId); - Map> projectWatches = watchConfig.getProjectWatches(); - boolean commit = false; - for (ProjectWatchKey key : projectWatchKeys) { - if (projectWatches.remove(key) != null) { - commit = true; - } - } - if (commit) { - commit(watchConfig); - } - } - - public synchronized void deleteAllProjectWatches(Account.Id accountId) - throws IOException, ConfigInvalidException { - WatchConfig watchConfig = read(accountId); - boolean commit = false; - if (!watchConfig.getProjectWatches().isEmpty()) { - watchConfig.getProjectWatches().clear(); - commit = true; - } - if (commit) { - commit(watchConfig); - } - } - - private WatchConfig read(Account.Id accountId) throws IOException, ConfigInvalidException { - try (Repository git = repoManager.openRepository(allUsersName)) { - WatchConfig watchConfig = new WatchConfig(accountId); - watchConfig.load(git); - return watchConfig; - } - } - - private void commit(WatchConfig watchConfig) throws IOException { - try (MetaDataUpdate md = - metaDataUpdateFactory - .get() - .create(allUsersName, userFactory.create(watchConfig.accountId))) { - watchConfig.commit(md); - } - } - } - +public class WatchConfig { @AutoValue public abstract static class ProjectWatchKey { public static ProjectWatchKey create(Project.NameKey project, @Nullable String filter) { @@ -199,25 +101,26 @@ public class WatchConfig extends VersionedMetaData implements ValidationError.Si public static final String KEY_NOTIFY = "notify"; private final Account.Id accountId; - private final String ref; + private final Config cfg; + private final ValidationError.Sink validationErrorSink; private Map> projectWatches; - private List validationErrors; - public WatchConfig(Account.Id accountId) { - this.accountId = accountId; - this.ref = RefNames.refsUsers(accountId); + public WatchConfig(Account.Id accountId, Config cfg, ValidationError.Sink validationErrorSink) { + this.accountId = checkNotNull(accountId, "accountId"); + this.cfg = checkNotNull(cfg, "cfg"); + this.validationErrorSink = checkNotNull(validationErrorSink, "validationErrorSink"); } - @Override - protected String getRefName() { - return ref; + public Map> getProjectWatches() { + if (projectWatches == null) { + parse(); + } + return projectWatches; } - @Override - protected void onLoad() throws IOException, ConfigInvalidException { - Config cfg = readConfig(WATCH_CONFIG); - projectWatches = parse(accountId, cfg, this); + public void parse() { + projectWatches = parse(accountId, cfg, validationErrorSink); } @VisibleForTesting @@ -248,24 +151,8 @@ public class WatchConfig extends VersionedMetaData implements ValidationError.Si return projectWatches; } - Map> getProjectWatches() { - checkLoaded(); - return projectWatches; - } - - public void setProjectWatches(Map> projectWatches) { + public Config save(Map> projectWatches) { this.projectWatches = projectWatches; - } - - @Override - protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException { - checkLoaded(); - - if (Strings.isNullOrEmpty(commit.getMessage())) { - commit.setMessage("Updated watch configuration\n"); - } - - Config cfg = readConfig(WATCH_CONFIG); for (String projectName : cfg.getSubsections(PROJECT)) { cfg.unsetSection(PROJECT, projectName); @@ -282,32 +169,7 @@ public class WatchConfig extends VersionedMetaData implements ValidationError.Si cfg.setStringList(PROJECT, e.getKey(), KEY_NOTIFY, new ArrayList<>(e.getValue())); } - saveConfig(WATCH_CONFIG, cfg); - return true; - } - - private void checkLoaded() { - checkState(projectWatches != null, "project watches not loaded yet"); - } - - @Override - public void error(ValidationError error) { - if (validationErrors == null) { - validationErrors = new ArrayList<>(4); - } - validationErrors.add(error); - } - - /** - * Get the validation errors, if any were discovered during load. - * - * @return list of errors; empty list if there are no errors. - */ - public List getValidationErrors() { - if (validationErrors != null) { - return ImmutableList.copyOf(validationErrors); - } - return ImmutableList.of(); + return cfg; } @AutoValue @@ -356,7 +218,7 @@ public class WatchConfig extends VersionedMetaData implements ValidationError.Si return create(filter, notifyTypes); } - public static NotifyValue create(@Nullable String filter, Set notifyTypes) { + public static NotifyValue create(@Nullable String filter, Collection notifyTypes) { return new AutoValue_WatchConfig_NotifyValue( Strings.emptyToNull(filter), Sets.immutableEnumSet(notifyTypes)); } diff --git a/java/com/google/gerrit/server/git/validators/AccountValidator.java b/java/com/google/gerrit/server/git/validators/AccountValidator.java index 282e215322..7fbb15811c 100644 --- a/java/com/google/gerrit/server/git/validators/AccountValidator.java +++ b/java/com/google/gerrit/server/git/validators/AccountValidator.java @@ -14,11 +14,14 @@ package com.google.gerrit.server.git.validators; +import static java.util.stream.Collectors.toSet; + import com.google.common.collect.ImmutableList; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountConfig; +import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.inject.Inject; import com.google.inject.Provider; @@ -47,15 +50,16 @@ public class AccountValidator { Optional oldAccount = Optional.empty(); if (oldId != null && !ObjectId.zeroId().equals(oldId)) { try { - oldAccount = loadAccount(accountId, rw, oldId); + oldAccount = loadAccount(accountId, rw, oldId, null); } catch (ConfigInvalidException e) { // ignore, maybe the new commit is repairing it now } } + List messages = new ArrayList<>(); Optional newAccount; try { - newAccount = loadAccount(accountId, rw, newId); + newAccount = loadAccount(accountId, rw, newId, messages); } catch (ConfigInvalidException e) { return ImmutableList.of( String.format( @@ -67,7 +71,6 @@ public class AccountValidator { return ImmutableList.of(String.format("account '%s' does not exist", accountId.get())); } - List messages = new ArrayList<>(); if (accountId.equals(self.get().getAccountId()) && !newAccount.get().isActive()) { messages.add("cannot deactivate own account"); } @@ -87,11 +90,20 @@ public class AccountValidator { return ImmutableList.copyOf(messages); } - private Optional loadAccount(Account.Id accountId, RevWalk rw, ObjectId commit) + private Optional loadAccount( + Account.Id accountId, RevWalk rw, ObjectId commit, @Nullable List messages) throws IOException, ConfigInvalidException { rw.reset(); AccountConfig accountConfig = new AccountConfig(accountId); - accountConfig.load(rw, commit); + accountConfig.setEagerParsing(true).load(rw, commit); + if (messages != null) { + messages.addAll( + accountConfig + .getValidationErrors() + .stream() + .map(ValidationError::getMessage) + .collect(toSet())); + } return accountConfig.getLoadedAccount(); } } diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java index fc280c261e..48b363bb52 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -34,7 +34,6 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.account.WatchConfig; import com.google.gerrit.server.account.externalids.ExternalIdsConsistencyChecker; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.AllUsersName; @@ -418,34 +417,6 @@ public class CommitValidators { } } - if (allUsers.equals(branch.getParentKey()) && RefNames.isRefsUsers(branch.get())) { - List messages = new ArrayList<>(); - Account.Id accountId = Account.Id.fromRef(branch.get()); - if (accountId != null) { - try { - WatchConfig wc = new WatchConfig(accountId); - wc.load(rw, receiveEvent.command.getNewId()); - if (!wc.getValidationErrors().isEmpty()) { - addError("Invalid project configuration:", messages); - for (ValidationError err : wc.getValidationErrors()) { - addError(" " + err.getMessage(), messages); - } - throw new ConfigInvalidException("invalid watch configuration"); - } - } catch (IOException | ConfigInvalidException e) { - log.error( - "User " - + user.getUserName() - + " tried to push an invalid watch configuration " - + receiveEvent.command.getNewId().name() - + " for account " - + accountId.get(), - e); - throw new CommitValidationException("invalid watch configuration", messages); - } - } - } - return Collections.emptyList(); } } diff --git a/java/com/google/gerrit/server/restapi/account/DeleteWatchedProjects.java b/java/com/google/gerrit/server/restapi/account/DeleteWatchedProjects.java index 1388523b24..5a1f6bf672 100644 --- a/java/com/google/gerrit/server/restapi/account/DeleteWatchedProjects.java +++ b/java/com/google/gerrit/server/restapi/account/DeleteWatchedProjects.java @@ -24,9 +24,8 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountResource; -import com.google.gerrit.server.account.WatchConfig; +import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; @@ -45,19 +44,16 @@ public class DeleteWatchedProjects implements RestModifyView> { private final Provider self; private final PermissionBackend permissionBackend; - private final AccountCache accountCache; - private final WatchConfig.Accessor watchConfig; + private final AccountsUpdate.User accountsUpdate; @Inject DeleteWatchedProjects( Provider self, PermissionBackend permissionBackend, - AccountCache accountCache, - WatchConfig.Accessor watchConfig) { + AccountsUpdate.User accountsUpdate) { this.self = self; this.permissionBackend = permissionBackend; - this.accountCache = accountCache; - this.watchConfig = watchConfig; + this.accountsUpdate = accountsUpdate; } @Override @@ -72,14 +68,18 @@ public class DeleteWatchedProjects } Account.Id accountId = rsrc.getUser().getAccountId(); - watchConfig.deleteProjectWatches( - accountId, - input - .stream() - .filter(Objects::nonNull) - .map(w -> ProjectWatchKey.create(new Project.NameKey(w.project), w.filter)) - .collect(toList())); - accountCache.evict(accountId); + accountsUpdate + .create() + .update( + "Delete Project Watches via API", + accountId, + u -> + u.deleteProjectWatches( + input + .stream() + .filter(Objects::nonNull) + .map(w -> ProjectWatchKey.create(new Project.NameKey(w.project), w.filter)) + .collect(toList()))); return Response.none(); } } diff --git a/java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java b/java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java index b4650299cf..3d461402a5 100644 --- a/java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java +++ b/java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java @@ -22,7 +22,7 @@ import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountResource; -import com.google.gerrit.server.account.WatchConfig; +import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.account.WatchConfig.NotifyType; import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.permissions.GlobalPermission; @@ -45,16 +45,14 @@ import org.eclipse.jgit.errors.ConfigInvalidException; public class GetWatchedProjects implements RestReadView { private final PermissionBackend permissionBackend; private final Provider self; - private final WatchConfig.Accessor watchConfig; + private final Accounts accounts; @Inject public GetWatchedProjects( - PermissionBackend permissionBackend, - Provider self, - WatchConfig.Accessor watchConfig) { + PermissionBackend permissionBackend, Provider self, Accounts accounts) { this.permissionBackend = permissionBackend; this.self = self; - this.watchConfig = watchConfig; + this.accounts = accounts; } @Override @@ -68,7 +66,7 @@ public class GetWatchedProjects implements RestReadView { Account.Id accountId = rsrc.getUser().getAccountId(); List projectWatchInfos = new ArrayList<>(); for (Map.Entry> e : - watchConfig.getProjectWatches(accountId).entrySet()) { + accounts.getProjectWatches(accountId).entrySet()) { ProjectWatchInfo pwi = new ProjectWatchInfo(); pwi.filter = e.getKey().filter(); pwi.project = e.getKey().project().get(); diff --git a/java/com/google/gerrit/server/restapi/account/PostWatchedProjects.java b/java/com/google/gerrit/server/restapi/account/PostWatchedProjects.java index 145ce0ee51..bceaaf6d5a 100644 --- a/java/com/google/gerrit/server/restapi/account/PostWatchedProjects.java +++ b/java/com/google/gerrit/server/restapi/account/PostWatchedProjects.java @@ -19,10 +19,9 @@ import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountResource; +import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.WatchConfig; import com.google.gerrit.server.account.WatchConfig.NotifyType; import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; @@ -49,8 +48,7 @@ public class PostWatchedProjects private final PermissionBackend permissionBackend; private final GetWatchedProjects getWatchedProjects; private final ProjectsCollection projectsCollection; - private final AccountCache accountCache; - private final WatchConfig.Accessor watchConfig; + private final AccountsUpdate.User accountsUpdate; @Inject public PostWatchedProjects( @@ -58,14 +56,12 @@ public class PostWatchedProjects PermissionBackend permissionBackend, GetWatchedProjects getWatchedProjects, ProjectsCollection projectsCollection, - AccountCache accountCache, - WatchConfig.Accessor watchConfig) { + AccountsUpdate.User accountsUpdate) { this.self = self; this.permissionBackend = permissionBackend; this.getWatchedProjects = getWatchedProjects; this.projectsCollection = projectsCollection; - this.accountCache = accountCache; - this.watchConfig = watchConfig; + this.accountsUpdate = accountsUpdate; } @Override @@ -76,9 +72,13 @@ public class PostWatchedProjects permissionBackend.user(self).check(GlobalPermission.ADMINISTRATE_SERVER); } - Account.Id accountId = rsrc.getUser().getAccountId(); - watchConfig.upsertProjectWatches(accountId, asMap(input)); - accountCache.evict(accountId); + Map> projectWatches = asMap(input); + accountsUpdate + .create() + .update( + "Update Project Watches via API", + rsrc.getUser().getAccountId(), + u -> u.updateProjectWatches(projectWatches)); return getWatchedProjects.apply(rsrc); } diff --git a/java/com/google/gerrit/server/schema/Schema_139.java b/java/com/google/gerrit/server/schema/Schema_139.java index 4dfc41a01c..990b67b61f 100644 --- a/java/com/google/gerrit/server/schema/Schema_139.java +++ b/java/com/google/gerrit/server/schema/Schema_139.java @@ -24,7 +24,8 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; -import com.google.gerrit.server.account.WatchConfig; +import com.google.gerrit.server.account.AccountConfig; +import com.google.gerrit.server.account.InternalAccountUpdate; import com.google.gerrit.server.account.WatchConfig.NotifyType; import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.config.AllUsersName; @@ -147,10 +148,14 @@ public class Schema_139 extends SchemaVersion { md.getCommitBuilder().setCommitter(serverUser); md.setMessage(MSG); - WatchConfig watchConfig = new WatchConfig(e.getKey()); - watchConfig.load(md); - watchConfig.setProjectWatches(projectWatches); - watchConfig.commit(md); + AccountConfig accountConfig = new AccountConfig(e.getKey()); + accountConfig.load(md); + accountConfig.setAccountUpdate( + InternalAccountUpdate.builder() + .deleteProjectWatches(accountConfig.getProjectWatches().keySet()) + .updateProjectWatches(projectWatches) + .build()); + accountConfig.commit(md); } } bru.execute(rw, NullProgressMonitor.INSTANCE); diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 4217317878..69b73cf35d 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -1335,7 +1335,7 @@ public class AccountIT extends AbstractDaemonTest { WatchConfig.WATCH_CONFIG, wc.toText()); PushOneCommit.Result r = push.to(RefNames.REFS_USERS_SELF); - r.assertErrorStatus("invalid watch configuration"); + r.assertErrorStatus("invalid account configuration"); r.assertMessage( String.format( "%s: Invalid project watch of account %d for project %s: %s", diff --git a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java index 228beef700..1236826f27 100644 --- a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java +++ b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java @@ -26,30 +26,21 @@ import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.StarsInput; import com.google.gerrit.extensions.common.GroupInfo; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.server.account.WatchConfig; import com.google.gerrit.server.account.WatchConfig.NotifyType; -import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; import com.google.gerrit.server.git.NotifyConfig; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.mail.Address; import com.google.gerrit.testing.FakeEmailSender.Message; -import com.google.inject.Inject; import java.util.EnumSet; -import java.util.HashMap; import java.util.List; -import java.util.Map; -import java.util.Set; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.junit.Test; @NoHttpd public class ProjectWatchIT extends AbstractDaemonTest { - @Inject private WatchConfig.Accessor watchConfig; - @Test public void newPatchSetsNotifyConfig() throws Exception { Address addr = new Address("Watcher", "watcher@example.com"); @@ -493,34 +484,6 @@ public class ProjectWatchIT extends AbstractDaemonTest { assertThat(sender.getMessages()).isEmpty(); } - @Test - public void deleteAllProjectWatches() throws Exception { - Map> watches = new HashMap<>(); - watches.put(ProjectWatchKey.create(project, "*"), ImmutableSet.of(NotifyType.ALL)); - watchConfig.upsertProjectWatches(admin.getId(), watches); - assertThat(watchConfig.getProjectWatches(admin.getId())).isNotEmpty(); - - watchConfig.deleteAllProjectWatches(admin.getId()); - assertThat(watchConfig.getProjectWatches(admin.getId())).isEmpty(); - } - - @Test - public void deleteAllProjectWatchesIfWatchConfigIsTheOnlyFileInUserBranch() throws Exception { - // Create account that has no files in its refs/users/ branch. - Account.Id id = accountCreator.create().id; - - // Add a project watch so that a watch.config file in the refs/users/ branch is created. - Map> watches = new HashMap<>(); - watches.put(ProjectWatchKey.create(project, "*"), ImmutableSet.of(NotifyType.ALL)); - watchConfig.upsertProjectWatches(id, watches); - assertThat(watchConfig.getProjectWatches(id)).isNotEmpty(); - - // Delete all project watches so that the watch.config file in the refs/users/ branch is - // deleted. - watchConfig.deleteAllProjectWatches(id); - assertThat(watchConfig.getProjectWatches(id)).isEmpty(); - } - @Test public void watchProjectNoNotificationForPrivateChange() throws Exception { // watch project From 56f4a159a193cfabcd36e91fa082174ee400c866 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 5 Jan 2018 14:29:33 +0100 Subject: [PATCH 2/7] ExternalIdNotes: Make constructor and load() method private ExternalIdNotes should either be created by the factories or the static load methods. The constructor and the load() method were not private because ExternalIdsUpdate needed them, but now after ExternalIdsUpdate was removed we can make them private. Change-Id: I19c8b140e60d8685595d831724d14cac075a578d Signed-off-by: Edwin Kempin --- .../gerrit/server/account/externalids/ExternalIdNotes.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java index f663247e4c..8e69f2e7b2 100644 --- a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java +++ b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java @@ -171,7 +171,7 @@ public class ExternalIdNotes extends VersionedMetaData { private Runnable afterReadRevision; private boolean readOnly = false; - ExternalIdNotes( + private ExternalIdNotes( ExternalIdCache externalIdCache, @Nullable AccountCache accountCache, Repository allUsersRepo) { @@ -205,7 +205,7 @@ public class ExternalIdNotes extends VersionedMetaData { * * @return {@link ExternalIdNotes} instance for chaining */ - ExternalIdNotes load() throws IOException, ConfigInvalidException { + private ExternalIdNotes load() throws IOException, ConfigInvalidException { load(repo); return this; } From 1b7bac945cb8efd635989cf98957d0eb6408dd15 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 5 Jan 2018 08:32:54 +0100 Subject: [PATCH 3/7] Allow to update accounts and general preferences atomically AccountsUpdate now also supports to update general preferences. This means account properties and general preferences can now be updated in the same transaction. Reading and writing general preferences is now done via AccountConfig. On load we always read the preferences.config file, but parsing it is done lazily when the general preferences are accessed for the first time. Most callers that load accounts are not interested in general preferences and this avoids unneeded parsing effort for them. PreferencesConfig encapsules all details about parsing and storing general preferences and also provides means to read and update the default general preferences. Change-Id: Ic0f2adfe4e96311af0c5bfa061fc7c0ca60d91fa Signed-off-by: Edwin Kempin --- .../server/account/AccountCacheImpl.java | 10 +- .../gerrit/server/account/AccountConfig.java | 48 ++- .../gerrit/server/account/Accounts.java | 16 +- .../gerrit/server/account/AccountsUpdate.java | 5 +- .../account/GeneralPreferencesLoader.java | 198 ---------- .../server/account/InternalAccountUpdate.java | 27 ++ .../server/account/PreferencesConfig.java | 368 ++++++++++++++++++ .../git/validators/AccountValidator.java | 15 +- .../git/validators/CommitValidators.java | 18 +- .../git/validators/MergeValidators.java | 2 +- .../restapi/account/SetPreferences.java | 59 +-- .../server/restapi/config/GetPreferences.java | 36 +- .../server/restapi/config/SetPreferences.java | 45 +-- .../gerrit/server/schema/Schema_139.java | 22 +- .../gerrit/server/schema/Schema_154.java | 5 +- .../api/config/GeneralPreferencesIT.java | 4 +- .../account/AbstractQueryAccountsTest.java | 8 +- .../server/schema/Schema_159_to_160_Test.java | 2 +- 18 files changed, 518 insertions(+), 370 deletions(-) delete mode 100644 java/com/google/gerrit/server/account/GeneralPreferencesLoader.java create mode 100644 java/com/google/gerrit/server/account/PreferencesConfig.java diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java index 5342710bc7..2ad31b2c27 100644 --- a/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java @@ -136,18 +136,12 @@ public class AccountCacheImpl implements AccountCache { static class ByIdLoader extends CacheLoader> { private final AllUsersName allUsersName; private final Accounts accounts; - private final GeneralPreferencesLoader loader; private final ExternalIds externalIds; @Inject - ByIdLoader( - AllUsersName allUsersName, - Accounts accounts, - GeneralPreferencesLoader loader, - ExternalIds externalIds) { + ByIdLoader(AllUsersName allUsersName, Accounts accounts, ExternalIds externalIds) { this.allUsersName = allUsersName; this.accounts = accounts; - this.loader = loader; this.externalIds = externalIds; } @@ -159,7 +153,7 @@ public class AccountCacheImpl implements AccountCache { } try { - account.setGeneralPreferences(loader.load(who)); + account.setGeneralPreferences(accounts.getGeneralPreferences(who)); } catch (IOException | ConfigInvalidException e) { log.warn("Cannot load GeneralPreferences for " + who + " (using default)", e); account.setGeneralPreferences(GeneralPreferencesInfo.defaults()); diff --git a/java/com/google/gerrit/server/account/AccountConfig.java b/java/com/google/gerrit/server/account/AccountConfig.java index b634961556..2314f5f41d 100644 --- a/java/com/google/gerrit/server/account/AccountConfig.java +++ b/java/com/google/gerrit/server/account/AccountConfig.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkState; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.account.WatchConfig.NotifyType; @@ -39,6 +40,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevSort; @@ -78,17 +80,20 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. public static final String KEY_STATUS = "status"; private final Account.Id accountId; + private final Repository repo; private final String ref; private Optional loadedAccount; private WatchConfig watchConfig; + private PreferencesConfig prefConfig; private Optional accountUpdate = Optional.empty(); private Timestamp registeredOn; private boolean eagerParsing; private List validationErrors; - public AccountConfig(Account.Id accountId) { - this.accountId = checkNotNull(accountId); + public AccountConfig(Account.Id accountId, Repository allUsersRepo) { + this.accountId = checkNotNull(accountId, "accountId"); + this.repo = checkNotNull(allUsersRepo, "allUsersRepo"); this.ref = RefNames.refsUsers(accountId); } @@ -112,6 +117,11 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. return ref; } + public AccountConfig load() throws IOException, ConfigInvalidException { + load(repo); + return this; + } + /** * Get the loaded account. * @@ -134,6 +144,16 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. return watchConfig.getProjectWatches(); } + /** + * Get the general preferences of the loaded account. + * + * @return the general preferences of the loaded account + */ + public GeneralPreferencesInfo getGeneralPreferences() { + checkLoaded(); + return prefConfig.getGeneralPreferences(); + } + /** * Sets the account. This means the loaded account will be overwritten with the given account. * @@ -142,7 +162,7 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. * @param account account that should be set * @throws IllegalStateException if the account was not loaded yet */ - public void setAccount(Account account) { + public AccountConfig setAccount(Account account) { checkLoaded(); this.loadedAccount = Optional.of(account); this.accountUpdate = @@ -154,6 +174,7 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. .setStatus(account.getStatus()) .build()); this.registeredOn = account.getRegisteredOn(); + return this; } /** @@ -182,8 +203,9 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. return loadedAccount.get(); } - public void setAccountUpdate(InternalAccountUpdate accountUpdate) { + public AccountConfig setAccountUpdate(InternalAccountUpdate accountUpdate) { this.accountUpdate = Optional.of(accountUpdate); + return this; } @Override @@ -198,8 +220,17 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. loadedAccount = Optional.of(parse(accountConfig, revision.name())); watchConfig = new WatchConfig(accountId, readConfig(WatchConfig.WATCH_CONFIG), this); + + prefConfig = + new PreferencesConfig( + accountId, + readConfig(PreferencesConfig.PREFERENCES_CONFIG), + PreferencesConfig.readDefaultConfig(repo), + this); + if (eagerParsing) { watchConfig.parse(); + prefConfig.parse(); } } else { loadedAccount = Optional.empty(); @@ -249,6 +280,7 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. Config accountConfig = saveAccount(); saveProjectWatches(); + saveGeneralPreferences(); // metaId is set in the commit(MetaDataUpdate) method after the commit is created loadedAccount = Optional.of(parse(accountConfig, null)); @@ -290,6 +322,14 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. } } + private void saveGeneralPreferences() throws IOException, ConfigInvalidException { + if (accountUpdate.isPresent() && accountUpdate.get().getGeneralPreferences().isPresent()) { + saveConfig( + PreferencesConfig.PREFERENCES_CONFIG, + prefConfig.saveGeneralPreferences(accountUpdate.get().getGeneralPreferences().get())); + } + } + /** * Sets/Unsets {@code account.active} in the given config. * diff --git a/java/com/google/gerrit/server/account/Accounts.java b/java/com/google/gerrit/server/account/Accounts.java index 258f5a981c..512483cbe5 100644 --- a/java/com/google/gerrit/server/account/Accounts.java +++ b/java/com/google/gerrit/server/account/Accounts.java @@ -19,6 +19,7 @@ import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.account.WatchConfig.NotifyType; @@ -130,9 +131,14 @@ public class Accounts { public Map> getProjectWatches(Account.Id accountId) throws IOException, ConfigInvalidException { try (Repository repo = repoManager.openRepository(allUsersName)) { - AccountConfig accountConfig = new AccountConfig(accountId); - accountConfig.load(repo); - return accountConfig.getProjectWatches(); + return new AccountConfig(accountId, repo).load().getProjectWatches(); + } + } + + public GeneralPreferencesInfo getGeneralPreferences(Account.Id accountId) + throws IOException, ConfigInvalidException { + try (Repository repo = repoManager.openRepository(allUsersName)) { + return new AccountConfig(accountId, repo).load().getGeneralPreferences(); } } @@ -144,9 +150,7 @@ public class Accounts { private Optional read(Repository allUsersRepository, Account.Id accountId) throws IOException, ConfigInvalidException { - AccountConfig accountConfig = new AccountConfig(accountId); - accountConfig.load(allUsersRepository); - return accountConfig.getLoadedAccount(); + return new AccountConfig(accountId, allUsersRepository).load().getLoadedAccount(); } public static Stream readUserRefs(Repository repo) throws IOException { diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java index 1b9f30cbd1..21e6c5fec2 100644 --- a/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/java/com/google/gerrit/server/account/AccountsUpdate.java @@ -424,11 +424,8 @@ public class AccountsUpdate { private AccountConfig read(Repository allUsersRepo, Account.Id accountId) throws IOException, ConfigInvalidException { - AccountConfig accountConfig = new AccountConfig(accountId); - accountConfig.load(allUsersRepo); - + AccountConfig accountConfig = new AccountConfig(accountId, allUsersRepo).load(); afterReadRevision.run(); - return accountConfig; } diff --git a/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java b/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java deleted file mode 100644 index 804377336f..0000000000 --- a/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java +++ /dev/null @@ -1,198 +0,0 @@ -// Copyright (C) 2015 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; - -import static com.google.gerrit.server.config.ConfigUtil.loadSection; -import static com.google.gerrit.server.config.ConfigUtil.skipField; -import static com.google.gerrit.server.git.UserConfigSections.CHANGE_TABLE; -import static com.google.gerrit.server.git.UserConfigSections.CHANGE_TABLE_COLUMN; -import static com.google.gerrit.server.git.UserConfigSections.KEY_ID; -import static com.google.gerrit.server.git.UserConfigSections.KEY_MATCH; -import static com.google.gerrit.server.git.UserConfigSections.KEY_TARGET; -import static com.google.gerrit.server.git.UserConfigSections.KEY_TOKEN; -import static com.google.gerrit.server.git.UserConfigSections.KEY_URL; -import static com.google.gerrit.server.git.UserConfigSections.URL_ALIAS; - -import com.google.common.base.Strings; -import com.google.common.collect.Lists; -import com.google.gerrit.extensions.client.GeneralPreferencesInfo; -import com.google.gerrit.extensions.client.MenuItem; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.UserConfigSections; -import com.google.inject.Inject; -import com.google.inject.Singleton; -import java.io.IOException; -import java.lang.reflect.Field; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.errors.RepositoryNotFoundException; -import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.Repository; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -@Singleton -public class GeneralPreferencesLoader { - private static final Logger log = LoggerFactory.getLogger(GeneralPreferencesLoader.class); - - private final GitRepositoryManager gitMgr; - private final AllUsersName allUsersName; - - @Inject - public GeneralPreferencesLoader(GitRepositoryManager gitMgr, AllUsersName allUsersName) { - this.gitMgr = gitMgr; - this.allUsersName = allUsersName; - } - - public GeneralPreferencesInfo load(Account.Id id) - throws IOException, ConfigInvalidException, RepositoryNotFoundException { - return read(id, null); - } - - public GeneralPreferencesInfo merge(Account.Id id, GeneralPreferencesInfo in) - throws IOException, ConfigInvalidException, RepositoryNotFoundException { - return read(id, in); - } - - private GeneralPreferencesInfo read(Account.Id id, GeneralPreferencesInfo in) - throws IOException, ConfigInvalidException, RepositoryNotFoundException { - try (Repository allUsers = gitMgr.openRepository(allUsersName)) { - // Load all users default prefs - VersionedAccountPreferences dp = VersionedAccountPreferences.forDefault(); - dp.load(allUsers); - - // Load user prefs - VersionedAccountPreferences p = VersionedAccountPreferences.forUser(id); - p.load(allUsers); - GeneralPreferencesInfo r = - loadSection( - p.getConfig(), - UserConfigSections.GENERAL, - null, - new GeneralPreferencesInfo(), - readDefaultsFromGit(dp.getConfig(), in), - in); - loadChangeTableColumns(r, p, dp); - return loadMyMenusAndUrlAliases(r, p, dp); - } - } - - public GeneralPreferencesInfo readDefaultsFromGit(Repository git, GeneralPreferencesInfo in) - throws ConfigInvalidException, IOException { - VersionedAccountPreferences dp = VersionedAccountPreferences.forDefault(); - dp.load(git); - return readDefaultsFromGit(dp.getConfig(), in); - } - - private GeneralPreferencesInfo readDefaultsFromGit(Config config, GeneralPreferencesInfo in) - throws ConfigInvalidException { - GeneralPreferencesInfo allUserPrefs = new GeneralPreferencesInfo(); - loadSection( - config, - UserConfigSections.GENERAL, - null, - allUserPrefs, - GeneralPreferencesInfo.defaults(), - in); - return updateDefaults(allUserPrefs); - } - - private GeneralPreferencesInfo updateDefaults(GeneralPreferencesInfo input) { - GeneralPreferencesInfo result = GeneralPreferencesInfo.defaults(); - try { - for (Field field : input.getClass().getDeclaredFields()) { - if (skipField(field)) { - continue; - } - Object newVal = field.get(input); - if (newVal != null) { - field.set(result, newVal); - } - } - } catch (IllegalAccessException e) { - log.error("Cannot get default general preferences from " + allUsersName.get(), e); - return GeneralPreferencesInfo.defaults(); - } - return result; - } - - public GeneralPreferencesInfo loadMyMenusAndUrlAliases( - GeneralPreferencesInfo r, VersionedAccountPreferences v, VersionedAccountPreferences d) { - r.my = my(v); - if (r.my.isEmpty() && !v.isDefaults()) { - r.my = my(d); - } - if (r.my.isEmpty()) { - r.my.add(new MenuItem("Changes", "#/dashboard/self", null)); - r.my.add(new MenuItem("Draft Comments", "#/q/has:draft", null)); - r.my.add(new MenuItem("Edits", "#/q/has:edit", null)); - r.my.add(new MenuItem("Watched Changes", "#/q/is:watched+is:open", null)); - r.my.add(new MenuItem("Starred Changes", "#/q/is:starred", null)); - r.my.add(new MenuItem("Groups", "#/groups/self", null)); - } - - r.urlAliases = urlAliases(v); - if (r.urlAliases == null && !v.isDefaults()) { - r.urlAliases = urlAliases(d); - } - return r; - } - - private static List my(VersionedAccountPreferences v) { - List my = new ArrayList<>(); - Config cfg = v.getConfig(); - for (String subsection : cfg.getSubsections(UserConfigSections.MY)) { - String url = my(cfg, subsection, KEY_URL, "#/"); - String target = my(cfg, subsection, KEY_TARGET, url.startsWith("#") ? null : "_blank"); - my.add(new MenuItem(subsection, url, target, my(cfg, subsection, KEY_ID, null))); - } - return my; - } - - private static String my(Config cfg, String subsection, String key, String defaultValue) { - String val = cfg.getString(UserConfigSections.MY, subsection, key); - return !Strings.isNullOrEmpty(val) ? val : defaultValue; - } - - public GeneralPreferencesInfo loadChangeTableColumns( - GeneralPreferencesInfo r, VersionedAccountPreferences v, VersionedAccountPreferences d) { - r.changeTable = changeTable(v); - - if (r.changeTable.isEmpty() && !v.isDefaults()) { - r.changeTable = changeTable(d); - } - return r; - } - - private static List changeTable(VersionedAccountPreferences v) { - return Lists.newArrayList(v.getConfig().getStringList(CHANGE_TABLE, null, CHANGE_TABLE_COLUMN)); - } - - private static Map urlAliases(VersionedAccountPreferences v) { - HashMap urlAliases = new HashMap<>(); - Config cfg = v.getConfig(); - for (String subsection : cfg.getSubsections(URL_ALIAS)) { - urlAliases.put( - cfg.getString(URL_ALIAS, subsection, KEY_MATCH), - cfg.getString(URL_ALIAS, subsection, KEY_TOKEN)); - } - return !urlAliases.isEmpty() ? urlAliases : null; - } -} diff --git a/java/com/google/gerrit/server/account/InternalAccountUpdate.java b/java/com/google/gerrit/server/account/InternalAccountUpdate.java index bede7b6902..05c431e97e 100644 --- a/java/com/google/gerrit/server/account/InternalAccountUpdate.java +++ b/java/com/google/gerrit/server/account/InternalAccountUpdate.java @@ -18,6 +18,7 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.WatchConfig.NotifyType; import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; @@ -111,6 +112,16 @@ public abstract class InternalAccountUpdate { */ public abstract ImmutableSet getDeletedProjectWatches(); + /** + * Returns the new value for the general preferences. + * + *

Only preferences that are non-null in the returned GeneralPreferencesInfo should be updated. + * + * @return the new value for the general preferences, {@code Optional#empty()} if the general + * preferences are not being updated, the wrapped value is never {@code null} + */ + public abstract Optional getGeneralPreferences(); + /** * Class to build an account update. * @@ -363,6 +374,16 @@ public abstract class InternalAccountUpdate { return this; } + /** + * Sets the general preferences for the account. + * + *

Updates any preference that is non-null in the provided GeneralPreferencesInfo. + * + * @param generalPreferences the general preferences that should be set + * @return the builder + */ + public abstract Builder setGeneralPreferences(GeneralPreferencesInfo generalPreferences); + /** * Builds the account update. * @@ -493,6 +514,12 @@ public abstract class InternalAccountUpdate { delegate.deleteProjectWatches(projectWatches); return this; } + + @Override + public Builder setGeneralPreferences(GeneralPreferencesInfo generalPreferences) { + delegate.setGeneralPreferences(generalPreferences); + return this; + } } } } diff --git a/java/com/google/gerrit/server/account/PreferencesConfig.java b/java/com/google/gerrit/server/account/PreferencesConfig.java new file mode 100644 index 0000000000..32df6591a0 --- /dev/null +++ b/java/com/google/gerrit/server/account/PreferencesConfig.java @@ -0,0 +1,368 @@ +// Copyright (C) 2018 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; + +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; +import static com.google.gerrit.server.config.ConfigUtil.loadSection; +import static com.google.gerrit.server.config.ConfigUtil.skipField; +import static com.google.gerrit.server.config.ConfigUtil.storeSection; +import static com.google.gerrit.server.git.UserConfigSections.CHANGE_TABLE; +import static com.google.gerrit.server.git.UserConfigSections.CHANGE_TABLE_COLUMN; +import static com.google.gerrit.server.git.UserConfigSections.KEY_ID; +import static com.google.gerrit.server.git.UserConfigSections.KEY_MATCH; +import static com.google.gerrit.server.git.UserConfigSections.KEY_TARGET; +import static com.google.gerrit.server.git.UserConfigSections.KEY_TOKEN; +import static com.google.gerrit.server.git.UserConfigSections.KEY_URL; +import static com.google.gerrit.server.git.UserConfigSections.URL_ALIAS; + +import com.google.common.base.Strings; +import com.google.common.collect.Lists; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; +import com.google.gerrit.extensions.client.MenuItem; +import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.git.UserConfigSections; +import com.google.gerrit.server.git.ValidationError; +import com.google.gerrit.server.git.VersionedMetaData; +import java.io.IOException; +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.Repository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class PreferencesConfig { + private static final Logger log = LoggerFactory.getLogger(PreferencesConfig.class); + + public static final String PREFERENCES_CONFIG = "preferences.config"; + + private final Account.Id accountId; + private final Config cfg; + private final Config defaultCfg; + private final ValidationError.Sink validationErrorSink; + + private GeneralPreferencesInfo generalPreferences; + + public PreferencesConfig( + Account.Id accountId, + Config cfg, + Config defaultCfg, + ValidationError.Sink validationErrorSink) { + this.accountId = checkNotNull(accountId, "accountId"); + this.cfg = checkNotNull(cfg, "cfg"); + this.defaultCfg = checkNotNull(defaultCfg, "defaultCfg"); + this.validationErrorSink = checkNotNull(validationErrorSink, "validationErrorSink"); + } + + public GeneralPreferencesInfo getGeneralPreferences() { + if (generalPreferences == null) { + parse(); + } + return generalPreferences; + } + + public void parse() { + generalPreferences = parse(null); + } + + public Config saveGeneralPreferences(GeneralPreferencesInfo input) throws ConfigInvalidException { + // merge configs + input = parse(input); + + storeSection( + cfg, UserConfigSections.GENERAL, null, input, parseDefaultPreferences(defaultCfg, null)); + setChangeTable(cfg, input.changeTable); + setMy(cfg, input.my); + setUrlAliases(cfg, input.urlAliases); + + // evict the cached general preferences + this.generalPreferences = null; + + return cfg; + } + + private GeneralPreferencesInfo parse(@Nullable GeneralPreferencesInfo input) { + try { + return parse(cfg, defaultCfg, input); + } catch (ConfigInvalidException e) { + validationErrorSink.error( + new ValidationError( + PREFERENCES_CONFIG, + String.format( + "Invalid general preferences for account %d: %s", + accountId.get(), e.getMessage()))); + return new GeneralPreferencesInfo(); + } + } + + private static GeneralPreferencesInfo parse( + Config cfg, @Nullable Config defaultCfg, @Nullable GeneralPreferencesInfo input) + throws ConfigInvalidException { + GeneralPreferencesInfo r = + loadSection( + cfg, + UserConfigSections.GENERAL, + null, + new GeneralPreferencesInfo(), + defaultCfg != null + ? parseDefaultPreferences(defaultCfg, input) + : GeneralPreferencesInfo.defaults(), + input); + if (input != null) { + r.changeTable = input.changeTable; + r.my = input.my; + r.urlAliases = input.urlAliases; + } else { + r.changeTable = parseChangeTableColumns(cfg, defaultCfg); + r.my = parseMyMenus(cfg, defaultCfg); + r.urlAliases = parseUrlAliases(cfg, defaultCfg); + } + return r; + } + + private static GeneralPreferencesInfo parseDefaultPreferences( + Config defaultCfg, GeneralPreferencesInfo input) throws ConfigInvalidException { + GeneralPreferencesInfo allUserPrefs = new GeneralPreferencesInfo(); + loadSection( + defaultCfg, + UserConfigSections.GENERAL, + null, + allUserPrefs, + GeneralPreferencesInfo.defaults(), + input); + return updateDefaults(allUserPrefs); + } + + private static GeneralPreferencesInfo updateDefaults(GeneralPreferencesInfo input) { + GeneralPreferencesInfo result = GeneralPreferencesInfo.defaults(); + try { + for (Field field : input.getClass().getDeclaredFields()) { + if (skipField(field)) { + continue; + } + Object newVal = field.get(input); + if (newVal != null) { + field.set(result, newVal); + } + } + } catch (IllegalAccessException e) { + log.error("Failed to apply default general preferences", e); + return GeneralPreferencesInfo.defaults(); + } + return result; + } + + private static List parseChangeTableColumns(Config cfg, @Nullable Config defaultCfg) { + List changeTable = changeTable(cfg); + if (changeTable == null && defaultCfg != null) { + changeTable = changeTable(defaultCfg); + } + return changeTable; + } + + private static List parseMyMenus(Config cfg, @Nullable Config defaultCfg) { + List my = my(cfg); + if (my.isEmpty() && defaultCfg != null) { + my = my(defaultCfg); + } + if (my.isEmpty()) { + my.add(new MenuItem("Changes", "#/dashboard/self", null)); + my.add(new MenuItem("Draft Comments", "#/q/has:draft", null)); + my.add(new MenuItem("Edits", "#/q/has:edit", null)); + my.add(new MenuItem("Watched Changes", "#/q/is:watched+is:open", null)); + my.add(new MenuItem("Starred Changes", "#/q/is:starred", null)); + my.add(new MenuItem("Groups", "#/groups/self", null)); + } + return my; + } + + private static Map parseUrlAliases(Config cfg, @Nullable Config defaultCfg) { + Map urlAliases = urlAliases(cfg); + if (urlAliases == null && defaultCfg != null) { + urlAliases = urlAliases(defaultCfg); + } + return urlAliases; + } + + public static GeneralPreferencesInfo readDefaultPreferences(Repository allUsersRepo) + throws IOException, ConfigInvalidException { + return parse(readDefaultConfig(allUsersRepo), null, null); + } + + static Config readDefaultConfig(Repository allUsersRepo) + throws IOException, ConfigInvalidException { + VersionedDefaultPreferences defaultPrefs = new VersionedDefaultPreferences(); + defaultPrefs.load(allUsersRepo); + return defaultPrefs.getConfig(); + } + + public static GeneralPreferencesInfo updateDefaultPreferences( + MetaDataUpdate md, GeneralPreferencesInfo input) throws IOException, ConfigInvalidException { + VersionedDefaultPreferences defaultPrefs = new VersionedDefaultPreferences(); + defaultPrefs.load(md); + storeSection( + defaultPrefs.getConfig(), + UserConfigSections.GENERAL, + null, + input, + GeneralPreferencesInfo.defaults()); + setMy(defaultPrefs.getConfig(), input.my); + setChangeTable(defaultPrefs.getConfig(), input.changeTable); + setUrlAliases(defaultPrefs.getConfig(), input.urlAliases); + defaultPrefs.commit(md); + + return parse(defaultPrefs.getConfig(), null, null); + } + + private static List changeTable(Config cfg) { + return Lists.newArrayList(cfg.getStringList(CHANGE_TABLE, null, CHANGE_TABLE_COLUMN)); + } + + private static void setChangeTable(Config cfg, List changeTable) { + if (changeTable != null) { + unsetSection(cfg, UserConfigSections.CHANGE_TABLE); + cfg.setStringList(UserConfigSections.CHANGE_TABLE, null, CHANGE_TABLE_COLUMN, changeTable); + } + } + + private static List my(Config cfg) { + List my = new ArrayList<>(); + for (String subsection : cfg.getSubsections(UserConfigSections.MY)) { + String url = my(cfg, subsection, KEY_URL, "#/"); + String target = my(cfg, subsection, KEY_TARGET, url.startsWith("#") ? null : "_blank"); + my.add(new MenuItem(subsection, url, target, my(cfg, subsection, KEY_ID, null))); + } + return my; + } + + private static String my(Config cfg, String subsection, String key, String defaultValue) { + String val = cfg.getString(UserConfigSections.MY, subsection, key); + return !Strings.isNullOrEmpty(val) ? val : defaultValue; + } + + private static void setMy(Config cfg, List my) { + if (my != null) { + unsetSection(cfg, UserConfigSections.MY); + for (MenuItem item : my) { + checkState(!isNullOrEmpty(item.name), "MenuItem.name must not be null or empty"); + checkState(!isNullOrEmpty(item.url), "MenuItem.url must not be null or empty"); + + setMy(cfg, item.name, KEY_URL, item.url); + setMy(cfg, item.name, KEY_TARGET, item.target); + setMy(cfg, item.name, KEY_ID, item.id); + } + } + } + + public static void validateMy(List my) throws BadRequestException { + if (my == null) { + return; + } + for (MenuItem item : my) { + checkRequiredMenuItemField(item.name, "name"); + checkRequiredMenuItemField(item.url, "URL"); + } + } + + private static void checkRequiredMenuItemField(String value, String name) + throws BadRequestException { + if (isNullOrEmpty(value)) { + throw new BadRequestException(name + " for menu item is required"); + } + } + + private static boolean isNullOrEmpty(String value) { + return value == null || value.trim().isEmpty(); + } + + private static void setMy(Config cfg, String section, String key, @Nullable String val) { + if (val == null || val.trim().isEmpty()) { + cfg.unset(UserConfigSections.MY, section.trim(), key); + } else { + cfg.setString(UserConfigSections.MY, section.trim(), key, val.trim()); + } + } + + private static Map urlAliases(Config cfg) { + HashMap urlAliases = new HashMap<>(); + for (String subsection : cfg.getSubsections(URL_ALIAS)) { + urlAliases.put( + cfg.getString(URL_ALIAS, subsection, KEY_MATCH), + cfg.getString(URL_ALIAS, subsection, KEY_TOKEN)); + } + return !urlAliases.isEmpty() ? urlAliases : null; + } + + private static void setUrlAliases(Config cfg, Map urlAliases) { + if (urlAliases != null) { + for (String subsection : cfg.getSubsections(URL_ALIAS)) { + cfg.unsetSection(URL_ALIAS, subsection); + } + + int i = 1; + for (Entry e : urlAliases.entrySet()) { + cfg.setString(URL_ALIAS, URL_ALIAS + i, KEY_MATCH, e.getKey()); + cfg.setString(URL_ALIAS, URL_ALIAS + i, KEY_TOKEN, e.getValue()); + i++; + } + } + } + + private static void unsetSection(Config cfg, String section) { + cfg.unsetSection(section, null); + for (String subsection : cfg.getSubsections(section)) { + cfg.unsetSection(section, subsection); + } + } + + private static class VersionedDefaultPreferences extends VersionedMetaData { + private Config cfg; + + @Override + protected String getRefName() { + return RefNames.REFS_USERS_DEFAULT; + } + + private Config getConfig() { + checkState(cfg != null, "Default preferences not loaded yet."); + return cfg; + } + + @Override + protected void onLoad() throws IOException, ConfigInvalidException { + cfg = readConfig(PREFERENCES_CONFIG); + } + + @Override + protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException { + if (Strings.isNullOrEmpty(commit.getMessage())) { + commit.setMessage("Update default preferences\n"); + } + saveConfig(PREFERENCES_CONFIG, cfg); + return true; + } + } +} diff --git a/java/com/google/gerrit/server/git/validators/AccountValidator.java b/java/com/google/gerrit/server/git/validators/AccountValidator.java index 7fbb15811c..bba49eae88 100644 --- a/java/com/google/gerrit/server/git/validators/AccountValidator.java +++ b/java/com/google/gerrit/server/git/validators/AccountValidator.java @@ -31,6 +31,7 @@ import java.util.List; import java.util.Optional; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevWalk; public class AccountValidator { @@ -45,12 +46,12 @@ public class AccountValidator { } public List validate( - Account.Id accountId, RevWalk rw, @Nullable ObjectId oldId, ObjectId newId) + Account.Id accountId, Repository repo, RevWalk rw, @Nullable ObjectId oldId, ObjectId newId) throws IOException { Optional oldAccount = Optional.empty(); if (oldId != null && !ObjectId.zeroId().equals(oldId)) { try { - oldAccount = loadAccount(accountId, rw, oldId, null); + oldAccount = loadAccount(accountId, repo, rw, oldId, null); } catch (ConfigInvalidException e) { // ignore, maybe the new commit is repairing it now } @@ -59,7 +60,7 @@ public class AccountValidator { List messages = new ArrayList<>(); Optional newAccount; try { - newAccount = loadAccount(accountId, rw, newId, messages); + newAccount = loadAccount(accountId, repo, rw, newId, messages); } catch (ConfigInvalidException e) { return ImmutableList.of( String.format( @@ -91,10 +92,14 @@ public class AccountValidator { } private Optional loadAccount( - Account.Id accountId, RevWalk rw, ObjectId commit, @Nullable List messages) + Account.Id accountId, + Repository repo, + RevWalk rw, + ObjectId commit, + @Nullable List messages) throws IOException, ConfigInvalidException { rw.reset(); - AccountConfig accountConfig = new AccountConfig(accountId); + AccountConfig accountConfig = new AccountConfig(accountId, repo); accountConfig.setEagerParsing(true).load(rw, commit); if (messages != null) { messages.addAll( diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java index 48b363bb52..40bb9c9e24 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -41,6 +41,7 @@ import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.git.BanCommit; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.permissions.PermissionBackend; @@ -84,6 +85,7 @@ public class CommitValidators { private final PersonIdent gerritIdent; private final String canonicalWebUrl; private final DynamicSet pluginValidators; + private final GitRepositoryManager repoManager; private final AllUsersName allUsers; private final AllProjectsName allProjects; private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker; @@ -97,6 +99,7 @@ public class CommitValidators { @CanonicalWebUrl @Nullable String canonicalWebUrl, @GerritServerConfig Config cfg, DynamicSet pluginValidators, + GitRepositoryManager repoManager, AllUsersName allUsers, AllProjectsName allProjects, ExternalIdsConsistencyChecker externalIdsConsistencyChecker, @@ -105,6 +108,7 @@ public class CommitValidators { this.gerritIdent = gerritIdent; this.canonicalWebUrl = canonicalWebUrl; this.pluginValidators = pluginValidators; + this.repoManager = repoManager; this.allUsers = allUsers; this.allProjects = allProjects; this.externalIdsConsistencyChecker = externalIdsConsistencyChecker; @@ -137,7 +141,7 @@ public class CommitValidators { new BannedCommitsValidator(rejectCommits), new PluginCommitValidationListener(pluginValidators), new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker), - new AccountCommitValidator(allUsers, accountValidator), + new AccountCommitValidator(repoManager, allUsers, accountValidator), new GroupCommitValidator(allUsers))); } @@ -163,7 +167,7 @@ public class CommitValidators { new ConfigValidator(branch, user, rw, allUsers, allProjects), new PluginCommitValidationListener(pluginValidators), new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker), - new AccountCommitValidator(allUsers, accountValidator), + new AccountCommitValidator(repoManager, allUsers, accountValidator), new GroupCommitValidator(allUsers))); } @@ -700,10 +704,15 @@ public class CommitValidators { } public static class AccountCommitValidator implements CommitValidationListener { + private final GitRepositoryManager repoManager; private final AllUsersName allUsers; private final AccountValidator accountValidator; - public AccountCommitValidator(AllUsersName allUsers, AccountValidator accountValidator) { + public AccountCommitValidator( + GitRepositoryManager repoManager, + AllUsersName allUsers, + AccountValidator accountValidator) { + this.repoManager = repoManager; this.allUsers = allUsers; this.accountValidator = accountValidator; } @@ -726,10 +735,11 @@ public class CommitValidators { return Collections.emptyList(); } - try { + try (Repository repo = repoManager.openRepository(allUsers)) { List errorMessages = accountValidator.validate( accountId, + repo, receiveEvent.revWalk, receiveEvent.command.getOldId(), receiveEvent.commit); diff --git a/java/com/google/gerrit/server/git/validators/MergeValidators.java b/java/com/google/gerrit/server/git/validators/MergeValidators.java index 22bb05f746..5b20ff6b64 100644 --- a/java/com/google/gerrit/server/git/validators/MergeValidators.java +++ b/java/com/google/gerrit/server/git/validators/MergeValidators.java @@ -285,7 +285,7 @@ public class MergeValidators { } try (RevWalk rw = new RevWalk(repo)) { - List errorMessages = accountValidator.validate(accountId, rw, null, commit); + List errorMessages = accountValidator.validate(accountId, repo, rw, null, commit); if (!errorMessages.isEmpty()) { throw new MergeValidationException( "invalid account configuration: " + Joiner.on("; ").join(errorMessages)); diff --git a/java/com/google/gerrit/server/restapi/account/SetPreferences.java b/java/com/google/gerrit/server/restapi/account/SetPreferences.java index 2c9f97a011..19c445c30f 100644 --- a/java/com/google/gerrit/server/restapi/account/SetPreferences.java +++ b/java/com/google/gerrit/server/restapi/account/SetPreferences.java @@ -14,7 +14,6 @@ package com.google.gerrit.server.restapi.account; -import static com.google.gerrit.server.config.ConfigUtil.storeSection; import static com.google.gerrit.server.git.UserConfigSections.CHANGE_TABLE_COLUMN; import static com.google.gerrit.server.git.UserConfigSections.KEY_ID; import static com.google.gerrit.server.git.UserConfigSections.KEY_MATCH; @@ -36,14 +35,14 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountResource; -import com.google.gerrit.server.account.GeneralPreferencesLoader; +import com.google.gerrit.server.account.AccountsUpdate; +import com.google.gerrit.server.account.PreferencesConfig; import com.google.gerrit.server.account.VersionedAccountPreferences; -import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.UserConfigSections; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -52,7 +51,6 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.Config; @Singleton @@ -60,9 +58,7 @@ public class SetPreferences implements RestModifyView self; private final AccountCache cache; private final PermissionBackend permissionBackend; - private final GeneralPreferencesLoader loader; - private final Provider metaDataUpdateFactory; - private final AllUsersName allUsersName; + private final AccountsUpdate.User accountsUpdate; private final DynamicMap downloadSchemes; @Inject @@ -70,62 +66,33 @@ public class SetPreferences implements RestModifyView self, AccountCache cache, PermissionBackend permissionBackend, - GeneralPreferencesLoader loader, - Provider metaDataUpdateFactory, - AllUsersName allUsersName, + AccountsUpdate.User accountsUpdate, DynamicMap downloadSchemes) { this.self = self; - this.loader = loader; this.cache = cache; this.permissionBackend = permissionBackend; - this.metaDataUpdateFactory = metaDataUpdateFactory; - this.allUsersName = allUsersName; + this.accountsUpdate = accountsUpdate; this.downloadSchemes = downloadSchemes; } @Override - public GeneralPreferencesInfo apply(AccountResource rsrc, GeneralPreferencesInfo i) + public GeneralPreferencesInfo apply(AccountResource rsrc, GeneralPreferencesInfo input) throws AuthException, BadRequestException, IOException, ConfigInvalidException, - PermissionBackendException { + PermissionBackendException, OrmException { if (self.get() != rsrc.getUser()) { permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT); } - checkDownloadScheme(i.downloadScheme); + checkDownloadScheme(input.downloadScheme); + PreferencesConfig.validateMy(input.my); Account.Id id = rsrc.getUser().getAccountId(); - GeneralPreferencesInfo n = loader.merge(id, i); - - n.changeTable = i.changeTable; - n.my = i.my; - n.urlAliases = i.urlAliases; - - writeToGit(id, n); + accountsUpdate + .create() + .update("Set Preferences via API", id, u -> u.setGeneralPreferences(input)); return cache.get(id).getAccount().getGeneralPreferencesInfo(); } - private void writeToGit(Account.Id id, GeneralPreferencesInfo i) - throws RepositoryNotFoundException, IOException, ConfigInvalidException, BadRequestException { - VersionedAccountPreferences prefs; - try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName)) { - prefs = VersionedAccountPreferences.forUser(id); - prefs.load(md); - - storeSection( - prefs.getConfig(), - UserConfigSections.GENERAL, - null, - i, - loader.readDefaultsFromGit(md.getRepository(), null)); - - storeMyChangeTableColumns(prefs, i.changeTable); - storeMyMenus(prefs, i.my); - storeUrlAliases(prefs, i.urlAliases); - prefs.commit(md); - cache.evict(id); - } - } - public static void storeMyMenus(VersionedAccountPreferences prefs, List my) throws BadRequestException { Config cfg = prefs.getConfig(); diff --git a/java/com/google/gerrit/server/restapi/config/GetPreferences.java b/java/com/google/gerrit/server/restapi/config/GetPreferences.java index c8a173f6fd..3c7453c325 100644 --- a/java/com/google/gerrit/server/restapi/config/GetPreferences.java +++ b/java/com/google/gerrit/server/restapi/config/GetPreferences.java @@ -14,33 +14,25 @@ package com.google.gerrit.server.restapi.config; -import static com.google.gerrit.server.config.ConfigUtil.loadSection; - import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.extensions.restapi.RestReadView; -import com.google.gerrit.server.account.GeneralPreferencesLoader; -import com.google.gerrit.server.account.VersionedAccountPreferences; +import com.google.gerrit.server.account.PreferencesConfig; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.ConfigResource; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.UserConfigSections; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.Repository; @Singleton public class GetPreferences implements RestReadView { - private final GeneralPreferencesLoader loader; private final GitRepositoryManager gitMgr; private final AllUsersName allUsersName; @Inject - public GetPreferences( - GeneralPreferencesLoader loader, GitRepositoryManager gitMgr, AllUsersName allUsersName) { - this.loader = loader; + public GetPreferences(GitRepositoryManager gitMgr, AllUsersName allUsersName) { this.gitMgr = gitMgr; this.allUsersName = allUsersName; } @@ -48,30 +40,8 @@ public class GetPreferences implements RestReadView { @Override public GeneralPreferencesInfo apply(ConfigResource rsrc) throws IOException, ConfigInvalidException { - return readFromGit(gitMgr, loader, allUsersName, null); - } - - static GeneralPreferencesInfo readFromGit( - GitRepositoryManager gitMgr, - GeneralPreferencesLoader loader, - AllUsersName allUsersName, - GeneralPreferencesInfo in) - throws IOException, ConfigInvalidException, RepositoryNotFoundException { try (Repository git = gitMgr.openRepository(allUsersName)) { - VersionedAccountPreferences p = VersionedAccountPreferences.forDefault(); - p.load(git); - - GeneralPreferencesInfo r = - loadSection( - p.getConfig(), - UserConfigSections.GENERAL, - null, - new GeneralPreferencesInfo(), - GeneralPreferencesInfo.defaults(), - in); - - // TODO(davido): Maintain cache of default values in AllUsers repository - return loader.loadMyMenusAndUrlAliases(r, p, null); + return PreferencesConfig.readDefaultPreferences(git); } } } diff --git a/java/com/google/gerrit/server/restapi/config/SetPreferences.java b/java/com/google/gerrit/server/restapi/config/SetPreferences.java index 17908c31fd..be990e2dd0 100644 --- a/java/com/google/gerrit/server/restapi/config/SetPreferences.java +++ b/java/com/google/gerrit/server/restapi/config/SetPreferences.java @@ -14,10 +14,7 @@ package com.google.gerrit.server.restapi.config; -import static com.google.gerrit.server.config.ConfigUtil.loadSection; import static com.google.gerrit.server.config.ConfigUtil.skipField; -import static com.google.gerrit.server.config.ConfigUtil.storeSection; -import static com.google.gerrit.server.restapi.config.GetPreferences.readFromGit; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.extensions.annotations.RequiresCapability; @@ -25,20 +22,16 @@ import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.server.account.AccountCache; -import com.google.gerrit.server.account.GeneralPreferencesLoader; -import com.google.gerrit.server.account.VersionedAccountPreferences; +import com.google.gerrit.server.account.PreferencesConfig; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.ConfigResource; -import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MetaDataUpdate; -import com.google.gerrit.server.git.UserConfigSections; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.lang.reflect.Field; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -47,57 +40,31 @@ import org.slf4j.LoggerFactory; public class SetPreferences implements RestModifyView { private static final Logger log = LoggerFactory.getLogger(SetPreferences.class); - private final GeneralPreferencesLoader loader; - private final GitRepositoryManager gitManager; private final Provider metaDataUpdateFactory; private final AllUsersName allUsersName; private final AccountCache accountCache; @Inject SetPreferences( - GeneralPreferencesLoader loader, - GitRepositoryManager gitManager, Provider metaDataUpdateFactory, AllUsersName allUsersName, AccountCache accountCache) { - this.loader = loader; - this.gitManager = gitManager; this.metaDataUpdateFactory = metaDataUpdateFactory; this.allUsersName = allUsersName; this.accountCache = accountCache; } @Override - public GeneralPreferencesInfo apply(ConfigResource rsrc, GeneralPreferencesInfo i) + public GeneralPreferencesInfo apply(ConfigResource rsrc, GeneralPreferencesInfo input) throws BadRequestException, IOException, ConfigInvalidException { - if (!hasSetFields(i)) { + if (!hasSetFields(input)) { throw new BadRequestException("unsupported option"); } - return writeToGit(readFromGit(gitManager, loader, allUsersName, i)); - } - - private GeneralPreferencesInfo writeToGit(GeneralPreferencesInfo i) - throws RepositoryNotFoundException, IOException, ConfigInvalidException, BadRequestException { + PreferencesConfig.validateMy(input.my); try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName)) { - VersionedAccountPreferences p = VersionedAccountPreferences.forDefault(); - p.load(md); - storeSection( - p.getConfig(), UserConfigSections.GENERAL, null, i, GeneralPreferencesInfo.defaults()); - com.google.gerrit.server.restapi.account.SetPreferences.storeMyMenus(p, i.my); - com.google.gerrit.server.restapi.account.SetPreferences.storeUrlAliases(p, i.urlAliases); - p.commit(md); - + GeneralPreferencesInfo updatedPrefs = PreferencesConfig.updateDefaultPreferences(md, input); accountCache.evictAllNoReindex(); - - GeneralPreferencesInfo r = - loadSection( - p.getConfig(), - UserConfigSections.GENERAL, - null, - new GeneralPreferencesInfo(), - GeneralPreferencesInfo.defaults(), - null); - return loader.loadMyMenusAndUrlAliases(r, p, null); + return updatedPrefs; } } diff --git a/java/com/google/gerrit/server/schema/Schema_139.java b/java/com/google/gerrit/server/schema/Schema_139.java index 990b67b61f..f2c30df2bc 100644 --- a/java/com/google/gerrit/server/schema/Schema_139.java +++ b/java/com/google/gerrit/server/schema/Schema_139.java @@ -1,16 +1,16 @@ -//Copyright (C) 2016 The Android Open Source Project +// Copyright (C) 2016 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 +// 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 +// 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. +// 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.schema; @@ -148,7 +148,7 @@ public class Schema_139 extends SchemaVersion { md.getCommitBuilder().setCommitter(serverUser); md.setMessage(MSG); - AccountConfig accountConfig = new AccountConfig(e.getKey()); + AccountConfig accountConfig = new AccountConfig(e.getKey(), git); accountConfig.load(md); accountConfig.setAccountUpdate( InternalAccountUpdate.builder() diff --git a/java/com/google/gerrit/server/schema/Schema_154.java b/java/com/google/gerrit/server/schema/Schema_154.java index a89f520102..0e5c1100ea 100644 --- a/java/com/google/gerrit/server/schema/Schema_154.java +++ b/java/com/google/gerrit/server/schema/Schema_154.java @@ -139,10 +139,7 @@ public class Schema_154 extends SchemaVersion { PersonIdent ident = serverIdent.get(); md.getCommitBuilder().setAuthor(ident); md.getCommitBuilder().setCommitter(ident); - AccountConfig accountConfig = new AccountConfig(account.getId()); - accountConfig.load(allUsersRepo); - accountConfig.setAccount(account); - accountConfig.commit(md); + new AccountConfig(account.getId(), allUsersRepo).load().setAccount(account).commit(md); } @FunctionalInterface diff --git a/javatests/com/google/gerrit/acceptance/api/config/GeneralPreferencesIT.java b/javatests/com/google/gerrit/acceptance/api/config/GeneralPreferencesIT.java index 6a1373a074..c6069824c6 100644 --- a/javatests/com/google/gerrit/acceptance/api/config/GeneralPreferencesIT.java +++ b/javatests/com/google/gerrit/acceptance/api/config/GeneralPreferencesIT.java @@ -27,7 +27,7 @@ public class GeneralPreferencesIT extends AbstractDaemonTest { @Test public void getGeneralPreferences() throws Exception { GeneralPreferencesInfo result = gApi.config().server().getDefaultPreferences(); - assertPrefs(result, GeneralPreferencesInfo.defaults(), "my"); + assertPrefs(result, GeneralPreferencesInfo.defaults(), "changeTable", "my"); } @Test @@ -41,6 +41,6 @@ public class GeneralPreferencesIT extends AbstractDaemonTest { result = gApi.config().server().getDefaultPreferences(); GeneralPreferencesInfo expected = GeneralPreferencesInfo.defaults(); expected.signedOffBy = newSignedOffBy; - assertPrefs(result, expected, "my"); + assertPrefs(result, expected, "changeTable", "my"); } } diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java index 42073930db..be87f6308d 100644 --- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java +++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java @@ -416,10 +416,10 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { PersonIdent ident = serverIdent.get(); md.getCommitBuilder().setAuthor(ident); md.getCommitBuilder().setCommitter(ident); - AccountConfig accountConfig = new AccountConfig(accountId); - accountConfig.load(repo); - accountConfig.setAccountUpdate(InternalAccountUpdate.builder().setFullName(newName).build()); - accountConfig.commit(md); + new AccountConfig(accountId, repo) + .load() + .setAccountUpdate(InternalAccountUpdate.builder().setFullName(newName).build()) + .commit(md); } assertQuery("name:" + quote(user1.name), user1); diff --git a/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java b/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java index 0bf9399dba..9b86c0e21f 100644 --- a/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java +++ b/javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java @@ -106,7 +106,7 @@ public class Schema_159_to_160_Test { assertThat(myMenusFromApi(accountId).keySet()).containsExactlyElementsIn(newNames).inOrder(); } - // Raw config values, bypassing the defaults set by GeneralPreferencesLoader. + // Raw config values, bypassing the defaults set by PreferencesConfig. private ImmutableMap myMenusFromNoteDb(Account.Id id) throws Exception { try (Repository repo = repoManager.openRepository(allUsersName)) { VersionedAccountPreferences prefs = VersionedAccountPreferences.forUser(id); From f8df5f724591e57553a427f4a1d0323b29ea5669 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 8 Jan 2018 12:57:02 +0100 Subject: [PATCH 4/7] AccountConfig: Read revision of the external IDs branch Later this revision can be used to lazily load the external IDs for the account. Change-Id: Iedb4bf4d964133048b570bf08848f9d0b0347d7e Signed-off-by: Edwin Kempin --- .../gerrit/server/account/AccountConfig.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/java/com/google/gerrit/server/account/AccountConfig.java b/java/com/google/gerrit/server/account/AccountConfig.java index 2314f5f41d..09a5244411 100644 --- a/java/com/google/gerrit/server/account/AccountConfig.java +++ b/java/com/google/gerrit/server/account/AccountConfig.java @@ -39,7 +39,9 @@ import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevSort; @@ -84,6 +86,7 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. private final String ref; private Optional loadedAccount; + private Optional externalIdsRev; private WatchConfig watchConfig; private PreferencesConfig prefConfig; private Optional accountUpdate = Optional.empty(); @@ -134,6 +137,17 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. return loadedAccount; } + /** + * Returns the revision of the {@code refs/meta/external-ids} branch. + * + * @return revision of the {@code refs/meta/external-ids} branch, {@link Optional#empty()} if no + * {@code refs/meta/external-ids} branch exists + */ + public Optional getExternalIdsRev() { + checkLoaded(); + return externalIdsRev; + } + /** * Get the project watches of the loaded account. * @@ -219,6 +233,10 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. Config accountConfig = readConfig(ACCOUNT_CONFIG); loadedAccount = Optional.of(parse(accountConfig, revision.name())); + Ref externalIdsRef = repo.exactRef(RefNames.REFS_EXTERNAL_IDS); + externalIdsRev = + externalIdsRef != null ? Optional.of(externalIdsRef.getObjectId()) : Optional.empty(); + watchConfig = new WatchConfig(accountId, readConfig(WatchConfig.WATCH_CONFIG), this); prefConfig = From 7bea8e2e0b3162f54ece733a0aa53a85e089c5f4 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 8 Jan 2018 11:41:26 +0100 Subject: [PATCH 5/7] ExternalIds: Allow to specify revision for getting ext IDs by account This will allow us to read an AccountState consistently. To create an AccountState we need the account properties, the project watches, the general preferences and the external IDs. AccountConfig loads account properties, project watches and general preferences. For the external IDs it only provides the rev of the external ID branch (because loading the whole external ID branch whenever an account is loaded would be too expensive). By allowing to specify the revision for getting external IDs by account this rev can be used to retrieve the external IDs for the account via the external ID cache. This way we have everything what is needed to instantiate an AccountState. Change-Id: I090bb11ee5044bb7fd13e78192d9964781c73d36 Signed-off-by: Edwin Kempin --- .../gerrit/server/account/AccountConfig.java | 4 ++++ .../externalids/DisabledExternalIdCache.java | 5 +++++ .../account/externalids/ExternalIdCache.java | 2 ++ .../account/externalids/ExternalIdCacheImpl.java | 11 ++++++++++- .../server/account/externalids/ExternalIds.java | 14 ++++++++++++++ 5 files changed, 35 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/account/AccountConfig.java b/java/com/google/gerrit/server/account/AccountConfig.java index 09a5244411..b20aa0be61 100644 --- a/java/com/google/gerrit/server/account/AccountConfig.java +++ b/java/com/google/gerrit/server/account/AccountConfig.java @@ -25,6 +25,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.account.WatchConfig.NotifyType; import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; +import com.google.gerrit.server.account.externalids.ExternalIds; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.git.VersionedMetaData; @@ -140,6 +141,9 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. /** * Returns the revision of the {@code refs/meta/external-ids} branch. * + *

This revision can be used to load the external IDs of the loaded account lazily via {@link + * ExternalIds#byAccount(com.google.gerrit.reviewdb.client.Account.Id, ObjectId)}. + * * @return revision of the {@code refs/meta/external-ids} branch, {@link Optional#empty()} if no * {@code refs/meta/external-ids} branch exists */ diff --git a/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java b/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java index 1033641337..c1ef26179d 100644 --- a/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java +++ b/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java @@ -63,6 +63,11 @@ public class DisabledExternalIdCache implements ExternalIdCache { throw new UnsupportedOperationException(); } + @Override + public ImmutableSet byAccount(Account.Id accountId, ObjectId rev) { + throw new UnsupportedOperationException(); + } + @Override public ImmutableSetMultimap allByAccount() throws IOException { throw new UnsupportedOperationException(); diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java b/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java index d928e15288..7878052381 100644 --- a/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java +++ b/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java @@ -55,6 +55,8 @@ interface ExternalIdCache { ImmutableSet byAccount(Account.Id accountId) throws IOException; + ImmutableSet byAccount(Account.Id accountId, ObjectId rev) throws IOException; + ImmutableSetMultimap allByAccount() throws IOException; ImmutableSetMultimap byEmails(String... emails) throws IOException; diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java b/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java index 6f80713817..814f19e6ca 100644 --- a/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java +++ b/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java @@ -169,6 +169,11 @@ class ExternalIdCacheImpl implements ExternalIdCache { return get().byAccount().get(accountId); } + @Override + public ImmutableSet byAccount(Account.Id accountId, ObjectId rev) throws IOException { + return get(rev).byAccount().get(accountId); + } + @Override public ImmutableSetMultimap allByAccount() throws IOException { return get().byAccount(); @@ -190,8 +195,12 @@ class ExternalIdCacheImpl implements ExternalIdCache { } private AllExternalIds get() throws IOException { + return get(externalIdReader.readRevision()); + } + + private AllExternalIds get(ObjectId rev) throws IOException { try { - return extIdsByAccount.get(externalIdReader.readRevision()); + return extIdsByAccount.get(rev); } catch (ExecutionException e) { throw new IOException("Cannot load external ids", e); } diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIds.java b/java/com/google/gerrit/server/account/externalids/ExternalIds.java index 5732bce737..a8ecc40ae0 100644 --- a/java/com/google/gerrit/server/account/externalids/ExternalIds.java +++ b/java/com/google/gerrit/server/account/externalids/ExternalIds.java @@ -75,6 +75,20 @@ public class ExternalIds { return byAccount(accountId).stream().filter(e -> e.key().isScheme(scheme)).collect(toSet()); } + /** Returns the external IDs of the specified account. */ + public Set byAccount(Account.Id accountId, ObjectId rev) throws IOException { + return externalIdCache.byAccount(accountId, rev); + } + + /** Returns the external IDs of the specified account that have the given scheme. */ + public Set byAccount(Account.Id accountId, String scheme, ObjectId rev) + throws IOException { + return byAccount(accountId, rev) + .stream() + .filter(e -> e.key().isScheme(scheme)) + .collect(toSet()); + } + /** Returns all external IDs by account. */ public ImmutableSetMultimap allByAccount() throws IOException { return externalIdCache.allByAccount(); From 246c7ad643914561b0857381eb024025f776a8a8 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 8 Jan 2018 15:19:44 +0100 Subject: [PATCH 6/7] Accounts: Load AccountState instead of Account This allows to get a consistent view on an account. So far we loaded account properties, external IDs, generel preferences and project watches separately and it was possible that the account was updated in between. Change-Id: Ie4feae69274bc4dc58c91e1ecce43206a5d1c819 Signed-off-by: Edwin Kempin --- .../become/BecomeAnyAccountLoginServlet.java | 13 ++--- .../server/account/AccountCacheImpl.java | 23 +-------- .../gerrit/server/account/Accounts.java | 51 +++++++++---------- .../account/AccountsConsistencyChecker.java | 12 ++--- .../group/db/GroupsConsistencyChecker.java | 3 +- .../restapi/account/GetWatchedProjects.java | 11 ++-- .../acceptance/api/accounts/AccountIT.java | 5 +- 7 files changed, 49 insertions(+), 69 deletions(-) diff --git a/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java b/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java index 1f095e0540..853d173acd 100644 --- a/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java +++ b/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java @@ -183,9 +183,9 @@ class BecomeAnyAccountLoginServlet extends HttpServlet { return HtmlDomUtil.toUTF8(doc); } - private AuthResult auth(Account account) { + private AuthResult auth(AccountState account) { if (account != null) { - return new AuthResult(account.getId(), null, false); + return new AuthResult(account.getAccount().getId(), null, false); } return null; } @@ -218,13 +218,8 @@ class BecomeAnyAccountLoginServlet extends HttpServlet { private AuthResult byPreferredEmail(String email) { try (ReviewDb db = schema.open()) { - Optional match = - queryProvider - .get() - .byPreferredEmail(email) - .stream() - .map(AccountState::getAccount) - .findFirst(); + Optional match = + queryProvider.get().byPreferredEmail(email).stream().findFirst(); return match.isPresent() ? auth(match.get()) : null; } catch (OrmException e) { getServletContext().log("cannot query database", e); diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java index 2ad31b2c27..a7a795969c 100644 --- a/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java @@ -20,7 +20,6 @@ import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; -import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.account.externalids.ExternalIds; @@ -134,34 +133,16 @@ public class AccountCacheImpl implements AccountCache { } static class ByIdLoader extends CacheLoader> { - private final AllUsersName allUsersName; private final Accounts accounts; - private final ExternalIds externalIds; @Inject - ByIdLoader(AllUsersName allUsersName, Accounts accounts, ExternalIds externalIds) { - this.allUsersName = allUsersName; + ByIdLoader(Accounts accounts) { this.accounts = accounts; - this.externalIds = externalIds; } @Override public Optional load(Account.Id who) throws Exception { - Account account = accounts.get(who); - if (account == null) { - return Optional.empty(); - } - - try { - account.setGeneralPreferences(accounts.getGeneralPreferences(who)); - } catch (IOException | ConfigInvalidException e) { - log.warn("Cannot load GeneralPreferences for " + who + " (using default)", e); - account.setGeneralPreferences(GeneralPreferencesInfo.defaults()); - } - - return Optional.of( - new AccountState( - allUsersName, account, externalIds.byAccount(who), accounts.getProjectWatches(who))); + return Optional.ofNullable(accounts.get(who)); } } } diff --git a/java/com/google/gerrit/server/account/Accounts.java b/java/com/google/gerrit/server/account/Accounts.java index 512483cbe5..e8b5d88bce 100644 --- a/java/com/google/gerrit/server/account/Accounts.java +++ b/java/com/google/gerrit/server/account/Accounts.java @@ -18,12 +18,11 @@ import static java.util.Comparator.comparing; import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.Nullable; -import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; -import com.google.gerrit.server.account.WatchConfig.NotifyType; -import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; +import com.google.gerrit.server.account.externalids.ExternalIds; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.inject.Inject; @@ -32,7 +31,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.List; -import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -49,23 +47,25 @@ public class Accounts { private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; + private final ExternalIds externalIds; @Inject - Accounts(GitRepositoryManager repoManager, AllUsersName allUsersName) { + Accounts(GitRepositoryManager repoManager, AllUsersName allUsersName, ExternalIds externalIds) { this.repoManager = repoManager; this.allUsersName = allUsersName; + this.externalIds = externalIds; } @Nullable - public Account get(Account.Id accountId) throws IOException, ConfigInvalidException { + public AccountState get(Account.Id accountId) throws IOException, ConfigInvalidException { try (Repository repo = repoManager.openRepository(allUsersName)) { return read(repo, accountId).orElse(null); } } - public List get(Collection accountIds) + public List get(Collection accountIds) throws IOException, ConfigInvalidException { - List accounts = new ArrayList<>(accountIds.size()); + List accounts = new ArrayList<>(accountIds.size()); try (Repository repo = repoManager.openRepository(allUsersName)) { for (Account.Id accountId : accountIds) { read(repo, accountId).ifPresent(accounts::add); @@ -79,9 +79,9 @@ public class Accounts { * * @return all accounts */ - public List all() throws IOException { + public List all() throws IOException { Set accountIds = allIds(); - List accounts = new ArrayList<>(accountIds.size()); + List accounts = new ArrayList<>(accountIds.size()); try (Repository repo = repoManager.openRepository(allUsersName)) { for (Account.Id accountId : accountIds) { try { @@ -128,29 +128,28 @@ public class Accounts { return readUserRefs(repo).findAny().isPresent(); } - public Map> getProjectWatches(Account.Id accountId) - throws IOException, ConfigInvalidException { - try (Repository repo = repoManager.openRepository(allUsersName)) { - return new AccountConfig(accountId, repo).load().getProjectWatches(); - } - } - - public GeneralPreferencesInfo getGeneralPreferences(Account.Id accountId) - throws IOException, ConfigInvalidException { - try (Repository repo = repoManager.openRepository(allUsersName)) { - return new AccountConfig(accountId, repo).load().getGeneralPreferences(); - } - } - private Stream readUserRefs() throws IOException { try (Repository repo = repoManager.openRepository(allUsersName)) { return readUserRefs(repo); } } - private Optional read(Repository allUsersRepository, Account.Id accountId) + private Optional read(Repository allUsersRepository, Account.Id accountId) throws IOException, ConfigInvalidException { - return new AccountConfig(accountId, allUsersRepository).load().getLoadedAccount(); + AccountConfig accountConfig = new AccountConfig(accountId, allUsersRepository).load(); + if (!accountConfig.getLoadedAccount().isPresent()) { + return Optional.empty(); + } + Account account = accountConfig.getLoadedAccount().get(); + account.setGeneralPreferences(accountConfig.getGeneralPreferences()); + return Optional.of( + new AccountState( + allUsersName, + account, + accountConfig.getExternalIdsRev().isPresent() + ? externalIds.byAccount(accountId, accountConfig.getExternalIdsRev().get()) + : ImmutableSet.of(), + accountConfig.getProjectWatches())); } public static Stream readUserRefs(Repository repo) throws IOException { diff --git a/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java b/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java index 0085303f4e..0b63927fe5 100644 --- a/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java +++ b/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java @@ -16,7 +16,6 @@ package com.google.gerrit.server.account; import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.server.account.externalids.ExternalIds; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -26,21 +25,20 @@ import java.util.List; @Singleton public class AccountsConsistencyChecker { private final Accounts accounts; - private final ExternalIds externalIds; @Inject - AccountsConsistencyChecker(Accounts accounts, ExternalIds externalIds) { + AccountsConsistencyChecker(Accounts accounts) { this.accounts = accounts; - this.externalIds = externalIds; } public List check() throws IOException { List problems = new ArrayList<>(); - for (Account account : accounts.all()) { + for (AccountState accountState : accounts.all()) { + Account account = accountState.getAccount(); if (account.getPreferredEmail() != null) { - if (!externalIds - .byAccount(account.getId()) + if (!accountState + .getExternalIds() .stream() .anyMatch(e -> account.getPreferredEmail().equals(e.email()))) { addError( diff --git a/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java b/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java index 541697a186..a0eae4a8ad 100644 --- a/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java +++ b/java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java @@ -20,6 +20,7 @@ import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.Consi import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.config.AllUsersName; @@ -113,7 +114,7 @@ public class GroupsConsistencyChecker { } for (Account.Id id : g.getMembers().asList()) { - Account account; + AccountState account; try { account = accounts.get(id); } catch (ConfigInvalidException e) { diff --git a/java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java b/java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java index 3d461402a5..ffddc7c676 100644 --- a/java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java +++ b/java/com/google/gerrit/server/restapi/account/GetWatchedProjects.java @@ -18,10 +18,12 @@ import com.google.common.base.Strings; import com.google.common.collect.ComparisonChain; import com.google.gerrit.extensions.client.ProjectWatchInfo; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountResource; +import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.account.WatchConfig.NotifyType; import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey; @@ -58,15 +60,18 @@ public class GetWatchedProjects implements RestReadView { @Override public List apply(AccountResource rsrc) throws OrmException, AuthException, IOException, ConfigInvalidException, - PermissionBackendException { + PermissionBackendException, ResourceNotFoundException { if (self.get() != rsrc.getUser()) { permissionBackend.user(self).check(GlobalPermission.ADMINISTRATE_SERVER); } Account.Id accountId = rsrc.getUser().getAccountId(); + AccountState account = accounts.get(accountId); + if (account == null) { + throw new ResourceNotFoundException(); + } List projectWatchInfos = new ArrayList<>(); - for (Map.Entry> e : - accounts.getProjectWatches(accountId).entrySet()) { + for (Map.Entry> e : account.getProjectWatches().entrySet()) { ProjectWatchInfo pwi = new ProjectWatchInfo(); pwi.filter = e.getKey().filter(); pwi.project = e.getKey().project().get(); diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 69b73cf35d..f433d84301 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -1925,7 +1925,8 @@ public class AccountIT extends AbstractDaemonTest { @Test public void checkMetaId() throws Exception { // metaId is set when account is loaded - assertThat(accounts.get(admin.getId()).getMetaId()).isEqualTo(getMetaId(admin.getId())); + assertThat(accounts.get(admin.getId()).getAccount().getMetaId()) + .isEqualTo(getMetaId(admin.getId())); // metaId is set when account is created AccountsUpdate au = accountsUpdate.create(); @@ -2096,7 +2097,7 @@ public class AccountIT extends AbstractDaemonTest { } assertThat(bgCounter.get()).isEqualTo(status.size()); - Account updatedAccount = accounts.get(admin.id); + Account updatedAccount = accounts.get(admin.id).getAccount(); assertThat(updatedAccount.getStatus()).isEqualTo(Iterables.getLast(status)); assertThat(updatedAccount.getFullName()).isEqualTo(admin.fullName); From f8f1684d8e8a5cce8e47d06a67ef6d1f66615ad3 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 8 Jan 2018 16:19:45 +0100 Subject: [PATCH 7/7] Move general preferences from Account to AccountState All data that is hosted in the Account class is persisted in the 'account.config' file, except general preferences which are stored in an own file. This makes general preferences similar to project watches which are also stored in an own file. Project watches are hosted in AccountState, hence to treat general preferences and project watches consistently it makes sense to move the general preferences to AccountState so that general preferences and project watches are hosted at the same place. Change-Id: Ic063facd85fd8b8060c0e95b3ed7037f04076e64 Signed-off-by: Edwin Kempin --- .../google/gerrit/pgm/init/InitAdminUser.java | 8 +++- .../gerrit/reviewdb/client/Account.java | 12 ----- .../server/account/AccountCacheImpl.java | 8 +++- .../gerrit/server/account/AccountState.java | 11 ++++- .../gerrit/server/account/Accounts.java | 4 +- .../server/git/receive/ReceiveCommits.java | 5 +- .../server/mail/send/OutgoingEmail.java | 48 ++++++++++--------- .../restapi/account/GetPreferences.java | 2 +- .../restapi/account/SetPreferences.java | 2 +- .../server/restapi/change/CreateChange.java | 2 +- .../gerrit/testing/FakeAccountCache.java | 4 +- .../index/account/AccountFieldTest.java | 14 +++++- .../FromAddressGeneratorProviderTest.java | 4 +- 13 files changed, 75 insertions(+), 49 deletions(-) diff --git a/java/com/google/gerrit/pgm/init/InitAdminUser.java b/java/com/google/gerrit/pgm/init/InitAdminUser.java index 3251c010ab..e9f5cd5b71 100644 --- a/java/com/google/gerrit/pgm/init/InitAdminUser.java +++ b/java/com/google/gerrit/pgm/init/InitAdminUser.java @@ -21,6 +21,7 @@ import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.client.AuthType; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.pgm.init.api.AllUsersNameOnInitProvider; import com.google.gerrit.pgm.init.api.ConsoleUI; import com.google.gerrit.pgm.init.api.InitFlags; @@ -151,7 +152,12 @@ public class InitAdminUser implements InitStep { } AccountState as = - new AccountState(new AllUsersName(allUsers.get()), a, extIds, new HashMap<>()); + new AccountState( + new AllUsersName(allUsers.get()), + a, + extIds, + new HashMap<>(), + GeneralPreferencesInfo.defaults()); for (AccountIndex accountIndex : accountIndexCollection.getWriteIndexes()) { accountIndex.replace(as); } diff --git a/java/com/google/gerrit/reviewdb/client/Account.java b/java/com/google/gerrit/reviewdb/client/Account.java index bce07aade0..1f9ae0e399 100644 --- a/java/com/google/gerrit/reviewdb/client/Account.java +++ b/java/com/google/gerrit/reviewdb/client/Account.java @@ -19,7 +19,6 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_STARRED_CHANGES; import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS; import com.google.gerrit.extensions.client.DiffPreferencesInfo; -import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gwtorm.client.Column; import com.google.gwtorm.client.IntKey; import java.sql.Timestamp; @@ -180,9 +179,6 @@ public final class Account { /** computed the username selected from the identities. */ protected String userName; - /** stored in git, used for caching the user's preferences. */ - private GeneralPreferencesInfo generalPreferences; - /** * ID of the user branch from which the account was read, {@code null} if the account was read * from ReviewDb. @@ -286,14 +282,6 @@ public final class Account { return registeredOn; } - public GeneralPreferencesInfo getGeneralPreferencesInfo() { - return generalPreferences; - } - - public void setGeneralPreferences(GeneralPreferencesInfo p) { - generalPreferences = p; - } - public String getMetaId() { return metaId; } diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java index a7a795969c..963da62d14 100644 --- a/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java @@ -20,6 +20,7 @@ import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.account.externalids.ExternalIds; @@ -129,7 +130,12 @@ public class AccountCacheImpl implements AccountCache { private AccountState missing(Account.Id accountId) { Account account = new Account(accountId, TimeUtil.nowTs()); account.setActive(false); - return new AccountState(allUsersName, account, Collections.emptySet(), new HashMap<>()); + return new AccountState( + allUsersName, + account, + Collections.emptySet(), + new HashMap<>(), + GeneralPreferencesInfo.defaults()); } static class ByIdLoader extends CacheLoader> { diff --git a/java/com/google/gerrit/server/account/AccountState.java b/java/com/google/gerrit/server/account/AccountState.java index 01d31c9f67..6601df7cf8 100644 --- a/java/com/google/gerrit/server/account/AccountState.java +++ b/java/com/google/gerrit/server/account/AccountState.java @@ -22,6 +22,7 @@ import com.google.common.base.Strings; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.CurrentUser.PropertyKey; import com.google.gerrit.server.IdentifiedUser; @@ -51,17 +52,20 @@ public class AccountState { private final Account account; private final Collection externalIds; private final Map> projectWatches; + private final GeneralPreferencesInfo generalPreferences; private Cache, Object> properties; public AccountState( AllUsersName allUsersName, Account account, Collection externalIds, - Map> projectWatches) { + Map> projectWatches, + GeneralPreferencesInfo generalPreferences) { this.allUsersName = allUsersName; this.account = account; this.externalIds = externalIds; this.projectWatches = projectWatches; + this.generalPreferences = generalPreferences; this.account.setUserName(getUserName(externalIds)); } @@ -117,6 +121,11 @@ public class AccountState { return projectWatches; } + /** The general preferences of the account. */ + public GeneralPreferencesInfo getGeneralPreferences() { + return generalPreferences; + } + public static String getUserName(Collection ids) { for (ExternalId extId : ids) { if (extId.isScheme(SCHEME_USERNAME)) { diff --git a/java/com/google/gerrit/server/account/Accounts.java b/java/com/google/gerrit/server/account/Accounts.java index e8b5d88bce..45831ae9c8 100644 --- a/java/com/google/gerrit/server/account/Accounts.java +++ b/java/com/google/gerrit/server/account/Accounts.java @@ -141,7 +141,6 @@ public class Accounts { return Optional.empty(); } Account account = accountConfig.getLoadedAccount().get(); - account.setGeneralPreferences(accountConfig.getGeneralPreferences()); return Optional.of( new AccountState( allUsersName, @@ -149,7 +148,8 @@ public class Accounts { accountConfig.getExternalIdsRev().isPresent() ? externalIds.byAccount(accountId, accountConfig.getExternalIdsRev().get()) : ImmutableSet.of(), - accountConfig.getProjectWatches())); + accountConfig.getProjectWatches(), + accountConfig.getGeneralPreferences())); } public static Stream readUserRefs(Repository repo) throws IOException { diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index a058aec797..00f25053e1 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -1333,11 +1333,10 @@ class ReceiveCommits { this.publish = cmd.getRefName().startsWith(MagicBranch.NEW_PUBLISH_CHANGE); this.labelTypes = labelTypes; this.notesMigration = notesMigration; - GeneralPreferencesInfo prefs = user.getAccount().getGeneralPreferencesInfo(); + GeneralPreferencesInfo prefs = user.state().getGeneralPreferences(); this.defaultPublishComments = prefs != null - ? firstNonNull( - user.getAccount().getGeneralPreferencesInfo().publishCommentsOnPush, false) + ? firstNonNull(user.state().getGeneralPreferences().publishCommentsOnPush, false) : false; } diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index ec67d9da33..8eb55fa7ed 100644 --- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -121,34 +121,38 @@ public abstract class OutgoingEmail { Set

smtpRcptToPlaintextOnly = new HashSet<>(); if (shouldSendMessage()) { if (fromId != null) { - final Account fromUser = args.accountCache.get(fromId).getAccount(); - GeneralPreferencesInfo senderPrefs = fromUser.getGeneralPreferencesInfo(); - - if (senderPrefs != null && senderPrefs.getEmailStrategy() == CC_ON_OWN_COMMENTS) { - // If we are impersonating a user, make sure they receive a CC of - // this message so they can always review and audit what we sent - // on their behalf to others. - // - add(RecipientType.CC, fromId); - } else if (!accountsToNotify.containsValue(fromId) && rcptTo.remove(fromId)) { - // If they don't want a copy, but we queued one up anyway, - // drop them from the recipient lists. - // - removeUser(fromUser); + AccountState fromUser = args.accountCache.get(fromId); + if (fromUser != null) { + GeneralPreferencesInfo senderPrefs = fromUser.getGeneralPreferences(); + if (senderPrefs != null && senderPrefs.getEmailStrategy() == CC_ON_OWN_COMMENTS) { + // If we are impersonating a user, make sure they receive a CC of + // this message so they can always review and audit what we sent + // on their behalf to others. + // + add(RecipientType.CC, fromId); + } else if (!accountsToNotify.containsValue(fromId) && rcptTo.remove(fromId)) { + // If they don't want a copy, but we queued one up anyway, + // drop them from the recipient lists. + // + removeUser(fromUser.getAccount()); + } } } // Check the preferences of all recipients. If any user has disabled // his email notifications then drop him from recipients' list. // In addition, check if users only want to receive plaintext email. for (Account.Id id : rcptTo) { - Account thisUser = args.accountCache.get(id).getAccount(); - GeneralPreferencesInfo prefs = thisUser.getGeneralPreferencesInfo(); - if (prefs == null || prefs.getEmailStrategy() == DISABLED) { - removeUser(thisUser); - } else if (useHtml() && prefs.getEmailFormat() == EmailFormat.PLAINTEXT) { - removeUser(thisUser); - smtpRcptToPlaintextOnly.add( - new Address(thisUser.getFullName(), thisUser.getPreferredEmail())); + AccountState thisUser = args.accountCache.get(id); + if (thisUser != null) { + Account thisUserAccount = thisUser.getAccount(); + GeneralPreferencesInfo prefs = thisUser.getGeneralPreferences(); + if (prefs == null || prefs.getEmailStrategy() == DISABLED) { + removeUser(thisUserAccount); + } else if (useHtml() && prefs.getEmailFormat() == EmailFormat.PLAINTEXT) { + removeUser(thisUserAccount); + smtpRcptToPlaintextOnly.add( + new Address(thisUserAccount.getFullName(), thisUserAccount.getPreferredEmail())); + } } if (smtpRcptTo.isEmpty() && smtpRcptToPlaintextOnly.isEmpty()) { return; diff --git a/java/com/google/gerrit/server/restapi/account/GetPreferences.java b/java/com/google/gerrit/server/restapi/account/GetPreferences.java index b071ade66d..46bc389cf1 100644 --- a/java/com/google/gerrit/server/restapi/account/GetPreferences.java +++ b/java/com/google/gerrit/server/restapi/account/GetPreferences.java @@ -50,6 +50,6 @@ public class GetPreferences implements RestReadView { } Account.Id id = rsrc.getUser().getAccountId(); - return accountCache.get(id).getAccount().getGeneralPreferencesInfo(); + return accountCache.get(id).getGeneralPreferences(); } } diff --git a/java/com/google/gerrit/server/restapi/account/SetPreferences.java b/java/com/google/gerrit/server/restapi/account/SetPreferences.java index 19c445c30f..8584240abc 100644 --- a/java/com/google/gerrit/server/restapi/account/SetPreferences.java +++ b/java/com/google/gerrit/server/restapi/account/SetPreferences.java @@ -90,7 +90,7 @@ public class SetPreferences implements RestModifyView u.setGeneralPreferences(input)); - return cache.get(id).getAccount().getGeneralPreferencesInfo(); + return cache.get(id).getGeneralPreferences(); } public static void storeMyMenus(VersionedAccountPreferences prefs, List my) diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java index d4e1c405d3..a6a4631aaf 100644 --- a/java/com/google/gerrit/server/restapi/change/CreateChange.java +++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java @@ -231,7 +231,7 @@ public class CreateChange IdentifiedUser me = user.get().asIdentifiedUser(); PersonIdent author = me.newCommitterIdent(now, serverTimeZone); AccountState account = accountCache.get(me.getAccountId()); - GeneralPreferencesInfo info = account.getAccount().getGeneralPreferencesInfo(); + GeneralPreferencesInfo info = account.getGeneralPreferences(); ObjectId treeId = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree(); ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, input.subject); diff --git a/java/com/google/gerrit/testing/FakeAccountCache.java b/java/com/google/gerrit/testing/FakeAccountCache.java index 825676509b..7668912462 100644 --- a/java/com/google/gerrit/testing/FakeAccountCache.java +++ b/java/com/google/gerrit/testing/FakeAccountCache.java @@ -17,6 +17,7 @@ package com.google.gerrit.testing; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountState; @@ -79,6 +80,7 @@ public class FakeAccountCache implements AccountCache { new AllUsersName(AllUsersNameProvider.DEFAULT), account, ImmutableSet.of(), - new HashMap<>()); + new HashMap<>(), + GeneralPreferencesInfo.defaults()); } } diff --git a/javatests/com/google/gerrit/server/index/account/AccountFieldTest.java b/javatests/com/google/gerrit/server/index/account/AccountFieldTest.java index 6bba51b8ae..1542fe5258 100644 --- a/javatests/com/google/gerrit/server/index/account/AccountFieldTest.java +++ b/javatests/com/google/gerrit/server/index/account/AccountFieldTest.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.account.AccountState; @@ -44,7 +45,12 @@ public class AccountFieldTest extends GerritBaseTests { List values = toStrings( AccountField.REF_STATE.get( - new AccountState(allUsersName, account, ImmutableSet.of(), ImmutableMap.of()))); + new AccountState( + allUsersName, + account, + ImmutableSet.of(), + ImmutableMap.of(), + GeneralPreferencesInfo.defaults()))); assertThat(values).hasSize(1); String expectedValue = allUsersName.get() + ":" + RefNames.refsUsers(account.getId()) + ":" + metaId; @@ -73,7 +79,11 @@ public class AccountFieldTest extends GerritBaseTests { toStrings( AccountField.EXTERNAL_ID_STATE.get( new AccountState( - null, account, ImmutableSet.of(extId1, extId2), ImmutableMap.of()))); + null, + account, + ImmutableSet.of(extId1, extId2), + ImmutableMap.of(), + GeneralPreferencesInfo.defaults()))); String expectedValue1 = extId1.key().sha1().name() + ":" + extId1.blobId().name(); String expectedValue2 = extId2.key().sha1().name() + ":" + extId2.blobId().name(); assertThat(values).containsExactly(expectedValue1, expectedValue2); diff --git a/javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java b/javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java index d65dd47501..01e8225e67 100644 --- a/javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java +++ b/javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java @@ -22,6 +22,7 @@ import static org.easymock.EasyMock.replay; import static org.easymock.EasyMock.verify; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountState; @@ -388,6 +389,7 @@ public class FromAddressGeneratorProviderTest { new AllUsersName(AllUsersNameProvider.DEFAULT), account, Collections.emptySet(), - new HashMap<>()); + new HashMap<>(), + GeneralPreferencesInfo.defaults()); } }