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 5c3b6294f9..c05c8f0459 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 @@ -131,7 +131,6 @@ public class LuceneChangeIndex implements ChangeIndex { private static final String APPROVAL_FIELD = ChangeField.APPROVAL.getName(); private static final String CHANGE_FIELD = ChangeField.CHANGE.getName(); private static final String DELETED_FIELD = ChangeField.DELETED.getName(); - private static final String ID_FIELD = ChangeField.LEGACY_ID2.getName(); private static final String MERGEABLE_FIELD = ChangeField.MERGEABLE.getName(); private static final String PATCH_SET_FIELD = ChangeField.PATCH_SET.getName(); private static final String REVIEWEDBY_FIELD = @@ -139,10 +138,6 @@ public class LuceneChangeIndex implements ChangeIndex { private static final String UPDATED_SORT_FIELD = sortFieldName(ChangeField.UPDATED); - private static final ImmutableSet FIELDS = ImmutableSet.of( - ADDED_FIELD, APPROVAL_FIELD, CHANGE_FIELD, DELETED_FIELD, ID_FIELD, - MERGEABLE_FIELD, PATCH_SET_FIELD, REVIEWEDBY_FIELD); - private static final Map CUSTOM_CHAR_MAPPING = ImmutableMap.of( "_", " ", ".", " "); @@ -438,10 +433,12 @@ public class LuceneChangeIndex implements ChangeIndex { List result = Lists.newArrayListWithCapacity(docs.scoreDocs.length); + Set fields = fields(opts); + String idFieldName = idFieldName(); for (int i = opts.start(); i < docs.scoreDocs.length; i++) { ScoreDoc sd = docs.scoreDocs[i]; - Document doc = searchers[sd.shardIndex].doc(sd.doc, FIELDS); - result.add(toChangeData(doc)); + Document doc = searchers[sd.shardIndex].doc(sd.doc, fields); + result.add(toChangeData(doc, fields, idFieldName)); } final List r = Collections.unmodifiableList(result); @@ -477,19 +474,62 @@ public class LuceneChangeIndex implements ChangeIndex { } } - private ChangeData toChangeData(Document doc) { + @SuppressWarnings("deprecation") + private Set fields(QueryOptions opts) { + if (schemaHasRequestedField(ChangeField.LEGACY_ID2, opts.fields()) + || schemaHasRequestedField(ChangeField.CHANGE, opts.fields()) + || schemaHasRequestedField(ChangeField.LEGACY_ID, opts.fields())) { + return opts.fields(); + } + // Request the numeric ID field even if the caller did not request it, + // otherwise we can't actually construct a ChangeData. + return Sets.union(opts.fields(), ImmutableSet.of(idFieldName())); + } + + private boolean schemaHasRequestedField(FieldDef field, + Set requested) { + return schema.hasField(field) && requested.contains(field.getName()); + } + + @SuppressWarnings("deprecation") + private String idFieldName() { + return schema.getField(ChangeField.LEGACY_ID2, ChangeField.LEGACY_ID).get() + .getName(); + } + + private ChangeData toChangeData(Document doc, Set fields, + String idFieldName) { + ChangeData cd; + // Either change or the ID field was guaranteed to be included in the call + // to fields() above. BytesRef cb = doc.getBinaryValue(CHANGE_FIELD); - if (cb == null) { - int id = doc.getField(ID_FIELD).numericValue().intValue(); - return changeDataFactory.create(db.get(), new Change.Id(id)); + if (cb != null) { + cd = changeDataFactory.create(db.get(), + ChangeProtoField.CODEC.decode(cb.bytes, cb.offset, cb.length)); + } else { + int id = doc.getField(idFieldName).numericValue().intValue(); + cd = changeDataFactory.create(db.get(), new Change.Id(id)); } - // Change proto. - Change change = ChangeProtoField.CODEC.decode( - cb.bytes, cb.offset, cb.length); - ChangeData cd = changeDataFactory.create(db.get(), change); + if (fields.contains(PATCH_SET_FIELD)) { + decodePatchSets(doc, cd); + } + if (fields.contains(APPROVAL_FIELD)) { + decodeApprovals(doc, cd); + } + if (fields.contains(ADDED_FIELD) && fields.contains(DELETED_FIELD)) { + decodeChangedLines(doc, cd); + } + if (fields.contains(MERGEABLE_FIELD)) { + decodeMergeable(doc, cd); + } + if (fields.contains(REVIEWEDBY_FIELD)) { + decodeReviewedBy(doc, cd); + } + return cd; + } - // Patch sets. + private void decodePatchSets(Document doc, ChangeData cd) { List patchSets = decodeProtos(doc, PATCH_SET_FIELD, PatchSetProtoField.CODEC); if (!patchSets.isEmpty()) { @@ -497,12 +537,14 @@ public class LuceneChangeIndex implements ChangeIndex { // this cannot be valid since a change needs at least one patch set. cd.setPatchSets(patchSets); } + } - // Approvals. + private void decodeApprovals(Document doc, ChangeData cd) { cd.setCurrentApprovals( decodeProtos(doc, APPROVAL_FIELD, PatchSetApprovalProtoField.CODEC)); + } - // Changed lines. + private void decodeChangedLines(Document doc, ChangeData cd) { IndexableField added = doc.getField(ADDED_FIELD); IndexableField deleted = doc.getField(DELETED_FIELD); if (added != null && deleted != null) { @@ -510,16 +552,18 @@ public class LuceneChangeIndex implements ChangeIndex { added.numericValue().intValue(), deleted.numericValue().intValue()); } + } - // Mergeable. + private void decodeMergeable(Document doc, ChangeData cd) { String mergeable = doc.get(MERGEABLE_FIELD); if ("1".equals(mergeable)) { cd.setMergeable(true); } else if ("0".equals(mergeable)) { cd.setMergeable(false); } + } - // Reviewed-by. + private void decodeReviewedBy(Document doc, ChangeData cd) { IndexableField[] reviewedBy = doc.getFields(REVIEWEDBY_FIELD); if (reviewedBy.length > 0) { Set accounts = @@ -533,8 +577,6 @@ public class LuceneChangeIndex implements ChangeIndex { } cd.setReviewedBy(accounts); } - - return cd; } private static List decodeProtos(Document doc, String fieldName, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java index 683f8cfe6d..28fa9f80fd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java @@ -50,7 +50,7 @@ public class IndexedChangeQuery extends Predicate int backendLimit = opts.config().maxLimit(); int limit = Ints.saturatedCast((long) opts.limit() + opts.start()); limit = Math.min(limit, backendLimit); - return QueryOptions.create(opts.config(), 0, limit); + return QueryOptions.create(opts.config(), 0, limit, opts.fields()); } private final ChangeIndex index; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java index df70292bf9..a605461d44 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java @@ -61,6 +61,8 @@ public class Schema { } private final ImmutableMap> fields; + private final ImmutableMap> storedFields; + private int version; protected Schema(Iterable> fields) { @@ -71,10 +73,15 @@ public class Schema { public Schema(int version, Iterable> fields) { this.version = version; ImmutableMap.Builder> b = ImmutableMap.builder(); + ImmutableMap.Builder> sb = ImmutableMap.builder(); for (FieldDef f : fields) { b.put(f.getName(), f); + if (f.isStored()) { + sb.put(f.getName(), f); + } } this.fields = b.build(); + this.storedFields = sb.build(); } public final int getVersion() { @@ -94,6 +101,14 @@ public class Schema { return fields; } + /** + * @return all fields in this schema where {@link FieldDef#isStored()} is + * true. + */ + public final ImmutableMap> getStoredFields() { + return storedFields; + } + /** * Look up fields in this schema. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java index 1964fa56b4..a70f892b02 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java @@ -17,29 +17,36 @@ package com.google.gerrit.server.query.change; import static com.google.common.base.Preconditions.checkArgument; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.server.index.IndexConfig; +import java.util.Set; + @AutoValue public abstract class QueryOptions { - public static QueryOptions create(IndexConfig config, int start, int limit) { + public static QueryOptions create(IndexConfig config, int start, int limit, + Set fields) { checkArgument(start >= 0, "start must be nonnegative: %s", start); checkArgument(limit > 0, "limit must be positive: %s", limit); - return new AutoValue_QueryOptions(config, start, limit); + return new AutoValue_QueryOptions(config, start, limit, + ImmutableSet.copyOf(fields)); } public static QueryOptions oneResult() { - return create(IndexConfig.createDefault(), 0, 1); + return create(IndexConfig.createDefault(), 0, 1, + ImmutableSet. of()); } public abstract IndexConfig config(); public abstract int start(); public abstract int limit(); + public abstract ImmutableSet fields(); public QueryOptions withLimit(int newLimit) { - return create(config(), start(), newLimit); + return create(config(), start(), newLimit, fields()); } public QueryOptions withStart(int newStart) { - return create(config(), newStart, limit()); + return create(config(), newStart, limit(), fields()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java index c3c70b379d..1a6ae02a0f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Ordering; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.metrics.Description; @@ -25,6 +26,8 @@ import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.metrics.Timer0; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.index.ChangeIndex; +import com.google.gerrit.server.index.IndexCollection; import com.google.gerrit.server.index.IndexConfig; import com.google.gerrit.server.index.IndexPredicate; import com.google.gerrit.server.index.IndexRewriter; @@ -39,11 +42,13 @@ import com.google.inject.Singleton; import java.util.ArrayList; import java.util.List; +import java.util.Set; public class QueryProcessor { private final Provider db; private final Provider userProvider; private final ChangeControl.GenericFactory changeControlFactory; + private final IndexCollection indexes; private final IndexRewriter rewriter; private final IndexConfig indexConfig; private final Metrics metrics; @@ -51,17 +56,20 @@ public class QueryProcessor { private int limitFromCaller; private int start; private boolean enforceVisibility = true; + private Set requestedFields; @Inject QueryProcessor(Provider db, Provider userProvider, ChangeControl.GenericFactory changeControlFactory, + IndexCollection indexes, IndexRewriter rewriter, IndexConfig indexConfig, Metrics metrics) { this.db = db; this.userProvider = userProvider; this.changeControlFactory = changeControlFactory; + this.indexes = indexes; this.rewriter = rewriter; this.indexConfig = indexConfig; this.metrics = metrics; @@ -82,6 +90,11 @@ public class QueryProcessor { return this; } + public QueryProcessor setRequestedFields(Set fields) { + requestedFields = fields; + return this; + } + /** * Query for changes that match a structured query. * @@ -150,7 +163,8 @@ public class QueryProcessor { "Cannot go beyond page " + indexConfig.maxPages() + "of results"); } - QueryOptions opts = QueryOptions.create(indexConfig, start, limit + 1); + QueryOptions opts = QueryOptions.create( + indexConfig, start, limit + 1, getRequestedFields()); Predicate s = rewriter.rewrite(q, opts); if (!(s instanceof ChangeDataSource)) { q = Predicate.and(open(), q); @@ -184,6 +198,16 @@ public class QueryProcessor { return out; } + private Set getRequestedFields() { + if (requestedFields != null) { + return requestedFields; + } + ChangeIndex index = indexes.getSearchIndex(); + return index != null + ? index.getSchema().getStoredFields().keySet() + : ImmutableSet. of(); + } + boolean isDisabled() { return getPermittedLimit() <= 0; } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java index 7b1185e09f..f44d1723a8 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java @@ -25,6 +25,7 @@ import static com.google.gerrit.server.query.Predicate.and; import static com.google.gerrit.server.query.Predicate.or; import static org.junit.Assert.assertEquals; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.query.AndPredicate; import com.google.gerrit.server.query.Predicate; @@ -285,7 +286,8 @@ public class IndexRewriterTest extends GerritBaseTests { } private static QueryOptions options(int start, int limit) { - return QueryOptions.create(CONFIG, start, limit); + return QueryOptions.create(CONFIG, start, limit, + ImmutableSet. of()); } private Set status(String query) throws QueryParseException { 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 dd2667f5ce..49e1796c80 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 @@ -56,6 +56,7 @@ import com.google.gerrit.server.change.ChangeTriplet; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.validators.CommitValidators; +import com.google.gerrit.server.index.ChangeField; import com.google.gerrit.server.index.IndexCollection; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; @@ -1263,6 +1264,30 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { cd.messages(); } + @Test + public void prepopulateOnlyRequestedFields() throws Exception { + assume().that(notesMigration.enabled()).isFalse(); + TestRepository repo = createProject("repo"); + Change change = insert(newChange(repo, null, null, null, null)); + + db = new DisabledReviewDb(); + requestContext.setContext(newRequestContext(userId)); + // Use QueryProcessor directly instead of API so we get ChangeDatas back. + List cds = queryProcessor + .setRequestedFields(ImmutableSet.of( + ChangeField.PATCH_SET.getName(), + ChangeField.CHANGE.getName())) + .queryChanges(queryBuilder.parse(change.getId().toString())) + .changes(); + assertThat(cds).hasSize(1); + + ChangeData cd = cds.get(0); + cd.change(); + cd.patchSets(); + + exception.expect(DisabledReviewDb.Disabled.class); + cd.currentApprovals(); + } protected ChangeInserter newChange( TestRepository repo, diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java index 7e7899b50c..53a9805bf0 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java @@ -78,6 +78,13 @@ public class LuceneQueryChangesV14Test extends LuceneQueryChangesTest { // Ignore. } + @Override + @Ignore + @Test + public void prepopulateOnlyRequestedFields() throws Exception { + // Ignore. + } + @Test public void isReviewed() throws Exception { clockStepMs = MILLISECONDS.convert(2, MINUTES);