PermissionBackend: support bulk evaluation of UiAction
Support a PermissionBackend to consider all actions on a single REST
API resource in one pass by aggregating PermissionBackendConditions
and passing them through bulkEvaluateTest.
An implementation should use PermissionBackendCondition.set(boolean)
to cache any decision from testOrFalse, allowing the final isVisible
and isEnabled evaluations to bypass calling testOrFalse dynamically.
Change-Id: I054a351294fafd917c111ad44ef7ee56fc1931a2
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java
index 19fdcfb..a1deb89 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ActionJson.java
@@ -16,7 +16,6 @@
import static com.google.common.base.Preconditions.checkNotNull;
-import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
@@ -36,6 +35,7 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
@@ -162,7 +162,7 @@
return out;
}
- FluentIterable<UiAction.Description> descs =
+ Iterable<UiAction.Description> descs =
uiActions.from(changeViews, changeResourceFactory.create(ctl));
// The followup action is a client-side only operation that does not
@@ -174,7 +174,7 @@
PrivateInternals_UiActionDescription.setMethod(descr, "POST");
descr.setTitle("Create follow-up change");
descr.setLabel("Follow-Up");
- descs = descs.append(descr);
+ descs = Iterables.concat(descs, Collections.singleton(descr));
}
ACTION:
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/extensions/webui/UiActions.java b/gerrit-server/src/main/java/com/google/gerrit/server/extensions/webui/UiActions.java
index ae15cfd..2993fa8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/extensions/webui/UiActions.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/extensions/webui/UiActions.java
@@ -14,8 +14,10 @@
package com.google.gerrit.server.extensions.webui;
+import static java.util.stream.Collectors.toList;
+
import com.google.common.base.Predicate;
-import com.google.common.collect.FluentIterable;
+import com.google.common.collect.Streams;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -24,13 +26,16 @@
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.webui.PrivateInternals_UiActionDescription;
import com.google.gerrit.extensions.webui.UiAction;
+import com.google.gerrit.extensions.webui.UiAction.Description;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendCondition;
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.List;
import java.util.Objects;
import java.util.Set;
import org.slf4j.Logger;
@@ -53,16 +58,35 @@
this.userProvider = userProvider;
}
- public <R extends RestResource> FluentIterable<UiAction.Description> from(
+ public <R extends RestResource> Iterable<UiAction.Description> from(
RestCollection<?, R> collection, R resource) {
return from(collection.views(), resource);
}
- public <R extends RestResource> FluentIterable<UiAction.Description> from(
+ public <R extends RestResource> Iterable<UiAction.Description> from(
DynamicMap<RestView<R>> views, R resource) {
- return FluentIterable.from(views)
- .transform((e) -> describe(e, resource))
- .filter(Objects::nonNull);
+ List<UiAction.Description> descs =
+ Streams.stream(views)
+ .map(e -> describe(e, resource))
+ .filter(Objects::nonNull)
+ .collect(toList());
+
+ List<PermissionBackendCondition> conds =
+ Streams.concat(
+ descs.stream().flatMap(u -> Streams.stream(visibleCondition(u))),
+ descs.stream().flatMap(u -> Streams.stream(enabledCondition(u))))
+ .collect(toList());
+ permissionBackend.bulkEvaluateTest(conds);
+
+ return descs.stream().filter(u -> u.isVisible()).collect(toList());
+ }
+
+ private static Iterable<PermissionBackendCondition> visibleCondition(Description u) {
+ return u.getVisibleCondition().children(PermissionBackendCondition.class);
+ }
+
+ private static Iterable<PermissionBackendCondition> enabledCondition(Description u) {
+ return u.getEnabledCondition().children(PermissionBackendCondition.class);
}
@Nullable
@@ -100,7 +124,7 @@
}
UiAction.Description dsc = ((UiAction<R>) view).getDescription(resource);
- if (dsc == null || !dsc.isVisible()) {
+ if (dsc == null) {
return null;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
index 0476ce2..5c0cf44 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -95,6 +95,24 @@
return user(checkNotNull(user, "Provider<CurrentUser>").get());
}
+ /**
+ * Bulk evaluate a collection 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) {
+ // Do nothing by default. The default implementation of PermissionBackendCondition
+ // delegates to the appropriate testOrFalse method in PermissionBackend.
+ }
+
/** PermissionBackend with an optional per-request ReviewDb handle. */
public abstract static class AcceptsReviewDb<T> {
protected Provider<ReviewDb> db;
