opendev/gerrit
Code Issues Proposed changes

Merge "Reduce BooleanCondition predicate tree before evaluating permissions"

Browse Source
This commit is contained in:
Patrick Hiesel
2018-01-26 08:27:23 +00:00
committed by Gerrit Code Review
parent 22fad17efe 2ce34c2b9e
commit 15d2c4fa2c
5 changed files with 281 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

105
java/com/google/gerrit/extensions/conditions/BooleanCondition.java
View File

@@ -59,6 +59,40 @@ public abstract class BooleanCondition {
*/
public abstract <T> Iterable<T> children(Class<T> type);
/**
* Reduce evaluation tree by cutting off branches that evaluate trivially and replacing them with
* a leave note corresponding to the value the branch evaluated to.
*
* <p><code>
* Example 1 (T=True, F=False, C=non-trivial check):
* OR
* / \ => T
* C T
* Example 2 (cuts off a not-trivial check):
* AND
* / \ => F
* C F
* Example 3:
* AND
* / \ => F
* T F
* </code>
*
* <p>There is no guarantee that the resulting tree is minimal. The only guarantee made is that
* branches that evaluate trivially will be cut off and replaced by primitive values.
*/
public abstract BooleanCondition reduce();
/**
* Check if the condition evaluates to either {@code true} or {@code false} without providing
* additional information to the evaluation tree, e.g. through checks to a remote service such as
* {@code PermissionBackend}.
*
* <p>In this case, the tree can be reduced to skip all non-trivial checks resulting in a
* performance gain.
*/
protected abstract boolean evaluatesTrivially();
private static final class And extends BooleanCondition {
private final BooleanCondition a;
private final BooleanCondition b;
@@ -70,6 +104,10 @@ public abstract class BooleanCondition {
@Override
public boolean value() {
if (evaluatesTriviallyToExpectedValue(a, false)
|| evaluatesTriviallyToExpectedValue(b, false)) {
return false;
}
return a.value() && b.value();
}
@@ -78,6 +116,14 @@ public abstract class BooleanCondition {
return Iterables.concat(a.children(type), b.children(type));
}
@Override
public BooleanCondition reduce() {
if (evaluatesTrivially()) {
return Value.valueOf(value());
}
return new And(a.reduce(), b.reduce());
}
@Override
public int hashCode() {
return a.hashCode() * 31 + b.hashCode();
@@ -96,6 +142,13 @@ public abstract class BooleanCondition {
public String toString() {
return "(" + maybeTrim(a, getClass()) + " && " + maybeTrim(a, getClass()) + ")";
}
@Override
protected boolean evaluatesTrivially() {
return evaluatesTriviallyToExpectedValue(a, false)
|| evaluatesTriviallyToExpectedValue(b, false)
|| (a.evaluatesTrivially() && b.evaluatesTrivially());
}
}
private static final class Or extends BooleanCondition {
@@ -109,6 +162,10 @@ public abstract class BooleanCondition {
@Override
public boolean value() {
if (evaluatesTriviallyToExpectedValue(a, true)
|| evaluatesTriviallyToExpectedValue(b, true)) {
return true;
}
return a.value() || b.value();
}
@@ -117,6 +174,14 @@ public abstract class BooleanCondition {
return Iterables.concat(a.children(type), b.children(type));
}
@Override
public BooleanCondition reduce() {
if (evaluatesTrivially()) {
return Value.valueOf(value());
}
return new Or(a.reduce(), b.reduce());
}
@Override
public int hashCode() {
return a.hashCode() * 31 + b.hashCode();
@@ -135,6 +200,13 @@ public abstract class BooleanCondition {
public String toString() {
return "(" + maybeTrim(a, getClass()) + " || " + maybeTrim(a, getClass()) + ")";
}
@Override
protected boolean evaluatesTrivially() {
return evaluatesTriviallyToExpectedValue(a, true)
|| evaluatesTriviallyToExpectedValue(b, true)
|| (a.evaluatesTrivially() && b.evaluatesTrivially());
}
}
private static final class Not extends BooleanCondition {
@@ -154,6 +226,14 @@ public abstract class BooleanCondition {
return cond.children(type);
}
@Override
public BooleanCondition reduce() {
if (evaluatesTrivially()) {
return Value.valueOf(value());
}
return this;
}
@Override
public int hashCode() {
return cond.hashCode() * 31;
@@ -168,6 +248,11 @@ public abstract class BooleanCondition {
public String toString() {
return "!" + cond;
}
@Override
protected boolean evaluatesTrivially() {
return cond.evaluatesTrivially();
}
}
private static final class Value extends BooleanCondition {
@@ -187,6 +272,11 @@ public abstract class BooleanCondition {
return Collections.emptyList();
}
@Override
public BooleanCondition reduce() {
return this;
}
@Override
public int hashCode() {
return value ? 1 : 0;
@@ -201,9 +291,17 @@ public abstract class BooleanCondition {
public String toString() {
return Boolean.toString(value);
}
@Override
protected boolean evaluatesTrivially() {
return true;
}
}
/** Remove leading '(' and trailing ')' if the type is the same as the parent. */
/**
* Helper for use in toString methods. Remove leading '(' and trailing ')' if the type is the same
* as the parent.
*/
static String maybeTrim(BooleanCondition cond, Class<? extends BooleanCondition> type) {
String s = cond.toString();
if (cond.getClass() == type
@@ -214,4 +312,9 @@ public abstract class BooleanCondition {
}
return s;
}
private static boolean evaluatesTriviallyToExpectedValue(
BooleanCondition cond, boolean expectedValue) {
return cond.evaluatesTrivially() && (cond.value() == expectedValue);
}
}

4
java/com/google/gerrit/server/extensions/webui/UiActions.java
View File

@@ -86,11 +86,11 @@ public class UiActions {
}
private static Iterable<PermissionBackendCondition> visibleCondition(Description u) {
return u.getVisibleCondition().children(PermissionBackendCondition.class);
return u.getVisibleCondition().reduce().children(PermissionBackendCondition.class);
}
private static Iterable<PermissionBackendCondition> enabledCondition(Description u) {
return u.getEnabledCondition().children(PermissionBackendCondition.class);
return u.getEnabledCondition().reduce().children(PermissionBackendCondition.class);
}
@Nullable

13
java/com/google/gerrit/server/permissions/PermissionBackendCondition.java
View File

@@ -38,6 +38,19 @@ public abstract class PermissionBackendCondition
@Override
public abstract String toString();
@Override
public boolean evaluatesTrivially() {
// PermissionBackendCondition needs to contact PermissionBackend so trivial evaluation is not
// possible.
return false;
}
@Override
public BooleanCondition reduce() {
// No reductions can be made
return this;
}
public static class WithUser extends PermissionBackendCondition {
private final PermissionBackend.WithUser impl;
private final GlobalOrPluginPermission perm;

10
javatests/com/google/gerrit/extensions/conditions/BUILD Normal file
View File

@@ -0,0 +1,10 @@
load("//tools/bzl:junit.bzl", "junit_tests")
junit_tests(
name = "conditions_tests",
srcs = glob(["*.java"]),
deps = [
"//java/com/google/gerrit/extensions:lib",
"//lib:truth",
],
)

152
javatests/com/google/gerrit/extensions/conditions/BooleanConditionTest.java Normal file
View File

@@ -0,0 +1,152 @@
// 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.extensions.conditions;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
import static com.google.gerrit.extensions.conditions.BooleanCondition.not;
import static com.google.gerrit.extensions.conditions.BooleanCondition.or;
import static com.google.gerrit.extensions.conditions.BooleanCondition.valueOf;
import static org.junit.Assert.assertEquals;
import org.junit.Test;
public class BooleanConditionTest {
private static final BooleanCondition NO_TRIVIAL_EVALUATION =
new BooleanCondition() {
@Override
public boolean value() {
throw new UnsupportedOperationException("value() is not supported");
}
@Override
public <T> Iterable<T> children(Class<T> type) {
throw new UnsupportedOperationException("children(Class<T> type) is not supported");
}
@Override
public BooleanCondition reduce() {
return this;
}
@Override
protected boolean evaluatesTrivially() {
return false;
}
};
@Test
public void reduceAnd_CutOffNonTrivialWhenPossible() throws Exception {
BooleanCondition nonReduced = and(false, NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = valueOf(false);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceAnd_CutOffNonTrivialWhenPossibleSwapped() throws Exception {
BooleanCondition nonReduced = and(NO_TRIVIAL_EVALUATION, valueOf(false));
BooleanCondition reduced = valueOf(false);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceAnd_KeepNonTrivialWhenNoCutOffPossible() throws Exception {
BooleanCondition nonReduced = and(true, NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = and(true, NO_TRIVIAL_EVALUATION);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceAnd_KeepNonTrivialWhenNoCutOffPossibleSwapped() throws Exception {
BooleanCondition nonReduced = and(NO_TRIVIAL_EVALUATION, valueOf(true));
BooleanCondition reduced = and(NO_TRIVIAL_EVALUATION, valueOf(true));
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceOr_CutOffNonTrivialWhenPossible() throws Exception {
BooleanCondition nonReduced = or(true, NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = valueOf(true);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceOr_CutOffNonTrivialWhenPossibleSwapped() throws Exception {
BooleanCondition nonReduced = or(NO_TRIVIAL_EVALUATION, valueOf(true));
BooleanCondition reduced = valueOf(true);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceOr_KeepNonTrivialWhenNoCutOffPossible() throws Exception {
BooleanCondition nonReduced = or(false, NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = or(false, NO_TRIVIAL_EVALUATION);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceOr_KeepNonTrivialWhenNoCutOffPossibleSwapped() throws Exception {
BooleanCondition nonReduced = or(NO_TRIVIAL_EVALUATION, valueOf(false));
BooleanCondition reduced = or(NO_TRIVIAL_EVALUATION, valueOf(false));
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceNot_ReduceIrrelevant() throws Exception {
BooleanCondition nonReduced = not(valueOf(true));
BooleanCondition reduced = valueOf(false);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceNot_ReduceIrrelevant2() throws Exception {
BooleanCondition nonReduced = not(valueOf(false));
BooleanCondition reduced = valueOf(true);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceNot_KeepNonTrivialWhenNoCutOffPossible() throws Exception {
BooleanCondition nonReduced = not(NO_TRIVIAL_EVALUATION);
BooleanCondition reduced = not(NO_TRIVIAL_EVALUATION);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceComplexTreeToSingleValue() throws Exception {
// AND
// / \
// OR NOT
// / \ \
// NTE NTE TRUE
BooleanCondition nonReduced =
and(or(NO_TRIVIAL_EVALUATION, NO_TRIVIAL_EVALUATION), not(valueOf(true)));
BooleanCondition reduced = valueOf(false);
assertEquals(nonReduced.reduce(), reduced);
}
@Test
public void reduceComplexTreeToSmallerTree() throws Exception {
// AND
// / \
// OR OR
// / \ / \
// NTE NTE T F
BooleanCondition nonReduced =
and(or(NO_TRIVIAL_EVALUATION, NO_TRIVIAL_EVALUATION), or(valueOf(true), valueOf(false)));
BooleanCondition reduced = and(or(NO_TRIVIAL_EVALUATION, NO_TRIVIAL_EVALUATION), valueOf(true));
assertEquals(nonReduced.reduce(), reduced);
}
}