keith-turner commented on code in PR #31:
URL: https://github.com/apache/accumulo-access/pull/31#discussion_r1462139810
##########
src/main/java/org/apache/accumulo/access/AccessExpression.java:
##########
@@ -34,70 +30,62 @@
* var auth3 = AccessExpression.quote("🦖");
* var visExp = AccessExpression
* .of("(" + auth1 + "&" + auth3 + ")|(" + auth1 + "&" + auth2 + "&" +
auth1 + ")");
- * System.out.println(visExp.getExpression());
- * System.out.println(visExp.normalize());
- * System.out.println(visExp.getAuthorizations());
+ * System.out.println(visExp);
+ * System.out.println(AccessExpression.getAuthorizations(visExp));
* }
* </pre>
*
* The above example will print the following.
*
* <pre>
* (CAT&"🦖")|(CAT&"🦕"&CAT)
- * ("🦕"&CAT)|("🦖"&CAT)
* [🦖, CAT, 🦕]
* </pre>
*
+ * The following code will throw an {@link IllegalAccessExpressionException}
because the expression
+ * is not valid.
+ *
+ * <pre>
+ * {@code
+ * AccessExpression.validate("A&B|C");
+ * }
+ * </pre>
+ *
+ *
* @see <a href="https://github.com/apache/accumulo-access";>Accumulo Access
Documentation</a>
* @since 1.0.0
*/
public interface AccessExpression {
Review Comment:
> so it's not readily apparent how to create the AccessExpression
At this point the way things have evolved, nothing else really uses the
AccessExpression type in the API. So it seemed like it made sense to turn the
object into a bag of static methods. Some of the static methods do not operate
on an access expression, but on parts of it and are used for constructing an
expression. For the methods that do operate on a whole expression, could use a
builder pattern like the following.
```java
AccessExpression.builder().of("my&expression").normalize().build()
// get unique authorizations
AccessExpression.builder().of("my&expression").build().getAuthorizations();
// validate the access expression
AccessExpression.builder().of("my&expression").build().validate();
//the remaining static method would be quote which is used to build an
expression
AccessExpression.quote("XYZ");
```
The above seems ok with me. The only one I have a concern about is the
`validate` method which minimizes object allocation in this PR, so creating the
access expression object to validate would create another object. We could
possibly still have a static validate method like the following.
```java
AccessExpression.builder().of("my&expression").normalize().build()
// get unique authorizations
AccessExpression.builder().of("my&expression").build().getAuthorizations();
// The two static methods on the type
AccessExpression.validate("my&expression");
AccessExpression.quote("XYZ");
```
--
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]
