From 4a28ca7a5a6d9b2c9ceeabf54f8310ea5e523fec Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 16 Jan 2018 10:46:53 +0100 Subject: [PATCH] Allow to update account and edit preferences atomically AccountsUpdate now also support to update the edit preferences and AccountState makes them accessible. This means edit preferences are now also cached, but since the parsing of the edit preferences is done lazily this doesn't make loading of accounts any more expensive (the preferences.config file that contains the edit preferences is already loaded because it also contains the general preferences). Some methods in PreferencesConfig exist now 3 times, once for general preferences, once for diff preferences and once for edit preferences. Future changes may make an effort to remove code duplication, but for now we are happy that these methods are in the same place now and that the handling of all preferences is consistent now. Change-Id: I31bcf18057da0312faa99ec9cf15c9fef5c25fbc Signed-off-by: Edwin Kempin --- .../gerrit/server/account/AccountConfig.java | 18 +++- .../gerrit/server/account/AccountState.java | 17 +++- .../server/account/InternalAccountUpdate.java | 27 ++++++ .../server/account/PreferencesConfig.java | 87 ++++++++++++++++++- .../restapi/account/GetEditPreferences.java | 40 ++------- .../restapi/account/SetEditPreferences.java | 63 ++++---------- 6 files changed, 166 insertions(+), 86 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountConfig.java b/java/com/google/gerrit/server/account/AccountConfig.java index 1293e86184..d4379a2dfe 100644 --- a/java/com/google/gerrit/server/account/AccountConfig.java +++ b/java/com/google/gerrit/server/account/AccountConfig.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.client.DiffPreferencesInfo; +import com.google.gerrit.extensions.client.EditPreferencesInfo; import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; @@ -186,6 +187,16 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. return prefConfig.getDiffPreferences(); } + /** + * Get the edit preferences of the loaded account. + * + * @return the edit preferences of the loaded account + */ + public EditPreferencesInfo getEditPreferences() { + checkLoaded(); + return prefConfig.getEditPreferences(); + } + /** * Sets the account. This means the loaded account will be overwritten with the given account. * @@ -367,14 +378,17 @@ public class AccountConfig extends VersionedMetaData implements ValidationError. private void savePreferences() throws IOException, ConfigInvalidException { if (!accountUpdate.isPresent() || (!accountUpdate.get().getGeneralPreferences().isPresent() - && !accountUpdate.get().getDiffPreferences().isPresent())) { + && !accountUpdate.get().getDiffPreferences().isPresent() + && !accountUpdate.get().getEditPreferences().isPresent())) { return; } saveConfig( PreferencesConfig.PREFERENCES_CONFIG, prefConfig.saveGeneralPreferences( - accountUpdate.get().getGeneralPreferences(), accountUpdate.get().getDiffPreferences())); + accountUpdate.get().getGeneralPreferences(), + accountUpdate.get().getDiffPreferences(), + accountUpdate.get().getEditPreferences())); } /** diff --git a/java/com/google/gerrit/server/account/AccountState.java b/java/com/google/gerrit/server/account/AccountState.java index 201e74b25a..8bd12c03f7 100644 --- a/java/com/google/gerrit/server/account/AccountState.java +++ b/java/com/google/gerrit/server/account/AccountState.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.client.DiffPreferencesInfo; +import com.google.gerrit.extensions.client.EditPreferencesInfo; import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.CurrentUser.PropertyKey; @@ -117,7 +118,8 @@ public class AccountState { extIds, Suppliers.memoize(() -> accountConfig.getProjectWatches()), Suppliers.memoize(() -> accountConfig.getGeneralPreferences()), - Suppliers.memoize(() -> accountConfig.getDiffPreferences()))); + Suppliers.memoize(() -> accountConfig.getDiffPreferences()), + Suppliers.memoize(() -> accountConfig.getEditPreferences()))); } /** @@ -148,7 +150,8 @@ public class AccountState { ImmutableSet.copyOf(extIds), Suppliers.ofInstance(ImmutableMap.of()), Suppliers.ofInstance(GeneralPreferencesInfo.defaults()), - Suppliers.ofInstance(DiffPreferencesInfo.defaults())); + Suppliers.ofInstance(DiffPreferencesInfo.defaults()), + Suppliers.ofInstance(EditPreferencesInfo.defaults())); } private final AllUsersName allUsersName; @@ -157,6 +160,7 @@ public class AccountState { private final Supplier>> projectWatches; private final Supplier generalPreferences; private final Supplier diffPreferences; + private final Supplier editPreferences; private Cache, Object> properties; private AccountState( @@ -165,13 +169,15 @@ public class AccountState { ImmutableSet externalIds, Supplier>> projectWatches, Supplier generalPreferences, - Supplier diffPreferences) { + Supplier diffPreferences, + Supplier editPreferences) { this.allUsersName = allUsersName; this.account = account; this.externalIds = externalIds; this.projectWatches = projectWatches; this.generalPreferences = generalPreferences; this.diffPreferences = diffPreferences; + this.editPreferences = editPreferences; this.account.setUserName(ExternalId.getUserName(externalIds).orElse(null)); } @@ -242,6 +248,11 @@ public class AccountState { return diffPreferences.get(); } + /** The edit preferences of the account. */ + public EditPreferencesInfo getEditPreferences() { + return editPreferences.get(); + } + /** * Lookup a previously stored property. * diff --git a/java/com/google/gerrit/server/account/InternalAccountUpdate.java b/java/com/google/gerrit/server/account/InternalAccountUpdate.java index c965f6fa1b..c7fa30913d 100644 --- a/java/com/google/gerrit/server/account/InternalAccountUpdate.java +++ b/java/com/google/gerrit/server/account/InternalAccountUpdate.java @@ -19,6 +19,7 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.gerrit.extensions.client.DiffPreferencesInfo; +import com.google.gerrit.extensions.client.EditPreferencesInfo; import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.WatchConfig.NotifyType; @@ -133,6 +134,16 @@ public abstract class InternalAccountUpdate { */ public abstract Optional getDiffPreferences(); + /** + * Returns the new value for the edit preferences. + * + *

Only preferences that are non-null in the returned DiffPreferencesInfo should be updated. + * + * @return the new value for the edit preferences, {@code Optional#empty()} if the edit + * preferences are not being updated, the wrapped value is never {@code null} + */ + public abstract Optional getEditPreferences(); + /** * Class to build an account update. * @@ -405,6 +416,16 @@ public abstract class InternalAccountUpdate { */ public abstract Builder setDiffPreferences(DiffPreferencesInfo diffPreferences); + /** + * Sets the edit preferences for the account. + * + *

Updates any preference that is non-null in the provided EditPreferencesInfo. + * + * @param editPreferences the edit preferences that should be set + * @return the builder + */ + public abstract Builder setEditPreferences(EditPreferencesInfo editPreferences); + /** * Builds the account update. * @@ -547,6 +568,12 @@ public abstract class InternalAccountUpdate { delegate.setDiffPreferences(diffPreferences); return this; } + + @Override + public Builder setEditPreferences(EditPreferencesInfo editPreferences) { + delegate.setEditPreferences(editPreferences); + return this; + } } } } diff --git a/java/com/google/gerrit/server/account/PreferencesConfig.java b/java/com/google/gerrit/server/account/PreferencesConfig.java index d499302f40..6d086ac785 100644 --- a/java/com/google/gerrit/server/account/PreferencesConfig.java +++ b/java/com/google/gerrit/server/account/PreferencesConfig.java @@ -32,6 +32,7 @@ import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.client.DiffPreferencesInfo; +import com.google.gerrit.extensions.client.EditPreferencesInfo; import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gerrit.extensions.client.MenuItem; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -68,6 +69,7 @@ public class PreferencesConfig { private GeneralPreferencesInfo generalPreferences; private DiffPreferencesInfo diffPreferences; + private EditPreferencesInfo editPreferences; public PreferencesConfig( Account.Id accountId, @@ -94,14 +96,23 @@ public class PreferencesConfig { return diffPreferences; } + public EditPreferencesInfo getEditPreferences() { + if (editPreferences == null) { + parse(); + } + return editPreferences; + } + public void parse() { generalPreferences = parseGeneralPreferences(null); diffPreferences = parseDiffPreferences(null); + editPreferences = parseEditPreferences(null); } public Config saveGeneralPreferences( Optional generalPreferencesInput, - Optional diffPreferencesInput) + Optional diffPreferencesInput, + Optional editPreferencesInput) throws ConfigInvalidException { if (generalPreferencesInput.isPresent()) { GeneralPreferencesInfo mergedGeneralPreferencesInput = @@ -136,6 +147,21 @@ public class PreferencesConfig { this.diffPreferences = null; } + if (editPreferencesInput.isPresent()) { + EditPreferencesInfo mergedEditPreferencesInput = + parseEditPreferences(editPreferencesInput.get()); + + storeSection( + cfg, + UserConfigSections.EDIT, + null, + mergedEditPreferencesInput, + parseDefaultEditPreferences(defaultCfg, null)); + + // evict the cached edit preferences + this.editPreferences = null; + } + return cfg; } @@ -166,6 +192,19 @@ public class PreferencesConfig { } } + private EditPreferencesInfo parseEditPreferences(@Nullable EditPreferencesInfo input) { + try { + return parseEditPreferences(cfg, defaultCfg, input); + } catch (ConfigInvalidException e) { + validationErrorSink.error( + new ValidationError( + PREFERENCES_CONFIG, + String.format( + "Invalid edit preferences for account %d: %s", accountId.get(), e.getMessage()))); + return new EditPreferencesInfo(); + } + } + private static GeneralPreferencesInfo parseGeneralPreferences( Config cfg, @Nullable Config defaultCfg, @Nullable GeneralPreferencesInfo input) throws ConfigInvalidException { @@ -205,6 +244,20 @@ public class PreferencesConfig { input); } + private static EditPreferencesInfo parseEditPreferences( + Config cfg, @Nullable Config defaultCfg, @Nullable EditPreferencesInfo input) + throws ConfigInvalidException { + return loadSection( + cfg, + UserConfigSections.EDIT, + null, + new EditPreferencesInfo(), + defaultCfg != null + ? parseDefaultEditPreferences(defaultCfg, input) + : EditPreferencesInfo.defaults(), + input); + } + private static GeneralPreferencesInfo parseDefaultGeneralPreferences( Config defaultCfg, GeneralPreferencesInfo input) throws ConfigInvalidException { GeneralPreferencesInfo allUserPrefs = new GeneralPreferencesInfo(); @@ -231,6 +284,19 @@ public class PreferencesConfig { return updateDiffPreferencesDefaults(allUserPrefs); } + private static EditPreferencesInfo parseDefaultEditPreferences( + Config defaultCfg, EditPreferencesInfo input) throws ConfigInvalidException { + EditPreferencesInfo allUserPrefs = new EditPreferencesInfo(); + loadSection( + defaultCfg, + UserConfigSections.EDIT, + null, + allUserPrefs, + EditPreferencesInfo.defaults(), + input); + return updateEditPreferencesDefaults(allUserPrefs); + } + private static GeneralPreferencesInfo updateGeneralPreferencesDefaults( GeneralPreferencesInfo input) { GeneralPreferencesInfo result = GeneralPreferencesInfo.defaults(); @@ -270,6 +336,25 @@ public class PreferencesConfig { return result; } + private static EditPreferencesInfo updateEditPreferencesDefaults(EditPreferencesInfo input) { + EditPreferencesInfo result = EditPreferencesInfo.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 edit preferences", e); + return EditPreferencesInfo.defaults(); + } + return result; + } + private static List parseChangeTableColumns(Config cfg, @Nullable Config defaultCfg) { List changeTable = changeTable(cfg); if (changeTable == null && defaultCfg != null) { diff --git a/java/com/google/gerrit/server/restapi/account/GetEditPreferences.java b/java/com/google/gerrit/server/restapi/account/GetEditPreferences.java index 95a6e7c582..a51da1062c 100644 --- a/java/com/google/gerrit/server/restapi/account/GetEditPreferences.java +++ b/java/com/google/gerrit/server/restapi/account/GetEditPreferences.java @@ -14,18 +14,13 @@ package com.google.gerrit.server.restapi.account; -import static com.google.gerrit.server.config.ConfigUtil.loadSection; - import com.google.gerrit.extensions.client.EditPreferencesInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.RestReadView; 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.VersionedAccountPreferences; -import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.git.GitRepositoryManager; -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; @@ -34,26 +29,19 @@ import com.google.inject.Provider; 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 GetEditPreferences implements RestReadView { private final Provider self; private final PermissionBackend permissionBackend; - private final AllUsersName allUsersName; - private final GitRepositoryManager gitMgr; + private final AccountCache accountCache; @Inject GetEditPreferences( - Provider self, - PermissionBackend permissionBackend, - AllUsersName allUsersName, - GitRepositoryManager gitMgr) { + Provider self, PermissionBackend permissionBackend, AccountCache accountCache) { this.self = self; this.permissionBackend = permissionBackend; - this.allUsersName = allUsersName; - this.gitMgr = gitMgr; + this.accountCache = accountCache; } @Override @@ -63,23 +51,7 @@ public class GetEditPreferences implements RestReadView { permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT); } - return readFromGit(rsrc.getUser().getAccountId(), gitMgr, allUsersName, null); - } - - static EditPreferencesInfo readFromGit( - Account.Id id, GitRepositoryManager gitMgr, AllUsersName allUsersName, EditPreferencesInfo in) - throws IOException, ConfigInvalidException, RepositoryNotFoundException { - try (Repository git = gitMgr.openRepository(allUsersName)) { - VersionedAccountPreferences p = VersionedAccountPreferences.forUser(id); - p.load(git); - - return loadSection( - p.getConfig(), - UserConfigSections.EDIT, - null, - new EditPreferencesInfo(), - EditPreferencesInfo.defaults(), - in); - } + Account.Id id = rsrc.getUser().getAccountId(); + return accountCache.get(id).getEditPreferences(); } } diff --git a/java/com/google/gerrit/server/restapi/account/SetEditPreferences.java b/java/com/google/gerrit/server/restapi/account/SetEditPreferences.java index 357437745c..f16bb9e5b7 100644 --- a/java/com/google/gerrit/server/restapi/account/SetEditPreferences.java +++ b/java/com/google/gerrit/server/restapi/account/SetEditPreferences.java @@ -14,25 +14,19 @@ package com.google.gerrit.server.restapi.account; -import static com.google.gerrit.server.config.ConfigUtil.loadSection; -import static com.google.gerrit.server.config.ConfigUtil.storeSection; -import static com.google.gerrit.server.restapi.account.GetEditPreferences.readFromGit; - import com.google.gerrit.extensions.client.EditPreferencesInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.RestModifyView; 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.VersionedAccountPreferences; -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.UserConfigSections; +import com.google.gerrit.server.account.AccountsUpdate; 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; @@ -44,61 +38,38 @@ import org.eclipse.jgit.errors.RepositoryNotFoundException; public class SetEditPreferences implements RestModifyView { private final Provider self; - private final Provider metaDataUpdateFactory; private final PermissionBackend permissionBackend; - private final GitRepositoryManager gitMgr; - private final AllUsersName allUsersName; + private final AccountCache accountCache; + private final AccountsUpdate.User accountsUpdate; @Inject SetEditPreferences( Provider self, - Provider metaDataUpdateFactory, PermissionBackend permissionBackend, - GitRepositoryManager gitMgr, - AllUsersName allUsersName) { + AccountCache accountCache, + AccountsUpdate.User accountsUpdate) { this.self = self; - this.metaDataUpdateFactory = metaDataUpdateFactory; this.permissionBackend = permissionBackend; - this.gitMgr = gitMgr; - this.allUsersName = allUsersName; + this.accountCache = accountCache; + this.accountsUpdate = accountsUpdate; } @Override - public EditPreferencesInfo apply(AccountResource rsrc, EditPreferencesInfo in) + public EditPreferencesInfo apply(AccountResource rsrc, EditPreferencesInfo input) throws AuthException, BadRequestException, RepositoryNotFoundException, IOException, - ConfigInvalidException, PermissionBackendException { + ConfigInvalidException, PermissionBackendException, OrmException { if (self.get() != rsrc.getUser()) { permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT); } - if (in == null) { + if (input == null) { throw new BadRequestException("input must be provided"); } - Account.Id accountId = rsrc.getUser().getAccountId(); - - VersionedAccountPreferences prefs; - EditPreferencesInfo out = new EditPreferencesInfo(); - try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName)) { - prefs = VersionedAccountPreferences.forUser(accountId); - prefs.load(md); - storeSection( - prefs.getConfig(), - UserConfigSections.EDIT, - null, - readFromGit(accountId, gitMgr, allUsersName, in), - EditPreferencesInfo.defaults()); - prefs.commit(md); - out = - loadSection( - prefs.getConfig(), - UserConfigSections.EDIT, - null, - out, - EditPreferencesInfo.defaults(), - null); - } - - return out; + Account.Id id = rsrc.getUser().getAccountId(); + accountsUpdate + .create() + .update("Set Diff Preferences via API", id, u -> u.setEditPreferences(input)); + return accountCache.get(id).getEditPreferences(); } }