From dc26bbfaec5ba4153eb5ce5b84d1359f78cf7652 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 6 Jul 2016 06:45:43 +0200 Subject: [PATCH] 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 --- Documentation/user-search.txt | 7 ++- .../server/index/change/ChangeField.java | 52 ++++++++++++++----- .../index/change/ChangeSchemaDefinitions.java | 8 +++ .../query/change/ChangeQueryBuilder.java | 22 +++++--- .../query/change/EqualsLabelPredicate.java | 2 +- .../server/query/change/LabelPredicate.java | 16 ++++-- .../change/AbstractQueryChangesTest.java | 21 ++++++++ 7 files changed, 103 insertions(+), 25 deletions(-) diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt index b04898eb7e..0b6de578a7 100644 --- a/Documentation/user-search.txt +++ b/Documentation/user-search.txt @@ -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. +`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`:: + 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. - == Magical Operators Most of these operators exist to support features of Gerrit Code diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java index 8de8092b6c..e8e99cf767 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java @@ -429,26 +429,47 @@ public class ChangeField { }; /** List of labels on the current patch set. */ + @Deprecated public static final FieldDef> LABEL = new FieldDef.Repeatable( ChangeQueryBuilder.FIELD_LABEL, FieldType.EXACT, false) { @Override public Iterable get(ChangeData input, FillArgs args) throws OrmException { - Set allApprovals = new HashSet<>(); - Set 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; + return getLabels(input, false); } }; + /** List of labels on the current patch set including change owner votes. */ + public static final FieldDef> LABEL2 = + new FieldDef.Repeatable( + "label2", FieldType.EXACT, false) { + @Override + public Iterable get(ChangeData input, FillArgs args) + throws OrmException { + return getLabels(input, true); + } + }; + + private static Iterable getLabels(ChangeData input, boolean owners) + throws OrmException { + Set allApprovals = new HashSet<>(); + Set 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 getAuthorParts(ChangeData cd) throws OrmException { try { return SchemaUtil.getPersonParts(cd.getAuthor()); @@ -544,7 +565,14 @@ public class ChangeField { public static String formatLabel(String label, int value, Account.Id accountId) { 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. */ diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java index c5f3ab034b..3dd08a8cec 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java @@ -88,9 +88,17 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions { .add(ChangeField.REVIEWER) .build(); + @Deprecated static final Schema V33 = schema(V32, ChangeField.ASSIGNEE); + @SuppressWarnings("deprecation") + static final Schema V34 = new Schema.Builder() + .add(V33) + .remove(ChangeField.LABEL) + .add(ChangeField.LABEL2) + .build(); + public static final String NAME = "changes"; public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions(); 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 aaf7966113..adbd1d095a 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 @@ -152,7 +152,8 @@ public class ChangeQueryBuilder extends QueryBuilder { public static final String ARG_ID_USER = "user"; 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 mydef = new QueryBuilder.Definition<>(ChangeQueryBuilder.class); @@ -600,6 +601,9 @@ public class ChangeQueryBuilder extends QueryBuilder { // label:CodeReview=1,group=android_approvers or // label:CodeReview=1,android_approvers // 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); name = splitReviewer[0]; // remove all but the vote piece, e.g.'CodeReview=1' @@ -609,7 +613,11 @@ public class ChangeQueryBuilder extends QueryBuilder { for (Map.Entry pair : lblArgs.keyValue.entrySet()) { 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)) { group = parseGroup(pair.getValue()).getUUID(); } else { @@ -624,7 +632,11 @@ public class ChangeQueryBuilder extends QueryBuilder { value + ")"); } try { - accounts = parseAccount(value); + if (value.equals(ARG_ID_OWNER)) { + accounts = Collections.singleton(OWNER_ACCOUNT_ID); + } else { + accounts = parseAccount(value); + } } catch (QueryParseException qpex) { // If it doesn't match an account, see if it matches a group // (accounts get precedence) @@ -652,9 +664,7 @@ public class ChangeQueryBuilder extends QueryBuilder { } } - return new LabelPredicate(args.projectCache, - args.changeControlGenericFactory, args.userFactory, args.db, - name, accounts, group); + return new LabelPredicate(args, name, accounts, group); } @Operator diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java index e752b05c35..0adf78fd0f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java @@ -43,7 +43,7 @@ class EqualsLabelPredicate extends ChangeIndexPredicate { EqualsLabelPredicate(LabelPredicate.Args args, String label, int expVal, Account.Id account) { - super(ChangeField.LABEL, ChangeField.formatLabel(label, expVal, account)); + super(args.field, ChangeField.formatLabel(label, expVal, account)); this.ccFactory = args.ccFactory; this.projectCache = args.projectCache; this.userFactory = args.userFactory; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java index 2f815b25af..9bed4b524c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java @@ -19,6 +19,8 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.server.ReviewDb; 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.ProjectCache; import com.google.gerrit.server.query.OrPredicate; @@ -36,6 +38,7 @@ public class LabelPredicate extends OrPredicate { private static final int MAX_LABEL_VALUE = 4; static class Args { + final FieldDef field; final ProjectCache projectCache; final ChangeControl.GenericFactory ccFactory; final IdentifiedUser.GenericFactory userFactory; @@ -45,6 +48,7 @@ public class LabelPredicate extends OrPredicate { final AccountGroup.UUID group; private Args( + FieldDef field, ProjectCache projectCache, ChangeControl.GenericFactory ccFactory, IdentifiedUser.GenericFactory userFactory, @@ -52,6 +56,7 @@ public class LabelPredicate extends OrPredicate { String value, Set accounts, AccountGroup.UUID group) { + this.field = field; this.projectCache = projectCache; this.ccFactory = ccFactory; this.userFactory = userFactory; @@ -76,11 +81,12 @@ public class LabelPredicate extends OrPredicate { private final String value; - LabelPredicate(ProjectCache projectCache, - ChangeControl.GenericFactory ccFactory, - IdentifiedUser.GenericFactory userFactory, Provider dbProvider, - String value, Set accounts, AccountGroup.UUID group) { - super(predicates(new Args(projectCache, ccFactory, userFactory, dbProvider, + @SuppressWarnings("deprecation") + LabelPredicate(ChangeQueryBuilder.Arguments a, String value, + Set accounts, AccountGroup.UUID group) { + super(predicates(new Args( + a.getSchema().getField(ChangeField.LABEL2, ChangeField.LABEL).get(), + a.projectCache, a.changeControlGenericFactory, a.userFactory, a.db, value, accounts, group))); this.value = value; } 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 dae2abbfcc..59ae73f2a2 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 @@ -619,6 +619,27 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { assertQuery("label:Code-Review=+1,user=user", reviewPlus1Change); assertQuery("label:Code-Review=+1,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 = 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 changes, int start,