Fix client option handling in ChangeList
I8858147cf made the mistake of mutating the supposedly constant
OPTIONS set used by utility methods in ChangeList. This had the effect
of making the options for every single query actually be the union of
all options made by any query over the client app's lifetime. This is
particularly unfortunate because the "My Changes" dashboard includes
the REVIEWED option, which requires loading all change messages of
all returned changes from the database on every request. Given that
this information is not stored in the index, this is unavoidable if
we actually need the reviewed bit (as "My Changes" does), but it is a
waste for all other queries, including other dashboards, Related
Changes, etc.
Simplify option handling by having all ChangeList methods take an
explicit option list. Move the common options to a constant in
ChangeTable, since these options are "defaults" only from the
perspective of a change table, not from the perspective of other
callers. Make those options unmodifiable sets to avoid this kind of
abuse in the future.
Change-Id: I363581836b17622e8a50c24e502f7b9f8f7d1de4
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 9aac81d..9a58702 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -463,7 +463,7 @@
assertThat(gApi.changes().query()
.withQuery(
"project:{" + project.get() + "} (status:open OR status:closed)")
- // Options should match defaults in ChangeList.
+ // Options should match defaults in ChangeTable.
.withOption(ListChangesOption.LABELS)
.withOption(ListChangesOption.DETAILED_ACCOUNTS)
.get())
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java
index 12a741f..5caa903 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ListChangesOption.java
@@ -15,6 +15,7 @@
package com.google.gerrit.extensions.client;
import java.util.EnumSet;
+import java.util.Set;
/** Output options available for retrieval change details. */
public enum ListChangesOption {
@@ -90,7 +91,7 @@
return r;
}
- public static int toBits(EnumSet<ListChangesOption> set) {
+ public static int toBits(Set<ListChangesOption> set) {
int r = 0;
for (ListChangesOption o : set) {
r |= 1 << o.value;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java
index d8b8365..9974532 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java
@@ -30,8 +30,16 @@
import java.util.Collections;
import java.util.Comparator;
import java.util.EnumSet;
+import java.util.Set;
public class AccountDashboardScreen extends Screen implements ChangeListScreen {
+ private static final Set<ListChangesOption> MY_DASHBOARD_OPTIONS;
+ static {
+ EnumSet<ListChangesOption> options = EnumSet.copyOf(ChangeTable.OPTIONS);
+ options.add(ListChangesOption.REVIEWED);
+ MY_DASHBOARD_OPTIONS = Collections.unmodifiableSet(options);
+ }
+
private final Account.Id ownerId;
private final boolean mine;
private ChangeTable table;
@@ -103,9 +111,7 @@
display(result);
}
},
- mine
- ? EnumSet.of(ListChangesOption.REVIEWED)
- : EnumSet.noneOf(ListChangesOption.class),
+ mine ? MY_DASHBOARD_OPTIONS : DashboardTable.OPTIONS,
queryOutgoing(who),
queryIncoming(who),
queryClosed(who) + " -age:4w limit:10");
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeList.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeList.java
index 8b318fe..f27ee82 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeList.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeList.java
@@ -20,20 +20,16 @@
import com.google.gwt.user.client.rpc.AsyncCallback;
import com.google.gwtorm.client.KeyUtil;
-import java.util.EnumSet;
+import java.util.Set;
/** List of changes available from {@code /changes/}. */
public class ChangeList extends JsArray<ChangeInfo> {
private static final String URI = "/changes/";
- // If changing default options, also update in
- // ChangeIT#defaultSearchDoesNotTouchDatabase().
- private static final EnumSet<ListChangesOption> OPTIONS = EnumSet.of(
- ListChangesOption.LABELS, ListChangesOption.DETAILED_ACCOUNTS);
/** Run multiple queries in a single remote invocation. */
public static void queryMultiple(
final AsyncCallback<JsArray<ChangeList>> callback,
- EnumSet<ListChangesOption> options,
+ Set<ListChangesOption> options,
String... queries) {
if (queries.length == 0) {
return;
@@ -42,8 +38,7 @@
for (String q : queries) {
call.addParameterRaw("q", KeyUtil.encode(q));
}
- OPTIONS.addAll(options);
- addOptions(call, OPTIONS);
+ addOptions(call, options);
if (queries.length == 1) {
// Server unwraps a single query, so wrap it back in an array for the
// callback.
@@ -66,7 +61,7 @@
}
public static void query(String query,
- EnumSet<ListChangesOption> options,
+ Set<ListChangesOption> options,
AsyncCallback<ChangeList> callback) {
RestApi call = newQuery(query);
addOptions(call, options);
@@ -75,19 +70,20 @@
public static void next(String query,
int start, int limit,
+ Set<ListChangesOption> options,
AsyncCallback<ChangeList> callback) {
RestApi call = newQuery(query);
if (limit > 0) {
call.addParameter("n", limit);
}
- addOptions(call, OPTIONS);
+ addOptions(call, options);
if (start != 0) {
call.addParameter("S", start);
}
call.get(callback);
}
- public static void addOptions(RestApi call, EnumSet<ListChangesOption> s) {
+ public static void addOptions(RestApi call, Set<ListChangesOption> s) {
call.addParameterRaw("O", Integer.toHexString(ListChangesOption.toBits(s)));
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java
index 2694871..c52ac32 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java
@@ -27,6 +27,7 @@
import com.google.gerrit.client.ui.NeedsSignInKeyCommand;
import com.google.gerrit.client.ui.ProjectLink;
import com.google.gerrit.common.PageLinks;
+import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.ReviewCategoryStrategy;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwt.dom.client.Element;
@@ -45,9 +46,17 @@
import java.util.ArrayList;
import java.util.Collections;
+import java.util.EnumSet;
import java.util.List;
+import java.util.Set;
public class ChangeTable extends NavigationTable<ChangeInfo> {
+ // If changing default options, also update in
+ // ChangeIT#defaultSearchDoesNotTouchDatabase().
+ static final Set<ListChangesOption> OPTIONS =
+ Collections.unmodifiableSet(EnumSet.of(
+ ListChangesOption.LABELS, ListChangesOption.DETAILED_ACCOUNTS));
+
private static final int C_STAR = 1;
private static final int C_ID = 2;
private static final int C_SUBJECT = 3;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/DashboardTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/DashboardTable.java
index df0fec0..26e11ae 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/DashboardTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/DashboardTable.java
@@ -20,14 +20,12 @@
import com.google.gerrit.client.ui.InlineHyperlink;
import com.google.gerrit.client.ui.Screen;
import com.google.gerrit.common.PageLinks;
-import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gwt.core.client.JsArray;
import com.google.gwt.event.dom.client.KeyPressEvent;
import com.google.gwt.http.client.URL;
import com.google.gwtexpui.globalkey.client.KeyCommand;
import java.util.ArrayList;
-import java.util.EnumSet;
import java.util.List;
import java.util.ListIterator;
@@ -116,7 +114,7 @@
finishDisplay();
}
},
- EnumSet.noneOf(ListChangesOption.class),
+ OPTIONS,
queries.toArray(new String[queries.size()]));
}
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/QueryScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/QueryScreen.java
index 5d1dcb9..86e7cf7 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/QueryScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/QueryScreen.java
@@ -74,7 +74,8 @@
@Override
protected void onLoad() {
super.onLoad();
- ChangeList.next(query, start, pageSize, loadCallback());
+ ChangeList.next(
+ query, start, pageSize, ChangeTable.OPTIONS, loadCallback());
}
private static boolean isSingleQuery(String query) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java
index 3ab6085..a56fba1 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/RebaseDialog.java
@@ -20,6 +20,7 @@
import com.google.gerrit.client.changes.Util;
import com.google.gerrit.client.rpc.GerritCallback;
import com.google.gerrit.client.rpc.Natives;
+import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.ClickHandler;
@@ -29,6 +30,7 @@
import com.google.gwtexpui.globalkey.client.GlobalKey;
import com.google.gwtexpui.safehtml.client.HighlightSuggestOracle;
+import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
@@ -76,8 +78,11 @@
public void onClick(ClickEvent event) {
boolean checked = ((CheckBox) event.getSource()).getValue();
if (checked) {
- ChangeList.next("project:" + project + " AND branch:" + branch
- + " AND is:open NOT age:90d", 0, 1000,
+ ChangeList.next(
+ "project:" + project + " AND branch:" + branch
+ + " AND is:open NOT age:90d",
+ 0, 1000,
+ Collections.<ListChangesOption> emptySet(),
new GerritCallback<ChangeList>() {
@Override
public void onSuccess(ChangeList result) {
