search

apache / accumulo-access

Apache Accumulo Access Control Library
https://accumulo.apache.org
Apache License 2.0
4 stars 11 forks source link

Replace `Authorizer` with `Predicate<String>` #64

Open DomGarguilo opened 8 months ago

DomGarguilo commented 8 months ago

Fixes #57

dlmarion commented 8 months ago

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.

DomGarguilo commented 8 months ago

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:

could at least extend Predicate with a default method, so either way of calling could be used

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.

dlmarion commented 8 months ago

@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.

keith-turner commented 8 months ago

If Authorizer is kept, then it should have the @FunctionalInterface annotation to signify intent and make sure that intent is not changed in the future. Since Authorizer is currently a functional interface its very easy to assign a predicate to it.