CachedPreferences: Set boolean `false` values when default is `true`
`ConfigUtil` did not set `false` values in the result object. This is
fine when the default is `false`. However, when the default is `true`
and the user explicitly wants to override it to `false` - we must set
the `false` value.
Release-Notes: skip
Google-Bug-Id: b/376424770
Change-Id: I9d86e502aa46bc8630bf955d64ab27a23361571f
diff --git a/java/com/google/gerrit/extensions/client/NullableBooleanPreferencesFieldComparator.java b/java/com/google/gerrit/extensions/client/NullableBooleanPreferencesFieldComparator.java
index 710e758..ccccceb 100644
--- a/java/com/google/gerrit/extensions/client/NullableBooleanPreferencesFieldComparator.java
+++ b/java/com/google/gerrit/extensions/client/NullableBooleanPreferencesFieldComparator.java
@@ -37,7 +37,7 @@
* <tr><td> true </td> <td> true </td> <td> true </td></tr>
* <tr><td> true </td> <td> false </td> <td> true </td></tr>
* <tr><td> true </td> <td> null </td> <td> true </td></tr>
- * <tr><td> false </td> <td> true </td> <td> true </td></tr>
+ * <tr><td> false </td> <td> true </td> <td> false </td></tr>
* <tr><td> false </td> <td> false </td> <td> null </td></tr>
* <tr><td> false </td> <td> null </td> <td> null </td></tr>
* <tr><td> null </td> <td> true </td> <td> true </td></tr>
@@ -49,7 +49,7 @@
* practically referring to {@code null} values as {@code false} anyway. Preferences equality
* methods should reflect this state.
*/
- static boolean equalBooleanPreferencesFields(@Nullable Boolean a, @Nullable Boolean b) {
+ public static boolean equalBooleanPreferencesFields(@Nullable Boolean a, @Nullable Boolean b) {
return Objects.equals(
Objects.requireNonNullElse(a, false), Objects.requireNonNullElse(b, false));
}
diff --git a/java/com/google/gerrit/server/config/ConfigUtil.java b/java/com/google/gerrit/server/config/ConfigUtil.java
index e76207c..121c62e 100644
--- a/java/com/google/gerrit/server/config/ConfigUtil.java
+++ b/java/com/google/gerrit/server/config/ConfigUtil.java
@@ -371,8 +371,12 @@
} else if (isLong(t)) {
f.set(s, cfg.getLong(section, sub, n, (Long) d));
} else if (isBoolean(t)) {
+ // Sets the field if:
+ // - 'cfg' value is 'true'.
+ // - the default value is 'true'.
+ // - i is set.
boolean b = cfg.getBoolean(section, sub, n, (Boolean) d);
- if (b || i != null) {
+ if (b || (Boolean) d || i != null) {
f.set(s, b);
}
} else if (t.isEnum()) {
@@ -427,10 +431,13 @@
requireNonNull(val, "Default cannot be null for: " + n);
}
}
- if (!isBoolean(t) || (boolean) val) {
+ if (!isBoolean(t) || (boolean) val || (Boolean) f.get(defaults)) {
// To reproduce the same behavior as in the loadSection method above, values are
// explicitly set for all types, except the boolean type. For the boolean type, the value
- // is set only if it is 'true' (so, the false value is omitted in the result object).
+ // is set only in the following cases:
+ // - 'cfg' value is 'true'.
+ // - the default value is 'true'.
+ // Otherwise, false values are omitted in the result object.
f.set(s, val);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/DiffPreferencesIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/DiffPreferencesIT.java
index e3380c0..4f27075 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/DiffPreferencesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/DiffPreferencesIT.java
@@ -64,6 +64,9 @@
DiffPreferencesInfo o = gApi.accounts().id(admin.id().get()).setDiffPreferences(i);
assertPrefs(o, i);
+ // Re-getting the preferences should yield the same fields
+ o = gApi.accounts().id(admin.id().get()).getDiffPreferences();
+ assertPrefs(o, i);
// Partially fill input record
i = new DiffPreferencesInfo();
@@ -74,6 +77,38 @@
}
@Test
+ public void setDiffPreferences_booleanHandling() throws Exception {
+ DiffPreferencesInfo update = new DiffPreferencesInfo();
+ update.showLineEndings = true; // Default is 'true'
+ update.lineWrapping = true; // Default is 'false'
+ update.showTabs = false; // Default is 'true'
+ update.hideTopMenu = false; // Default is 'false'
+ update.intralineDifference = null; // Default is 'true'
+ update.retainHeader = null; // Default is 'false'
+
+ DiffPreferencesInfo o = gApi.accounts().id(admin.id().get()).setDiffPreferences(update);
+
+ // Explicitly assert configured values
+ assertThat(o.showLineEndings).isTrue();
+ assertThat(o.lineWrapping).isTrue();
+ assertThat(o.showTabs).isFalse();
+ assertThat(o.hideTopMenu).isNull(); // Both new value and default are false, omitted
+ assertThat(o.intralineDifference).isTrue();
+ assertThat(o.retainHeader).isNull(); // new value is 'null' and default is false, omitted
+
+ // assert unaffected fields
+ assertPrefs(
+ o,
+ DiffPreferencesInfo.defaults(),
+ "showLineEndings",
+ "lineWrapping",
+ "showTabs",
+ "hideTopMenu",
+ "intralineDifference",
+ "retainHeader");
+ }
+
+ @Test
public void getDiffPreferencesWithConfiguredDefaults() throws Exception {
DiffPreferencesInfo d = DiffPreferencesInfo.defaults();
int newLineLength = d.lineLength + 10;
diff --git a/javatests/com/google/gerrit/server/config/ConfigUtilTest.java b/javatests/com/google/gerrit/server/config/ConfigUtilTest.java
index 035878c..f8c04a9 100644
--- a/javatests/com/google/gerrit/server/config/ConfigUtilTest.java
+++ b/javatests/com/google/gerrit/server/config/ConfigUtilTest.java
@@ -107,7 +107,7 @@
assertThat(out.ld).isEqualTo(d.ld);
assertThat(out.b).isEqualTo(in.b);
assertThat(out.bb).isEqualTo(in.bb);
- assertThat(out.bd).isNull();
+ assertThat(out.bd).isFalse();
assertThat(out.s).isEqualTo(in.s);
assertThat(out.sd).isEqualTo(d.sd);
assertThat(out.nd).isNull();
