Consider removing AccessEvaluator as an API or improving its entry points

[ctubbsii]

I think the entry point for AccessEvaluator entry point has room for improvement. Currently, the entry point is a static method named of that is overloaded. However, it almost exclusively takes an Authorizations object of some form.

Rather than construct an Authorizations object, then construct a separate AccessEvaluator object, it would probably make more sense to have a method that dangles off of Authorizations that evaluates.

For example:

var evaluator = AccessEvaluator.of(Authorizations.of("a", "b"));
// vs.
var evaluator = Authorizations.of("a", "b").evaluator();

What I'm suggesting is similar to the way Java regex Patterns can construct a Matcher. You wouldn't do Matcher m = new Matcher(Pattern.compile("p"), "other");. Instead, you do Pattern.compile("p").matcher("other"). You start with the thing you have, then you derive the related objects from it. In our case, our starting point is the Authorizations.

Also, while Authorizations and AccessExpression also use of as named entry points, I think it makes more sense to use for those than it does for AccessEvaluator. For the first two, you're literally composing them "of" the string provided, because they are simple objects that wrap and represent the thing it is being composed "of". That's not quite true for AccessEvaluator, which isn't merely wrapping, but is a tool to do evaluating work using the Authorizations used to initialize it. I think something like .compile(auths), .using(auths), or even .with(auths) might make more sense. However I'd still rather see if we can do away with it entirely, and just have an entry point off of Authorizations instead.

[keith-turner]

The following proposed API

var evaluator = Authorizations.of("a", "b").evaluator();

does not support the functionality of this existing API

[keith-turner]

I think something like .compile(auths), .using(auths), or even .with(auths) might make more sense.

I like the concept of replacing of with one of those.

[ctubbsii]

The following proposed API

var evaluator = Authorizations.of("a", "b").evaluator();

does not support the functionality of this existing API

Yeah, I thought about that, but I was wondering if it would make more sense to do something like:

Stream.of(authorizations1, authorizations2).allMatch(authorizations -> authorizations.canAccess(expression));
// or
Stream.of(authorizations1, authorizations2).anyMatch(authorizations -> authorizations.canAccess(expression));

I don't know how much optimization is actually being done, or how much utility there is in making the evaluator operate on sets of authorizations, rather than on something that I think is a bit more clear like the above on the caller's end. My instinct is that it's not much of an optimization and the above would be more clear, so we don't actually need the multiple-sets API, as it's superfluous, but I don't know for sure without digging more.

[dlmarion]

Stream.of(authorizations1, authorizations2).allMatch(authorizations -> authorizations.canAccess(expression));
// or
Stream.of(authorizations1, authorizations2).anyMatch(authorizations -> authorizations.canAccess(expression));

I don't think it would be this simple, there is some escape-handling that is occurring in the AccessEvaluatorImpl constructors.

[keith-turner]

This is the code the does the evaluation. It seems like it could be decomposed, would lose the reuse of the thread local. That code used to be a stream instead of a for loop, but we discovered that creating a stream for evaluating each access expression was cause a huge performance hit when benchmarking.

[keith-turner]

Opened #61 after looking at code for this issue. For any changes made for this issue we can benchmark the changes before and after to see what the impact is.

[ctubbsii]

@keith-turner wrote:

This is the code the does the evaluation. It seems like it could be decomposed, would lose the reuse of the thread local. That code used to be a stream instead of a for loop, but we discovered that creating a stream for evaluating each access expression was cause a huge performance hit when benchmarking.

In case my use of Streams had any bearing on your comment, my example only used Streams for a concise expression of allMatch and anyMatch. Of course, the caller could use any functionally equivalent construction of their choosing, but it doesn't really have any bearing on my proposal. But, the performance cost of losing the reuse of internal reusable objects would certainly have a bearing on my proposal.

Also, just a thought, but I wonder if using a parallel stream would have been better than switching to a for loop in the case you mentioned. It seems like we could easily evaluate Authorizations against an expression in parallel, and that has potential over the mandatory sequential nature of for loops. But, maybe there's some setup overhead that erases that. I'm not really sure. I'm sure there's lots of scenarios we could try to benchmark here.

@keith-turner wrote:

Opened #61 after looking at code for this issue. For any changes made for this issue we can benchmark the changes before and after to see what the impact is.

I haven't had a chance to play around with benchmarking my suggestions here, but if benchmarking shows that the difference is meaningful because of the reuse of internal objects that we'd be losing, then we could still have a cleaner single entry point through Authorizations, but we would have to do it differently to allow evaluation of multiple Authorizations from a single evaluator without using Evaluator itself as an entry point.

For example:

// using an intermediate obj something like AuthorizationsSet
Authorizations.of(auths1).and(Authorizations.of(auths2).evaluator();
Authorizations.of(auths1).or(Authorizations.of(auths2).evaluator();
// final obj is a new evaluator, that takes elements of the first, but updated to account for the additional authorizations to evalute
Authorizations.of(auths1).evalutor().and(Authorizations.of(auths2));
Authorizations.of(auths1).evalutor().or(Authorizations.of(auths2));
// explicit use of an authorizations set
AuthorizationsSet.of(auths1, auths2).evaluateForAll();
AuthorizationsSet.of(auths1, auths2).evaluateForAny();

Each of these constructions could be implemented to keep the reuse optimizations, but without needing Evaluator as a separate API entry point.

@dlmarion wrote:

I don't think it would be this simple, there is some escape-handling that is occurring in the AccessEvaluatorImpl constructors.

I'm not sure I understand this comment or how it relates to what I'm proposing. I think that the internal escape-handling implementation would be unaffected by anything I've suggested here.

[keith-turner]

@ctubbsii the changes in #71 move a bit closer to some the things mentioned in this PR. From a performance perspective one thing I learned from #71 is that you probably only want to convert the expression to a byte array (from a string) once when there are multiple evaluators.

[ctubbsii]

@ctubbsii the changes in #71 move a bit closer to some the things mentioned in this PR. From a performance perspective one thing I learned from #71 is that you probably only want to convert the expression to a byte array (from a string) once when there are multiple evaluators.

I like some of the improvements there. I'm not sure it addresses everything I had an interest in improving. I'll have to revisit this in more detail now that those changes are merged.