Label-predicate: Add support for change owner votes

Change owner votes may have special meanings and different workflows
may be enforced for them:

o reject votes by change owner can denote Work In Progress (WIP) changes
o approval votes by change owner can mean multiple things, depending on
  project policies:
  oo review schould not be done on those changes and the change is
     only uploaded for verification purposes on different platform and
     architecture combinations
  oo auto_merge self approved changes when verification was successful
  oo enforcing "non-self approval policy", but not forbidding it to
     still be able to self approve changes in emergency cases.
     Given that this is exception from the policy, there should be an
     easy way to detect this policy violation with native gerrit
     predicate

Currently there is no built in way to detect change owner votes.
Having a built in way and not depending on external scripts to detect
change owner votes, would create addition value by allowing to combine
different predicates and use the result in gerrit itself, e.g.
dashboards can skip WIP changes.

This change extends label predicate semantic by adding additional
value for change owner votes: "label,owner". Schema version is bumped
to force reindexing. Label field is renamed to Label2 and query parser
is taught to route to the correct predicate depending on the live index
version.

With this change, change owner votes can be detected and used in
different workflows:

o skip WIP changes, rejected by change owner:
  is:open NOT label:Code-Review-2,owner
o skip non reviewable changes, approval by change owner:
  is:open NOT label:Code-Review+2,owner
o suggest changes for auto merge: approval by change owner + verify
  by the bot (assuming default label set: CRVW + VRFY):
  project:foo
  branch:master
  is:open
  is:mergeable
  label:Code-Review+2,owner
  label:Verified+1,buildbot
  NOT label:Code-Review-2
  NOT label:Verified-1
o detect changes, that violates "non-self approval policy":
  label:Code-Review+2,owner

Change-Id: I56ff6b1416f099dc627794c175aa4c2de3f2db9f
This commit is contained in:
David Ostrovsky 2016-07-06 06:45:43 +02:00 committed by Khai Do
parent 4475f68df0
commit dc26bbfaec
7 changed files with 103 additions and 25 deletions

View File

@ -482,6 +482,12 @@ Matches changes with a +2 code review where the reviewer or group is aname.
+ +
Matches changes with a +2 code review where the reviewer is jsmith. Matches changes with a +2 code review where the reviewer is jsmith.
`label:Code-Review=+2,user=owner`::
`label:Code-Review=+2,owner`::
+
The special "owner" parameter corresponds to the change owner. Matches
all changes that have a +2 vote from the change owner.
`label:Code-Review=+1,group=ldap/linux.workflow`:: `label:Code-Review=+1,group=ldap/linux.workflow`::
+ +
Matches changes with a +1 code review where the reviewer is in the Matches changes with a +1 code review where the reviewer is in the
@ -499,7 +505,6 @@ Matches changes that are ready to be submitted.
+ +
Changes that are blocked from submission due to a blocking score. Changes that are blocked from submission due to a blocking score.
== Magical Operators == Magical Operators
Most of these operators exist to support features of Gerrit Code Most of these operators exist to support features of Gerrit Code

View File

@ -429,26 +429,47 @@ public class ChangeField {
}; };
/** List of labels on the current patch set. */ /** List of labels on the current patch set. */
@Deprecated
public static final FieldDef<ChangeData, Iterable<String>> LABEL = public static final FieldDef<ChangeData, Iterable<String>> LABEL =
new FieldDef.Repeatable<ChangeData, String>( new FieldDef.Repeatable<ChangeData, String>(
ChangeQueryBuilder.FIELD_LABEL, FieldType.EXACT, false) { ChangeQueryBuilder.FIELD_LABEL, FieldType.EXACT, false) {
@Override @Override
public Iterable<String> get(ChangeData input, FillArgs args) public Iterable<String> get(ChangeData input, FillArgs args)
throws OrmException { throws OrmException {
Set<String> allApprovals = new HashSet<>(); return getLabels(input, false);
Set<String> distinctApprovals = new HashSet<>();
for (PatchSetApproval a : input.currentApprovals()) {
if (a.getValue() != 0 && !a.isLegacySubmit()) {
allApprovals.add(formatLabel(a.getLabel(), a.getValue(),
a.getAccountId()));
distinctApprovals.add(formatLabel(a.getLabel(), a.getValue()));
}
}
allApprovals.addAll(distinctApprovals);
return allApprovals;
} }
}; };
/** List of labels on the current patch set including change owner votes. */
public static final FieldDef<ChangeData, Iterable<String>> LABEL2 =
new FieldDef.Repeatable<ChangeData, String>(
"label2", FieldType.EXACT, false) {
@Override
public Iterable<String> get(ChangeData input, FillArgs args)
throws OrmException {
return getLabels(input, true);
}
};
private static Iterable<String> getLabels(ChangeData input, boolean owners)
throws OrmException {
Set<String> allApprovals = new HashSet<>();
Set<String> distinctApprovals = new HashSet<>();
for (PatchSetApproval a : input.currentApprovals()) {
if (a.getValue() != 0 && !a.isLegacySubmit()) {
allApprovals.add(formatLabel(a.getLabel(), a.getValue(),
a.getAccountId()));
if (owners && input.change().getOwner().equals(a.getAccountId())) {
allApprovals.add(formatLabel(a.getLabel(), a.getValue(),
ChangeQueryBuilder.OWNER_ACCOUNT_ID));
}
distinctApprovals.add(formatLabel(a.getLabel(), a.getValue()));
}
}
allApprovals.addAll(distinctApprovals);
return allApprovals;
}
public static Set<String> getAuthorParts(ChangeData cd) throws OrmException { public static Set<String> getAuthorParts(ChangeData cd) throws OrmException {
try { try {
return SchemaUtil.getPersonParts(cd.getAuthor()); return SchemaUtil.getPersonParts(cd.getAuthor());
@ -544,7 +565,14 @@ public class ChangeField {
public static String formatLabel(String label, int value, Account.Id accountId) { public static String formatLabel(String label, int value, Account.Id accountId) {
return label.toLowerCase() + (value >= 0 ? "+" : "") + value return label.toLowerCase() + (value >= 0 ? "+" : "") + value
+ (accountId != null ? "," + accountId.get() : ""); + (accountId != null ? "," + formatAccount(accountId) : "");
}
private static String formatAccount(Account.Id accountId) {
if (ChangeQueryBuilder.OWNER_ACCOUNT_ID.equals(accountId)) {
return ChangeQueryBuilder.ARG_ID_OWNER;
}
return Integer.toString(accountId.get());
} }
/** Commit message of the current patch set. */ /** Commit message of the current patch set. */

View File

@ -88,9 +88,17 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
.add(ChangeField.REVIEWER) .add(ChangeField.REVIEWER)
.build(); .build();
@Deprecated
static final Schema<ChangeData> V33 = static final Schema<ChangeData> V33 =
schema(V32, ChangeField.ASSIGNEE); schema(V32, ChangeField.ASSIGNEE);
@SuppressWarnings("deprecation")
static final Schema<ChangeData> V34 = new Schema.Builder<ChangeData>()
.add(V33)
.remove(ChangeField.LABEL)
.add(ChangeField.LABEL2)
.build();
public static final String NAME = "changes"; public static final String NAME = "changes";
public static final ChangeSchemaDefinitions INSTANCE = public static final ChangeSchemaDefinitions INSTANCE =
new ChangeSchemaDefinitions(); new ChangeSchemaDefinitions();

View File

@ -152,7 +152,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public static final String ARG_ID_USER = "user"; public static final String ARG_ID_USER = "user";
public static final String ARG_ID_GROUP = "group"; public static final String ARG_ID_GROUP = "group";
public static final String ARG_ID_OWNER = "owner";
public static final Account.Id OWNER_ACCOUNT_ID = new Account.Id(0);
private static final QueryBuilder.Definition<ChangeData, ChangeQueryBuilder> mydef = private static final QueryBuilder.Definition<ChangeData, ChangeQueryBuilder> mydef =
new QueryBuilder.Definition<>(ChangeQueryBuilder.class); new QueryBuilder.Definition<>(ChangeQueryBuilder.class);
@ -600,6 +601,9 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
// label:CodeReview=1,group=android_approvers or // label:CodeReview=1,group=android_approvers or
// label:CodeReview=1,android_approvers // label:CodeReview=1,android_approvers
// user/groups without a label will first attempt to match user // user/groups without a label will first attempt to match user
// Special case: votes by owners can be tracked with ",owner":
// label:Code-Review+2,owner
// label:Code-Review+2,user=owner
String[] splitReviewer = name.split(",", 2); String[] splitReviewer = name.split(",", 2);
name = splitReviewer[0]; // remove all but the vote piece, e.g.'CodeReview=1' name = splitReviewer[0]; // remove all but the vote piece, e.g.'CodeReview=1'
@ -609,7 +613,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
for (Map.Entry<String, String> pair : lblArgs.keyValue.entrySet()) { for (Map.Entry<String, String> pair : lblArgs.keyValue.entrySet()) {
if (pair.getKey().equalsIgnoreCase(ARG_ID_USER)) { if (pair.getKey().equalsIgnoreCase(ARG_ID_USER)) {
accounts = parseAccount(pair.getValue()); if (pair.getValue().equals(ARG_ID_OWNER)) {
accounts = Collections.singleton(OWNER_ACCOUNT_ID);
} else {
accounts = parseAccount(pair.getValue());
}
} else if (pair.getKey().equalsIgnoreCase(ARG_ID_GROUP)) { } else if (pair.getKey().equalsIgnoreCase(ARG_ID_GROUP)) {
group = parseGroup(pair.getValue()).getUUID(); group = parseGroup(pair.getValue()).getUUID();
} else { } else {
@ -624,7 +632,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
value + ")"); value + ")");
} }
try { try {
accounts = parseAccount(value); if (value.equals(ARG_ID_OWNER)) {
accounts = Collections.singleton(OWNER_ACCOUNT_ID);
} else {
accounts = parseAccount(value);
}
} catch (QueryParseException qpex) { } catch (QueryParseException qpex) {
// If it doesn't match an account, see if it matches a group // If it doesn't match an account, see if it matches a group
// (accounts get precedence) // (accounts get precedence)
@ -652,9 +664,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
} }
} }
return new LabelPredicate(args.projectCache, return new LabelPredicate(args, name, accounts, group);
args.changeControlGenericFactory, args.userFactory, args.db,
name, accounts, group);
} }
@Operator @Operator

View File

@ -43,7 +43,7 @@ class EqualsLabelPredicate extends ChangeIndexPredicate {
EqualsLabelPredicate(LabelPredicate.Args args, String label, int expVal, EqualsLabelPredicate(LabelPredicate.Args args, String label, int expVal,
Account.Id account) { Account.Id account) {
super(ChangeField.LABEL, ChangeField.formatLabel(label, expVal, account)); super(args.field, ChangeField.formatLabel(label, expVal, account));
this.ccFactory = args.ccFactory; this.ccFactory = args.ccFactory;
this.projectCache = args.projectCache; this.projectCache = args.projectCache;
this.userFactory = args.userFactory; this.userFactory = args.userFactory;

View File

@ -19,6 +19,8 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.index.FieldDef;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.OrPredicate; import com.google.gerrit.server.query.OrPredicate;
@ -36,6 +38,7 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
private static final int MAX_LABEL_VALUE = 4; private static final int MAX_LABEL_VALUE = 4;
static class Args { static class Args {
final FieldDef<ChangeData, ?> field;
final ProjectCache projectCache; final ProjectCache projectCache;
final ChangeControl.GenericFactory ccFactory; final ChangeControl.GenericFactory ccFactory;
final IdentifiedUser.GenericFactory userFactory; final IdentifiedUser.GenericFactory userFactory;
@ -45,6 +48,7 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
final AccountGroup.UUID group; final AccountGroup.UUID group;
private Args( private Args(
FieldDef<ChangeData, ?> field,
ProjectCache projectCache, ProjectCache projectCache,
ChangeControl.GenericFactory ccFactory, ChangeControl.GenericFactory ccFactory,
IdentifiedUser.GenericFactory userFactory, IdentifiedUser.GenericFactory userFactory,
@ -52,6 +56,7 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
String value, String value,
Set<Account.Id> accounts, Set<Account.Id> accounts,
AccountGroup.UUID group) { AccountGroup.UUID group) {
this.field = field;
this.projectCache = projectCache; this.projectCache = projectCache;
this.ccFactory = ccFactory; this.ccFactory = ccFactory;
this.userFactory = userFactory; this.userFactory = userFactory;
@ -76,11 +81,12 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
private final String value; private final String value;
LabelPredicate(ProjectCache projectCache, @SuppressWarnings("deprecation")
ChangeControl.GenericFactory ccFactory, LabelPredicate(ChangeQueryBuilder.Arguments a, String value,
IdentifiedUser.GenericFactory userFactory, Provider<ReviewDb> dbProvider, Set<Account.Id> accounts, AccountGroup.UUID group) {
String value, Set<Account.Id> accounts, AccountGroup.UUID group) { super(predicates(new Args(
super(predicates(new Args(projectCache, ccFactory, userFactory, dbProvider, a.getSchema().getField(ChangeField.LABEL2, ChangeField.LABEL).get(),
a.projectCache, a.changeControlGenericFactory, a.userFactory, a.db,
value, accounts, group))); value, accounts, group)));
this.value = value; this.value = value;
} }

View File

@ -619,6 +619,27 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
assertQuery("label:Code-Review=+1,user=user", reviewPlus1Change); assertQuery("label:Code-Review=+1,user=user", reviewPlus1Change);
assertQuery("label:Code-Review=+1,Administrators", reviewPlus1Change); assertQuery("label:Code-Review=+1,Administrators", reviewPlus1Change);
assertQuery("label:Code-Review=+1,group=Administrators", reviewPlus1Change); assertQuery("label:Code-Review=+1,group=Administrators", reviewPlus1Change);
assertQuery("label:Code-Review=+1,user=owner", reviewPlus1Change);
assertQuery("label:Code-Review=+1,owner", reviewPlus1Change);
assertQuery("label:Code-Review=+2,owner", reviewPlus2Change);
assertQuery("label:Code-Review=-2,owner", reviewMinus2Change);
}
@Test
public void byLabelNotOwner() throws Exception {
TestRepository<Repo> repo = createProject("repo");
ChangeInserter ins = newChange(repo, null, null, null, null);
Account.Id user1 = createAccount("user1");
Change reviewPlus1Change = insert(repo, ins);
// post a review with user1
requestContext.setContext(newRequestContext(user1));
gApi.changes().id(reviewPlus1Change.getId().get()).current()
.review(ReviewInput.recommend());
assertQuery("label:Code-Review=+1,user=user1", reviewPlus1Change);
assertQuery("label:Code-Review=+1,owner");
} }
private Change[] codeReviewInRange(Map<Integer, Change> changes, int start, private Change[] codeReviewInRange(Map<Integer, Change> changes, int start,