From ddd8ec8d0680348dcb9edf3fcc9fdb87376e8acf Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Thu, 29 Oct 2015 22:54:24 +0100 Subject: [PATCH] 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 --- .../rest/account/EditPreferencesIT.java | 12 +++++++++++ .../server/account/GetEditPreferences.java | 14 +++++++++++-- .../server/account/SetEditPreferences.java | 8 ++++++- .../gerrit/server/config/ConfigUtil.java | 14 +++++++++---- .../gerrit/server/config/ConfigUtilTest.java | 21 ++++++++++++++++++- 5 files changed, 61 insertions(+), 8 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/EditPreferencesIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/EditPreferencesIT.java index 8770c3c47e..cf89c5a256 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/EditPreferencesIT.java +++ b/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) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetEditPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetEditPreferences.java index e6e76445f2..d99b68fa2a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetEditPreferences.java +++ b/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 { 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); } } } \ No newline at end of file diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/SetEditPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/SetEditPreferences.java index 2df1a77077..eabe31d59a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/SetEditPreferences.java +++ b/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 self; private final Provider metaDataUpdateFactory; + private final GitRepositoryManager gitMgr; private final AllUsersName allUsersName; @Inject SetEditPreferences(Provider self, Provider 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 { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigUtil.java index 2b6ee41f04..be9d984f54 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigUtil.java +++ b/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. *

* Fields marked with final or transient modifiers are skipped. - *

- * 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 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) { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/config/ConfigUtilTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/config/ConfigUtilTest.java index ab00ba8ee3..4f409d1046 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/config/ConfigUtilTest.java +++ b/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"));