ctubbsii commented on code in PR #106:
URL: https://github.com/apache/accumulo-access/pull/106#discussion_r2913785814
##########
modules/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java:
##########
@@ -77,7 +77,11 @@ public static CharSequence quote(CharSequence term) {
}
public static CharSequence unquote(CharSequence term) {
- if (term.length() >= 2 && term.charAt(0) == '"' &&
term.charAt(term.length() - 1) == '"') {
+ int len = term.length();
+ if (len >= 2 && term.charAt(0) == '"' && term.charAt(len - 1) == '"') {
+ if (len == 2) {
+ throw new IllegalArgumentException("Empty strings are not legal
authorizations.");
+ }
term = AccessEvaluatorImpl.unescape(term.subSequence(1, term.length() -
1));
Review Comment:
@keith-turner and I discussed trying to avoid multiple throws statements
that effectively handled the same case, and we didn't think it was worth
optimizing the exceptional case. AccessEvaluatorImpl.unescape for this case
trivially creates a `new String(new char[0])`.
I think this probably gets you what you want with less code, and without the
redundant throws:
```suggestion
term = len == 2 ? "" :
AccessEvaluatorImpl.unescape(term.subSequence(1, term.length() - 1));
```
A (possibly) better option would be to handle this in
`AccessEvaluatorImpl.unescape`, so it returns an empty string literal instead
of `new String(new char[0])`.
--
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]
