apache / accumulo-access

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

Modified Authorizations.of to handle duplicate input elements #68

Closed dlmarion closed 6 months ago

dlmarion commented 6 months ago

Authorizations.of will either throw an IllegalArgumentException or deduplicate the input parameters depending on the value of a new boolean parameter.

Closes #66

dlmarion commented 6 months ago

I don't really like the boolean, but these change fix the problem of the inconsistent behavior and document it and test it. Personally I think we should drop the boolean and pick one consistent behavior (either dedupe or throw an exception) for both methods and document the behavior in javadoc and test it.

I don't like the boolean or throwing the exception, but this is middle-ground. Personally, I think we should just change the API to accept a Set instead of a Collection or array. That removes all ambiguity and places the burden on the user to supply us with a unique set of values without throwing an exception.

keith-turner commented 6 months ago

Personally, I think we should just change the API to accept a Set instead of a Collection or array. That removes all ambiguity and places the burden on the user to supply us with a unique set of values without throwing an exception.

I like a single method that takes a set. I saw that comment on #67 and meant to respond to, but did not immediately respond and then forgot about it. There is only one thing I don't like about the single set method is that it makes writing examples a bit more verbose, but losing that is not a bit deal. I don't think the varargs of(String ... auhs) method has much practical use that I can think of, like I doubt it would ever be used outside of example or unit test code. So having the single method that takes a set seems like a good overall solution.

keith-turner commented 6 months ago

We may want to consider removing this code also. I have no idea what its behavior is for dupes. Its not documented and I did not look at the impl. This method also does not have much practical use other than making example code less verbose.

dlmarion commented 6 months ago

I ran the benchmark and compared to main the changes were in the noise.

Main:


Benchmark                                           Mode  Cnt   Score   Error   Units
AccessExpressionBenchmark.measureBytesValidation   thrpt   12  33.087 ± 0.139  ops/us
AccessExpressionBenchmark.measureEvaluation        thrpt   12  12.863 ± 0.109  ops/us
AccessExpressionBenchmark.measureStringValidation  thrpt   12  25.500 ± 0.144  ops/us

This branch:

Benchmark                                           Mode  Cnt   Score   Error   Units
AccessExpressionBenchmark.measureBytesValidation   thrpt   12  33.022 ± 0.171  ops/us
AccessExpressionBenchmark.measureEvaluation        thrpt   12  12.949 ± 0.355  ops/us
AccessExpressionBenchmark.measureStringValidation  thrpt   12  25.577 ± 0.314  ops/us