Add visibility check for account queries

Change-Id: I0785952bf2cee111bc14c7824bb75759da201b7e
Signed-off-by: Edwin Kempin <ekempin@google.com>

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,
+      String anonymousCowardName,
-      final Provider<String> canonicalUrl,
+      Provider<String> canonicalUrl,
-      final AccountCache accountCache,
+      AccountCache accountCache,
-      final GroupBackend groupBackend,
+      GroupBackend groupBackend,
-      final Boolean disableReverseDnsLookup,
+      Boolean disableReverseDnsLookup,
-      @Nullable final Provider<SocketAddress> remotePeerProvider,
+      @Nullable Provider<SocketAddress> remotePeerProvider,
-      final Account.Id id,
+      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<String> canonicalUrl,
+      AccountCache accountCache,
+      GroupBackend groupBackend,
+      Boolean disableReverseDnsLookup,
+      @Nullable Provider<SocketAddress> remotePeerProvider,
+      Account.Id id,
       @Nullable CurrentUser realUser) {
     super(capabilityControlFactory);
     this.canonicalUrl = canonicalUrl;

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<AccountGroup.UUID> usersGroups = groupsOf(otherUser);
+        Set<AccountGroup.UUID> 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<AccountGroup.UUID> usersGroups = groupsOf(otherUser);
+        Set<AccountGroup.UUID> 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<AccountGroup.UUID> groupsOf(Account.Id account) {
+  private Set<AccountGroup.UUID> groupsOf(IdentifiedUser user) {
     return new HashSet<>(Sets.filter(
-      userFactory.create(account).getEffectiveGroups().getKnownGroups(),
+      user.getEffectiveGroups().getKnownGroups(),
       new Predicate<AccountGroup.UUID>() {
         @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();
+  }
 }

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<ChangeData> {
       Predicate<ChangeData> in,
       List<Predicate<ChangeData>> all) {
     if (in instanceof AndPredicate) {
-      return new AndSource(all);
+      return new AndChangeSource(all);
     } else if (in instanceof OrPredicate) {
       return new OrSource(all);
     }

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<T> extends AndPredicate<T>
+    implements DataSource<T>, Comparator<Predicate<T>> {
+  protected final DataSource<T> source;
+
+  private final int start;
+  private final int cardinality;
+
+
+  public AndSource(Collection<? extends Predicate<T>> that) {
+    this(that, 0);
+  }
+
+  public AndSource(Collection<? extends Predicate<T>> that, int start) {
+    super(that);
+    checkArgument(start >= 0, "negative start: %s", start);
+    this.start = start;
+
+    int c = Integer.MAX_VALUE;
+    DataSource<T> s = null;
+    int minCost = Integer.MAX_VALUE;
+    for (Predicate<T> 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<T> read() throws OrmException {
+    try {
+      return readImpl();
+    } catch (OrmRuntimeException err) {
+      Throwables.propagateIfInstanceOf(err.getCause(), OrmException.class);
+      throw new OrmException(err);
+    }
+  }
+
+  private ResultSet<T> readImpl() throws OrmException {
+    if (source == null) {
+      throw new OrmException("No DataSource: " + this);
+    }
+    List<T> 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<T> p = (Paginated<T>) source;
+      while (skipped && r.size() < p.getOptions().limit() + start) {
+        skipped = false;
+        ResultSet<T> 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<T> buffer(ResultSet<T> scanner) {
+    return FluentIterable.from(Iterables.partition(scanner, 50))
+        .transformAndConcat(new Function<List<T>, List<T>>() {
+          @Override
+          public List<T> apply(List<T> buffer) {
+            return transformBuffer(buffer);
+          }
+        });
+  }
+
+  protected List<T> transformBuffer(List<T> buffer) throws OrmRuntimeException {
+    return buffer;
+  }
+
+  @Override
+  public int getCardinality() {
+    return cardinality;
+  }
+
+  private List<Predicate<T>> sort(Collection<? extends Predicate<T>> that) {
+    List<Predicate<T>> r = new ArrayList<>(that);
+    Collections.sort(r, this);
+    return r;
+  }
+
+  @Override
+  public int compare(Predicate<T> a, Predicate<T> 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<T> toDataSource(Predicate<T> pred) {
+    return (DataSource<T>) pred;
+  }
+}

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<T> {
     }
   }
 
-  private final IndexCollection<?, T, ? extends Index<?, T>> indexes;
+  protected final Provider<CurrentUser> userProvider;
+
   private final Metrics metrics;
   private final SchemaDefinitions<T> schemaDef;
   private final IndexConfig indexConfig;
+  private final IndexCollection<?, T, ? extends Index<?, T>> indexes;
   private final IndexRewriter<T> rewriter;
   private final String limitField;
 
   protected int start;
 
+  private boolean enforceVisibility = true;
   private int limitFromCaller;
   private Set<String> requestedFields;
 
-  protected QueryProcessor(Metrics metrics,
+  protected QueryProcessor(
+      Provider<CurrentUser> userProvider,
+      Metrics metrics,
       SchemaDefinitions<T> schemaDef,
       IndexConfig indexConfig,
       IndexCollection<?, T, ? extends Index<?, T>> indexes,
       IndexRewriter<T> rewriter,
       String limitField) {
+    this.userProvider = userProvider;
     this.metrics = metrics;
     this.schemaDef = schemaDef;
     this.indexConfig = indexConfig;
@@ -85,6 +94,11 @@ public abstract class QueryProcessor<T> {
     return this;
   }
 
+  public QueryProcessor<T> enforceVisibility(boolean enforce) {
+    enforceVisibility = enforce;
+    return this;
+  }
+
   public QueryProcessor<T> setLimit(int n) {
     limitFromCaller = n;
     return this;
@@ -157,7 +171,10 @@ public abstract class QueryProcessor<T> {
       // ask for one more result from the query.
       QueryOptions opts =
           createOptions(indexConfig, page, limit, getRequestedFields());
-      Predicate<T> pred = postRewrite(rewriter.rewrite(q, opts));
+      Predicate<T> pred = rewriter.rewrite(q, opts);
+      if (enforceVisibility) {
+        pred = enforceVisibility(pred);
+      }
       predicates.add(pred);
 
       @SuppressWarnings("unchecked")
@@ -192,15 +209,13 @@ public abstract class QueryProcessor<T> {
   }
 
   /**
-   * Invoked after the query was rewritten. Subclasses may overwrite this method
+   * Invoked after the query was rewritten. Subclasses must overwrite this
-   * to add additional predicates to the query before it is executed.
+   * method to filter out results that are not visible to the calling user.
    *
    * @param pred the query
    * @return the modified query
    */
-  protected Predicate<T> postRewrite(Predicate<T> pred) {
+  protected abstract Predicate<T> enforceVisibility(Predicate<T> pred);
-    return pred;
-  }
 
   private Set<String> getRequestedFields() {
     if (requestedFields != null) {
@@ -216,7 +231,12 @@ public abstract class QueryProcessor<T> {
     return getPermittedLimit() <= 0;
   }
 
-  protected int getPermittedLimit() {
+  private int getPermittedLimit() {
+    if (enforceVisibility) {
+      return userProvider.get().getCapabilities()
+        .getRange(GlobalCapability.QUERY_LIMIT)
+        .getMax();
+    }
     return Integer.MAX_VALUE;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryBuilder.java

@@ -38,6 +38,7 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
 
   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<AccountState, AccountQueryBuilder> mydef =
       new QueryBuilder.Definition<>(AccountQueryBuilder.class);

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<AccountState> {
+  private final AccountControl.Factory accountControlFactory;
 
-  protected AccountQueryProcessor(Metrics metrics,
+  @Inject
+  protected AccountQueryProcessor(Provider<CurrentUser> userProvider,
+      Metrics metrics,
       IndexConfig indexConfig,
       AccountIndexCollection indexes,
-      AccountIndexRewriter rewriter) {
+      AccountIndexRewriter rewriter,
-    super(metrics, AccountSchemaDefinitions.INSTANCE, indexConfig, indexes,
+      AccountControl.Factory accountControlFactory) {
-        rewriter, FIELD_LIMIT);
+    super(userProvider, metrics, AccountSchemaDefinitions.INSTANCE, indexConfig,
+        indexes, rewriter, FIELD_LIMIT);
+    this.accountControlFactory = accountControlFactory;
+  }
+
+  @Override
+  protected Predicate<AccountState> enforceVisibility(
+      Predicate<AccountState> pred) {
+    return new AndSource<>(ImmutableList.of(pred,
+        new IsVisibleToPredicate(accountControlFactory.get())));
   }
 }

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<AccountState> {
+  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;
+  }
+}

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<ChangeData>
+    implements ChangeDataSource {
+
+  public AndChangeSource(Collection<? extends Predicate<ChangeData>> that) {
+    super(that, 0);
+  }
+
+  public AndChangeSource(Collection<? extends Predicate<ChangeData>> that,
+      int start) {
+    super(that, start);
+  }
+
+  @Override
+  public boolean hasChange() {
+    return source != null && source instanceof ChangeDataSource
+        && ((ChangeDataSource) source).hasChange();
+  }
+
+  @Override
+  protected List<ChangeData> transformBuffer(List<ChangeData> 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<ChangeData> a, Predicate<ChangeData> 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;
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/query/change/AndSource.java

@@ -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<ChangeData>
-    implements ChangeDataSource {
-  private static final Comparator<Predicate<ChangeData>> CMP =
-      new Comparator<Predicate<ChangeData>>() {
-        @Override
-        public int compare(Predicate<ChangeData> a, Predicate<ChangeData> 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<Predicate<ChangeData>> sort(
-      Collection<? extends Predicate<ChangeData>> that) {
-    List<Predicate<ChangeData>> r = new ArrayList<>(that);
-    Collections.sort(r, CMP);
-    return r;
-  }
-
-  private final int start;
-  private int cardinality = -1;
-
-  public AndSource(Collection<? extends Predicate<ChangeData>> that) {
-    this(that, 0);
-  }
-
-  public AndSource(Collection<? extends Predicate<ChangeData>> 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<ChangeData> read() throws OrmException {
-    try {
-      return readImpl();
-    } catch (OrmRuntimeException err) {
-      Throwables.propagateIfInstanceOf(err.getCause(), OrmException.class);
-      throw new OrmException(err);
-    }
-  }
-
-  private ResultSet<ChangeData> readImpl() throws OrmException {
-    ChangeDataSource source = source();
-    if (source == null) {
-      throw new OrmException("No ChangeDataSource: " + this);
-    }
-    List<ChangeData> 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<ChangeData> p = (Paginated<ChangeData>) source;
-      while (skipped && r.size() < p.getOptions().limit() + start) {
-        skipped = false;
-        ResultSet<ChangeData> 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<ChangeData> buffer(
-      ChangeDataSource source,
-      ResultSet<ChangeData> scanner) {
-    final boolean loadChange = !source.hasChange();
-    return FluentIterable
-      .from(Iterables.partition(scanner, 50))
-      .transformAndConcat(new Function<List<ChangeData>, List<ChangeData>>() {
-        @Override
-        public List<ChangeData> apply(List<ChangeData> 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<ChangeData> s = null;
-    for (Predicate<ChangeData> 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<ChangeData> p : getChildren()) {
-        if (p instanceof ChangeDataSource) {
-          int c = ((ChangeDataSource) p).getCardinality();
-          cardinality = Math.min(cardinality, c);
-        }
-      }
-    }
-    return cardinality;
-  }
-}

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<ChangeData> {
   private final Provider<ReviewDb> db;
-  private final Provider<CurrentUser> 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<ChangeData> {
   }
 
   @Inject
-  ChangeQueryProcessor(Metrics metrics,
+  ChangeQueryProcessor(Provider<CurrentUser> userProvider,
+      Metrics metrics,
       IndexConfig indexConfig,
       ChangeIndexCollection indexes,
       ChangeIndexRewriter rewriter,
       Provider<ReviewDb> db,
-      Provider<CurrentUser> 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<ChangeData> {
   }
 
   @Override
-  protected Predicate<ChangeData> postRewrite(Predicate<ChangeData> pred) {
+  protected Predicate<ChangeData> enforceVisibility(
-    if (enforceVisibility) {
+      Predicate<ChangeData> pred) {
-      return new AndSource(ImmutableList.of(pred, new IsVisibleToPredicate(db,
+    return new AndChangeSource(ImmutableList.of(pred, new IsVisibleToPredicate(db,
-          notesFactory, changeControlFactory, userProvider.get())), start);
+        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;
   }
 }

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<ChangeData> in = parse("foo:a");
     Predicate<ChangeData> 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<ChangeData> in = parse("foo:a OR foo:b");
     Predicate<ChangeData> 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<ChangeData> in = parse("foo:a file:b");
     Predicate<ChangeData> 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<ChangeData> in = parse("-foo:a (file:b OR file:c)");
     Predicate<ChangeData> 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<ChangeData> in = parse("status:new bar:p file:a");
     Predicate<ChangeData> 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<ChangeData> in =
         parse("(status:new OR status:draft) bar:p file:a");
     Predicate<ChangeData> 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<ChangeData> in =
         parse("(status:new OR file:a) bar:p file:b");
     Predicate<ChangeData> 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<ChangeData> in = parse("limit:1 file:a limit:3");
     Predicate<ChangeData> 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<ChangeData>... preds) {
+  private static AndChangeSource andSource(Predicate<ChangeData>... preds) {
-    return new AndSource(Arrays.asList(preds));
+    return new AndChangeSource(Arrays.asList(preds));
   }
 
   private Predicate<ChangeData> rewrite(Predicate<ChangeData> in)

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