Deduplicate PermissionBackendConditions

This change adapts the PermissionBackend to only accept a Set of
PermissionBackendConditions and put the burden of deduplication onto
Gerrit rather than the PermissionBackend.

This makes sense since most likely all PermissionBackends want to
deduplicate calls before evaluating them assuming evaluations are in
general more expensive compared to in-memory operations like
deduplications.

This commit adds logic for deduplicating PermissionBackendConditions to
UIActions - the only caller of the method. It also adds a test to ensure
that deduplication works as expected.

Change-Id: I2571cbc9d9785843fae14b90dfe3910c50094b0e

java/com/google/gerrit/server/extensions/webui/UiActions.java

@@ -18,6 +18,7 @@ import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
 import static com.google.gerrit.extensions.conditions.BooleanCondition.or;
 import static java.util.stream.Collectors.toList;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Predicate;
 import com.google.common.collect.Streams;
 import com.google.gerrit.common.Nullable;
@@ -38,8 +39,10 @@ import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import org.slf4j.Logger;
@@ -80,11 +83,26 @@ public class UiActions {
                 descs.stream().flatMap(u -> Streams.stream(visibleCondition(u))),
                 descs.stream().flatMap(u -> Streams.stream(enabledCondition(u))))
             .collect(toList());
-    permissionBackend.bulkEvaluateTest(conds);
+
+    evaluatePermissionBackendConditions(permissionBackend, conds);
 
     return descs.stream().filter(u -> u.isVisible()).collect(toList());
   }
 
+  @VisibleForTesting
+  static void evaluatePermissionBackendConditions(
+      PermissionBackend perm, List<PermissionBackendCondition> conds) {
+    Map<PermissionBackendCondition, PermissionBackendCondition> dedupedConds =
+        new HashMap<>(conds.size());
+    for (PermissionBackendCondition cond : conds) {
+      dedupedConds.put(cond, cond);
+    }
+    perm.bulkEvaluateTest(dedupedConds.keySet());
+    for (PermissionBackendCondition cond : conds) {
+      cond.set(dedupedConds.get(cond).value());
+    }
+  }
+
   private static Iterable<PermissionBackendCondition> visibleCondition(Description u) {
     return u.getVisibleCondition().reduce().children(PermissionBackendCondition.class);
   }

java/com/google/gerrit/server/permissions/PermissionBackend.java

@@ -95,19 +95,15 @@ public abstract class PermissionBackend {
   }
 
   /**
-   * Bulk evaluate a collection of {@link PermissionBackendCondition} for view handling.
+   * Bulk evaluate a set of {@link PermissionBackendCondition} for view handling.
    *
    * <p>Overridden implementations should call {@link PermissionBackendCondition#set(boolean)} to
    * cache the result of {@code testOrFalse} in the condition for later evaluation. Caching the
    * result will bypass the usual invocation of {@code testOrFalse}.
    *
-   * <p>{@code conds} may contain duplicate entries (such as same user, resource, permission
-   * triplet). When duplicates exist, implementations should set a result into all instances to
-   * ensure {@code testOrFalse} does not get invoked during evaluation of the containing condition.
-   *
    * @param conds conditions to consider.
    */
-  public void bulkEvaluateTest(Collection<PermissionBackendCondition> conds) {
+  public void bulkEvaluateTest(Set<PermissionBackendCondition> conds) {
     // Do nothing by default. The default implementation of PermissionBackendCondition
     // delegates to the appropriate testOrFalse method in PermissionBackend.
   }

javatests/com/google/gerrit/server/extensions/webui/UiActionsTest.java

@@ -0,0 +1,123 @@
+// Copyright (C) 2018 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.extensions.webui;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.account.GroupMembership;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackend.ForProject;
+import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
+import com.google.gerrit.server.permissions.PermissionBackendCondition;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.ProjectPermission;
+import java.util.Collection;
+import java.util.Set;
+import org.easymock.EasyMock;
+import org.junit.Test;
+
+public class UiActionsTest {
+
+  private static class FakeForProject extends ForProject {
+    private boolean allowValueQueries = true;
+
+    @Override
+    public CurrentUser user() {
+      return new CurrentUser() {
+        @Override
+        public GroupMembership getEffectiveGroups() {
+          throw new UnsupportedOperationException("not implemented");
+        }
+
+        @Override
+        public boolean isIdentifiedUser() {
+          return true;
+        }
+
+        @Override
+        public Account.Id getAccountId() {
+          return new Account.Id(1);
+        }
+      };
+    }
+
+    @Override
+    public String resourcePath() {
+      return "/projects/test-project";
+    }
+
+    @Override
+    public ForProject user(CurrentUser user) {
+      throw new UnsupportedOperationException("not implemented");
+    }
+
+    @Override
+    public ForRef ref(String ref) {
+      throw new UnsupportedOperationException("not implemented");
+    }
+
+    @Override
+    public void check(ProjectPermission perm) throws AuthException, PermissionBackendException {
+      throw new UnsupportedOperationException("not implemented");
+    }
+
+    @Override
+    public Set<ProjectPermission> test(Collection<ProjectPermission> permSet)
+        throws PermissionBackendException {
+      assertThat(allowValueQueries).isTrue();
+      return ImmutableSet.of(ProjectPermission.READ);
+    }
+
+    private void disallowValueQueries() {
+      allowValueQueries = false;
+    }
+  }
+
+  @Test
+  public void permissionBackendConditionEvaluationDeduplicatesAndBackfills() throws Exception {
+    FakeForProject forProject = new FakeForProject();
+
+    // Create three conditions, two of which are identical
+    PermissionBackendCondition cond1 =
+        (PermissionBackendCondition) forProject.testCond(ProjectPermission.CREATE_CHANGE);
+    PermissionBackendCondition cond2 =
+        (PermissionBackendCondition) forProject.testCond(ProjectPermission.READ);
+    PermissionBackendCondition cond3 =
+        (PermissionBackendCondition) forProject.testCond(ProjectPermission.CREATE_CHANGE);
+
+    // Set up the Mock to expect a call of bulkEvaluateTest to only contain cond{1,2} since cond3
+    // needs to be identified as duplicate and not called out explicitly.
+    PermissionBackend permissionBackendMock = EasyMock.createMock(PermissionBackend.class);
+    permissionBackendMock.bulkEvaluateTest(ImmutableSet.of(cond1, cond2));
+    EasyMock.replay(permissionBackendMock);
+
+    UiActions.evaluatePermissionBackendConditions(
+        permissionBackendMock, ImmutableList.of(cond1, cond2, cond3));
+
+    // Disallow queries for value to ensure that cond3 (previously left behind) is backfilled with
+    // the value of cond1 and issues no additional call to PermissionBackend.
+    forProject.disallowValueQueries();
+
+    // Assert the values of all conditions
+    assertThat(cond1.value()).isFalse();
+    assertThat(cond2.value()).isTrue();
+    assertThat(cond3.value()).isFalse();
+  }
+}