Fix set preferences REST handler for partially filled input record

It was erroneously assumed, that all attributes for input record are
always filled. This may not be the case. When input record only
partially filled, the current settings must be read from the git
backend, without nullifying false values and merged with partially
filled input record. Only after this merging new values can be written
to the git backend.

Change-Id: Iff5763fd408918622d6e82ac122bf09a99d9aaba

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/EditPreferencesIT.java

@@ -69,6 +69,18 @@ public class EditPreferencesIT extends AbstractDaemonTest {
     assertThat(r.getStatusCode()).isEqualTo(HttpStatus.SC_OK);
     EditPreferencesInfo info = getEditPrefInfo(r);
     assertEditPreferences(info, out);
+
+    // Partially filled input record
+    EditPreferencesInfo in = new EditPreferencesInfo();
+    in.tabSize = 42;
+    r = adminSession.put(endPoint, in);
+    assertThat(r.getStatusCode()).isEqualTo(HttpStatus.SC_NO_CONTENT);
+
+    r = adminSession.get(endPoint);
+    assertThat(r.getStatusCode()).isEqualTo(HttpStatus.SC_OK);
+    info = getEditPrefInfo(r);
+    out.tabSize = in.tabSize;
+    assertEditPreferences(info, out);
   }
 
   private EditPreferencesInfo getEditPrefInfo(RestResponse r)

gerrit-server/src/main/java/com/google/gerrit/server/account/GetEditPreferences.java

@@ -19,6 +19,7 @@ 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.config.AllUsersName;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -28,6 +29,7 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.Repository;
 
 import java.io.IOException;
@@ -55,13 +57,21 @@ public class GetEditPreferences implements RestReadView<AccountResource> {
       throw new AuthException("restricted to members of Modify Accounts");
     }
 
+    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(rsrc.getUser().getAccountId());
+          VersionedAccountPreferences.forUser(id);
       p.load(git);
 
       return loadSection(p.getConfig(), UserConfigSections.EDIT, null,
-          new EditPreferencesInfo(), EditPreferencesInfo.defaults());
+          new EditPreferencesInfo(), EditPreferencesInfo.defaults(), in);
     }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/account/SetEditPreferences.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.account;
 
+import static com.google.gerrit.server.account.GetEditPreferences.readFromGit;
 import static com.google.gerrit.server.config.ConfigUtil.storeSection;
 
 import com.google.gerrit.extensions.client.EditPreferencesInfo;
@@ -24,6 +25,7 @@ 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.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.inject.Inject;
@@ -41,14 +43,17 @@ public class SetEditPreferences implements
 
   private final Provider<CurrentUser> self;
   private final Provider<MetaDataUpdate.User> metaDataUpdateFactory;
+  private final GitRepositoryManager gitMgr;
   private final AllUsersName allUsersName;
 
   @Inject
   SetEditPreferences(Provider<CurrentUser> self,
       Provider<MetaDataUpdate.User> metaDataUpdateFactory,
+      GitRepositoryManager gitMgr,
       AllUsersName allUsersName) {
     this.self = self;
     this.metaDataUpdateFactory = metaDataUpdateFactory;
+    this.gitMgr = gitMgr;
     this.allUsersName = allUsersName;
   }
 
@@ -72,7 +77,8 @@ public class SetEditPreferences implements
     try {
       prefs = VersionedAccountPreferences.forUser(accountId);
       prefs.load(md);
-      storeSection(prefs.getConfig(), UserConfigSections.EDIT, null, in,
+      storeSection(prefs.getConfig(), UserConfigSections.EDIT, null,
+          readFromGit(accountId, gitMgr, allUsersName, in),
           EditPreferencesInfo.defaults());
       prefs.commit(md);
     } finally {

gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigUtil.java

@@ -314,19 +314,19 @@ public class ConfigUtil {
    * The loading is performed eagerly: all values are set.
    * <p>
    * Fields marked with final or transient modifiers are skipped.
-   * <p>
-   * Boolean fields are only set when their values are true.
    *
    * @param cfg config from which the values are loaded
    * @param section section
    * @param sub subsection
    * @param s instance of class in which the values are set
    * @param defaults instance of class with default values
+   * @param i instance to merge during the load. When present, the
+   * boolean fields are not nullified when their values are false
    * @return loaded instance
    * @throws ConfigInvalidException
    */
   public static <T> T loadSection(Config cfg, String section, String sub,
-      T s, T defaults) throws ConfigInvalidException {
+      T s, T defaults, T i) throws ConfigInvalidException {
     try {
       for (Field f : s.getClass().getDeclaredFields()) {
         if (skipField(f)) {
@@ -345,7 +345,7 @@ public class ConfigUtil {
           f.set(s, cfg.getLong(section, sub, n, (Long) d));
         } else if (isBoolean(t)) {
           boolean b = cfg.getBoolean(section, sub, n, (Boolean) d);
-          if (b) {
+          if (b || i != null) {
             f.set(s, b);
           }
         } else if (t.isEnum()) {
@@ -353,6 +353,12 @@ public class ConfigUtil {
         } else {
           throw new ConfigInvalidException("type is unknown: " + t.getName());
         }
+        if (i != null) {
+          Object o = f.get(i);
+          if (o != null) {
+            f.set(s, o);
+          }
+        }
       }
     } catch (SecurityException | IllegalArgumentException
         | IllegalAccessException e) {

gerrit-server/src/test/java/com/google/gerrit/server/config/ConfigUtilTest.java

@@ -98,7 +98,7 @@ public class ConfigUtilTest {
     assertThat(cfg.getString(SECT, SUB, "sd")).isNull();
 
     SectionInfo out = new SectionInfo();
-    ConfigUtil.loadSection(cfg, SECT, SUB, out, d);
+    ConfigUtil.loadSection(cfg, SECT, SUB, out, d, null);
     assertThat(out.i).isEqualTo(in.i);
     assertThat(out.ii).isEqualTo(in.ii);
     assertThat(out.id).isEqualTo(d.id);
@@ -114,6 +114,25 @@ public class ConfigUtilTest {
     assertThat(out.td).isEqualTo(d.td);
   }
 
+  @Test
+  public void mergeSection() throws Exception {
+    SectionInfo d = SectionInfo.defaults();
+    Config cfg = new Config();
+    ConfigUtil.storeSection(cfg, SECT, SUB, d, d);
+
+    SectionInfo in = new SectionInfo();
+    in.i = 42;
+
+    SectionInfo out = new SectionInfo();
+    ConfigUtil.loadSection(cfg, SECT, SUB, out, d, in);
+    // Check original values preserved
+    assertThat(out.id).isEqualTo(d.id);
+    // Check merged values
+    assertThat(out.i).isEqualTo(in.i);
+    // Check that boolean attribute not nullified
+    assertThat(out.bb).isFalse();
+  }
+
   @Test
   public void testTimeUnit() {
     assertEquals(ms(0, MILLISECONDS), parse("0"));