opendev/gerrit
Code Issues Proposed changes

Store reviewer field in the index

Browse Source 
Prior to this change, draft changes in search results may have
resulted in touching the database, since ChangeControl#isDraftVisible
checks if the user is a reviewer, and there are not enough stored
fields to compute the reviewer set from the index. (We store approvals
on the current patch set, not all approvals; even the latter would be
insufficient with NoteDb.)

Add a new reviewer field that stores enough information to reconstruct
the whole ReviewerSet, and also to search by reviewer state. Shove
this all in one repeated field with a little comma-separated format,
similar to what we already do for labels. The current
ReviewerPredicate implementation matches the old behavior of returning
results independent of state, although it's now expressed as a
predicate tree.

Change-Id: Ie54b9e2decf847185ec741bc50bae7eb680787a0
This commit is contained in:
Dave Borowitz 2016-06-02 17:50:00 -04:00
parent 3b8ff49c6e
commit 4315f01324
12 changed files with 282 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@ -988,6 +988,7 @@ public class ChangeIT extends AbstractDaemonTest {
@Test
public void defaultSearchDoesNotTouchDatabase() throws Exception {
setApiUser(admin);
PushOneCommit.Result r1 = createChange();
gApi.changes()
.id(r1.getChangeId())
@ -999,8 +1000,9 @@ public class ChangeIT extends AbstractDaemonTest {
.submit();
createChange();
createDraftChange();
setApiUserAnonymous(); // Identified user may async get stars from DB.
setApiUser(user);
AcceptanceTestRequestScope.Context ctx = disableDb();
try {
assertThat(gApi.changes().query()

19
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
View File

@ -24,7 +24,9 @@ import static com.google.gerrit.server.index.change.ChangeField.PROJECT;
import static com.google.gerrit.server.index.change.IndexRewriter.CLOSED_STATUSES;
import static com.google.gerrit.server.index.change.IndexRewriter.OPEN_STATUSES;
import com.google.common.base.Function;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Multimap;
@ -117,6 +119,7 @@ public class LuceneChangeIndex implements ChangeIndex {
private static final String PATCH_SET_FIELD = ChangeField.PATCH_SET.getName();
private static final String REVIEWEDBY_FIELD =
ChangeField.REVIEWEDBY.getName();
private static final String REVIEWER_FIELD = ChangeField.REVIEWER.getName();
private static final String HASHTAG_FIELD =
ChangeField.HASHTAG_CASE_AWARE.getName();
private static final String STAR_FIELD = ChangeField.STAR.getName();
@ -426,6 +429,9 @@ public class LuceneChangeIndex implements ChangeIndex {
if (fields.contains(STAR_FIELD)) {
decodeStar(doc, cd);
}
if (fields.contains(REVIEWER_FIELD)) {
decodeReviewers(doc, cd);
}
return cd;
}
@ -512,6 +518,19 @@ public class LuceneChangeIndex implements ChangeIndex {
cd.setStars(stars);
}
private void decodeReviewers(Document doc, ChangeData cd) {
cd.setReviewers(
ChangeField.parseReviewerFieldValues(
FluentIterable.of(doc.getFields(REVIEWER_FIELD))
.transform(
new Function<IndexableField, String>() {
@Override
public String apply(IndexableField in) {
return in.stringValue();
}
})));
}
private static <T> List<T> decodeProtos(Document doc, String fieldName,
ProtobufCodec<T> codec) {
BytesRef[] bytesRefs = doc.getBinaryValues(fieldName);

56
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java
View File

@ -17,13 +17,16 @@ package com.google.gerrit.server.index.change;
import static com.google.common.base.MoreObjects.firstNonNull;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.base.Splitter;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.common.collect.Table;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@ -31,10 +34,12 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.index.FieldDef;
import com.google.gerrit.server.index.FieldType;
import com.google.gerrit.server.index.SchemaUtil;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
@ -295,7 +300,8 @@ public class ChangeField {
};
/** Reviewer(s) associated with the change. */
public static final FieldDef<ChangeData, Iterable<Integer>> REVIEWER =
@Deprecated
public static final FieldDef<ChangeData, Iterable<Integer>> LEGACY_REVIEWER =
new FieldDef.Repeatable<ChangeData, Integer>(
ChangeQueryBuilder.FIELD_REVIEWER, FieldType.INTEGER, false) {
@Override
@ -316,6 +322,54 @@ public class ChangeField {
}
};
/** Reviewer(s) associated with the change. */
public static final FieldDef<ChangeData, Iterable<String>> REVIEWER =
new FieldDef.Repeatable<ChangeData, String>(
"reviewer2", FieldType.EXACT, true) {
@Override
public Iterable<String> get(ChangeData input, FillArgs args)
throws OrmException {
return getReviewerFieldValues(input.reviewers());
}
};
@VisibleForTesting
static List<String> getReviewerFieldValues(ReviewerSet reviewers) {
List<String> r = new ArrayList<>(reviewers.asTable().size() * 2);
for (Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> c
: reviewers.asTable().cellSet()) {
String v = getReviewerFieldValue(c.getRowKey(), c.getColumnKey());
r.add(v);
r.add(v + ',' + c.getValue().getTime());
}
return r;
}
public static String getReviewerFieldValue(ReviewerStateInternal state,
Account.Id id) {
return state.toString() + ',' + id;
}
public static ReviewerSet parseReviewerFieldValues(Iterable<String> values) {
Table<ReviewerStateInternal, Account.Id, Timestamp> table =
HashBasedTable.create();
for (String v : values) {
int f = v.indexOf(',');
if (f < 0) {
continue;
}
int l = v.lastIndexOf(',');
if (l == f) {
continue;
}
table.put(
ReviewerStateInternal.valueOf(v.substring(0, f)),
Account.Id.parse(v.substring(f + 1, l)),
new Timestamp(Long.valueOf(v.substring(l + 1, v.length()))));
}
return ReviewerSet.fromTable(table);
}
/** Commit ID of any patch set on the change, using prefix match. */
public static final FieldDef<ChangeData, Iterable<String>> COMMIT =
new FieldDef.Repeatable<ChangeData, String>(

11
gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
View File

@ -35,7 +35,7 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
ChangeField.FILE_PART,
ChangeField.PATH,
ChangeField.OWNER,
ChangeField.REVIEWER,
ChangeField.LEGACY_REVIEWER,
ChangeField.COMMIT,
ChangeField.TR,
ChangeField.LABEL,
@ -75,12 +75,19 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
static final Schema<ChangeData> V30 =
schema(V29, ChangeField.STAR, ChangeField.STARBY);
@SuppressWarnings("deprecation")
@Deprecated
static final Schema<ChangeData> V31 = new Schema.Builder<ChangeData>()
.add(V30)
.remove(ChangeField.STARREDBY)
.build();
@SuppressWarnings("deprecation")
static final Schema<ChangeData> V32 = new Schema.Builder<ChangeData>()
.add(V31)
.remove(ChangeField.LEGACY_REVIEWER)
.add(ChangeField.REVIEWER)
.build();
public static final String NAME = "changes";
public static final ChangeSchemaDefinitions INSTANCE =
new ChangeSchemaDefinitions();

8
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@ -195,8 +195,14 @@ public class ChangeControl {
/** Can this user see this change? */
public boolean isVisible(ReviewDb db) throws OrmException {
return isVisible(db, null);
}
/** Can this user see this change? */
public boolean isVisible(ReviewDb db, @Nullable ChangeData cd)
throws OrmException {
if (getChange().getStatus() == Change.Status.DRAFT
&& !isDraftVisible(db, null)) {
&& !isDraftVisible(db, cd)) {
return false;
}
return isRefVisible();

10
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@ -349,6 +349,7 @@ public class ChangeData {
@Deprecated
private Set<Account.Id> starredByUser;
private ImmutableMultimap<Account.Id, String> stars;
private ReviewerSet reviewers;
private PersonIdent author;
private PersonIdent committer;
@ -905,7 +906,14 @@ public void setPatchSets(Collection<PatchSet> patchSets) {
}
public ReviewerSet reviewers() throws OrmException {
return approvalsUtil.getReviewers(notes(), approvals().values());
if (reviewers == null) {
reviewers = approvalsUtil.getReviewers(notes(), approvals().values());
}
return reviewers;
}
public void setReviewers(ReviewerSet reviewers) {
this.reviewers = reviewers;
}
public Collection<PatchLineComment> publishedComments()

6
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@ -464,7 +464,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
}
if ("reviewer".equalsIgnoreCase(value)) {
return new ReviewerPredicate(self(), args.allowsDrafts);
return ReviewerPredicate.create(args, self());
}
if ("mergeable".equalsIgnoreCase(value)) {
@ -814,9 +814,9 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public Predicate<ChangeData> reviewer(String who)
throws QueryParseException, OrmException {
Set<Account.Id> m = parseAccount(who);
List<ReviewerPredicate> p = Lists.newArrayListWithCapacity(m.size());
List<Predicate<ChangeData>> p = Lists.newArrayListWithCapacity(m.size());
for (Account.Id id : m) {
p.add(new ReviewerPredicate(id, args.allowsDrafts));
p.add(ReviewerPredicate.create(args, id));
}
return Predicate.or(p);
}

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsVisibleToPredicate.java
View File

@ -63,7 +63,7 @@ class IsVisibleToPredicate extends OperatorPredicate<ChangeData> {
ChangeNotes notes = notesFactory.createFromIndexedChange(c);
ChangeControl cc = changeControl.controlFor(notes, user);
if (cc.isVisible(db.get())) {
if (cc.isVisible(db.get(), cd)) {
cd.cacheVisibleTo(cc);
return true;
}

44
gerrit-server/src/main/java/com/google/gerrit/server/query/change/LegacyReviewerPredicate.java Normal file
View File

@ -0,0 +1,44 @@
// Copyright (C) 2016 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 com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.index.IndexPredicate;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gwtorm.server.OrmException;
@Deprecated
class LegacyReviewerPredicate extends IndexPredicate<ChangeData> {
private final Account.Id id;
LegacyReviewerPredicate(Account.Id id) {
super(ChangeField.LEGACY_REVIEWER, id.toString());
this.id = id;
}
Account.Id getAccountId() {
return id;
}
@Override
public boolean match(ChangeData object) throws OrmException {
return object.reviewers().all().contains(id);
}
@Override
public int getCost() {
return 1;
}
}

56
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java
View File

@ -18,16 +18,45 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.index.IndexPredicate;
import com.google.gerrit.server.index.change.ChangeField;
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 ReviewerPredicate extends IndexPredicate<ChangeData> {
private final Account.Id id;
private boolean allowDrafts;
import java.util.ArrayList;
import java.util.List;
ReviewerPredicate(Account.Id id, boolean allowDrafts) {
super(ChangeField.REVIEWER, id.toString());
class ReviewerPredicate extends IndexPredicate<ChangeData> {
@SuppressWarnings("deprecation")
static Predicate<ChangeData> create(Arguments args, Account.Id id) {
List<Predicate<ChangeData>> and = new ArrayList<>(2);
if (args.getSchema().hasField(ChangeField.REVIEWER)) {
ReviewerStateInternal[] states = ReviewerStateInternal.values();
List<Predicate<ChangeData>> or = new ArrayList<>(states.length - 1);
for (ReviewerStateInternal state : states) {
if (state != ReviewerStateInternal.REMOVED) {
or.add(new ReviewerPredicate(state, id));
}
}
and.add(Predicate.or(or));
} else {
and.add(new LegacyReviewerPredicate(id));
}
// TODO(dborowitz): This really belongs much higher up e.g. QueryProcessor.
if (!args.allowsDrafts) {
and.add(Predicate.not(new ChangeStatusPredicate(Change.Status.DRAFT)));
}
return Predicate.and(and);
}
private final ReviewerStateInternal state;
private final Account.Id id;
ReviewerPredicate(ReviewerStateInternal state, Account.Id id) {
super(ChangeField.REVIEWER, ChangeField.getReviewerFieldValue(state, id));
this.state = state;
this.id = id;
this.allowDrafts = allowDrafts;
}
Account.Id getAccountId() {
@ -35,21 +64,12 @@ class ReviewerPredicate extends IndexPredicate<ChangeData> {
}
@Override
public boolean match(final ChangeData object) throws OrmException {
if (!allowDrafts &&
object.change().getStatus() == Change.Status.DRAFT) {
return false;
}
for (Account.Id accountId : object.reviewers().all()) {
if (id.equals(accountId)) {
return true;
}
}
return false;
public boolean match(ChangeData cd) throws OrmException {
return cd.reviewers().asTable().get(state, id) != null;
}
@Override
public int getCost() {
return 2;
return 1;
}
}

73
gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeFieldTest.java Normal file
View File

@ -0,0 +1,73 @@
// Copyright (C) 2016 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.index.change;
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.Table;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.testutil.GerritBaseTests;
import com.google.gerrit.testutil.TestTimeUtil;
import com.google.gwtorm.client.KeyUtil;
import com.google.gwtorm.server.StandardKeyEncoder;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import java.sql.Timestamp;
import java.util.List;
import java.util.concurrent.TimeUnit;
public class ChangeFieldTest extends GerritBaseTests {
static {
KeyUtil.setEncoderImpl(new StandardKeyEncoder());
}
@Before
public void setUp() {
TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
}
@After
public void tearDown() {
TestTimeUtil.useSystemTime();
}
@Test
public void reviewerFieldValues() {
Table<ReviewerStateInternal, Account.Id, Timestamp> t =
HashBasedTable.create();
Timestamp t1 = TimeUtil.nowTs();
t.put(ReviewerStateInternal.REVIEWER, new Account.Id(1), t1);
Timestamp t2 = TimeUtil.nowTs();
t.put(ReviewerStateInternal.CC, new Account.Id(2), t2);
ReviewerSet reviewers = ReviewerSet.fromTable(t);
List<String> values = ChangeField.getReviewerFieldValues(reviewers);
assertThat(values).containsExactly(
"REVIEWER,1",
"REVIEWER,1," + t1.getTime(),
"CC,2",
"CC,2," + t2.getTime());
assertThat(ChangeField.parseReviewerFieldValues(values))
.isEqualTo(reviewers);
}
}

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

@ -1397,6 +1397,26 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
assertThat(actual.get(1).reviewed).isTrue();
}
@Test
public void reviewer() throws Exception {
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChange(repo));
Change change2 = insert(repo, newChange(repo));
insert(repo, newChange(repo));
gApi.changes()
.id(change1.getId().get())
.current()
.review(ReviewInput.approve());
gApi.changes()
.id(change2.getId().get())
.current()
.review(ReviewInput.approve());
Account.Id id = user.getAccountId();
assertQuery("reviewer:" + id, change2, change1);
}
@Test
public void byCommitsOnBranchNotMerged() throws Exception {
TestRepository<Repo> repo = createProject("repo");
@ -1451,6 +1471,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
cd.currentApprovals();
cd.changedLines();
cd.reviewedBy();
cd.reviewers();
// TODO(dborowitz): Swap out GitRepositoryManager somehow? Will probably be
// necessary for NoteDb anyway.