ctubbsii commented on code in PR #104:
URL: https://github.com/apache/accumulo-access/pull/104#discussion_r2914808588
##########
modules/core/src/main/java/org/apache/accumulo/access/impl/CharsWrapper.java:
##########
@@ -26,7 +26,7 @@ public final class CharsWrapper implements CharSequence {
private int offset;
private int len;
- CharsWrapper(char[] wrapped) {
+ public CharsWrapper(char[] wrapped) {
Review Comment:
This triggers a spotbugs notification about exposing the implementation.
Since this is only needed for a test, it would be good to leave this
non-public, and add:
```suggestion
public static CharsWrapper copyAndWrap(char[] wrapped) {
return new CharsWrapper(Arrays.copyOf(wrapped, wrapped.length));
}
CharsWrapper(char[] wrapped) {
```
##########
modules/core/src/test/java/org/apache/accumulo/access/tests/AccessEvaluatorTest.java:
##########
@@ -200,8 +202,16 @@ public void testQuote() {
assertEquals("九", access.unquote(access.quote("九")));
assertEquals("\"五十\"", access.quote("五十"));
assertEquals("五十", access.unquote(access.quote("五十")));
+
assertThrows(IllegalArgumentException.class, () -> access.quote(""));
- assertThrows(IllegalArgumentException.class, () -> access.unquote(""));
+ for (var illegalInput : List.of("", "\"\"", "\"", "AB\"", "\"AB")) {
+ assertThrows(IllegalArgumentException.class, () ->
access.unquote(illegalInput),
+ illegalInput);
+ // test with an input that is not a string
+ CharSequence charSeq = new CharsWrapper(illegalInput.toCharArray());
Review Comment:
```suggestion
var charSeq = CharsWrapper.copyAndWrap(illegalInput.toCharArray());
```
##########
modules/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java:
##########
@@ -78,8 +78,16 @@ public static CharSequence quote(CharSequence term) {
public static CharSequence unquote(CharSequence term) {
final int len = term.length();
- if (len >= 2 && term.charAt(0) == '"' && term.charAt(len - 1) == '"') {
- term = len == 2 ? "" : AccessEvaluatorImpl.unescape(term.subSequence(1,
len - 1));
+ if (len >= 1) {
+ final boolean firstIsQuote = term.charAt(0) == '"';
+ final boolean lastIsQuote = term.charAt(len - 1) == '"';
+ if (firstIsQuote || lastIsQuote) {
+ if (len == 1 || (firstIsQuote ^ lastIsQuote)) {
Review Comment:
I think `!=` is more intuitive for people who aren't doing bitwise ops on a
regular basis.
```suggestion
if (len == 1 || (firstIsQuote != lastIsQuote)) {
```
##########
modules/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java:
##########
@@ -78,8 +78,16 @@ public static CharSequence quote(CharSequence term) {
public static CharSequence unquote(CharSequence term) {
final int len = term.length();
- if (len >= 2 && term.charAt(0) == '"' && term.charAt(len - 1) == '"') {
- term = len == 2 ? "" : AccessEvaluatorImpl.unescape(term.subSequence(1,
len - 1));
+ if (len >= 1) {
+ final boolean firstIsQuote = term.charAt(0) == '"';
+ final boolean lastIsQuote = term.charAt(len - 1) == '"';
+ if (firstIsQuote || lastIsQuote) {
+ if (len == 1 || (firstIsQuote ^ lastIsQuote)) {
+ throw new IllegalArgumentException("Unbalanced quotes : " + term);
+ }
+
+ term = len == 2 ? "" :
AccessEvaluatorImpl.unescape(term.subSequence(1, len - 1));
Review Comment:
In testing this, I wanted to see how AccessEvaluatorImpl.unescape handles
quoted strings. It correctly fails (by the time it reaches this method, the
outer quotes should be removed, and there should be no unescaped quotes in the
string), but the error message is wrong. It says there is an illegal character
after a slash, when I didn't provide any slashes in the string, just two quotes.
##########
modules/core/src/test/java/org/apache/accumulo/access/tests/AccessEvaluatorTest.java:
##########
@@ -200,8 +202,16 @@ public void testQuote() {
assertEquals("九", access.unquote(access.quote("九")));
assertEquals("\"五十\"", access.quote("五十"));
assertEquals("五十", access.unquote(access.quote("五十")));
+
assertThrows(IllegalArgumentException.class, () -> access.quote(""));
- assertThrows(IllegalArgumentException.class, () -> access.unquote(""));
+ for (var illegalInput : List.of("", "\"\"", "\"", "AB\"", "\"AB")) {
Review Comment:
Add some single-character with mismatched quote, because it's a special case
where the length is equal to 2, which may force it down a different code path.
```suggestion
for (var illegalInput : List.of("", "\"\"", "\"", "AB\"", "\"AB", "\"A",
"B\"")) {
```
--
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]
