opendev/gerrit
Code Issues Proposed changes

Support > and < in LabelPredicate, with refactorings

Browse Source 
Use an Args class and a Parsed class to cut down on the number of
variables passed around to all these static methods. Use a string
switch instead of Test because we can.

Change-Id: Ia439a16cb94612e3d6c1205d8506491d0bbd9d92
This commit is contained in:
Dave Borowitz
2013-12-13 13:01:10 -08:00
parent b73a773856
commit d1ba6f51a2
3 changed files with 103 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
View File

@@ -42,20 +42,17 @@ class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
private final Account.Id account; private final Account.Id account;
private final AccountGroup.UUID group; private final AccountGroup.UUID group;
EqualsLabelPredicate(ProjectCache projectCache, EqualsLabelPredicate(LabelPredicate.Args args, String label, int expVal,
ChangeControl.GenericFactory ccFactory, Account.Id account) {
IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
String label, int expVal, Account.Id account,
AccountGroup.UUID group) {
super(ChangeField.LABEL, ChangeField.formatLabel(label, expVal, account)); super(ChangeField.LABEL, ChangeField.formatLabel(label, expVal, account));
this.ccFactory = ccFactory; this.ccFactory = args.ccFactory;
this.projectCache = projectCache; this.projectCache = args.projectCache;
this.userFactory = userFactory; this.userFactory = args.userFactory;
this.dbProvider = dbProvider; this.dbProvider = args.dbProvider;
this.group = args.group;
this.label = label; this.label = label;
this.expVal = expVal; this.expVal = expVal;
this.account = account; this.account = account;
this.group = group;
} }
@Override @Override

176
gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java
View File

@@ -34,30 +34,42 @@ import java.util.regex.Pattern;
public class LabelPredicate extends OrPredicate<ChangeData> { public class LabelPredicate extends OrPredicate<ChangeData> {
private static final int MAX_LABEL_VALUE = 4; private static final int MAX_LABEL_VALUE = 4;
private static enum Test { static class Args {
EQ, GT_EQ, LT_EQ; final ProjectCache projectCache;
final ChangeControl.GenericFactory ccFactory;
final IdentifiedUser.GenericFactory userFactory;
final Provider<ReviewDb> dbProvider;
final String value;
final Set<Account.Id> accounts;
final AccountGroup.UUID group;
boolean isEq() { private Args(
return EQ.equals(this); ProjectCache projectCache,
ChangeControl.GenericFactory ccFactory,
IdentifiedUser.GenericFactory userFactory,
Provider<ReviewDb> dbProvider,
String value,
Set<Account.Id> accounts,
AccountGroup.UUID group) {
this.projectCache = projectCache;
this.ccFactory = ccFactory;
this.userFactory = userFactory;
this.dbProvider = dbProvider;
this.value = value;
this.accounts = accounts;
this.group = group;
}
} }
boolean isGtEq() { private static class Parsed {
return GT_EQ.equals(this); private final String label;
} private final String test;
private final int expVal;
static Test op(String op) { private Parsed(String label, String test, int expVal) {
if ("=".equals(op)) { this.label = label;
return EQ; this.test = test;
this.expVal = expVal;
} else if (">=".equals(op)) {
return GT_EQ;
} else if ("<=".equals(op)) {
return LT_EQ;
} else {
throw new IllegalArgumentException("Unsupported operation " + op);
}
} }
} }
@@ -67,73 +79,79 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
ChangeControl.GenericFactory ccFactory, ChangeControl.GenericFactory ccFactory,
IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider, IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
String value, Set<Account.Id> accounts, AccountGroup.UUID group) { String value, Set<Account.Id> accounts, AccountGroup.UUID group) {
super(predicates(projectCache, ccFactory, userFactory, dbProvider, value, super(predicates(new Args(projectCache, ccFactory, userFactory, dbProvider,
accounts, group)); value, accounts, group)));
this.value = value; this.value = value;
} }
private static List<Predicate<ChangeData>> predicates( private static List<Predicate<ChangeData>> predicates(Args args) {
ProjectCache projectCache, ChangeControl.GenericFactory ccFactory, String v = args.value;
IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider, Parsed parsed = null;
String value, Set<Account.Id> accounts, AccountGroup.UUID group) {
String label = null;
Test test = null;
int expVal = 0;
try { try {
LabelVote v = LabelVote.parse(value); LabelVote lv = LabelVote.parse(v);
test = Test.EQ; parsed = new Parsed(lv.getLabel(), "=", lv.getValue());
label = v.getLabel();
expVal = v.getValue();
} catch (IllegalArgumentException e) { } catch (IllegalArgumentException e) {
// Try next format. // Try next format.
} }
try { try {
LabelVote v = LabelVote.parseWithEquals(value); LabelVote lv = LabelVote.parseWithEquals(v);
test = Test.EQ; parsed = new Parsed(lv.getLabel(), "=", lv.getValue());
label = v.getLabel();
expVal = v.getValue();
} catch (IllegalArgumentException e) { } catch (IllegalArgumentException e) {
// Try next format. // Try next format.
} }
if (label == null) { if (parsed == null) {
Matcher m = Pattern.compile("(>=|<=)([+-]?\\d+)$").matcher(value); Matcher m = Pattern.compile("(>|>=|=|<|<=)([+-]?\\d+)$").matcher(v);
if (m.find()) { if (m.find()) {
label = value.substring(0, m.start()); parsed = new Parsed(v.substring(0, m.start()), m.group(1),
test = Test.op(m.group(1)); value(m.group(2)));
expVal = value(m.group(2));
} else { } else {
label = value; parsed = new Parsed(v, "=", 1);
test = Test.EQ;
expVal = 1;
} }
} }
List<Predicate<ChangeData>> r = Lists.newArrayListWithCapacity(2 * MAX_LABEL_VALUE); int min, max;
if (test.isEq()) { switch (parsed.test) {
if (expVal != 0) { case "=":
r.add(equalsLabelPredicate(projectCache, ccFactory, userFactory, default:
dbProvider, label, expVal, accounts, group)); min = max = parsed.expVal;
} else { break;
r.add(noLabelQuery(projectCache, ccFactory, userFactory, case ">":
dbProvider, label, accounts, group)); min = parsed.expVal + 1;
} max = MAX_LABEL_VALUE;
} else { break;
for (int i = test.isGtEq() ? expVal : neg(expVal); i <= MAX_LABEL_VALUE; i++) { case ">=":
if (i != 0) { min = parsed.expVal;
r.add(equalsLabelPredicate(projectCache, ccFactory, userFactory, max = MAX_LABEL_VALUE;
dbProvider, label, test.isGtEq() ? i : neg(i), accounts, group)); break;
} else { case "<":
r.add(noLabelQuery(projectCache, ccFactory, userFactory, min = -MAX_LABEL_VALUE;
dbProvider, label, accounts, group)); max = parsed.expVal - 1;
} break;
case "<=":
min = -MAX_LABEL_VALUE;
max = parsed.expVal;
break;
} }
List<Predicate<ChangeData>> r =
Lists.newArrayListWithCapacity(max - min + 1);
for (int i = min; i <= max; i++) {
r.add(onePredicate(args, parsed.label, i));
} }
return r; return r;
} }
private static Predicate<ChangeData> onePredicate(Args args, String label,
int expVal) {
if (expVal != 0) {
return equalsLabelPredicate(args, label, expVal);
} else {
return noLabelQuery(args, label);
}
}
private static int value(String value) { private static int value(String value) {
if (value.startsWith("+")) { if (value.startsWith("+")) {
value = value.substring(1); value = value.substring(1);
@@ -141,38 +159,24 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
return Integer.parseInt(value); return Integer.parseInt(value);
} }
private static int neg(int value) { private static Predicate<ChangeData> noLabelQuery(Args args, String label) {
return -1 * value;
}
private static Predicate<ChangeData> noLabelQuery(ProjectCache projectCache,
ChangeControl.GenericFactory ccFactory,
IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider,
String label, Set<Account.Id> accounts, AccountGroup.UUID group) {
List<Predicate<ChangeData>> r = List<Predicate<ChangeData>> r =
Lists.newArrayListWithCapacity(2 * MAX_LABEL_VALUE); Lists.newArrayListWithCapacity(2 * MAX_LABEL_VALUE);
for (int i = 1; i <= MAX_LABEL_VALUE; i++) { for (int i = 1; i <= MAX_LABEL_VALUE; i++) {
r.add(not(equalsLabelPredicate(projectCache, ccFactory, userFactory, r.add(not(equalsLabelPredicate(args, label, i)));
dbProvider, label, i, accounts, group))); r.add(not(equalsLabelPredicate(args, label, -i)));
r.add(not(equalsLabelPredicate(projectCache, ccFactory, userFactory,
dbProvider, label, neg(i), accounts, group)));
} }
return and(r); return and(r);
} }
private static Predicate<ChangeData> equalsLabelPredicate( private static Predicate<ChangeData> equalsLabelPredicate(Args args,
ProjectCache projectCache, ChangeControl.GenericFactory ccFactory, String label, int expVal) {
IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider, if (args.accounts == null || args.accounts.isEmpty()) {
String label, int expVal, Set<Account.Id> accounts, return new EqualsLabelPredicate(args, label, expVal, null);
AccountGroup.UUID group) {
if (accounts == null || accounts.isEmpty()) {
return new EqualsLabelPredicate(projectCache, ccFactory, userFactory,
dbProvider, label, expVal, null, group);
} else { } else {
List<Predicate<ChangeData>> r = Lists.newArrayList(); List<Predicate<ChangeData>> r = Lists.newArrayList();
for (Account.Id a : accounts) { for (Account.Id a : args.accounts) {
r.add(new EqualsLabelPredicate(projectCache, ccFactory, userFactory, r.add(new EqualsLabelPredicate(args, label, expVal, a));
dbProvider, label, expVal, a, group));
} }
return or(r); return or(r);
} }

12
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -379,19 +379,17 @@ public abstract class AbstractQueryChangesTest {
assertTrue(query("label:Code-Review=2").isEmpty()); assertTrue(query("label:Code-Review=2").isEmpty());
assertTrue(query("label:Code-Review+2").isEmpty()); assertTrue(query("label:Code-Review+2").isEmpty());
// TODO(dborowitz): > and < are broken at head.
assertResultEquals(change, queryOne("label:Code-Review>=0")); assertResultEquals(change, queryOne("label:Code-Review>=0"));
//assertResultEquals(change, queryOne("label:Code-Review>0")); assertResultEquals(change, queryOne("label:Code-Review>0"));
assertResultEquals(change, queryOne("label:Code-Review>=1")); assertResultEquals(change, queryOne("label:Code-Review>=1"));
//assertTrue(query("label:Code-Review>1").isEmpty()); assertTrue(query("label:Code-Review>1").isEmpty());
assertTrue(query("label:Code-Review>=2").isEmpty()); assertTrue(query("label:Code-Review>=2").isEmpty());
assertResultEquals(change, queryOne("label: Code-Review<=2")); assertResultEquals(change, queryOne("label: Code-Review<=2"));
//assertResultEquals(change, queryOne("label: Code-Review<2")); assertResultEquals(change, queryOne("label: Code-Review<2"));
assertResultEquals(change, queryOne("label: Code-Review<=1")); assertResultEquals(change, queryOne("label: Code-Review<=1"));
//assertTrue(query("label: Code-Review<1").isEmpty()); assertTrue(query("label:Code-Review<1").isEmpty());
assertTrue(query("label: Code-Review<=0").isEmpty()); assertTrue(query("label:Code-Review<=0").isEmpty());
assertTrue(query("label:Code-Review=+1,anotheruser").isEmpty()); assertTrue(query("label:Code-Review=+1,anotheruser").isEmpty());
assertResultEquals(change, queryOne("label:Code-Review=+1,user")); assertResultEquals(change, queryOne("label:Code-Review=+1,user"));