keith-turner commented on code in PR #93:
URL: https://github.com/apache/accumulo-access/pull/93#discussion_r2641339252
##########
core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java:
##########
@@ -80,6 +80,14 @@ public sealed interface AccessEvaluator permits
AccessEvaluatorImpl, MultiAccess
*/
boolean canAccess(AccessExpression accessExpression);
+ /**
+ * @param parsedAccessExpression object resulting from call to
AccessExpression.parse. This method
+ * would be useful if passing a single AccessExpression to multiple
AccessEvaluators.
+ * @return true if the expression is visible using the authorizations
supplied at creation, false
+ * otherwise
+ */
+ boolean canAccess(ParsedAccessExpression parsedAccessExpression);
Review Comment:
We could avoid adding this if AccessExpression had a `boolean hasParsed()`
method that would return true when there is an existing parsed expression and
one does not need to be created. The the impl look like the following. This
would always use the parse tree if it happens to be there.
```java
boolean canAccess(AccessExpression accessExpression){
if(accessExpression.hasParsed()){
// call can access w/ existing parse tree
canAccess(accessExpression.parse());
}else{
// call canAccess w/ expression
}
}
```
##########
core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java:
##########
@@ -46,21 +45,26 @@ private MultiAccessEvaluatorImpl(Collection<Authorizations>
authorizationSets) {
@Override
public boolean canAccess(String accessExpression) throws
InvalidAccessExpressionException {
- return canAccess(accessExpression.getBytes(UTF_8));
+ return canAccess(AccessExpression.of(accessExpression));
}
@Override
public boolean canAccess(byte[] accessExpression) throws
InvalidAccessExpressionException {
+ return canAccess(AccessExpression.of(accessExpression));
+ }
+
+ @Override
+ public boolean canAccess(AccessExpression accessExpression) {
+ return canAccess(accessExpression.parse());
Review Comment:
Have you done any testing to confirm this is faster? The code that does not
create the parse tree was a lot faster when benchmarking, but not sure by how
much. Not sure what the numbers are but if it runs in 1/5 of the time then
would need 5 auth sets before it makes sense to parse.
--
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]
