diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java index df818aca9f..168dbf75e0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java @@ -83,6 +83,13 @@ public class IdentifiedUser extends CurrentUser { this.disableReverseDnsLookup = disableReverseDnsLookup; } + public IdentifiedUser create(AccountState state) { + return new IdentifiedUser(capabilityControlFactory, authConfig, realm, + anonymousCowardName, canonicalUrl, accountCache, groupBackend, + disableReverseDnsLookup, Providers.of((SocketAddress) null), state, + null); + } + public IdentifiedUser create(Account.Id id) { return create((SocketAddress) null, id); } @@ -179,15 +186,33 @@ public class IdentifiedUser extends CurrentUser { private IdentifiedUser( CapabilityControl.Factory capabilityControlFactory, - final AuthConfig authConfig, + AuthConfig authConfig, Realm realm, - final String anonymousCowardName, - final Provider canonicalUrl, - final AccountCache accountCache, - final GroupBackend groupBackend, - final Boolean disableReverseDnsLookup, - @Nullable final Provider remotePeerProvider, - final Account.Id id, + String anonymousCowardName, + Provider canonicalUrl, + AccountCache accountCache, + GroupBackend groupBackend, + Boolean disableReverseDnsLookup, + @Nullable Provider remotePeerProvider, + AccountState state, + @Nullable CurrentUser realUser) { + this(capabilityControlFactory, authConfig, realm, anonymousCowardName, + canonicalUrl, accountCache, groupBackend, disableReverseDnsLookup, + remotePeerProvider, state.getAccount().getId(), realUser); + this.state = state; + } + + private IdentifiedUser( + CapabilityControl.Factory capabilityControlFactory, + AuthConfig authConfig, + Realm realm, + String anonymousCowardName, + Provider canonicalUrl, + AccountCache accountCache, + GroupBackend groupBackend, + Boolean disableReverseDnsLookup, + @Nullable Provider remotePeerProvider, + Account.Id id, @Nullable CurrentUser realUser) { super(capabilityControlFactory); this.canonicalUrl = canonicalUrl; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountControl.java index 05ae51fc0a..3f6fd6ff4a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountControl.java @@ -78,6 +78,10 @@ public class AccountControl { this.accountVisibility = accountVisibility; } + public CurrentUser getUser() { + return user; + } + /** * Returns true if the current user is allowed to see the otherUser, based * on the account visibility policy. Depending on the group membership @@ -86,7 +90,7 @@ public class AccountControl { * {@link GroupMembership#getKnownGroups()} may only return a subset of the * effective groups. */ - public boolean canSee(final Account otherUser) { + public boolean canSee(Account otherUser) { return canSee(otherUser.getId()); } @@ -99,6 +103,42 @@ public class AccountControl { * effective groups. */ public boolean canSee(final Account.Id otherUser) { + return canSee(new OtherUser() { + @Override + Account.Id getId() { + return otherUser; + } + + @Override + IdentifiedUser createUser() { + return userFactory.create(otherUser); + } + }); + } + + /** + * Returns true if the current user is allowed to see the otherUser, based + * on the account visibility policy. Depending on the group membership + * realms supported, this may not be able to determine SAME_GROUP or + * VISIBLE_GROUP correctly (defaulting to not being visible). This is because + * {@link GroupMembership#getKnownGroups()} may only return a subset of the + * effective groups. + */ + public boolean canSee(final AccountState otherUser) { + return canSee(new OtherUser() { + @Override + Account.Id getId() { + return otherUser.getAccount().getId(); + } + + @Override + IdentifiedUser createUser() { + return userFactory.create(otherUser); + } + }); + } + + private boolean canSee(OtherUser otherUser) { // Special case: I can always see myself. if (user.isIdentifiedUser() && user.getAccountId().equals(otherUser)) { return true; @@ -111,7 +151,7 @@ public class AccountControl { case ALL: return true; case SAME_GROUP: { - Set usersGroups = groupsOf(otherUser); + Set usersGroups = groupsOf(otherUser.getUser()); for (PermissionRule rule : accountsSection.getSameGroupVisibility()) { if (rule.isBlock() || rule.isDeny()) { usersGroups.remove(rule.getGroup().getUUID()); @@ -124,7 +164,7 @@ public class AccountControl { break; } case VISIBLE_GROUP: { - Set usersGroups = groupsOf(otherUser); + Set usersGroups = groupsOf(otherUser.getUser()); for (AccountGroup.UUID usersGroup : usersGroups) { try { if (groupControlFactory.controlFor(usersGroup).isVisible()) { @@ -144,9 +184,9 @@ public class AccountControl { return false; } - private Set groupsOf(Account.Id account) { + private Set groupsOf(IdentifiedUser user) { return new HashSet<>(Sets.filter( - userFactory.create(account).getEffectiveGroups().getKnownGroups(), + user.getEffectiveGroups().getKnownGroups(), new Predicate() { @Override public boolean apply(AccountGroup.UUID in) { @@ -154,4 +194,18 @@ public class AccountControl { } })); } + + private static abstract class OtherUser { + IdentifiedUser user; + + IdentifiedUser getUser() { + if (user == null) { + user = createUser(); + } + return user; + } + + abstract IdentifiedUser createUser(); + abstract Account.Id getId(); + } } 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 1fff70d53a..3523e5ff28 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 @@ -30,7 +30,7 @@ import com.google.gerrit.server.query.NotPredicate; import com.google.gerrit.server.query.OrPredicate; 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.AndChangeSource; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeDataSource; import com.google.gerrit.server.query.change.ChangeQueryBuilder; @@ -272,7 +272,7 @@ public class ChangeIndexRewriter implements IndexRewriter { Predicate in, List> all) { if (in instanceof AndPredicate) { - return new AndSource(all); + return new AndChangeSource(all); } else if (in instanceof OrPredicate) { return new OrSource(all); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/AndSource.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/AndSource.java new file mode 100644 index 0000000000..4b623332af --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/AndSource.java @@ -0,0 +1,176 @@ +// 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 static com.google.common.base.Preconditions.checkArgument; + +import com.google.common.base.Function; +import com.google.common.base.Throwables; +import com.google.common.collect.FluentIterable; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.gwtorm.server.ListResultSet; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.OrmRuntimeException; +import com.google.gwtorm.server.ResultSet; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; + +public class AndSource extends AndPredicate + implements DataSource, Comparator> { + protected final DataSource source; + + private final int start; + private final int cardinality; + + + public AndSource(Collection> that) { + this(that, 0); + } + + public AndSource(Collection> that, int start) { + super(that); + checkArgument(start >= 0, "negative start: %s", start); + this.start = start; + + int c = Integer.MAX_VALUE; + DataSource s = null; + int minCost = Integer.MAX_VALUE; + for (Predicate p : sort(getChildren())) { + if (p instanceof DataSource) { + c = Math.min(c, ((DataSource) p).getCardinality()); + + if (p.getCost() < minCost) { + s = toDataSource(p); + minCost = p.getCost(); + } + } + } + this.source = s; + this.cardinality = c; + } + + @Override + public ResultSet read() throws OrmException { + try { + return readImpl(); + } catch (OrmRuntimeException err) { + Throwables.propagateIfInstanceOf(err.getCause(), OrmException.class); + throw new OrmException(err); + } + } + + private ResultSet readImpl() throws OrmException { + if (source == null) { + throw new OrmException("No DataSource: " + this); + } + List r = new ArrayList<>(); + T last = null; + int nextStart = 0; + boolean skipped = false; + for (T data : buffer(source.read())) { + if (match(data)) { + r.add(data); + } else { + skipped = true; + } + last = data; + nextStart++; + } + + if (skipped && last != null && source instanceof Paginated) { + // If our source is a paginated source and we skipped at + // least one of its results, we may not have filled the full + // limit the caller wants. Restart the source and continue. + // + @SuppressWarnings("unchecked") + Paginated p = (Paginated) source; + while (skipped && r.size() < p.getOptions().limit() + start) { + skipped = false; + ResultSet next = p.restart(nextStart); + + for (T data : buffer(next)) { + if (match(data)) { + r.add(data); + } else { + skipped = true; + } + nextStart++; + } + } + } + + if (start >= r.size()) { + r = ImmutableList.of(); + } else if (start > 0) { + r = ImmutableList.copyOf(r.subList(start, r.size())); + } + return new ListResultSet<>(r); + } + + private Iterable buffer(ResultSet scanner) { + return FluentIterable.from(Iterables.partition(scanner, 50)) + .transformAndConcat(new Function, List>() { + @Override + public List apply(List buffer) { + return transformBuffer(buffer); + } + }); + } + + protected List transformBuffer(List buffer) throws OrmRuntimeException { + return buffer; + } + + @Override + public int getCardinality() { + return cardinality; + } + + private List> sort(Collection> that) { + List> r = new ArrayList<>(that); + Collections.sort(r, this); + return r; + } + + @Override + public int compare(Predicate a, Predicate b) { + int ai = a instanceof DataSource ? 0 : 1; + int bi = b instanceof DataSource ? 0 : 1; + int cmp = ai - bi; + + if (cmp == 0) { + cmp = a.getCost() - b.getCost(); + } + + if (cmp == 0 + && a instanceof DataSource + && b instanceof DataSource) { + DataSource as = (DataSource) a; + DataSource bs = (DataSource) b; + cmp = as.getCardinality() - bs.getCardinality(); + } + return cmp; + } + + @SuppressWarnings("unchecked") + private DataSource toDataSource(Predicate pred) { + return (DataSource) pred; + } +} 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 index 33341170cd..d971d86e40 100644 --- 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 @@ -18,10 +18,12 @@ 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.Field; import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.metrics.Timer1; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.index.Index; import com.google.gerrit.server.index.IndexCollection; import com.google.gerrit.server.index.IndexConfig; @@ -31,6 +33,7 @@ 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.Provider; import com.google.inject.Singleton; import java.util.ArrayList; @@ -54,24 +57,30 @@ public abstract class QueryProcessor { } } - private final IndexCollection> indexes; + protected final Provider userProvider; + private final Metrics metrics; private final SchemaDefinitions schemaDef; private final IndexConfig indexConfig; + private final IndexCollection> indexes; private final IndexRewriter rewriter; private final String limitField; protected int start; + private boolean enforceVisibility = true; private int limitFromCaller; private Set requestedFields; - protected QueryProcessor(Metrics metrics, + protected QueryProcessor( + Provider userProvider, + Metrics metrics, SchemaDefinitions schemaDef, IndexConfig indexConfig, IndexCollection> indexes, IndexRewriter rewriter, String limitField) { + this.userProvider = userProvider; this.metrics = metrics; this.schemaDef = schemaDef; this.indexConfig = indexConfig; @@ -85,6 +94,11 @@ public abstract class QueryProcessor { return this; } + public QueryProcessor enforceVisibility(boolean enforce) { + enforceVisibility = enforce; + return this; + } + public QueryProcessor setLimit(int n) { limitFromCaller = n; return this; @@ -157,7 +171,10 @@ public abstract class QueryProcessor { // ask for one more result from the query. QueryOptions opts = createOptions(indexConfig, page, limit, getRequestedFields()); - Predicate pred = postRewrite(rewriter.rewrite(q, opts)); + Predicate pred = rewriter.rewrite(q, opts); + if (enforceVisibility) { + pred = enforceVisibility(pred); + } predicates.add(pred); @SuppressWarnings("unchecked") @@ -192,15 +209,13 @@ public abstract class QueryProcessor { } /** - * Invoked after the query was rewritten. Subclasses may overwrite this method - * to add additional predicates to the query before it is executed. + * Invoked after the query was rewritten. Subclasses must overwrite this + * method to filter out results that are not visible to the calling user. * * @param pred the query * @return the modified query */ - protected Predicate postRewrite(Predicate pred) { - return pred; - } + protected abstract Predicate enforceVisibility(Predicate pred); private Set getRequestedFields() { if (requestedFields != null) { @@ -216,7 +231,12 @@ public abstract class QueryProcessor { return getPermittedLimit() <= 0; } - protected int getPermittedLimit() { + private 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/account/AccountQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java index 63b3030ba0..4082e0834e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java @@ -38,6 +38,7 @@ public class AccountQueryBuilder extends QueryBuilder { public static final String FIELD_ACCOUNT = "account"; public static final String FIELD_LIMIT = "limit"; + public static final String FIELD_VISIBLETO = "visibleto"; private static final QueryBuilder.Definition mydef = new QueryBuilder.Definition<>(AccountQueryBuilder.class); 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 index 22e9d3d757..b2be7b5113 100644 --- 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 @@ -16,20 +16,39 @@ package com.google.gerrit.server.query.account; import static com.google.gerrit.server.query.account.AccountQueryBuilder.FIELD_LIMIT; +import com.google.common.collect.ImmutableList; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.AccountControl; 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.AndSource; +import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.QueryProcessor; +import com.google.inject.Inject; +import com.google.inject.Provider; public class AccountQueryProcessor extends QueryProcessor { + private final AccountControl.Factory accountControlFactory; - protected AccountQueryProcessor(Metrics metrics, + @Inject + protected AccountQueryProcessor(Provider userProvider, + Metrics metrics, IndexConfig indexConfig, AccountIndexCollection indexes, - AccountIndexRewriter rewriter) { - super(metrics, AccountSchemaDefinitions.INSTANCE, indexConfig, indexes, - rewriter, FIELD_LIMIT); + AccountIndexRewriter rewriter, + AccountControl.Factory accountControlFactory) { + super(userProvider, metrics, AccountSchemaDefinitions.INSTANCE, indexConfig, + indexes, rewriter, FIELD_LIMIT); + this.accountControlFactory = accountControlFactory; + } + + @Override + protected Predicate enforceVisibility( + Predicate pred) { + return new AndSource<>(ImmutableList.of(pred, + new IsVisibleToPredicate(accountControlFactory.get()))); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/IsVisibleToPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/IsVisibleToPredicate.java new file mode 100644 index 0000000000..8fbe4cfee5 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/IsVisibleToPredicate.java @@ -0,0 +1,53 @@ +// 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.CurrentUser; +import com.google.gerrit.server.account.AccountControl; +import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.query.OperatorPredicate; +import com.google.gerrit.server.query.change.SingleGroupUser; +import com.google.gwtorm.server.OrmException; + +public class IsVisibleToPredicate extends OperatorPredicate { + private static String describe(CurrentUser user) { + if (user.isIdentifiedUser()) { + return user.getAccountId().toString(); + } + if (user instanceof SingleGroupUser) { + return "group:" + user.getEffectiveGroups().getKnownGroups() // + .iterator().next().toString(); + } + return user.toString(); + } + + private final AccountControl accountControl; + + IsVisibleToPredicate(AccountControl accountControl) { + super(AccountQueryBuilder.FIELD_VISIBLETO, + describe(accountControl.getUser())); + this.accountControl = accountControl; + } + + @Override + public boolean match(AccountState accountState) throws OrmException { + return accountControl.canSee(accountState); + } + + @Override + public int getCost() { + return 1; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AndChangeSource.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AndChangeSource.java new file mode 100644 index 0000000000..d40e53f1ca --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AndChangeSource.java @@ -0,0 +1,67 @@ +// 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 com.google.gerrit.server.query.AndSource; +import com.google.gerrit.server.query.Predicate; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.OrmRuntimeException; + +import java.util.Collection; +import java.util.List; + +public class AndChangeSource extends AndSource + implements ChangeDataSource { + + public AndChangeSource(Collection> that) { + super(that, 0); + } + + public AndChangeSource(Collection> that, + int start) { + super(that, start); + } + + @Override + public boolean hasChange() { + return source != null && source instanceof ChangeDataSource + && ((ChangeDataSource) source).hasChange(); + } + + @Override + protected List transformBuffer(List buffer) + throws OrmRuntimeException { + if (!hasChange()) { + try { + ChangeData.ensureChangeLoaded(buffer); + } catch (OrmException e) { + throw new OrmRuntimeException(e); + } + } + return super.transformBuffer(buffer); + } + + @Override + public int compare(Predicate a, Predicate b) { + int cmp = super.compare(a, b); + if (cmp == 0 && a instanceof ChangeDataSource + && b instanceof ChangeDataSource) { + ChangeDataSource as = (ChangeDataSource) a; + ChangeDataSource bs = (ChangeDataSource) b; + cmp = (as.hasChange() ? 0 : 1) - (bs.hasChange() ? 0 : 1); + } + return cmp; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AndSource.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AndSource.java deleted file mode 100644 index 75847c3dbc..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AndSource.java +++ /dev/null @@ -1,201 +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.checkArgument; - -import com.google.common.base.Function; -import com.google.common.base.Throwables; -import com.google.common.collect.FluentIterable; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; -import com.google.gerrit.server.query.AndPredicate; -import com.google.gerrit.server.query.Paginated; -import com.google.gerrit.server.query.Predicate; -import com.google.gwtorm.server.ListResultSet; -import com.google.gwtorm.server.OrmException; -import com.google.gwtorm.server.OrmRuntimeException; -import com.google.gwtorm.server.ResultSet; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.Comparator; -import java.util.List; - -public class AndSource extends AndPredicate - implements ChangeDataSource { - private static final Comparator> CMP = - new Comparator>() { - @Override - public int compare(Predicate a, Predicate b) { - int ai = a instanceof ChangeDataSource ? 0 : 1; - int bi = b instanceof ChangeDataSource ? 0 : 1; - int cmp = ai - bi; - - if (cmp == 0) { - cmp = a.getCost() - b.getCost(); - } - - if (cmp == 0 // - && a instanceof ChangeDataSource // - && b instanceof ChangeDataSource) { - ChangeDataSource as = (ChangeDataSource) a; - ChangeDataSource bs = (ChangeDataSource) b; - cmp = as.getCardinality() - bs.getCardinality(); - - if (cmp == 0) { - cmp = (as.hasChange() ? 0 : 1) - - (bs.hasChange() ? 0 : 1); - } - } - - return cmp; - } - }; - - private static List> sort( - Collection> that) { - List> r = new ArrayList<>(that); - Collections.sort(r, CMP); - return r; - } - - private final int start; - private int cardinality = -1; - - public AndSource(Collection> that) { - this(that, 0); - } - - public AndSource(Collection> that, - int start) { - super(sort(that)); - checkArgument(start >= 0, "negative start: %s", start); - this.start = start; - } - - @Override - public boolean hasChange() { - ChangeDataSource source = source(); - return source != null && source.hasChange(); - } - - @Override - public ResultSet read() throws OrmException { - try { - return readImpl(); - } catch (OrmRuntimeException err) { - Throwables.propagateIfInstanceOf(err.getCause(), OrmException.class); - throw new OrmException(err); - } - } - - private ResultSet readImpl() throws OrmException { - ChangeDataSource source = source(); - if (source == null) { - throw new OrmException("No ChangeDataSource: " + this); - } - List r = new ArrayList<>(); - ChangeData last = null; - int nextStart = 0; - boolean skipped = false; - for (ChangeData data : buffer(source, source.read())) { - if (match(data)) { - r.add(data); - } else { - skipped = true; - } - last = data; - nextStart++; - } - - if (skipped && last != null && source instanceof Paginated) { - // If our source is a paginated source and we skipped at - // least one of its results, we may not have filled the full - // limit the caller wants. Restart the source and continue. - // - @SuppressWarnings("unchecked") - Paginated p = (Paginated) source; - while (skipped && r.size() < p.getOptions().limit() + start) { - skipped = false; - ResultSet next = p.restart(nextStart); - - for (ChangeData data : buffer(source, next)) { - if (match(data)) { - r.add(data); - } else { - skipped = true; - } - nextStart++; - } - } - } - - if (start >= r.size()) { - r = ImmutableList.of(); - } else if (start > 0) { - r = ImmutableList.copyOf(r.subList(start, r.size())); - } - return new ListResultSet<>(r); - } - - private Iterable buffer( - ChangeDataSource source, - ResultSet scanner) { - final boolean loadChange = !source.hasChange(); - return FluentIterable - .from(Iterables.partition(scanner, 50)) - .transformAndConcat(new Function, List>() { - @Override - public List apply(List buffer) { - if (loadChange) { - try { - ChangeData.ensureChangeLoaded(buffer); - } catch (OrmException e) { - throw new OrmRuntimeException(e); - } - } - return buffer; - } - }); - } - - private ChangeDataSource source() { - int minCost = Integer.MAX_VALUE; - Predicate s = null; - for (Predicate p : getChildren()) { - if (p instanceof ChangeDataSource && p.getCost() < minCost) { - s = p; - minCost = p.getCost(); - } - } - return (ChangeDataSource) s; - } - - @Override - public int getCardinality() { - if (cardinality < 0) { - cardinality = Integer.MAX_VALUE; - for (Predicate p : getChildren()) { - if (p instanceof ChangeDataSource) { - int c = ((ChangeDataSource) p).getCardinality(); - cardinality = Math.min(cardinality, c); - } - } - } - return cardinality; - } -} 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 index e4ba721dbb..4e534c81ab 100644 --- 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 @@ -18,7 +18,6 @@ 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; @@ -39,12 +38,9 @@ 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( @@ -53,24 +49,24 @@ public class ChangeQueryProcessor extends QueryProcessor { } @Inject - ChangeQueryProcessor(Metrics metrics, + ChangeQueryProcessor(Provider userProvider, + Metrics metrics, IndexConfig indexConfig, ChangeIndexCollection indexes, ChangeIndexRewriter rewriter, Provider db, - Provider userProvider, ChangeControl.GenericFactory changeControlFactory, ChangeNotes.Factory notesFactory) { - super(metrics, ChangeSchemaDefinitions.INSTANCE, indexConfig, indexes, + super(userProvider, metrics, ChangeSchemaDefinitions.INSTANCE, indexConfig, indexes, rewriter, FIELD_LIMIT); this.db = db; - this.userProvider = userProvider; this.changeControlFactory = changeControlFactory; this.notesFactory = notesFactory; } + @Override public ChangeQueryProcessor enforceVisibility(boolean enforce) { - enforceVisibility = enforce; + super.enforceVisibility(enforce); return this; } @@ -82,22 +78,9 @@ public class ChangeQueryProcessor extends QueryProcessor { } @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; + protected Predicate enforceVisibility( + Predicate pred) { + return new AndChangeSource(ImmutableList.of(pred, new IsVisibleToPredicate(db, + notesFactory, changeControlFactory, userProvider.get())), start); } } 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 35c1e74fd1..ac7aed7ce7 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 @@ -32,7 +32,7 @@ import com.google.gerrit.server.index.QueryOptions; import com.google.gerrit.server.query.AndPredicate; 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.AndChangeSource; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeQueryBuilder; import com.google.gerrit.server.query.change.ChangeStatusPredicate; @@ -74,7 +74,7 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { public void testNonIndexPredicate() throws Exception { Predicate in = parse("foo:a"); Predicate out = rewrite(in); - assertThat(AndSource.class).isSameAs(out.getClass()); + assertThat(AndChangeSource.class).isSameAs(out.getClass()); assertThat(out.getChildren()) .containsExactly(query(ChangeStatusPredicate.open()), in) .inOrder(); @@ -90,7 +90,7 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { public void testNonIndexPredicates() throws Exception { Predicate in = parse("foo:a OR foo:b"); Predicate out = rewrite(in); - assertThat(AndSource.class).isSameAs(out.getClass()); + assertThat(AndChangeSource.class).isSameAs(out.getClass()); assertThat(out.getChildren()) .containsExactly(query(ChangeStatusPredicate.open()), in) .inOrder(); @@ -100,7 +100,7 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { public void testOneIndexPredicate() throws Exception { Predicate in = parse("foo:a file:b"); Predicate out = rewrite(in); - assertThat(AndSource.class).isSameAs(out.getClass()); + assertThat(AndChangeSource.class).isSameAs(out.getClass()); assertThat(out.getChildren()) .containsExactly( query(in.getChild(1)), @@ -120,7 +120,7 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { public void testThreeLevelTreeWithSomeIndexPredicates() throws Exception { Predicate in = parse("-foo:a (file:b OR file:c)"); Predicate out = rewrite(in); - assertThat(out.getClass()).isSameAs(AndSource.class); + assertThat(out.getClass()).isSameAs(AndChangeSource.class); assertThat(out.getChildren()) .containsExactly( query(in.getChild(1)), @@ -146,7 +146,7 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { public void testIndexAndNonIndexPredicates() throws Exception { Predicate in = parse("status:new bar:p file:a"); Predicate out = rewrite(in); - assertThat(AndSource.class).isSameAs(out.getClass()); + assertThat(AndChangeSource.class).isSameAs(out.getClass()); assertThat(out.getChildren()) .containsExactly( query(and(in.getChild(0), in.getChild(2))), @@ -159,7 +159,7 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { Predicate in = parse("(status:new OR status:draft) bar:p file:a"); Predicate out = rewrite(in); - assertThat(out.getClass()).isEqualTo(AndSource.class); + assertThat(out.getClass()).isEqualTo(AndChangeSource.class); assertThat(out.getChildren()) .containsExactly( query(and(in.getChild(0), in.getChild(2))), @@ -172,7 +172,7 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { Predicate in = parse("(status:new OR file:a) bar:p file:b"); Predicate out = rewrite(in); - assertThat(out.getClass()).isEqualTo(AndSource.class); + assertThat(out.getClass()).isEqualTo(AndChangeSource.class); assertThat(out.getChildren()) .containsExactly( query(and(in.getChild(0), in.getChild(2))), @@ -185,7 +185,7 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { throws Exception { Predicate in = parse("limit:1 file:a limit:3"); Predicate out = rewrite(in, options(0, 5)); - assertThat(out.getClass()).isEqualTo(AndSource.class); + assertThat(out.getClass()).isEqualTo(AndChangeSource.class); assertThat(out.getChildren()) .containsExactly( query(in.getChild(1), 5), @@ -271,8 +271,8 @@ public class ChangeIndexRewriterTest extends GerritBaseTests { } @SafeVarargs - private static AndSource andSource(Predicate... preds) { - return new AndSource(Arrays.asList(preds)); + private static AndChangeSource andSource(Predicate... preds) { + return new AndChangeSource(Arrays.asList(preds)); } private Predicate rewrite(Predicate in) diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeChangeIndex.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeChangeIndex.java index 05bc5529c5..43039f89cd 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeChangeIndex.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeChangeIndex.java @@ -49,7 +49,7 @@ public class FakeChangeIndex implements ChangeIndex { @Override public int getCardinality() { - throw new UnsupportedOperationException(); + return 1; } @Override