From aa332ab1c22b38d08cd4111757f2dcc23b728b71 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 21 Jun 2016 14:53:48 +0200 Subject: [PATCH] Add AccountQueryProcessor This change extracts the generic parts from (Change)QueryProcessor into a base class that can also be used as base for AccountQueryProcessor. Change-Id: Iaa76a69ff938f3ed4155add22fc49c03a878f28b Signed-off-by: Edwin Kempin --- .../gerrit/server/change/AbandonUtil.java | 8 +- .../gerrit/server/change/ChangeJson.java | 20 +- .../index/change/ChangeIndexRewriter.java | 16 ++ .../gerrit/server/query/QueryProcessor.java | 239 +++++++++++++++ .../query/{change => }/QueryResult.java | 31 +- .../query/account/AccountQueryProcessor.java | 33 +++ .../query/change/ChangeQueryProcessor.java | 103 +++++++ .../query/change/InternalChangeQuery.java | 8 +- .../query/change/OutputStreamQuery.java | 15 +- .../server/query/change/QueryChanges.java | 9 +- .../server/query/change/QueryProcessor.java | 271 ------------------ .../index/change/ChangeIndexRewriterTest.java | 13 +- .../change/AbstractQueryChangesTest.java | 10 +- 13 files changed, 453 insertions(+), 323 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java rename gerrit-server/src/main/java/com/google/gerrit/server/query/{change => }/QueryResult.java (62%) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java index efb2d4d44d..f84599d85d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java @@ -21,7 +21,7 @@ import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.query.QueryParseException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeQueryBuilder; -import com.google.gerrit.server.query.change.QueryProcessor; +import com.google.gerrit.server.query.change.ChangeQueryProcessor; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -38,7 +38,7 @@ public class AbandonUtil { private final ChangeCleanupConfig cfg; private final InternalUser.Factory internalUserFactory; - private final QueryProcessor queryProcessor; + private final ChangeQueryProcessor queryProcessor; private final ChangeQueryBuilder queryBuilder; private final Abandon abandon; @@ -46,7 +46,7 @@ public class AbandonUtil { AbandonUtil( ChangeCleanupConfig cfg, InternalUser.Factory internalUserFactory, - QueryProcessor queryProcessor, + ChangeQueryProcessor queryProcessor, ChangeQueryBuilder queryBuilder, Abandon abandon) { this.cfg = cfg; @@ -69,7 +69,7 @@ public class AbandonUtil { query += " -is:mergeable"; } List changesToAbandon = queryProcessor.enforceVisibility(false) - .queryChanges(queryBuilder.parse(query)).changes(); + .query(queryBuilder.parse(query)).entities(); int count = 0; for (ChangeData cd : changesToAbandon) { try { 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 5665615fc3..322d4551bd 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 @@ -104,9 +104,9 @@ import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.SubmitRuleEvaluator; +import com.google.gerrit.server.query.QueryResult; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData.ChangedLines; -import com.google.gerrit.server.query.change.QueryResult; import com.google.gwtorm.server.OrmException; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; @@ -273,22 +273,22 @@ public class ChangeJson { return format(cd, Optional.of(rsrc.getPatchSet().getId()), true); } - public List> formatQueryResults(List in) - throws OrmException { + public List> formatQueryResults( + List> in) throws OrmException { accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); - ensureLoaded(FluentIterable.from(in) - .transformAndConcat(new Function>() { + ensureLoaded(FluentIterable.from(in).transformAndConcat( + new Function, List>() { @Override - public List apply(QueryResult in) { - return in.changes(); + public List apply(QueryResult in) { + return in.entities(); } })); List> res = Lists.newArrayListWithCapacity(in.size()); Map out = new HashMap<>(); - for (QueryResult r : in) { - List infos = toChangeInfo(out, r.changes()); - if (!infos.isEmpty() && r.moreChanges()) { + for (QueryResult r : in) { + List infos = toChangeInfo(out, r.entities()); + if (!infos.isEmpty() && r.more()) { infos.get(infos.size() - 1)._moreChanges = true; } res.add(infos); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java index 71c575dcd4..1fff70d53a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.index.change; +import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open; + import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.gerrit.reviewdb.client.Change; @@ -30,6 +32,7 @@ import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.QueryParseException; import com.google.gerrit.server.query.change.AndSource; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.ChangeDataSource; import com.google.gerrit.server.query.change.ChangeQueryBuilder; import com.google.gerrit.server.query.change.ChangeStatusPredicate; import com.google.gerrit.server.query.change.OrSource; @@ -135,6 +138,19 @@ public class ChangeIndexRewriter implements IndexRewriter { @Override public Predicate rewrite(Predicate in, QueryOptions opts) throws QueryParseException { + Predicate s = rewriteImpl(in, opts); + if (!(s instanceof ChangeDataSource)) { + in = Predicate.and(open(), in); + s = rewriteImpl(in, opts); + } + if (!(s instanceof ChangeDataSource)) { + throw new QueryParseException("invalid query: " + s); + } + return s; + } + + private Predicate rewriteImpl(Predicate in, + QueryOptions opts) throws QueryParseException { ChangeIndex index = indexes.getSearchIndex(); MutableInteger leafTerms = new MutableInteger(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java new file mode 100644 index 0000000000..7a01802dd1 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java @@ -0,0 +1,239 @@ +// 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; + +import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Ordering; +import com.google.gerrit.metrics.Description; +import com.google.gerrit.metrics.Field; +import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.metrics.Timer1; +import com.google.gerrit.server.index.Index; +import com.google.gerrit.server.index.IndexCollection; +import com.google.gerrit.server.index.IndexConfig; +import com.google.gerrit.server.index.IndexRewriter; +import com.google.gerrit.server.index.QueryOptions; +import com.google.gerrit.server.index.SchemaDefinitions; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.ResultSet; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; + +public abstract class QueryProcessor { + @Singleton + protected static class Metrics { + final Timer1 executionTime; + + @Inject + Metrics(MetricMaker metricMaker) { + Field index = Field.ofString("index", "index name"); + executionTime = metricMaker.newTimer("query/query_latency", + new Description("Successful query latency," + + " accumulated over the life of the process").setCumulative() + .setUnit(Description.Units.MILLISECONDS), + index); + } + } + + private final IndexCollection> indexes; + private final Metrics metrics; + private final SchemaDefinitions schemaDef; + private final IndexConfig indexConfig; + private final IndexRewriter rewriter; + private final String limitField; + + protected int start; + + private int limitFromCaller; + private Set requestedFields; + + protected QueryProcessor(Metrics metrics, + SchemaDefinitions schemaDef, + IndexConfig indexConfig, + IndexCollection> indexes, + IndexRewriter rewriter, + String limitField) { + this.metrics = metrics; + this.schemaDef = schemaDef; + this.indexConfig = indexConfig; + this.indexes = indexes; + this.rewriter = rewriter; + this.limitField = limitField; + } + + public QueryProcessor setStart(int n) { + start = n; + return this; + } + + public QueryProcessor setLimit(int n) { + limitFromCaller = n; + return this; + } + + public QueryProcessor setRequestedFields(Set fields) { + requestedFields = fields; + return this; + } + + /** + * Query for entities that match a structured query. + * + * @see #query(List) + * @param query the query. + * @return results of the query. + */ + public QueryResult query(Predicate query) + throws OrmException, QueryParseException { + return query(ImmutableList.of(query)).get(0); + } + + /* + * Perform multiple queries over a list of query strings. + *

+ * If a limit was specified using {@link #setLimit(int)} this method may + * return up to {@code limit + 1} results, allowing the caller to determine if + * there are more than {@code limit} matches and suggest to its own caller + * that the query could be retried with {@link #setStart(int)}. + * + * @param queries the queries. + * @return results of the queries, one list per input query. + */ + public List> query(List> queries) + throws OrmException, QueryParseException { + try { + return query(null, queries); + } catch (OrmException e) { + Throwables.propagateIfInstanceOf(e.getCause(), QueryParseException.class); + throw e; + } + } + + private List> query(List queryStrings, + List> queries) + throws OrmException, QueryParseException { + @SuppressWarnings("resource") + Timer1.Context context = metrics.executionTime.start(schemaDef.getName()); + + int cnt = queries.size(); + // Parse and rewrite all queries. + List limits = new ArrayList<>(cnt); + List> predicates = new ArrayList<>(cnt); + List> sources = new ArrayList<>(cnt); + for (Predicate q : queries) { + int limit = getEffectiveLimit(q); + limits.add(limit); + + if (limit == getBackendSupportedLimit()) { + limit--; + } + + int page = (start / limit) + 1; + if (page > indexConfig.maxPages()) { + throw new QueryParseException( + "Cannot go beyond page " + indexConfig.maxPages() + "of results"); + } + + // Always bump limit by 1, even if this results in exceeding the permitted + // max for this user. The only way to see if there are more entities is to + // ask for one more result from the query. + QueryOptions opts = + createOptions(indexConfig, page, limit, getRequestedFields()); + Predicate pred = postRewrite(rewriter.rewrite(q, opts)); + predicates.add(pred); + + @SuppressWarnings("unchecked") + DataSource s = (DataSource) pred; + sources.add(s); + } + + // Run each query asynchronously, if supported. + List> matches = new ArrayList<>(cnt); + for (DataSource s : sources) { + matches.add(s.read()); + } + + List> out = new ArrayList<>(cnt); + for (int i = 0; i < cnt; i++) { + out.add(QueryResult.create( + queryStrings != null ? queryStrings.get(i) : null, + predicates.get(i), + limits.get(i), + matches.get(i).toList())); + } + context.close(); // only measure successful queries + return out; + } + + protected QueryOptions createOptions(IndexConfig indexConfig, int start, + int limit, Set requestedFields) { + return QueryOptions.create(indexConfig, start, limit, requestedFields); + } + + /** + * Invoked after the query was rewritten. Subclasses may overwrite this method + * to add additional predicates to the query before it is executed. + * + * @param pred the query + * @return the modified query + */ + protected Predicate postRewrite(Predicate pred) { + return pred; + } + + private Set getRequestedFields() { + if (requestedFields != null) { + return requestedFields; + } + Index index = indexes.getSearchIndex(); + return index != null + ? index.getSchema().getStoredFields().keySet() + : ImmutableSet. of(); + } + + public boolean isDisabled() { + return getPermittedLimit() <= 0; + } + + protected int getPermittedLimit() { + return Integer.MAX_VALUE; + } + + private int getBackendSupportedLimit() { + return indexConfig.maxLimit(); + } + + private int getEffectiveLimit(Predicate p) { + List possibleLimits = new ArrayList<>(4); + possibleLimits.add(getBackendSupportedLimit()); + possibleLimits.add(getPermittedLimit()); + if (limitFromCaller > 0) { + possibleLimits.add(limitFromCaller); + } + if (limitField != null) { + Integer limitFromPredicate = LimitPredicate.getLimit(limitField, p); + if (limitFromPredicate != null) { + possibleLimits.add(limitFromPredicate); + } + } + return Ordering.natural().min(possibleLimits); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryResult.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryResult.java similarity index 62% rename from gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryResult.java rename to gerrit-server/src/main/java/com/google/gerrit/server/query/QueryResult.java index a93f7ac405..b35bde362e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryResult.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryResult.java @@ -12,27 +12,26 @@ // See the License for the specific language governing permissions and // limitations under the License. -package com.google.gerrit.server.query.change; +package com.google.gerrit.server.query; import com.google.auto.value.AutoValue; import com.google.gerrit.common.Nullable; -import com.google.gerrit.server.query.Predicate; import java.util.List; -/** Results of a query over changes. */ +/** Results of a query over entities. */ @AutoValue -public abstract class QueryResult { - static QueryResult create(@Nullable String query, - Predicate predicate, int limit, List changes) { - boolean moreChanges; - if (changes.size() > limit) { - moreChanges = true; - changes = changes.subList(0, limit); +public abstract class QueryResult { + static QueryResult create(@Nullable String query, + Predicate predicate, int limit, List entites) { + boolean more; + if (entites.size() > limit) { + more = true; + entites = entites.subList(0, limit); } else { - moreChanges = false; + more = false; } - return new AutoValue_QueryResult(query, predicate, changes, moreChanges); + return new AutoValue_QueryResult<>(query, predicate, entites, more); } /** @@ -45,15 +44,15 @@ public abstract class QueryResult { * @return the predicate after all rewriting and other modification by the * query subsystem. */ - public abstract Predicate predicate(); + public abstract Predicate predicate(); /** @return the query results. */ - public abstract List changes(); + public abstract List entities(); /** * @return whether the query could be retried with * {@link QueryProcessor#setStart(int)} to produce more results. Never - * true if {@link #changes()} is empty. + * true if {@link #entities()} is empty. */ - public abstract boolean moreChanges(); + public abstract boolean more(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java new file mode 100644 index 0000000000..a4ce58b4b9 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java @@ -0,0 +1,33 @@ +// 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.account; + +import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.index.IndexConfig; +import com.google.gerrit.server.index.account.AccountIndexCollection; +import com.google.gerrit.server.index.account.AccountIndexRewriter; +import com.google.gerrit.server.index.account.AccountSchemaDefinitions; +import com.google.gerrit.server.query.QueryProcessor; + +public class AccountQueryProcessor extends QueryProcessor { + + protected AccountQueryProcessor(Metrics metrics, + IndexConfig indexConfig, + AccountIndexCollection indexes, + AccountIndexRewriter rewriter) { + super(metrics, AccountSchemaDefinitions.INSTANCE, indexConfig, indexes, + rewriter, null); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java new file mode 100644 index 0000000000..e4ba721dbb --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java @@ -0,0 +1,103 @@ +// Copyright (C) 2010 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.checkState; +import static com.google.gerrit.server.query.change.ChangeQueryBuilder.FIELD_LIMIT; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.common.data.GlobalCapability; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.index.IndexConfig; +import com.google.gerrit.server.index.IndexPredicate; +import com.google.gerrit.server.index.QueryOptions; +import com.google.gerrit.server.index.change.ChangeIndexCollection; +import com.google.gerrit.server.index.change.ChangeIndexRewriter; +import com.google.gerrit.server.index.change.ChangeSchemaDefinitions; +import com.google.gerrit.server.index.change.IndexedChangeQuery; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.query.Predicate; +import com.google.gerrit.server.query.QueryProcessor; +import com.google.inject.Inject; +import com.google.inject.Provider; + +import java.util.Set; + +public class ChangeQueryProcessor extends QueryProcessor { + private final Provider db; + private final Provider userProvider; + private final ChangeControl.GenericFactory changeControlFactory; + private final ChangeNotes.Factory notesFactory; + + private boolean enforceVisibility = true; + + static { + // It is assumed that basic rewrites do not touch visibleto predicates. + checkState( + !IsVisibleToPredicate.class.isAssignableFrom(IndexPredicate.class), + "ChangeQueryProcessor assumes visibleto is not used by the index rewriter."); + } + + @Inject + ChangeQueryProcessor(Metrics metrics, + IndexConfig indexConfig, + ChangeIndexCollection indexes, + ChangeIndexRewriter rewriter, + Provider db, + Provider userProvider, + ChangeControl.GenericFactory changeControlFactory, + ChangeNotes.Factory notesFactory) { + super(metrics, ChangeSchemaDefinitions.INSTANCE, indexConfig, indexes, + rewriter, FIELD_LIMIT); + this.db = db; + this.userProvider = userProvider; + this.changeControlFactory = changeControlFactory; + this.notesFactory = notesFactory; + } + + public ChangeQueryProcessor enforceVisibility(boolean enforce) { + enforceVisibility = enforce; + return this; + } + + @Override + protected QueryOptions createOptions(IndexConfig indexConfig, int start, + int limit, Set requestedFields) { + return IndexedChangeQuery.createOptions(indexConfig, start, limit + 1, + requestedFields); + } + + @Override + protected Predicate postRewrite(Predicate pred) { + if (enforceVisibility) { + return new AndSource(ImmutableList.of(pred, new IsVisibleToPredicate(db, + notesFactory, changeControlFactory, userProvider.get())), start); + } + + return super.postRewrite(pred); + } + + @Override + protected int getPermittedLimit() { + if (enforceVisibility) { + return userProvider.get().getCapabilities() + .getRange(GlobalCapability.QUERY_LIMIT) + .getMax(); + } + return Integer.MAX_VALUE; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java index 31e88d36eb..680e796174 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java @@ -59,7 +59,7 @@ import java.util.Set; * Execute a single query over changes, for use by Gerrit internals. *

* By default, visibility of returned changes is not enforced (unlike in {@link - * QueryProcessor}). The methods in this class are not typically used by + * ChangeQueryProcessor}). The methods in this class are not typically used by * user-facing paths, but rather by internal callers that need to process all * matching results. */ @@ -85,14 +85,14 @@ public class InternalChangeQuery { } private final IndexConfig indexConfig; - private final QueryProcessor qp; + private final ChangeQueryProcessor qp; private final ChangeIndexCollection indexes; private final ChangeData.Factory changeDataFactory; private final ChangeNotes.Factory notesFactory; @Inject InternalChangeQuery(IndexConfig indexConfig, - QueryProcessor queryProcessor, + ChangeQueryProcessor queryProcessor, ChangeIndexCollection indexes, ChangeData.Factory changeDataFactory, ChangeNotes.Factory notesFactory) { @@ -303,7 +303,7 @@ public class InternalChangeQuery { public List query(Predicate p) throws OrmException { try { - return qp.queryChanges(p).changes(); + return qp.query(p).entities(); } catch (QueryParseException e) { throw new OrmException(e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java index 91dfc8c6b2..496eff65cc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java @@ -32,6 +32,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.query.QueryParseException; +import com.google.gerrit.server.query.QueryResult; import com.google.gson.Gson; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -75,7 +76,7 @@ public class OutputStreamQuery { private final ReviewDb db; private final GitRepositoryManager repoManager; private final ChangeQueryBuilder queryBuilder; - private final QueryProcessor queryProcessor; + private final ChangeQueryProcessor queryProcessor; private final EventFactory eventFactory; private final TrackingFooters trackingFooters; private final CurrentUser user; @@ -99,7 +100,7 @@ public class OutputStreamQuery { ReviewDb db, GitRepositoryManager repoManager, ChangeQueryBuilder queryBuilder, - QueryProcessor queryProcessor, + ChangeQueryProcessor queryProcessor, EventFactory eventFactory, TrackingFooters trackingFooters, CurrentUser user) { @@ -195,18 +196,18 @@ public class OutputStreamQuery { Map repos = new HashMap<>(); Map revWalks = new HashMap<>(); - QueryResult results = - queryProcessor.queryChanges(queryBuilder.parse(queryString)); + QueryResult results = + queryProcessor.query(queryBuilder.parse(queryString)); try { - for (ChangeData d : results.changes()) { + for (ChangeData d : results.entities()) { show(buildChangeAttribute(d, repos, revWalks)); } } finally { closeAll(revWalks.values(), repos.values()); } - stats.rowCount = results.changes().size(); - stats.moreChanges = results.moreChanges(); + stats.rowCount = results.entities().size(); + stats.moreChanges = results.more(); stats.runTimeMilliseconds = TimeUtil.nowMs() - stats.runTimeMilliseconds; show(stats); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java index e5ff46a719..69a392b6ca 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java @@ -22,6 +22,7 @@ import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.server.change.ChangeJson; import com.google.gerrit.server.query.QueryParseException; +import com.google.gerrit.server.query.QueryResult; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -37,7 +38,7 @@ import java.util.regex.Pattern; public class QueryChanges implements RestReadView { private final ChangeJson.Factory json; private final ChangeQueryBuilder qb; - private final QueryProcessor imp; + private final ChangeQueryProcessor imp; private EnumSet options; @Option(name = "--query", aliases = {"-q"}, metaVar = "QUERY", usage = "Query string") @@ -66,7 +67,7 @@ public class QueryChanges implements RestReadView { @Inject QueryChanges(ChangeJson.Factory json, ChangeQueryBuilder qb, - QueryProcessor qp) { + ChangeQueryProcessor qp) { this.json = json; this.qb = qb; this.imp = qp; @@ -119,12 +120,12 @@ public class QueryChanges implements RestReadView { } int cnt = queries.size(); - List results = imp.queryChanges(qb.parse(queries)); + List> results = imp.query(qb.parse(queries)); List> res = json.create(options) .formatQueryResults(results); for (int n = 0; n < cnt; n++) { List info = res.get(n); - if (results.get(n).moreChanges()) { + if (results.get(n).more()) { info.get(info.size() - 1)._moreChanges = true; } } 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 deleted file mode 100644 index 54726569f2..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java +++ /dev/null @@ -1,271 +0,0 @@ -// Copyright (C) 2010 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.checkState; -import static com.google.gerrit.server.query.change.ChangeQueryBuilder.FIELD_LIMIT; -import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open; - -import com.google.common.base.Throwables; -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; -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.IndexConfig; -import com.google.gerrit.server.index.IndexPredicate; -import com.google.gerrit.server.index.QueryOptions; -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.index.change.IndexedChangeQuery; -import com.google.gerrit.server.notedb.ChangeNotes; -import com.google.gerrit.server.project.ChangeControl; -import com.google.gerrit.server.query.LimitPredicate; -import com.google.gerrit.server.query.Predicate; -import com.google.gerrit.server.query.QueryParseException; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.ResultSet; -import com.google.inject.Inject; -import com.google.inject.Provider; -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 ChangeNotes.Factory notesFactory; - private final ChangeIndexCollection indexes; - private final ChangeIndexRewriter rewriter; - private final IndexConfig indexConfig; - private final Metrics metrics; - - private int limitFromCaller; - private int start; - private boolean enforceVisibility = true; - private Set requestedFields; - - @Inject - QueryProcessor(Provider db, - Provider userProvider, - ChangeControl.GenericFactory changeControlFactory, - ChangeNotes.Factory notesFactory, - ChangeIndexCollection indexes, - ChangeIndexRewriter rewriter, - IndexConfig indexConfig, - Metrics metrics) { - this.db = db; - this.userProvider = userProvider; - this.changeControlFactory = changeControlFactory; - this.notesFactory = notesFactory; - this.indexes = indexes; - this.rewriter = rewriter; - this.indexConfig = indexConfig; - this.metrics = metrics; - } - - public QueryProcessor enforceVisibility(boolean enforce) { - enforceVisibility = enforce; - return this; - } - - public QueryProcessor setLimit(int n) { - limitFromCaller = n; - return this; - } - - public QueryProcessor setStart(int n) { - start = n; - return this; - } - - public QueryProcessor setRequestedFields(Set fields) { - requestedFields = fields; - return this; - } - - /** - * Query for changes that match a structured query. - * - * @see #queryChanges(List) - * @param query the query. - * @return results of the query. - */ - public QueryResult queryChanges(Predicate query) - throws OrmException, QueryParseException { - return queryChanges(ImmutableList.of(query)).get(0); - } - - /* - * Perform multiple queries over a list of query strings. - *

- * If a limit was specified using {@link #setLimit(int)} this method may - * return up to {@code limit + 1} results, allowing the caller to determine if - * there are more than {@code limit} matches and suggest to its own caller - * that the query could be retried with {@link #setStart(int)}. - * - * @param queries the queries. - * @return results of the queries, one list per input query. - */ - public List queryChanges(List> queries) - throws OrmException, QueryParseException { - try { - return queryChanges(null, queries); - } catch (OrmException e) { - Throwables.propagateIfInstanceOf(e.getCause(), QueryParseException.class); - throw e; - } - } - - static { - // In addition to this assumption, this queryChanges assumes the basic - // rewrites do not touch visibleto predicates either. - checkState( - !IsVisibleToPredicate.class.isAssignableFrom(IndexPredicate.class), - "QueryProcessor assumes visibleto is not used by the index rewriter."); - } - - private List queryChanges(List queryStrings, - List> queries) - throws OrmException, QueryParseException { - @SuppressWarnings("resource") - Timer0.Context context = metrics.executionTime.start(); - - Predicate visibleToMe = enforceVisibility - ? new IsVisibleToPredicate(db, notesFactory, changeControlFactory, - userProvider.get()) - : null; - int cnt = queries.size(); - - // Parse and rewrite all queries. - List limits = new ArrayList<>(cnt); - List> predicates = new ArrayList<>(cnt); - List sources = new ArrayList<>(cnt); - for (Predicate q : queries) { - int limit = getEffectiveLimit(q); - limits.add(limit); - - if (limit == getBackendSupportedLimit()) { - limit--; - } - - int page = (start / limit) + 1; - if (page > indexConfig.maxPages()) { - throw new QueryParseException( - "Cannot go beyond page " + indexConfig.maxPages() + "of results"); - } - - // Always bump limit by 1, even if this results in exceeding the permitted - // max for this user. The only way to see if there are more changes is to - // ask for one more result from the query. - QueryOptions opts = IndexedChangeQuery.createOptions( - indexConfig, start, limit + 1, getRequestedFields()); - Predicate s = rewriter.rewrite(q, opts); - if (!(s instanceof ChangeDataSource)) { - q = Predicate.and(open(), q); - s = rewriter.rewrite(q, opts); - } - if (!(s instanceof ChangeDataSource)) { - throw new QueryParseException("invalid query: " + s); - } - if (enforceVisibility) { - s = new AndSource(ImmutableList.of(s, visibleToMe), start); - } - predicates.add(s); - sources.add((ChangeDataSource) s); - } - - // Run each query asynchronously, if supported. - List> matches = new ArrayList<>(cnt); - for (ChangeDataSource s : sources) { - matches.add(s.read()); - } - - List out = new ArrayList<>(cnt); - for (int i = 0; i < cnt; i++) { - out.add(QueryResult.create( - queryStrings != null ? queryStrings.get(i) : null, - predicates.get(i), - limits.get(i), - matches.get(i).toList())); - } - context.close(); // only measure successful queries - 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; - } - - private int getPermittedLimit() { - if (enforceVisibility) { - return userProvider.get().getCapabilities() - .getRange(GlobalCapability.QUERY_LIMIT) - .getMax(); - } - return Integer.MAX_VALUE; - } - - private int getBackendSupportedLimit() { - return indexConfig.maxLimit(); - } - - private int getEffectiveLimit(Predicate p) { - List possibleLimits = new ArrayList<>(4); - possibleLimits.add(getBackendSupportedLimit()); - possibleLimits.add(getPermittedLimit()); - if (limitFromCaller > 0) { - possibleLimits.add(limitFromCaller); - } - Integer limitFromPredicate = LimitPredicate.getLimit(FIELD_LIMIT, p); - if (limitFromPredicate != null) { - possibleLimits.add(limitFromPredicate); - } - return Ordering.natural().min(possibleLimits); - } - - @Singleton - static class Metrics { - final Timer0 executionTime; - - @Inject - Metrics(MetricMaker metricMaker) { - executionTime = metricMaker.newTimer( - "change/query/query_latency", - new Description("Successful change query latency," - + " accumulated over the life of the process") - .setCumulative() - .setUnit(Description.Units.MILLISECONDS)); - } - } -} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java index 36fc76e7fc..35c1e74fd1 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java @@ -35,6 +35,7 @@ import com.google.gerrit.server.query.QueryParseException; import com.google.gerrit.server.query.change.AndSource; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeQueryBuilder; +import com.google.gerrit.server.query.change.ChangeStatusPredicate; import com.google.gerrit.server.query.change.OrSource; import com.google.gerrit.testutil.GerritBaseTests; @@ -72,7 +73,11 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { @Test public void testNonIndexPredicate() throws Exception { Predicate in = parse("foo:a"); - assertThat(in).isSameAs(rewrite(in)); + Predicate out = rewrite(in); + assertThat(AndSource.class).isSameAs(out.getClass()); + assertThat(out.getChildren()) + .containsExactly(query(ChangeStatusPredicate.open()), in) + .inOrder(); } @Test @@ -84,7 +89,11 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { @Test public void testNonIndexPredicates() throws Exception { Predicate in = parse("foo:a OR foo:b"); - assertThat(in).isEqualTo(rewrite(in)); + Predicate out = rewrite(in); + assertThat(AndSource.class).isSameAs(out.getClass()); + assertThat(out.getChildren()) + .containsExactly(query(ChangeStatusPredicate.open()), in) + .inOrder(); } @Test 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 21e8d348a3..5a53d65443 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 @@ -126,7 +126,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { @Inject protected ChangeNotes.Factory notesFactory; @Inject protected PatchSetInserter.Factory patchSetFactory; @Inject protected ChangeControl.GenericFactory changeControlFactory; - @Inject protected QueryProcessor queryProcessor; + @Inject protected ChangeQueryProcessor queryProcessor; @Inject protected SchemaCreator schemaCreator; @Inject protected Sequences seq; @Inject protected ThreadLocalRequestContext requestContext; @@ -1460,8 +1460,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { requestContext.setContext(newRequestContext(userId)); // Use QueryProcessor directly instead of API so we get ChangeDatas back. List cds = queryProcessor - .queryChanges(queryBuilder.parse(change.getId().toString())) - .changes(); + .query(queryBuilder.parse(change.getId().toString())) + .entities(); assertThat(cds).hasSize(1); ChangeData cd = cds.get(0); @@ -1493,8 +1493,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { .setRequestedFields(ImmutableSet.of( ChangeField.PATCH_SET.getName(), ChangeField.CHANGE.getName())) - .queryChanges(queryBuilder.parse(change.getId().toString())) - .changes(); + .query(queryBuilder.parse(change.getId().toString())) + .entities(); assertThat(cds).hasSize(1); ChangeData cd = cds.get(0);