ChangeStatusPredicate: Silently ignore invalid status
Instead of throwing an exception from the ChangeStatusPredicate
constructor complaining about an invalid change status, return a real
index predicate that is guaranteed to match no changes in the index.
This is similar to the behavior of other query engines that are vaguely
Gerrit-like; for example, in Gmail, searching for [has:attachment] has
meaning, whereas [has:fhqwhgads] successfully returns no results.
The proximate motivation for this change is so user dashboards and other
links containing [is:draft] predicates continue to work after removing
the draft workflow. While it might be reasonable to return 400 for a
pure [is:draft] search after drafts are removed, a user might expect
[owner:self (is:draft OR is:new)] to continue working. Similarly, we can
write a migration that removes all [is:draft] custom dashboard entries
from user preferences, but there is no single migration step that works
in the general case of a query containing [is:draft] at arbitrary points
in the query tree.
This change is straightforward and general, but is not the only way to
achieve this goal. Another possibility would be to special-case
[is:draft] specifically to return ChangeStatusPredicate.NONE. That would
be the most minimal behavior change, but requires more messy
hard-coding.
Change-Id: I32119ff660d914a64da64bcd0769b0434d68a8d6
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
index 28843c9..824fd4f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
@@ -83,7 +83,8 @@
private static EnumSet<Change.Status> extractStatus(Predicate<ChangeData> in) {
if (in instanceof ChangeStatusPredicate) {
- return EnumSet.of(((ChangeStatusPredicate) in).getStatus());
+ Status status = ((ChangeStatusPredicate) in).getStatus();
+ return status != null ? EnumSet.of(status) : null;
} else if (in instanceof NotPredicate) {
EnumSet<Status> s = extractStatus(in.getChild(0));
return s != null ? EnumSet.complementOf(s) : null;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 309f15c..beff5f2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -496,7 +496,7 @@
}
@Operator
- public Predicate<ChangeData> status(String statusName) throws QueryParseException {
+ public Predicate<ChangeData> status(String statusName) {
if ("reviewed".equalsIgnoreCase(statusName)) {
return IsReviewedPredicate.create();
}
@@ -619,13 +619,7 @@
throw new QueryParseException("'is:wip' operator is not supported by change index version");
}
- try {
- return status(value);
- } catch (IllegalArgumentException e) {
- // not status: alias?
- }
-
- throw error("Invalid query");
+ return status(value);
}
@Operator
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
index edead5a..155b016 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
@@ -14,8 +14,11 @@
package com.google.gerrit.server.query.change;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.index.query.Predicate;
-import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.server.index.change.ChangeField;
@@ -24,6 +27,7 @@
import java.util.List;
import java.util.Map;
import java.util.NavigableMap;
+import java.util.Objects;
import java.util.TreeMap;
/**
@@ -36,6 +40,9 @@
* <p>Status names are looked up by prefix case-insensitively.
*/
public final class ChangeStatusPredicate extends ChangeIndexPredicate {
+ private static final String INVALID_STATUS = "__invalid__";
+ private static final Predicate<ChangeData> NONE = new ChangeStatusPredicate(null);
+
private static final TreeMap<String, Predicate<ChangeData>> PREDICATES;
private static final Predicate<ChangeData> CLOSED;
private static final Predicate<ChangeData> OPEN;
@@ -46,8 +53,14 @@
List<Predicate<ChangeData>> closed = new ArrayList<>();
for (Change.Status s : Change.Status.values()) {
- ChangeStatusPredicate p = new ChangeStatusPredicate(s);
- PREDICATES.put(canonicalize(s), p);
+ ChangeStatusPredicate p = forStatus(s);
+ String str = canonicalize(s);
+ checkState(
+ !INVALID_STATUS.equals(str),
+ "invalid status sentinel %s cannot match canonicalized status string %s",
+ INVALID_STATUS,
+ str);
+ PREDICATES.put(str, p);
(s.isOpen() ? open : closed).add(p);
}
@@ -63,7 +76,7 @@
return status.name().toLowerCase();
}
- public static Predicate<ChangeData> parse(String value) throws QueryParseException {
+ public static Predicate<ChangeData> parse(String value) {
String lower = value.toLowerCase();
NavigableMap<String, Predicate<ChangeData>> head = PREDICATES.tailMap(lower, true);
if (!head.isEmpty()) {
@@ -73,7 +86,7 @@
return e.getValue();
}
}
- throw new QueryParseException("invalid change status: " + value);
+ return NONE;
}
public static Predicate<ChangeData> open() {
@@ -84,13 +97,23 @@
return CLOSED;
}
- private final Change.Status status;
+ public static ChangeStatusPredicate forStatus(Change.Status status) {
+ return new ChangeStatusPredicate(checkNotNull(status));
+ }
- public ChangeStatusPredicate(Change.Status status) {
- super(ChangeField.STATUS, canonicalize(status));
+ @Nullable private final Change.Status status;
+
+ private ChangeStatusPredicate(@Nullable Change.Status status) {
+ super(ChangeField.STATUS, status != null ? canonicalize(status) : INVALID_STATUS);
this.status = status;
}
+ /**
+ * Get the status for this predicate.
+ *
+ * @return the status, or null if this predicate is intended to never match any changes.
+ */
+ @Nullable
public Change.Status getStatus() {
return status;
}
@@ -98,7 +121,7 @@
@Override
public boolean match(ChangeData object) throws OrmException {
Change change = object.change();
- return change != null && status.equals(change.getStatus());
+ return change != null && Objects.equals(status, change.getStatus());
}
@Override
@@ -108,16 +131,13 @@
@Override
public int hashCode() {
- return status.hashCode();
+ return Objects.hashCode(status);
}
@Override
public boolean equals(Object other) {
- if (other instanceof ChangeStatusPredicate) {
- final ChangeStatusPredicate p = (ChangeStatusPredicate) other;
- return status.equals(p.status);
- }
- return false;
+ return (other instanceof ChangeStatusPredicate)
+ && Objects.equals(status, ((ChangeStatusPredicate) other).status);
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index 2316bd0..4d10c0e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -66,7 +66,7 @@
}
private static Predicate<ChangeData> status(Change.Status status) {
- return new ChangeStatusPredicate(status);
+ return ChangeStatusPredicate.forStatus(status);
}
private static Predicate<ChangeData> commit(String id) {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
index 97d8ae8..4a6663a 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
@@ -175,13 +175,16 @@
@Test
public void getPossibleStatus() throws Exception {
- assertThat(status("file:a")).isEqualTo(EnumSet.allOf(Change.Status.class));
+ Set<Change.Status> all = EnumSet.allOf(Change.Status.class);
+ assertThat(status("file:a")).isEqualTo(all);
assertThat(status("is:new")).containsExactly(NEW);
assertThat(status("is:new OR is:merged")).containsExactly(NEW, MERGED);
+ assertThat(status("is:new OR is:x")).isEqualTo(all);
assertThat(status("is:new is:merged")).isEmpty();
assertThat(status("(is:new) (is:merged)")).isEmpty();
assertThat(status("(is:new) (is:merged)")).isEmpty();
+ assertThat(status("is:new is:x")).containsExactly(NEW);
}
@Test
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index b1e600a..1e722fc 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -324,6 +324,8 @@
assertQuery("is:new", change1);
assertQuery("status:merged", change2);
assertQuery("is:merged", change2);
+ assertQuery("status:draft");
+ assertQuery("is:draft");
}
@Test
@@ -381,10 +383,8 @@
assertQuery("status:N", change1);
assertQuery("status:nE", change1);
assertQuery("status:neW", change1);
- assertThatQueryException("status:nx").hasMessageThat().isEqualTo("invalid change status: nx");
- assertThatQueryException("status:newx")
- .hasMessageThat()
- .isEqualTo("invalid change status: newx");
+ assertQuery("status:nx");
+ assertQuery("status:newx");
}
@Test
