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 <ekempin@google.com>

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()));
   }
 
   /**

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<ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>>> projectWatches;
   private final Supplier<GeneralPreferencesInfo> generalPreferences;
   private final Supplier<DiffPreferencesInfo> diffPreferences;
+  private final Supplier<EditPreferencesInfo> editPreferences;
   private Cache<IdentifiedUser.PropertyKey<Object>, Object> properties;
 
   private AccountState(
@@ -165,13 +169,15 @@ public class AccountState {
       ImmutableSet<ExternalId> externalIds,
       Supplier<ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>>> projectWatches,
       Supplier<GeneralPreferencesInfo> generalPreferences,
-      Supplier<DiffPreferencesInfo> diffPreferences) {
+      Supplier<DiffPreferencesInfo> diffPreferences,
+      Supplier<EditPreferencesInfo> 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.
    *

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<DiffPreferencesInfo> getDiffPreferences();
 
+  /**
+   * Returns the new value for the edit preferences.
+   *
+   * <p>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<EditPreferencesInfo> 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.
+     *
+     * <p>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;
+      }
     }
   }
 }

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<GeneralPreferencesInfo> generalPreferencesInput,
-      Optional<DiffPreferencesInfo> diffPreferencesInput)
+      Optional<DiffPreferencesInfo> diffPreferencesInput,
+      Optional<EditPreferencesInfo> 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<String> parseChangeTableColumns(Config cfg, @Nullable Config defaultCfg) {
     List<String> changeTable = changeTable(cfg);
     if (changeTable == null && defaultCfg != null) {

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<AccountResource> {
   private final Provider<CurrentUser> self;
   private final PermissionBackend permissionBackend;
-  private final AllUsersName allUsersName;
+  private final AccountCache accountCache;
-  private final GitRepositoryManager gitMgr;
 
   @Inject
   GetEditPreferences(
-      Provider<CurrentUser> self,
+      Provider<CurrentUser> self, PermissionBackend permissionBackend, AccountCache accountCache) {
-      PermissionBackend permissionBackend,
-      AllUsersName allUsersName,
-      GitRepositoryManager gitMgr) {
     this.self = self;
     this.permissionBackend = permissionBackend;
-    this.allUsersName = allUsersName;
+    this.accountCache = accountCache;
-    this.gitMgr = gitMgr;
   }
 
   @Override
@@ -63,23 +51,7 @@ public class GetEditPreferences implements RestReadView<AccountResource> {
       permissionBackend.user(self).check(GlobalPermission.MODIFY_ACCOUNT);
     }
 
-    return readFromGit(rsrc.getUser().getAccountId(), gitMgr, allUsersName, null);
+    Account.Id id = rsrc.getUser().getAccountId();
-  }
+    return accountCache.get(id).getEditPreferences();
-
-  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);
-    }
   }
 }

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.account.AccountsUpdate;
-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.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<AccountResource, EditPreferencesInfo> {
 
   private final Provider<CurrentUser> self;
-  private final Provider<MetaDataUpdate.User> metaDataUpdateFactory;
   private final PermissionBackend permissionBackend;
-  private final GitRepositoryManager gitMgr;
+  private final AccountCache accountCache;
-  private final AllUsersName allUsersName;
+  private final AccountsUpdate.User accountsUpdate;
 
   @Inject
   SetEditPreferences(
       Provider<CurrentUser> self,
-      Provider<MetaDataUpdate.User> metaDataUpdateFactory,
       PermissionBackend permissionBackend,
-      GitRepositoryManager gitMgr,
+      AccountCache accountCache,
-      AllUsersName allUsersName) {
+      AccountsUpdate.User accountsUpdate) {
     this.self = self;
-    this.metaDataUpdateFactory = metaDataUpdateFactory;
     this.permissionBackend = permissionBackend;
-    this.gitMgr = gitMgr;
+    this.accountCache = accountCache;
-    this.allUsersName = allUsersName;
+    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();
+    Account.Id id = rsrc.getUser().getAccountId();
-
+    accountsUpdate
-    VersionedAccountPreferences prefs;
+        .create()
-    EditPreferencesInfo out = new EditPreferencesInfo();
+        .update("Set Diff Preferences via API", id, u -> u.setEditPreferences(input));
-    try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsersName)) {
+    return accountCache.get(id).getEditPreferences();
-      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;
   }
 }