Replace Authorizer with Predicate<String>
#64
Conversation
# Conflicts: # src/main/java/org/apache/accumulo/access/AccessEvaluator.java
|
|
||
| /** | ||
| * Creates an AccessEvaluator from an Authorizer object | ||
| * Creates an AccessEvaluator from a Predicate<String> |
There was a problem hiding this comment.
I wonder if we need a little more verbiage to explain when the predicate should return true vs false. Given that this is part of the public API we may want to explain that the AccessEvaluator implementation is going to call test passing in a single Authorization and it should return true if the exact String matches.
There was a problem hiding this comment.
Added to the javadoc in 4e80d25 to try to address this.
| } | ||
|
|
||
| /** | ||
| * An interface that is used to check if an authorization seen in an access expression is |
There was a problem hiding this comment.
This javadoc should be moved (with rewording) to the method that now takes a predicate. Need to explain what the purpose of the predicate is, which this javadoc kinda does.
There was a problem hiding this comment.
Added to the javadoc in 4e80d25 to try to address this.
|
Looking at the amount of code that has changed in this PR, it's not much at all. I think all we have done here is remove an interface which makes clear based on the name of the method and argument how it's used and replaced it with a generic object that is unclear. Because of that we have to try and explain to the user what they should implement in javadoc based on how it's going to be used. I'm not sold on the fact that we should make this change. |
Makes sense to me. I can go ahead and close this PR for now. The ticket also mentioned:
Does anyone have an opinion on whether this might be worth it or not? I am fine with just closing this PR and the ticket but just wanted to explore all the ideas mentioned in the ticket. |
|
@DomGarguilo - I'm going to take @keith-turner 's thumbs up to my comment to mean that he agrees with me. Given that the issue was created by @ctubbsii, maybe leave this open and un-merged to see if he has any thoughts. |
|
If Authorizer is kept, then it should have the |
3 participants
Fixes #57