From 9e8af7f0dc3d4662740bc632ed756e515af7e7f9 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 16 Mar 2017 20:57:38 +0100 Subject: [PATCH] Add reviewers by email to ChangeIndex This change adds reviewers by email to the ChangeIndex and adds tests for the new code. It also expands the 'reviewer' and 'cc' queries to match on reviewerByEmail as well. Bug: Issue 4134 Change-Id: Iae9ada56e2ab9a03b6d5c20de4bca53ec27a4767 --- .../rest/change/ChangeReviewersByEmailIT.java | 29 +++++++ .../elasticsearch/ElasticChangeIndex.java | 11 +++ .../gerrit/lucene/LuceneChangeIndex.java | 11 +++ .../gerrit/server/change/ChangeJson.java | 3 +- .../server/index/change/ChangeField.java | 48 ++++++++++++ .../index/change/ChangeSchemaDefinitions.java | 4 +- .../gerrit/server/mail/send/ChangeEmail.java | 6 +- .../server/query/change/ChangeData.java | 20 +++++ .../query/change/ChangeIndexPredicate.java | 10 +++ .../query/change/ChangeQueryBuilder.java | 44 +++++++++-- .../change/ReviewerByEmailPredicate.java | 55 +++++++++++++ .../query/change/ReviewerPredicate.java | 17 ++-- .../change/AbstractQueryChangesTest.java | 78 +++++++++++++++++++ 13 files changed, 312 insertions(+), 24 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerByEmailPredicate.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java index 2911b62a5c..bc841ada62 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java @@ -19,6 +19,7 @@ import static com.google.common.truth.TruthJUnit.assume; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; @@ -239,6 +240,34 @@ public class ChangeReviewersByEmailIT extends AbstractDaemonTest { gApi.changes().id(r.getChangeId()).addReviewer("Foo Bar "); } + @Test + public void reviewersByEmailAreServedFromIndex() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com"); + + for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) { + PushOneCommit.Result r = createChange(); + + AddReviewerInput input = new AddReviewerInput(); + input.reviewer = toRfcAddressString(acc); + input.state = state; + gApi.changes().id(r.getChangeId()).addReviewer(input); + + notesMigration.setFailOnLoad(true); + try { + ChangeInfo info = + Iterables.getOnlyElement( + gApi.changes() + .query(r.getChangeId()) + .withOption(ListChangesOption.DETAILED_LABELS) + .get()); + assertThat(info.reviewers).isEqualTo(ImmutableMap.of(state, ImmutableList.of(acc))); + } finally { + notesMigration.setFailOnLoad(false); + } + } + } + private static String toRfcAddressString(AccountInfo info) { return (new Address(info.name, info.email)).toString(); } diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java index 933e84b969..fdc14d7c36 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java @@ -34,6 +34,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change.Id; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; @@ -342,6 +343,16 @@ class ElasticChangeIndex extends AbstractElasticIndex cd.setReviewers(ReviewerSet.empty()); } + if (source.get(ChangeField.REVIEWER_BY_EMAIL.getName()) != null) { + cd.setReviewersByEmail( + ChangeField.parseReviewerByEmailFieldValues( + FluentIterable.from( + source.get(ChangeField.REVIEWER_BY_EMAIL.getName()).getAsJsonArray()) + .transform(JsonElement::getAsString))); + } else if (fields.contains(ChangeField.REVIEWER_BY_EMAIL.getName())) { + cd.setReviewersByEmail(ReviewerByEmailSet.empty()); + } + decodeSubmitRecords( source, ChangeField.STORED_SUBMIT_RECORD_STRICT.getName(), diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java index e68e29f479..3afcb07470 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -121,6 +121,7 @@ public class LuceneChangeIndex implements ChangeIndex { private static final String REF_STATE_PATTERN_FIELD = ChangeField.REF_STATE_PATTERN.getName(); private static final String REVIEWEDBY_FIELD = ChangeField.REVIEWEDBY.getName(); private static final String REVIEWER_FIELD = ChangeField.REVIEWER.getName(); + private static final String REVIEWER_BY_EMAIL_FIELD = ChangeField.REVIEWER_BY_EMAIL.getName(); private static final String HASHTAG_FIELD = ChangeField.HASHTAG_CASE_AWARE.getName(); private static final String STAR_FIELD = ChangeField.STAR.getName(); private static final String SUBMIT_RECORD_LENIENT_FIELD = @@ -459,6 +460,9 @@ public class LuceneChangeIndex implements ChangeIndex { if (fields.contains(REVIEWER_FIELD)) { decodeReviewers(doc, cd); } + if (fields.contains(REVIEWER_BY_EMAIL_FIELD)) { + decodeReviewersByEmail(doc, cd); + } decodeSubmitRecords( doc, SUBMIT_RECORD_STRICT_FIELD, ChangeField.SUBMIT_RULE_OPTIONS_STRICT, cd); decodeSubmitRecords( @@ -555,6 +559,13 @@ public class LuceneChangeIndex implements ChangeIndex { FluentIterable.from(doc.get(REVIEWER_FIELD)).transform(IndexableField::stringValue))); } + private void decodeReviewersByEmail(ListMultimap doc, ChangeData cd) { + cd.setReviewersByEmail( + ChangeField.parseReviewerByEmailFieldValues( + FluentIterable.from(doc.get(REVIEWER_BY_EMAIL_FIELD)) + .transform(IndexableField::stringValue))); + } + private void decodeSubmitRecords( ListMultimap doc, String field, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index bfa2ee93f6..066c1a2f4d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -532,9 +532,8 @@ public class ChangeJson { cd.reviewers().asTable().rowMap().entrySet()) { out.reviewers.put(e.getKey().asReviewerState(), toAccountInfo(e.getValue().keySet())); } - // TODO(hiesel) Load from ChangeData instead after the data was added there for (Map.Entry> e : - cd.notes().getReviewersByEmail().asTable().rowMap().entrySet()) { + cd.reviewersByEmail().asTable().rowMap().entrySet()) { out.reviewers.put( e.getKey().asReviewerState(), toAccountInfoByEmail(e.getValue().keySet())); } 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 6a33b31393..d7fe713984 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 @@ -46,6 +46,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.OutputFormat; +import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.index.FieldDef; @@ -53,6 +54,7 @@ import com.google.gerrit.server.index.FieldDef.FillArgs; import com.google.gerrit.server.index.SchemaUtil; import com.google.gerrit.server.index.change.StalenessChecker.RefState; import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern; +import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.ReviewerStateInternal; @@ -184,6 +186,12 @@ public class ChangeField { public static final FieldDef> REVIEWER = exact("reviewer2").stored().buildRepeatable(cd -> getReviewerFieldValues(cd.reviewers())); + /** Reviewer(s) associated with the change that do not have a gerrit account. */ + public static final FieldDef> REVIEWER_BY_EMAIL = + exact("reviewer_by_email") + .stored() + .buildRepeatable(cd -> getReviewerByEmailFieldValues(cd.reviewersByEmail())); + @VisibleForTesting static List getReviewerFieldValues(ReviewerSet reviewers) { List r = new ArrayList<>(reviewers.asTable().size() * 2); @@ -200,6 +208,27 @@ public class ChangeField { return state.toString() + ',' + id; } + @VisibleForTesting + static List getReviewerByEmailFieldValues(ReviewerByEmailSet reviewersByEmail) { + List r = new ArrayList<>(reviewersByEmail.asTable().size() * 2); + for (Table.Cell c : + reviewersByEmail.asTable().cellSet()) { + String v = getReviewerByEmailFieldValue(c.getRowKey(), c.getColumnKey()); + r.add(v); + if (c.getColumnKey().getName() != null) { + // Add another entry without the name to provide search functionality on the email + Address emailOnly = new Address(c.getColumnKey().getEmail()); + r.add(getReviewerByEmailFieldValue(c.getRowKey(), emailOnly)); + } + r.add(v + ',' + c.getValue().getTime()); + } + return r; + } + + public static String getReviewerByEmailFieldValue(ReviewerStateInternal state, Address adr) { + return state.toString() + ',' + adr; + } + public static ReviewerSet parseReviewerFieldValues(Iterable values) { ImmutableTable.Builder b = ImmutableTable.builder(); @@ -220,6 +249,25 @@ public class ChangeField { return ReviewerSet.fromTable(b.build()); } + public static ReviewerByEmailSet parseReviewerByEmailFieldValues(Iterable values) { + ImmutableTable.Builder b = ImmutableTable.builder(); + for (String v : values) { + int f = v.indexOf(','); + if (f < 0) { + continue; + } + int l = v.lastIndexOf(','); + if (l == f) { + continue; + } + b.put( + ReviewerStateInternal.valueOf(v.substring(0, f)), + Address.parse(v.substring(f + 1, l)), + new Timestamp(Long.valueOf(v.substring(l + 1, v.length())))); + } + return ReviewerByEmailSet.fromTable(b.build()); + } + /** Commit ID of any patch set on the change, using prefix match. */ public static final FieldDef> COMMIT = prefix(ChangeQueryBuilder.FIELD_COMMIT).buildRepeatable(ChangeField::getRevisions); 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 038c35e461..ba7c1ecc59 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 @@ -92,7 +92,9 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions { @Deprecated static final Schema V39 = schema(V38); - static final Schema V40 = schema(V39, ChangeField.PRIVATE); + @Deprecated static final Schema V40 = schema(V39, ChangeField.PRIVATE); + + static final Schema V41 = schema(V40, ChangeField.REVIEWER_BY_EMAIL); 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/mail/send/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 3c76319ef4..18fc08322c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java @@ -183,13 +183,11 @@ public abstract class ChangeEmail extends NotificationEmail { if (notify.ordinal() >= NotifyHandling.OWNER_REVIEWERS.ordinal()) { try { - // TODO(hiesel) Load from index instead addByEmail( - RecipientType.CC, - changeData.notes().getReviewersByEmail().byState(ReviewerStateInternal.CC)); + RecipientType.CC, changeData.reviewersByEmail().byState(ReviewerStateInternal.CC)); addByEmail( RecipientType.TO, - changeData.notes().getReviewersByEmail().byState(ReviewerStateInternal.REVIEWER)); + changeData.reviewersByEmail().byState(ReviewerStateInternal.REVIEWER)); } catch (OrmException e) { throw new EmailException("Failed to add unregistered CCs " + change.getChangeId(), e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index b62e3ebb35..1dbe5cd28f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -52,6 +52,7 @@ import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchSetUtil; +import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; import com.google.gerrit.server.StarredChangesUtil; @@ -352,6 +353,7 @@ public class ChangeData { private StarsOf starsOf; private ImmutableMap starRefs; private ReviewerSet reviewers; + private ReviewerByEmailSet reviewersByEmail; private List reviewerUpdates; private PersonIdent author; private PersonIdent committer; @@ -954,6 +956,24 @@ public class ChangeData { return reviewers; } + public ReviewerByEmailSet reviewersByEmail() throws OrmException { + if (reviewersByEmail == null) { + if (!lazyLoad) { + return ReviewerByEmailSet.empty(); + } + reviewersByEmail = notes().getReviewersByEmail(); + } + return reviewersByEmail; + } + + public void setReviewersByEmail(ReviewerByEmailSet reviewersByEmail) { + this.reviewersByEmail = reviewersByEmail; + } + + public ReviewerByEmailSet getReviewersByEmail() { + return reviewersByEmail; + } + public List reviewerUpdates() throws OrmException { if (reviewerUpdates == null) { if (!lazyLoad) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java index 0604f8bb03..0362c85582 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java @@ -14,9 +14,12 @@ package com.google.gerrit.server.query.change; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.index.FieldDef; import com.google.gerrit.server.index.IndexPredicate; import com.google.gerrit.server.query.Matchable; +import com.google.gerrit.server.query.Predicate; +import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments; public abstract class ChangeIndexPredicate extends IndexPredicate implements Matchable { @@ -27,4 +30,11 @@ public abstract class ChangeIndexPredicate extends IndexPredicate protected ChangeIndexPredicate(FieldDef def, String name, String value) { super(def, name, value); } + + protected static Predicate create(Arguments args, Predicate p) { + if (!args.allowsDrafts) { + return Predicate.and(p, Predicate.not(new ChangeStatusPredicate(Change.Status.DRAFT))); + } + return p; + } } 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 c47bbb57a9..6b66c41879 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 @@ -61,8 +61,10 @@ import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.index.change.ChangeIndex; import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.server.index.change.ChangeIndexRewriter; +import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ListChildProjects; @@ -942,17 +944,12 @@ public class ChangeQueryBuilder extends QueryBuilder { @Operator public Predicate reviewer(String who) throws QueryParseException, OrmException { - return Predicate.or( - parseAccount(who) - .stream() - .map(id -> ReviewerPredicate.reviewer(args, id)) - .collect(toList())); + return reviewerByState(who, ReviewerStateInternal.REVIEWER); } @Operator public Predicate cc(String who) throws QueryParseException, OrmException { - return Predicate.or( - parseAccount(who).stream().map(id -> ReviewerPredicate.cc(args, id)).collect(toList())); + return reviewerByState(who, ReviewerStateInternal.CC); } @Operator @@ -1181,4 +1178,37 @@ public class ChangeQueryBuilder extends QueryBuilder { private Account.Id self() throws QueryParseException { return args.getIdentifiedUser().getAccountId(); } + + public Predicate reviewerByState(String who, ReviewerStateInternal state) + throws QueryParseException, OrmException { + Predicate reviewerByEmailPredicate = null; + if (args.index.getSchema().hasField(ChangeField.REVIEWER_BY_EMAIL)) { + Address address = Address.tryParse(who); + if (address != null) { + reviewerByEmailPredicate = ReviewerByEmailPredicate.forState(args, address, state); + } + } + + Predicate reviewerPredicate = null; + try { + reviewerPredicate = + Predicate.or( + parseAccount(who) + .stream() + .map(id -> ReviewerPredicate.forState(args, id, state)) + .collect(toList())); + } catch (QueryParseException e) { + // Propagate this exception only if we can't use 'who' to query by email + if (reviewerByEmailPredicate == null) { + throw e; + } + } + + if (reviewerPredicate != null && reviewerByEmailPredicate != null) { + return Predicate.or(reviewerPredicate, reviewerByEmailPredicate); + } else if (reviewerPredicate != null) { + return reviewerPredicate; + } + return reviewerByEmailPredicate; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerByEmailPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerByEmailPredicate.java new file mode 100644 index 0000000000..a040e18c3b --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerByEmailPredicate.java @@ -0,0 +1,55 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.query.change; + +import static com.google.common.base.Preconditions.checkArgument; + +import com.google.gerrit.server.index.change.ChangeField; +import com.google.gerrit.server.mail.Address; +import com.google.gerrit.server.notedb.ReviewerStateInternal; +import com.google.gerrit.server.query.Predicate; +import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments; +import com.google.gwtorm.server.OrmException; + +class ReviewerByEmailPredicate extends ChangeIndexPredicate { + + static Predicate forState(Arguments args, Address adr, ReviewerStateInternal state) { + checkArgument(state != ReviewerStateInternal.REMOVED, "can't query by removed reviewer"); + return create(args, new ReviewerByEmailPredicate(state, adr)); + } + + private final ReviewerStateInternal state; + private final Address adr; + + private ReviewerByEmailPredicate(ReviewerStateInternal state, Address adr) { + super(ChangeField.REVIEWER_BY_EMAIL, ChangeField.getReviewerByEmailFieldValue(state, adr)); + this.state = state; + this.adr = adr; + } + + Address getAddress() { + return adr; + } + + @Override + public boolean match(ChangeData cd) throws OrmException { + return cd.reviewersByEmail().asTable().get(state, adr) != null; + } + + @Override + public int getCost() { + return 1; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java index 6ce02fbf30..5b8649476d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java @@ -14,10 +14,10 @@ package com.google.gerrit.server.query.change; +import static com.google.common.base.Preconditions.checkArgument; import static java.util.stream.Collectors.toList; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.query.Predicate; @@ -26,6 +26,12 @@ import com.google.gwtorm.server.OrmException; import java.util.stream.Stream; class ReviewerPredicate extends ChangeIndexPredicate { + static Predicate forState( + Arguments args, Account.Id id, ReviewerStateInternal state) { + checkArgument(state != ReviewerStateInternal.REMOVED, "can't query by removed reviewer"); + return create(args, new ReviewerPredicate(state, id)); + } + static Predicate reviewer(Arguments args, Account.Id id) { Predicate p; if (args.notesMigration.readChanges()) { @@ -54,15 +60,6 @@ class ReviewerPredicate extends ChangeIndexPredicate { .collect(toList())); } - private static Predicate create(Arguments args, Predicate p) { - if (!args.allowsDrafts) { - // TODO(dborowitz): This really belongs much higher up e.g. QueryProcessor. Also, why are we - // even doing this? - return Predicate.and(p, Predicate.not(new ChangeStatusPredicate(Change.Status.DRAFT))); - } - return p; - } - private final ReviewerStateInternal state; private final Account.Id id; 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 2b937fffd0..c2f28eb32f 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 @@ -71,6 +71,8 @@ import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ChangeTriplet; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.index.IndexConfig; import com.google.gerrit.server.index.QueryOptions; @@ -83,6 +85,7 @@ import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.schema.SchemaCreator; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.util.RequestContext; @@ -151,6 +154,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { @Inject protected SchemaCreator schemaCreator; @Inject protected Sequences seq; @Inject protected ThreadLocalRequestContext requestContext; + @Inject protected ProjectCache projectCache; + @Inject protected MetaDataUpdate.Server metaDataUpdateFactory; protected Injector injector; protected LifecycleManager lifecycle; @@ -1544,6 +1549,71 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { } } + @Test + public void reviewerAndCcByEmail() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + Project.NameKey project = new Project.NameKey("repo"); + TestRepository repo = createProject(project.get()); + ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); + cfg.setEnableReviewerByEmail(true); + saveProjectConfig(project, cfg); + + String userByEmail = "un.registered@reviewer.com"; + String userByEmailWithName = "John Doe <" + userByEmail + ">"; + + Change change1 = insert(repo, newChange(repo)); + Change change2 = insert(repo, newChange(repo)); + insert(repo, newChange(repo)); + + AddReviewerInput rin = new AddReviewerInput(); + rin.reviewer = userByEmailWithName; + rin.state = ReviewerState.REVIEWER; + gApi.changes().id(change1.getId().get()).addReviewer(rin); + + rin = new AddReviewerInput(); + rin.reviewer = userByEmailWithName; + rin.state = ReviewerState.CC; + gApi.changes().id(change2.getId().get()).addReviewer(rin); + + assertQuery("reviewer:\"" + userByEmailWithName + "\"", change1); + assertQuery("cc:\"" + userByEmailWithName + "\"", change2); + + // Omitting the name: + assertQuery("reviewer:\"" + userByEmail + "\"", change1); + assertQuery("cc:\"" + userByEmail + "\"", change2); + } + + @Test + public void reviewerAndCcByEmailWithQueryForDifferentUser() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + Project.NameKey project = new Project.NameKey("repo"); + TestRepository repo = createProject(project.get()); + ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); + cfg.setEnableReviewerByEmail(true); + saveProjectConfig(project, cfg); + + String userByEmail = "John Doe "; + + Change change1 = insert(repo, newChange(repo)); + Change change2 = insert(repo, newChange(repo)); + insert(repo, newChange(repo)); + + AddReviewerInput rin = new AddReviewerInput(); + rin.reviewer = userByEmail; + rin.state = ReviewerState.REVIEWER; + gApi.changes().id(change1.getId().get()).addReviewer(rin); + + rin = new AddReviewerInput(); + rin.reviewer = userByEmail; + rin.state = ReviewerState.CC; + gApi.changes().id(change2.getId().get()).addReviewer(rin); + + assertQuery("reviewer:\"someone@example.com\""); + assertQuery("cc:\"someone@example.com\""); + } + @Test public void submitRecords() throws Exception { Account.Id user1 = createAccount("user1"); @@ -2027,4 +2097,12 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { Patch.COMMIT_MSG, ImmutableList.of(comment)); gApi.changes().id(changeId).current().review(input); } + + private void saveProjectConfig(Project.NameKey p, ProjectConfig cfg) throws Exception { + try (MetaDataUpdate md = metaDataUpdateFactory.create(p)) { + md.setAuthor(userFactory.create(userId)); + cfg.commit(md); + } + projectCache.evict(cfg.getProject()); + } }