keith-turner commented on code in PR #34:
URL: https://github.com/apache/accumulo-access/pull/34#discussion_r1466703272
##########
src/main/java/org/apache/accumulo/access/AccessEvaluatorImpl.java:
##########
@@ -44,17 +43,33 @@ class AccessEvaluatorImpl implements AccessEvaluator {
private final ThreadLocal<Tokenizer> tokenizers =
ThreadLocal.withInitial(() -> new Tokenizer(EMPTY));
- private AccessEvaluatorImpl(Authorizer authorizationChecker) {
+ AccessEvaluatorImpl(Authorizer authorizationChecker) {
this.authorizedPredicates = List.of(auth ->
authorizationChecker.isAuthorized(unescape(auth)));
}
- public AccessEvaluatorImpl(Collection<List<byte[]>> authorizationSets) {
- authorizedPredicates = authorizationSets.stream()
- .map(authorizations -> authorizations.stream()
- .map(auth -> AccessEvaluatorImpl.escape(auth,
false)).map(BytesWrapper::new)
- .collect(toSet()))
- .map(escapedAuths -> (Predicate<BytesWrapper>) escapedAuths::contains)
- .collect(Collectors.toUnmodifiableList());
+ AccessEvaluatorImpl(final List<byte[]> authorizationSet) {
+ this(Collections.singletonList(authorizationSet));
+ }
+
+ AccessEvaluatorImpl(final Collection<List<byte[]>> authorizationSets) {
+
+ for (final List<byte[]> auths : authorizationSets) {
+ for (final byte[] auth : auths) {
+ if (auth.length == 0) {
+ throw new IllegalArgumentException("Empty authorization");
+ }
+ }
+ }
+
+ final List<Predicate<BytesWrapper>> predicates = new
ArrayList<>(authorizationSets.size());
+ for (final List<byte[]> authorizationList : authorizationSets) {
+ final Set<BytesWrapper> authBytes = new
HashSet<>(authorizationList.size());
+ for (final byte[] authorization : authorizationList) {
+ authBytes.add(new
BytesWrapper(AccessEvaluatorImpl.escape(authorization, false)));
+ }
+ predicates.add((auth) -> authBytes.contains(auth));
+ }
+ authorizedPredicates = Collections.unmodifiableList(predicates);
Review Comment:
Assuming authorizedPredicates is read a lot more frequently than its
created, the following would be a bit more expensive to create but probably
faster to read because there is less indirection.
```suggestion
authorizedPredicates = List.copyOf(predicates);
```
##########
src/main/java/org/apache/accumulo/access/AccessEvaluator.java:
##########
@@ -74,88 +82,112 @@ public interface AccessEvaluator {
boolean canAccess(AccessExpression accessExpression);
/**
- * An interface that is used to check if an authorization seen in an access
expression is
- * authorized.
+ * Creates an AccessEvaluator from an Authorizations object
*
- * @since 1.0.0
+ * @param authorizations auths to use in the AccessEvaluator
+ * @return AccessEvaluator object
*/
- interface Authorizer {
- boolean isAuthorized(String auth);
+ static AccessEvaluator of(Authorizations authorizations) {
+ final Set<String> authSet = authorizations.asSet();
+ final List<byte[]> authList = new ArrayList<>(authSet.size());
+ for (final String auth : authSet) {
+ authList.add(auth.getBytes(UTF_8));
+ }
+ return new AccessEvaluatorImpl(authList);
}
- interface AuthorizationsBuilder {
-
- EvaluatorBuilder authorizations(Authorizations authorizations);
-
- /**
- * Allows providing multiple sets of authorizations. Each expression will
be evaluated
- * independently against each set of authorizations and will only be
deemed accessible if
- * accessible for all. For example the following code would print false,
true, and then false.
- *
- * <pre>
- * {@code
- * Collection<Authorizations> authSets =
- * List.of(Authorizations.of("A", "B"), Authorizations.of("C", "D"));
- * var evaluator =
AccessEvaluator.builder().authorizations(authSets).build();
- *
- * System.out.println(evaluator.canAccess("A"));
- * System.out.println(evaluator.canAccess("A|D"));
- * System.out.println(evaluator.canAccess("A&D"));
- *
- * }
- * </pre>
- *
- * <p>
- * The following table shows how each expression in the example above will
evaluate for each
- * authorization set. In order to return true for {@code canAccess()} the
expression must
- * evaluate to true for each authorization set.
- *
- * <table>
- * <caption>Evaluations</caption>
- * <tr>
- * <td></td>
- * <td>[A,B]</td>
- * <td>[C,D]</td>
- * </tr>
- * <tr>
- * <td>A</td>
- * <td>True</td>
- * <td>False</td>
- * </tr>
- * <tr>
- * <td>A|D</td>
- * <td>True</td>
- * <td>True</td>
- * </tr>
- * <tr>
- * <td>A&D</td>
- * <td>False</td>
- * <td>False</td>
- * </tr>
- *
- * </table>
- *
- *
- *
- */
- EvaluatorBuilder authorizations(Collection<Authorizations> authorizations);
-
- /**
- * Allows specifying a single set of authorizations.
- */
- EvaluatorBuilder authorizations(String... authorizations);
+ /**
+ * Creates an AccessEvaluator from an Authorizer object
+ *
+ * @param authorizer authorizer to use in the AccessEvaluator
+ * @return AccessEvaluator object
+ */
+ static AccessEvaluator of(Authorizer authorizer) {
+ return new AccessEvaluatorImpl(authorizer);
+ }
- /**
- * Allows specifying an authorizer that is analogous to a single set of
authorization.
- */
- EvaluatorBuilder authorizations(Authorizer authorizer);
+ /**
+ * Allows providing multiple sets of authorizations. Each expression will be
evaluated
+ * independently against each set of authorizations and will only be deemed
accessible if
+ * accessible for all. For example the following code would print false,
true, and then false.
+ *
+ * <pre>
+ * {@code
+ * Collection<Authorizations> authSets =
+ * List.of(Authorizations.of("A", "B"), Authorizations.of("C", "D"));
+ * var evaluator = AccessEvaluator.of(authSets);
+ *
+ * System.out.println(evaluator.canAccess("A"));
+ * System.out.println(evaluator.canAccess("A|D"));
+ * System.out.println(evaluator.canAccess("A&D"));
+ *
+ * }
+ * </pre>
+ *
+ * <p>
+ * The following table shows how each expression in the example above will
evaluate for each
+ * authorization set. In order to return true for {@code canAccess()} the
expression must evaluate
+ * to true for each authorization set.
+ *
+ * <table>
+ * <caption>Evaluations</caption>
+ * <tr>
+ * <td></td>
+ * <td>[A,B]</td>
+ * <td>[C,D]</td>
+ * </tr>
+ * <tr>
+ * <td>A</td>
+ * <td>True</td>
+ * <td>False</td>
+ * </tr>
+ * <tr>
+ * <td>A|D</td>
+ * <td>True</td>
+ * <td>True</td>
+ * </tr>
+ * <tr>
+ * <td>A&D</td>
+ * <td>False</td>
+ * <td>False</td>
+ * </tr>
+ *
+ * </table>
+ *
+ *
+ *
+ */
+ static AccessEvaluator of(Collection<Authorizations> authorizationSets) {
+ final List<List<byte[]>> authorizationLists = new
ArrayList<>(authorizationSets.size());
Review Comment:
I think its nice to minimize the code in the interface in order to make it
easier for someone to quickly read the entire source code for the interface.
Could push this into a static method in AccessEvaluatorImpl in order to
minimize the size of the interface source code. This is a style comment, so
feel free to ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
