Merge "Add reviewers by email to ChangeIndex"

This commit is contained in:
ekempin
2017-03-27 10:58:12 +00:00
committed by Gerrit Code Review
13 changed files with 312 additions and 24 deletions

View File

@@ -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 <foo.bar@gerritcodereview.com>");
}
@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();
}

View File

@@ -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<Change.Id, ChangeData>
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(),

View File

@@ -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<String, IndexableField> doc, ChangeData cd) {
cd.setReviewersByEmail(
ChangeField.parseReviewerByEmailFieldValues(
FluentIterable.from(doc.get(REVIEWER_BY_EMAIL_FIELD))
.transform(IndexableField::stringValue)));
}
private void decodeSubmitRecords(
ListMultimap<String, IndexableField> doc,
String field,

View File

@@ -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<ReviewerStateInternal, Map<Address, Timestamp>> e :
cd.notes().getReviewersByEmail().asTable().rowMap().entrySet()) {
cd.reviewersByEmail().asTable().rowMap().entrySet()) {
out.reviewers.put(
e.getKey().asReviewerState(), toAccountInfoByEmail(e.getValue().keySet()));
}

View File

@@ -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<ChangeData, Iterable<String>> 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<ChangeData, Iterable<String>> REVIEWER_BY_EMAIL =
exact("reviewer_by_email")
.stored()
.buildRepeatable(cd -> getReviewerByEmailFieldValues(cd.reviewersByEmail()));
@VisibleForTesting
static List<String> getReviewerFieldValues(ReviewerSet reviewers) {
List<String> r = new ArrayList<>(reviewers.asTable().size() * 2);
@@ -200,6 +208,27 @@ public class ChangeField {
return state.toString() + ',' + id;
}
@VisibleForTesting
static List<String> getReviewerByEmailFieldValues(ReviewerByEmailSet reviewersByEmail) {
List<String> r = new ArrayList<>(reviewersByEmail.asTable().size() * 2);
for (Table.Cell<ReviewerStateInternal, Address, Timestamp> 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<String> values) {
ImmutableTable.Builder<ReviewerStateInternal, Account.Id, Timestamp> b =
ImmutableTable.builder();
@@ -220,6 +249,25 @@ public class ChangeField {
return ReviewerSet.fromTable(b.build());
}
public static ReviewerByEmailSet parseReviewerByEmailFieldValues(Iterable<String> values) {
ImmutableTable.Builder<ReviewerStateInternal, Address, Timestamp> 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<ChangeData, Iterable<String>> COMMIT =
prefix(ChangeQueryBuilder.FIELD_COMMIT).buildRepeatable(ChangeField::getRevisions);

View File

@@ -92,7 +92,9 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
@Deprecated static final Schema<ChangeData> V39 = schema(V38);
static final Schema<ChangeData> V40 = schema(V39, ChangeField.PRIVATE);
@Deprecated static final Schema<ChangeData> V40 = schema(V39, ChangeField.PRIVATE);
static final Schema<ChangeData> V41 = schema(V40, ChangeField.REVIEWER_BY_EMAIL);
public static final String NAME = "changes";
public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions();

View File

@@ -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);
}

View File

@@ -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<Account.Id, StarRef> starRefs;
private ReviewerSet reviewers;
private ReviewerByEmailSet reviewersByEmail;
private List<ReviewerStatusUpdate> 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<ReviewerStatusUpdate> reviewerUpdates() throws OrmException {
if (reviewerUpdates == null) {
if (!lazyLoad) {

View File

@@ -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<ChangeData>
implements Matchable<ChangeData> {
@@ -27,4 +30,11 @@ public abstract class ChangeIndexPredicate extends IndexPredicate<ChangeData>
protected ChangeIndexPredicate(FieldDef<ChangeData, ?> def, String name, String value) {
super(def, name, value);
}
protected static Predicate<ChangeData> create(Arguments args, Predicate<ChangeData> p) {
if (!args.allowsDrafts) {
return Predicate.and(p, Predicate.not(new ChangeStatusPredicate(Change.Status.DRAFT)));
}
return p;
}
}

View File

@@ -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<ChangeData> {
@Operator
public Predicate<ChangeData> 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<ChangeData> 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<ChangeData> {
private Account.Id self() throws QueryParseException {
return args.getIdentifiedUser().getAccountId();
}
public Predicate<ChangeData> reviewerByState(String who, ReviewerStateInternal state)
throws QueryParseException, OrmException {
Predicate<ChangeData> 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<ChangeData> 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;
}
}

View File

@@ -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<ChangeData> 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;
}
}

View File

@@ -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<ChangeData> 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<ChangeData> reviewer(Arguments args, Account.Id id) {
Predicate<ChangeData> p;
if (args.notesMigration.readChanges()) {
@@ -54,15 +60,6 @@ class ReviewerPredicate extends ChangeIndexPredicate {
.collect(toList()));
}
private static Predicate<ChangeData> create(Arguments args, Predicate<ChangeData> 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;

View File

@@ -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> 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> repo = createProject(project.get());
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
cfg.setEnableReviewerByEmail(true);
saveProjectConfig(project, cfg);
String userByEmail = "John Doe <un.registered@reviewer.com>";
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.<ReviewInput.CommentInput>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());
}
}