From 37172e7689581a8b89be7f8b9b0e6daaa3d49564 Mon Sep 17 00:00:00 2001 From: Christopher Tubbs Date: Mon, 16 Mar 2026 21:19:49 -0400 Subject: [PATCH 1/2] Exception handling and build improvements Exception handling improvements: * Use InvalidAuthorizationException instead of raw IllegalArgumentException where it makes sense, especially to follow the javadoc that says that it will be thrown when parsing an invalid authorization inside an access expression * Inline quote/unquote methods from AccessEvaluatorImpl to th eonly place they are called in AccessImpl, so they can be simplified without redundant checks for empty strings * Move (un)escape methods from AccessEvaluatorImpl to CharUtils, since they are not used in AccessEvaluatorImpl after the quote/unquote methods were inlined Build improvements: * Add junit-jupiter-engine explicitly to the classpath for tests (required to run tests in IDE; surefire on the command-line adds it automatically, so it's only needed for running tests in the IDE) * Relocate tests and generated files into standard directories * Put common test loader code in separate module, removing unneeded spotbugs suppressions and redundant test code --- modules/antlr4-example/pom.xml | 13 +- .../src/main/java/module-info.java | 2 +- .../AccessExpressionAntlrEvaluator.java | 10 +- .../antlr4/AccessExpressionAntlrParser.java | 6 +- .../antlr => antlr4}/Antlr4Tests.java | 19 ++- .../AccessExpressionAntlrBenchmark.java | 12 +- .../grammars}/SpecificationTest.java | 6 +- .../src/test/resources/testdata.json | 1 - modules/core/pom.xml | 14 +- .../access/InvalidAuthorizationException.java | 36 +++++- .../access/impl/AccessEvaluatorImpl.java | 82 ------------ .../access/impl/AccessExpressionImpl.java | 40 ------ .../accumulo/access/impl/AccessImpl.java | 29 ++++- .../accumulo/access/impl/CharUtils.java | 79 ++++++++++++ .../impl/ParsedAccessExpressionImpl.java | 11 +- .../accumulo/access/impl/ParserEvaluator.java | 4 +- .../AccessExpressionBenchmark.java | 34 +++-- .../{tests => impl}/AccessEvaluatorTest.java | 122 ++++++------------ .../{tests => impl}/AccessExpressionTest.java | 4 +- .../{tests => impl}/AuthorizationTest.java | 5 +- .../ParsedAccessExpressionTest.java | 5 +- modules/examples/pom.xml | 5 + .../{test => }/AccessExampleTest.java | 5 +- .../{test => }/ParseExamplesTest.java | 5 +- modules/test-data/pom.xml | 40 ++++++ .../test-data/src/main/java/module-info.java | 22 ++++ .../access/testdata}/TestDataLoader.java | 7 +- .../accumulo/access/testdata}/testdata.json | 0 pom.xml | 33 ++++- src/build/spotbugs-exclude.xml | 7 +- 30 files changed, 358 insertions(+), 300 deletions(-) rename modules/antlr4-example/src/test/java/org/apache/accumulo/access/{grammar/antlr => antlr4}/Antlr4Tests.java (91%) rename modules/antlr4-example/src/test/java/org/apache/accumulo/access/{grammar/antlr => antlr4/benchmark}/AccessExpressionAntlrBenchmark.java (92%) rename modules/antlr4-example/src/test/java/org/apache/accumulo/access/{grammar => antlr4/grammars}/SpecificationTest.java (93%) delete mode 120000 modules/antlr4-example/src/test/resources/testdata.json rename modules/core/src/test/java/org/apache/accumulo/access/{tests => benchmark}/AccessExpressionBenchmark.java (90%) rename modules/core/src/test/java/org/apache/accumulo/access/{tests => impl}/AccessEvaluatorTest.java (67%) rename modules/core/src/test/java/org/apache/accumulo/access/{tests => impl}/AccessExpressionTest.java (99%) rename modules/core/src/test/java/org/apache/accumulo/access/{tests => impl}/AuthorizationTest.java (98%) rename modules/core/src/test/java/org/apache/accumulo/access/{tests => impl}/ParsedAccessExpressionTest.java (98%) rename modules/examples/src/test/java/org/apache/accumulo/access/examples/{test => }/AccessExampleTest.java (92%) rename modules/examples/src/test/java/org/apache/accumulo/access/examples/{test => }/ParseExamplesTest.java (97%) create mode 100644 modules/test-data/pom.xml create mode 100644 modules/test-data/src/main/java/module-info.java rename modules/{antlr4-example/src/test/java/org/apache/accumulo/access/antlr => test-data/src/main/java/org/apache/accumulo/access/testdata}/TestDataLoader.java (94%) rename modules/{core/src/test/resources => test-data/src/main/resources/org/apache/accumulo/access/testdata}/testdata.json (100%) diff --git a/modules/antlr4-example/pom.xml b/modules/antlr4-example/pom.xml index 384c438..bcb0be1 100644 --- a/modules/antlr4-example/pom.xml +++ b/modules/antlr4-example/pom.xml @@ -38,8 +38,8 @@ accumulo-access-core - com.google.code.gson - gson + org.apache.accumulo + accumulo-access-test-data test @@ -47,6 +47,11 @@ junit-jupiter-api test + + org.junit.jupiter + junit-jupiter-engine + test + org.openjdk.jmh jmh-core @@ -88,9 +93,9 @@ -package - org.apache.accumulo.access.grammars + org.apache.accumulo.access.antlr4.grammars -o - ${project.build.directory}/generated-sources/antlr4/org/apache/accumulo/access/grammars + ${project.build.directory}/generated-sources/antlr4/org/apache/accumulo/access/antlr4/grammars false false diff --git a/modules/antlr4-example/src/main/java/module-info.java b/modules/antlr4-example/src/main/java/module-info.java index aaae71b..0802938 100644 --- a/modules/antlr4-example/src/main/java/module-info.java +++ b/modules/antlr4-example/src/main/java/module-info.java @@ -18,7 +18,7 @@ */ module org.apache.accumulo.access.examples.antlr { exports org.apache.accumulo.access.antlr4; - exports org.apache.accumulo.access.grammars; + exports org.apache.accumulo.access.antlr4.grammars; requires transitive org.apache.accumulo.access.core; requires transitive org.antlr.antlr4.runtime; } diff --git a/modules/antlr4-example/src/main/java/org/apache/accumulo/access/antlr4/AccessExpressionAntlrEvaluator.java b/modules/antlr4-example/src/main/java/org/apache/accumulo/access/antlr4/AccessExpressionAntlrEvaluator.java index ecf101d..bf0b976 100644 --- a/modules/antlr4-example/src/main/java/org/apache/accumulo/access/antlr4/AccessExpressionAntlrEvaluator.java +++ b/modules/antlr4-example/src/main/java/org/apache/accumulo/access/antlr4/AccessExpressionAntlrEvaluator.java @@ -26,11 +26,11 @@ import org.antlr.v4.runtime.tree.ParseTree; import org.antlr.v4.runtime.tree.TerminalNode; import org.apache.accumulo.access.Access; -import org.apache.accumulo.access.grammars.AccessExpressionParser.Access_expressionContext; -import org.apache.accumulo.access.grammars.AccessExpressionParser.Access_tokenContext; -import org.apache.accumulo.access.grammars.AccessExpressionParser.And_operatorContext; -import org.apache.accumulo.access.grammars.AccessExpressionParser.Or_expressionContext; -import org.apache.accumulo.access.grammars.AccessExpressionParser.Or_operatorContext; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser.Access_expressionContext; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser.Access_tokenContext; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser.And_operatorContext; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser.Or_expressionContext; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser.Or_operatorContext; public class AccessExpressionAntlrEvaluator { diff --git a/modules/antlr4-example/src/main/java/org/apache/accumulo/access/antlr4/AccessExpressionAntlrParser.java b/modules/antlr4-example/src/main/java/org/apache/accumulo/access/antlr4/AccessExpressionAntlrParser.java index 95e14c3..71cc1eb 100644 --- a/modules/antlr4-example/src/main/java/org/apache/accumulo/access/antlr4/AccessExpressionAntlrParser.java +++ b/modules/antlr4-example/src/main/java/org/apache/accumulo/access/antlr4/AccessExpressionAntlrParser.java @@ -30,9 +30,9 @@ import org.antlr.v4.runtime.RecognitionException; import org.antlr.v4.runtime.Recognizer; import org.apache.accumulo.access.InvalidAccessExpressionException; -import org.apache.accumulo.access.grammars.AccessExpressionLexer; -import org.apache.accumulo.access.grammars.AccessExpressionParser; -import org.apache.accumulo.access.grammars.AccessExpressionParser.Access_expressionContext; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionLexer; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser.Access_expressionContext; public class AccessExpressionAntlrParser { diff --git a/modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/antlr/Antlr4Tests.java b/modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/Antlr4Tests.java similarity index 91% rename from modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/antlr/Antlr4Tests.java rename to modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/Antlr4Tests.java index 0d20ac0..b0f1902 100644 --- a/modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/antlr/Antlr4Tests.java +++ b/modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/Antlr4Tests.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.grammar.antlr; +package org.apache.accumulo.access.antlr4; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -41,17 +41,16 @@ import org.apache.accumulo.access.Access; import org.apache.accumulo.access.AccessEvaluator; import org.apache.accumulo.access.InvalidAccessExpressionException; -import org.apache.accumulo.access.antlr.TestDataLoader; -import org.apache.accumulo.access.antlr.TestDataLoader.ExpectedResult; -import org.apache.accumulo.access.antlr.TestDataLoader.TestDataSet; -import org.apache.accumulo.access.antlr.TestDataLoader.TestExpressions; -import org.apache.accumulo.access.antlr4.AccessExpressionAntlrEvaluator; -import org.apache.accumulo.access.grammars.AccessExpressionLexer; -import org.apache.accumulo.access.grammars.AccessExpressionParser; -import org.apache.accumulo.access.grammars.AccessExpressionParser.Access_expressionContext; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionLexer; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser.Access_expressionContext; +import org.apache.accumulo.access.testdata.TestDataLoader; +import org.apache.accumulo.access.testdata.TestDataLoader.ExpectedResult; +import org.apache.accumulo.access.testdata.TestDataLoader.TestDataSet; +import org.apache.accumulo.access.testdata.TestDataLoader.TestExpressions; import org.junit.jupiter.api.Test; -public class Antlr4Tests { +class Antlr4Tests { public static final Access ACCESS = Access.builder().build(); diff --git a/modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/antlr/AccessExpressionAntlrBenchmark.java b/modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/benchmark/AccessExpressionAntlrBenchmark.java similarity index 92% rename from modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/antlr/AccessExpressionAntlrBenchmark.java rename to modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/benchmark/AccessExpressionAntlrBenchmark.java index ed48963..246c7b5 100644 --- a/modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/antlr/AccessExpressionAntlrBenchmark.java +++ b/modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/benchmark/AccessExpressionAntlrBenchmark.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.grammar.antlr; +package org.apache.accumulo.access.antlr4.benchmark; import static java.nio.charset.StandardCharsets.UTF_8; @@ -29,10 +29,12 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import org.apache.accumulo.access.antlr.TestDataLoader; import org.apache.accumulo.access.antlr4.AccessExpressionAntlrEvaluator; import org.apache.accumulo.access.antlr4.AccessExpressionAntlrParser; -import org.apache.accumulo.access.grammars.AccessExpressionParser.Access_expressionContext; +import org.apache.accumulo.access.antlr4.grammars.AccessExpressionParser.Access_expressionContext; +import org.apache.accumulo.access.testdata.TestDataLoader; +import org.apache.accumulo.access.testdata.TestDataLoader.ExpectedResult; +import org.apache.accumulo.access.testdata.TestDataLoader.TestDataSet; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.Mode; import org.openjdk.jmh.annotations.Scope; @@ -79,7 +81,7 @@ public static class BenchmarkState { @Setup public void loadData() throws IOException, URISyntaxException { - List testData = TestDataLoader.readTestData(); + List testData = TestDataLoader.readTestData(); allTestExpressions = new ArrayList<>(); allTestExpressionsStr = new ArrayList<>(); evaluatorTests = new ArrayList<>(); @@ -93,7 +95,7 @@ public void loadData() throws IOException, URISyntaxException { Stream.of(testDataSet.getAuths()).map(a -> Set.of(a)).collect(Collectors.toList())); for (var tests : testDataSet.getTests()) { - if (tests.getExpectedResult() != TestDataLoader.ExpectedResult.ERROR) { + if (tests.getExpectedResult() != ExpectedResult.ERROR) { for (var exp : tests.getExpressions()) { allTestExpressionsStr.add(exp); byte[] byteExp = exp.getBytes(UTF_8); diff --git a/modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/SpecificationTest.java b/modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/grammars/SpecificationTest.java similarity index 93% rename from modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/SpecificationTest.java rename to modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/grammars/SpecificationTest.java index 5535f4b..53dce19 100644 --- a/modules/antlr4-example/src/test/java/org/apache/accumulo/access/grammar/SpecificationTest.java +++ b/modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr4/grammars/SpecificationTest.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.grammar; +package org.apache.accumulo.access.antlr4.grammars; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -29,14 +29,12 @@ import org.antlr.v4.runtime.LexerNoViableAltException; import org.antlr.v4.runtime.RecognitionException; import org.antlr.v4.runtime.Recognizer; -import org.apache.accumulo.access.grammars.AbnfLexer; -import org.apache.accumulo.access.grammars.AbnfParser; import org.junit.jupiter.api.Test; // This test uses the ANTLR ABNF grammar to parse the // AccessExpression ANBF specification to validate that // it is proper ANBF. -public class SpecificationTest { +class SpecificationTest { @Test public void testAbnfSpecificationParses() throws Exception { diff --git a/modules/antlr4-example/src/test/resources/testdata.json b/modules/antlr4-example/src/test/resources/testdata.json deleted file mode 120000 index 5bc6f30..0000000 --- a/modules/antlr4-example/src/test/resources/testdata.json +++ /dev/null @@ -1 +0,0 @@ -../../../../core/src/test/resources/testdata.json \ No newline at end of file diff --git a/modules/core/pom.xml b/modules/core/pom.xml index e351503..6ea5d5f 100644 --- a/modules/core/pom.xml +++ b/modules/core/pom.xml @@ -30,13 +30,8 @@ accumulo-access-core - com.github.spotbugs - spotbugs-annotations - test - - - com.google.code.gson - gson + org.apache.accumulo + accumulo-access-test-data test @@ -49,6 +44,11 @@ junit-jupiter-api test + + org.junit.jupiter + junit-jupiter-engine + test + org.openjdk.jmh jmh-core diff --git a/modules/core/src/main/java/org/apache/accumulo/access/InvalidAuthorizationException.java b/modules/core/src/main/java/org/apache/accumulo/access/InvalidAuthorizationException.java index 74df35f..1e7b4e5 100644 --- a/modules/core/src/main/java/org/apache/accumulo/access/InvalidAuthorizationException.java +++ b/modules/core/src/main/java/org/apache/accumulo/access/InvalidAuthorizationException.java @@ -19,7 +19,7 @@ package org.apache.accumulo.access; /** - * A RuntimeException for invalid authorizations. + * An exception that is thrown when an authorization is not valid. * * @since 1.0.0 */ @@ -27,7 +27,37 @@ public class InvalidAuthorizationException extends IllegalArgumentException { private static final long serialVersionUID = 1L; - public InvalidAuthorizationException(String auth) { - super("authorization : '" + auth + "'"); + private final String auth; + private final String reason; + + public static InvalidAuthorizationException emptyString() { + return new InvalidAuthorizationException("", "empty string"); + } + + public static InvalidAuthorizationException unablancedQuotes(CharSequence auth) { + return new InvalidAuthorizationException(auth, "unbalanced quotes"); + } + + public static InvalidAuthorizationException invalidChars(CharSequence auth) { + return new InvalidAuthorizationException(auth, "invalid characters"); + } + + public static InvalidAuthorizationException illegalEscape(CharSequence auth) { + return new InvalidAuthorizationException(auth, "invalid escape sequence"); } + + public static InvalidAuthorizationException unescapedQuote(CharSequence auth) { + return new InvalidAuthorizationException(auth, "unescaped quote"); + } + + private InvalidAuthorizationException(CharSequence auth, String reason) { + this.auth = auth.toString(); + this.reason = reason; + } + + @Override + public String getMessage() { + return "authorization : '" + auth + "' (" + reason + ")"; + } + } diff --git a/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java b/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java index e71fe29..a568b4b 100644 --- a/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java +++ b/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java @@ -18,12 +18,6 @@ */ package org.apache.accumulo.access.impl; -import static org.apache.accumulo.access.impl.CharUtils.BACKSLASH; -import static org.apache.accumulo.access.impl.CharUtils.QUOTE; -import static org.apache.accumulo.access.impl.CharUtils.isBackslashSymbol; -import static org.apache.accumulo.access.impl.CharUtils.isQuoteOrSlash; -import static org.apache.accumulo.access.impl.CharUtils.isQuoteSymbol; - import java.util.HashSet; import java.util.Set; import java.util.function.Consumer; @@ -63,82 +57,6 @@ public final class AccessEvaluatorImpl implements AccessEvaluator { this.authorizationValidator = authorizationValidator; } - public static CharSequence unescape(CharSequence auth) { - int escapeCharCount = 0; - final int authLength = auth.length(); - for (int i = 0; i < authLength; i++) { - char c = auth.charAt(i); - if (isQuoteOrSlash(c)) { - escapeCharCount++; - } - } - - if (escapeCharCount > 0) { - if (escapeCharCount % 2 == 1) { - throw new IllegalArgumentException("Illegal escape sequence in auth : " + auth); - } - - char[] unescapedCopy = new char[authLength - escapeCharCount / 2]; - int pos = 0; - for (int i = 0; i < authLength; i++) { - char c = auth.charAt(i); - if (isBackslashSymbol(c)) { - i++; - c = auth.charAt(i); - if (!isQuoteOrSlash(c)) { - throw new IllegalArgumentException("Illegal escape sequence in auth : " + auth); - } - } else if (isQuoteSymbol(c)) { - // should only see quote after a slash - throw new IllegalArgumentException("Unescaped quote in auth : " + auth); - } - - unescapedCopy[pos++] = c; - } - - return new String(unescapedCopy); - } else { - return auth; - } - } - - /** - * Properly escapes an authorization string. The string can be quoted if desired. - * - * @param auth authorization string, as UTF-8 encoded bytes - * @param shouldQuote true to wrap escaped authorization in quotes - * @return escaped authorization string - */ - public static CharSequence escape(CharSequence auth, boolean shouldQuote) { - int escapeCount = 0; - final int authLength = auth.length(); - for (int i = 0; i < authLength; i++) { - if (isQuoteOrSlash(auth.charAt(i))) { - escapeCount++; - } - } - - if (escapeCount > 0 || shouldQuote) { - char[] escapedAuth = new char[authLength + escapeCount + (shouldQuote ? 2 : 0)]; - int index = shouldQuote ? 1 : 0; - for (int i = 0; i < authLength; i++) { - char c = auth.charAt(i); - if (isQuoteOrSlash(c)) { - escapedAuth[index++] = BACKSLASH; - } - escapedAuth[index++] = c; - } - - if (shouldQuote) { - escapedAuth[0] = QUOTE; - escapedAuth[escapedAuth.length - 1] = QUOTE; - } - - auth = new String(escapedAuth); - } - return auth; - } - @Override public boolean canAccess(String expression) throws InvalidAccessExpressionException { return evaluate(expression); diff --git a/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java b/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java index 75de7ee..b42e1db 100644 --- a/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java +++ b/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java @@ -55,44 +55,4 @@ public ParsedAccessExpression parse() { return parseTree; } - public static CharSequence quote(CharSequence term) { - if (term.isEmpty()) { - throw new IllegalArgumentException("Empty strings are not legal authorizations."); - } - - boolean needsQuote = false; - final int len = term.length(); - for (int i = 0; i < len; i++) { - if (!Tokenizer.isValidAuthChar(term.charAt(i))) { - needsQuote = true; - break; - } - } - - if (!needsQuote) { - return term; - } - - return AccessEvaluatorImpl.escape(term, true); - } - - public static CharSequence unquote(CharSequence term) { - final int len = term.length(); - 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)); - } - } - if (term.isEmpty()) { - throw new IllegalArgumentException("Empty strings are not legal authorizations."); - } - return term; - } - } diff --git a/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessImpl.java b/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessImpl.java index 9120ed5..037dad5 100644 --- a/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessImpl.java +++ b/modules/core/src/main/java/org/apache/accumulo/access/impl/AccessImpl.java @@ -41,10 +41,10 @@ public class AccessImpl implements Access { private void validateAuthArgument(CharSequence auth) { if (auth.isEmpty()) { - throw new IllegalArgumentException("Empty string is not a valid authorization"); + throw InvalidAuthorizationException.emptyString(); } if (!authValidator.test(auth, ANY)) { - throw new InvalidAuthorizationException(auth.toString()); + throw InvalidAuthorizationException.invalidChars(auth); } } @@ -75,14 +75,33 @@ public void findAuthorizations(String expression, Consumer authorization @Override public String quote(String authorization) { validateAuthArgument(authorization); - return AccessExpressionImpl.quote(authorization).toString(); + boolean needsQuote = false; + final int len = authorization.length(); + for (int i = 0; i < len; i++) { + if (!Tokenizer.isValidAuthChar(authorization.charAt(i))) { + needsQuote = true; + break; + } + } + return needsQuote ? CharUtils.escape(authorization, true) : authorization; } @Override public String unquote(String authorization) { - var unquoted = AccessExpressionImpl.unquote(authorization); + String unquoted = authorization; + final int len = unquoted.length(); + if (len >= 1) { + final boolean firstIsQuote = unquoted.charAt(0) == '"'; + final boolean lastIsQuote = unquoted.charAt(len - 1) == '"'; + if (firstIsQuote || lastIsQuote) { + if (len == 1 || (firstIsQuote != lastIsQuote)) { + throw InvalidAuthorizationException.unablancedQuotes(authorization); // unbalanced quotes + } + unquoted = len == 2 ? "" : CharUtils.unescape(unquoted.substring(1, len - 1)).toString(); + } + } validateAuthArgument(unquoted); - return unquoted.toString(); + return unquoted; } @Override diff --git a/modules/core/src/main/java/org/apache/accumulo/access/impl/CharUtils.java b/modules/core/src/main/java/org/apache/accumulo/access/impl/CharUtils.java index 696fbb1..1c14db5 100644 --- a/modules/core/src/main/java/org/apache/accumulo/access/impl/CharUtils.java +++ b/modules/core/src/main/java/org/apache/accumulo/access/impl/CharUtils.java @@ -18,6 +18,8 @@ */ package org.apache.accumulo.access.impl; +import org.apache.accumulo.access.InvalidAuthorizationException; + /** * This class exists to avoid repeat conversions from byte to char as well as to provide helper * methods for comparing them. @@ -55,4 +57,81 @@ static boolean isOrOperator(char b) { static boolean isAndOrOperator(char b) { return isAndOperator(b) || isOrOperator(b); } + + /** + * Properly escapes an authorization string. The string can be quoted if desired. + * + * @param auth authorization string, as UTF-8 encoded bytes + * @param shouldQuote true to wrap escaped authorization in quotes + * @return escaped authorization string + */ + static String escape(String auth, boolean shouldQuote) { + int escapeCount = 0; + final int authLength = auth.length(); + for (int i = 0; i < authLength; i++) { + if (isQuoteOrSlash(auth.charAt(i))) { + escapeCount++; + } + } + + if (escapeCount > 0 || shouldQuote) { + char[] escapedAuth = new char[authLength + escapeCount + (shouldQuote ? 2 : 0)]; + int index = shouldQuote ? 1 : 0; + for (int i = 0; i < authLength; i++) { + char c = auth.charAt(i); + if (isQuoteOrSlash(c)) { + escapedAuth[index++] = BACKSLASH; + } + escapedAuth[index++] = c; + } + + if (shouldQuote) { + escapedAuth[0] = QUOTE; + escapedAuth[escapedAuth.length - 1] = QUOTE; + } + + auth = new String(escapedAuth); + } + return auth; + } + + static CharSequence unescape(CharSequence auth) { + int escapeCharCount = 0; + final int authLength = auth.length(); + for (int i = 0; i < authLength; i++) { + char c = auth.charAt(i); + if (isQuoteOrSlash(c)) { + escapeCharCount++; + } + } + + if (escapeCharCount > 0) { + if (escapeCharCount % 2 == 1) { + throw InvalidAuthorizationException.illegalEscape(auth); + } + + char[] unescapedCopy = new char[authLength - escapeCharCount / 2]; + int pos = 0; + for (int i = 0; i < authLength; i++) { + char c = auth.charAt(i); + if (isBackslashSymbol(c)) { + i++; + c = auth.charAt(i); + if (!isQuoteOrSlash(c)) { + throw InvalidAuthorizationException.illegalEscape(auth); + } + } else if (isQuoteSymbol(c)) { + // should only see quote after a slash + throw InvalidAuthorizationException.unescapedQuote(auth); + } + + unescapedCopy[pos++] = c; + } + + return new String(unescapedCopy); + } else { + return auth; + } + } + } diff --git a/modules/core/src/main/java/org/apache/accumulo/access/impl/ParsedAccessExpressionImpl.java b/modules/core/src/main/java/org/apache/accumulo/access/impl/ParsedAccessExpressionImpl.java index c41559b..3d99059 100644 --- a/modules/core/src/main/java/org/apache/accumulo/access/impl/ParsedAccessExpressionImpl.java +++ b/modules/core/src/main/java/org/apache/accumulo/access/impl/ParsedAccessExpressionImpl.java @@ -31,6 +31,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.apache.accumulo.access.AuthorizationValidator; +import org.apache.accumulo.access.InvalidAccessExpressionException; import org.apache.accumulo.access.InvalidAuthorizationException; import org.apache.accumulo.access.ParsedAccessExpression; @@ -51,11 +52,13 @@ public final class ParsedAccessExpressionImpl extends ParsedAccessExpression { private ParsedAccessExpressionImpl(char operator, String wholeExpression, int offset, int length, List children) { if (children.isEmpty()) { - throw new IllegalArgumentException("Must have children with an operator"); + throw new InvalidAccessExpressionException("Must have children with an operator", + wholeExpression, offset); } if (operator != AND_OPERATOR && operator != OR_OPERATOR) { - throw new IllegalArgumentException("Unknown operator " + operator); + throw new InvalidAccessExpressionException("Unknown operator " + operator, wholeExpression, + offset); } else if (operator == AND_OPERATOR) { this.type = AND; } else { @@ -182,7 +185,7 @@ private static ParsedAccessExpressionImpl parseParenExpressionOrAuthorization(To if (CharUtils.isQuoteSymbol(auth.data[auth.start])) { wrapper.set(auth.data, auth.start + 1, auth.len - 2); if (auth.hasEscapes) { - unquotedAuth = AccessEvaluatorImpl.unescape(wrapper); + unquotedAuth = CharUtils.unescape(wrapper); } else { unquotedAuth = wrapper; } @@ -193,7 +196,7 @@ private static ParsedAccessExpressionImpl parseParenExpressionOrAuthorization(To quoting = AuthorizationValidator.AuthorizationCharacters.BASIC; } if (!authorizationValidator.test(unquotedAuth, quoting)) { - throw new InvalidAuthorizationException(unquotedAuth.toString()); + throw InvalidAuthorizationException.invalidChars(unquotedAuth); } return new ParsedAccessExpressionImpl(wholeExpression, auth.start, auth.len); } diff --git a/modules/core/src/main/java/org/apache/accumulo/access/impl/ParserEvaluator.java b/modules/core/src/main/java/org/apache/accumulo/access/impl/ParserEvaluator.java index 557d89a..6b6fd54 100644 --- a/modules/core/src/main/java/org/apache/accumulo/access/impl/ParserEvaluator.java +++ b/modules/core/src/main/java/org/apache/accumulo/access/impl/ParserEvaluator.java @@ -63,12 +63,12 @@ static CharSequence validateAuth(AuthorizationValidator authValidator, charsWrapper.set(authToken.data, authToken.start, authToken.len); CharSequence authorizations; if (authToken.hasEscapes) { - authorizations = AccessEvaluatorImpl.unescape(charsWrapper); + authorizations = CharUtils.unescape(charsWrapper); } else { authorizations = charsWrapper; } if (!authValidator.test(authorizations, authToken.quoting)) { - throw new InvalidAuthorizationException(authorizations.toString()); + throw InvalidAuthorizationException.invalidChars(authorizations); } return authorizations; } diff --git a/modules/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java b/modules/core/src/test/java/org/apache/accumulo/access/benchmark/AccessExpressionBenchmark.java similarity index 90% rename from modules/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java rename to modules/core/src/test/java/org/apache/accumulo/access/benchmark/AccessExpressionBenchmark.java index b64df5b..f5ff50f 100644 --- a/modules/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java +++ b/modules/core/src/test/java/org/apache/accumulo/access/benchmark/AccessExpressionBenchmark.java @@ -16,11 +16,10 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.tests; +package org.apache.accumulo.access.benchmark; import static java.nio.charset.StandardCharsets.UTF_8; -import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -30,6 +29,9 @@ import org.apache.accumulo.access.Access; import org.apache.accumulo.access.AccessEvaluator; +import org.apache.accumulo.access.testdata.TestDataLoader; +import org.apache.accumulo.access.testdata.TestDataLoader.ExpectedResult; +import org.apache.accumulo.access.testdata.TestDataLoader.TestDataSet; import org.apache.accumulo.core.security.ColumnVisibility; import org.apache.accumulo.core.security.VisibilityEvaluator; import org.apache.accumulo.core.security.VisibilityParseException; @@ -42,14 +44,11 @@ import org.openjdk.jmh.profile.JavaFlightRecorderProfiler; import org.openjdk.jmh.runner.NoBenchmarksException; import org.openjdk.jmh.runner.Runner; -import org.openjdk.jmh.runner.RunnerException; import org.openjdk.jmh.runner.options.OptionsBuilder; import org.openjdk.jmh.runner.options.TimeValue; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; - /** * Benchmarks Access Expressions using JMH. To run, use the following commands. * @@ -63,7 +62,6 @@ * * */ -@SuppressFBWarnings(value = {"NP_UNWRITTEN_FIELD"}, justification = "Field is written by Gson") public class AccessExpressionBenchmark { private static final Logger LOG = LoggerFactory.getLogger(AccessExpressionBenchmark.class); @@ -94,9 +92,9 @@ public static class BenchmarkState { private ArrayList visibilityEvaluatorTests; @Setup - public void loadData() throws IOException { + public void loadData() throws Exception { access = Access.builder().build(); - List testData = AccessEvaluatorTest.readTestData(); + List testData = TestDataLoader.readTestData(); allTestExpressions = new ArrayList<>(); allTestExpressionsStr = new ArrayList<>(); evaluatorTests = new ArrayList<>(); @@ -109,12 +107,12 @@ public void loadData() throws IOException { vet.expressions = new ArrayList<>(); vet.columnVisibilities = new ArrayList<>(); - if (testDataSet.auths.length == 1) { + if (testDataSet.getAuths().length == 1) { vet.evaluator = List.of(new VisibilityEvaluator( - new org.apache.accumulo.core.security.Authorizations(testDataSet.auths[0]))); + new org.apache.accumulo.core.security.Authorizations(testDataSet.getAuths()[0]))); } else { List veList = new ArrayList<>(); - for (String[] auths : testDataSet.auths) { + for (String[] auths : testDataSet.getAuths()) { veList.add(new VisibilityEvaluator( new org.apache.accumulo.core.security.Authorizations(auths))); } @@ -125,17 +123,17 @@ public void loadData() throws IOException { EvaluatorTests et = new EvaluatorTests(); et.expressions = new ArrayList<>(); - if (testDataSet.auths.length == 1) { - et.evaluator = access.newEvaluator(Set.of(testDataSet.auths[0])); + if (testDataSet.getAuths().length == 1) { + et.evaluator = access.newEvaluator(Set.of(testDataSet.getAuths()[0])); } else { var authSets = - Stream.of(testDataSet.auths).map(a -> Set.of(a)).collect(Collectors.toList()); + Stream.of(testDataSet.getAuths()).map(a -> Set.of(a)).collect(Collectors.toList()); et.evaluator = access.newEvaluator(authSets); } - for (var tests : testDataSet.tests) { - if (tests.expectedResult != AccessEvaluatorTest.ExpectedResult.ERROR) { - for (var exp : tests.expressions) { + for (var tests : testDataSet.getTests()) { + if (tests.getExpectedResult() != ExpectedResult.ERROR) { + for (var exp : tests.getExpressions()) { allTestExpressionsStr.add(exp); byte[] byteExp = exp.getBytes(UTF_8); allTestExpressions.add(byteExp); @@ -250,7 +248,7 @@ public void measureLegacyParseAndEvaluation(BenchmarkState state, Blackhole blac } } - public static void main(String[] args) throws RunnerException, IOException { + public static void main(String[] args) throws Exception { var state = new BenchmarkState(); state.loadData(); diff --git a/modules/core/src/test/java/org/apache/accumulo/access/tests/AccessEvaluatorTest.java b/modules/core/src/test/java/org/apache/accumulo/access/impl/AccessEvaluatorTest.java similarity index 67% rename from modules/core/src/test/java/org/apache/accumulo/access/tests/AccessEvaluatorTest.java rename to modules/core/src/test/java/org/apache/accumulo/access/impl/AccessEvaluatorTest.java index c445bf4..c170c95 100644 --- a/modules/core/src/test/java/org/apache/accumulo/access/tests/AccessEvaluatorTest.java +++ b/modules/core/src/test/java/org/apache/accumulo/access/impl/AccessEvaluatorTest.java @@ -16,18 +16,13 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.tests; +package org.apache.accumulo.access.impl; -import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import java.io.IOException; -import java.lang.reflect.Type; -import java.nio.CharBuffer; -import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -38,67 +33,36 @@ import org.apache.accumulo.access.AccessEvaluator; import org.apache.accumulo.access.AuthorizationValidator; import org.apache.accumulo.access.InvalidAccessExpressionException; -import org.apache.accumulo.access.impl.AccessEvaluatorImpl; -import org.apache.accumulo.access.impl.AccessExpressionImpl; +import org.apache.accumulo.access.InvalidAuthorizationException; +import org.apache.accumulo.access.testdata.TestDataLoader; +import org.apache.accumulo.access.testdata.TestDataLoader.ExpectedResult; +import org.apache.accumulo.access.testdata.TestDataLoader.TestDataSet; import org.junit.jupiter.api.Test; -import com.google.gson.Gson; -import com.google.gson.reflect.TypeToken; - -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; - -@SuppressFBWarnings(value = {"UWF_UNWRITTEN_FIELD", "NP_UNWRITTEN_FIELD"}, - justification = "Field is written by Gson") -public class AccessEvaluatorTest { - - enum ExpectedResult { - ACCESSIBLE, INACCESSIBLE, ERROR - } - - public static class TestExpressions { - ExpectedResult expectedResult; - String[] expressions; - } - - public static class TestDataSet { - String description; - String[][] auths; - List tests; - } - - static List readTestData() throws IOException { - try (var input = AccessEvaluatorTest.class.getResourceAsStream("/testdata.json")) { - if (input == null) { - throw new IllegalStateException("could not find resource : testdata.json"); - } - var json = new String(input.readAllBytes(), UTF_8); - - Type listType = new TypeToken>() {}.getType(); - return new Gson().fromJson(json, listType); - } - } +class AccessEvaluatorTest { @Test - public void runTestCases() throws IOException { - List testData = readTestData(); + public void runTestCases() throws Exception { + List testData = TestDataLoader.readTestData(); assertFalse(testData.isEmpty()); var access = Access.builder().build(); for (var testSet : testData) { - System.out.println("runTestCases for " + testSet.description); + System.out.println("runTestCases for " + testSet.getDescription()); AccessEvaluator evaluator; - assertTrue(testSet.auths.length >= 1); - if (testSet.auths.length == 1) { - evaluator = access.newEvaluator(Set.of(testSet.auths[0])); + assertTrue(testSet.getAuths().length >= 1); + if (testSet.getAuths().length == 1) { + evaluator = access.newEvaluator(Set.of(testSet.getAuths()[0])); runTestCases(access, testSet, evaluator); - Set auths = Stream.of(testSet.auths[0]).collect(Collectors.toSet()); + Set auths = Stream.of(testSet.getAuths()[0]).collect(Collectors.toSet()); evaluator = access.newEvaluator(auths::contains); runTestCases(access, testSet, evaluator); } else { - var authSets = Stream.of(testSet.auths).map(a -> Set.of(a)).collect(Collectors.toList()); + var authSets = + Stream.of(testSet.getAuths()).map(a -> Set.of(a)).collect(Collectors.toList()); evaluator = access.newEvaluator(authSets); runTestCases(access, testSet, evaluator); } @@ -108,18 +72,18 @@ public void runTestCases() throws IOException { private static void runTestCases(Access accumuloAccess, TestDataSet testSet, AccessEvaluator evaluator) { - assertFalse(testSet.tests.isEmpty()); + assertFalse(testSet.getTests().isEmpty()); - for (var tests : testSet.tests) { + for (var tests : testSet.getTests()) { - assertTrue(tests.expressions.length > 0); + assertTrue(tests.getExpressions().length > 0); - for (var expression : tests.expressions) { + for (var expression : tests.getExpressions()) { // Call various APIs with well-formed access expressions to ensure they do not throw an // exception - if (tests.expectedResult == ExpectedResult.ACCESSIBLE - || tests.expectedResult == ExpectedResult.INACCESSIBLE) { + if (tests.getExpectedResult() == ExpectedResult.ACCESSIBLE + || tests.getExpectedResult() == ExpectedResult.INACCESSIBLE) { accumuloAccess.validateExpression(expression); assertEquals(expression, accumuloAccess.newExpression(expression).getExpression()); // parsing an expression will strip unneeded outer parens @@ -128,7 +92,7 @@ private static void runTestCases(Access accumuloAccess, TestDataSet testSet, accumuloAccess.findAuthorizations(expression, auth -> {}); } - switch (tests.expectedResult) { + switch (tests.getExpectedResult()) { case ACCESSIBLE -> { assertTrue(evaluator.canAccess(expression), expression); assertTrue(evaluator.canAccess(accumuloAccess.newExpression(expression)), expression); @@ -165,10 +129,10 @@ private static void runTestCases(Access accumuloAccess, TestDataSet testSet, @Test public void testEmptyAuthorizations() { var access = Access.builder().build(); - assertThrows(IllegalArgumentException.class, () -> access.newEvaluator(Set.of(""))); - assertThrows(IllegalArgumentException.class, () -> access.newEvaluator(Set.of("", "A"))); - assertThrows(IllegalArgumentException.class, () -> access.newEvaluator(Set.of("A", ""))); - assertThrows(IllegalArgumentException.class, () -> access.newEvaluator(Set.of(""))); + assertThrows(InvalidAuthorizationException.class, () -> access.newEvaluator(Set.of(""))); + assertThrows(InvalidAuthorizationException.class, () -> access.newEvaluator(Set.of("", "A"))); + assertThrows(InvalidAuthorizationException.class, () -> access.newEvaluator(Set.of("A", ""))); + assertThrows(InvalidAuthorizationException.class, () -> access.newEvaluator(Set.of(""))); } @Test @@ -202,34 +166,29 @@ public void testQuote() { assertEquals("\"五十\"", access.quote("五十")); assertEquals("五十", access.unquote(access.quote("五十"))); - var e = assertThrows(IllegalArgumentException.class, () -> access.quote("")); - assertTrue(e.getMessage().contains("Empty string")); + var e = assertThrows(InvalidAuthorizationException.class, () -> access.quote("")); + assertEquals("authorization : '' (empty string)", e.getMessage()); - testUnquoteError(access, "\"\"\"\"", "Unescaped quote"); + testUnquoteError(access, "\"\"\"\"", "unescaped quote"); for (var illegalInput : List.of("", "\"\"")) { - testUnquoteError(access, illegalInput, "Empty string"); + testUnquoteError(access, illegalInput, "empty string"); } for (var illegalInput : List.of("\"", "AB\"", "\"AB", "\"A", "B\"")) { - testUnquoteError(access, illegalInput, "Unbalanced quotes"); + testUnquoteError(access, illegalInput, "unbalanced quotes"); } } private static void testUnquoteError(Access access, String illegalInput, String expectedErrorMsg) { - var e = assertThrows(IllegalArgumentException.class, () -> access.unquote(illegalInput), - illegalInput); - assertTrue(e.getMessage().contains(expectedErrorMsg)); - // test with an input that is not a string - CharSequence charSeq = CharBuffer.wrap(illegalInput); - e = assertThrows(IllegalArgumentException.class, () -> AccessExpressionImpl.unquote(charSeq), + var e = assertThrows(InvalidAuthorizationException.class, () -> access.unquote(illegalInput), illegalInput); assertTrue(e.getMessage().contains(expectedErrorMsg)); } private static String unescape(String s) { - return AccessEvaluatorImpl.unescape(s).toString(); + return CharUtils.unescape(s).toString(); } @Test @@ -240,11 +199,16 @@ public void testUnescape() { assertEquals("\\\"", unescape("\\\\\\\"")); assertEquals("a\\b\\c\\d", unescape("a\\\\b\\\\c\\\\d")); - final String message = "Expected failure to unescape invalid escape sequence"; - final var invalidEscapeSeqList = List.of("a\\b", "a\\b\\c", "a\"b\\"); - - invalidEscapeSeqList - .forEach(seq -> assertThrows(IllegalArgumentException.class, () -> unescape(seq), message)); + List.of("a\\b", "a\\b\\c").forEach(seq -> { + var e = assertThrows(InvalidAuthorizationException.class, () -> unescape(seq), + "Expected failure to unescape invalid escape sequence: " + seq); + assertTrue(e.getMessage().contains("invalid escape"), seq + " -> " + e.getMessage()); + }); + List.of("a\"b\\").forEach(seq -> { + var e = assertThrows(InvalidAuthorizationException.class, () -> unescape(seq), + "Expected failure to unescape invalid escape sequence: " + seq); + assertTrue(e.getMessage().contains("unescaped quote"), seq + " -> " + e.getMessage()); + }); } @Test diff --git a/modules/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionTest.java b/modules/core/src/test/java/org/apache/accumulo/access/impl/AccessExpressionTest.java similarity index 99% rename from modules/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionTest.java rename to modules/core/src/test/java/org/apache/accumulo/access/impl/AccessExpressionTest.java index 55abca3..bff14a6 100644 --- a/modules/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionTest.java +++ b/modules/core/src/test/java/org/apache/accumulo/access/impl/AccessExpressionTest.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.tests; +package org.apache.accumulo.access.impl; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toList; @@ -44,7 +44,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.function.Executable; -public class AccessExpressionTest { +class AccessExpressionTest { @Test public void testGetAuthorizations() { diff --git a/modules/core/src/test/java/org/apache/accumulo/access/tests/AuthorizationTest.java b/modules/core/src/test/java/org/apache/accumulo/access/impl/AuthorizationTest.java similarity index 98% rename from modules/core/src/test/java/org/apache/accumulo/access/tests/AuthorizationTest.java rename to modules/core/src/test/java/org/apache/accumulo/access/impl/AuthorizationTest.java index 25855e2..c2afc7c 100644 --- a/modules/core/src/test/java/org/apache/accumulo/access/tests/AuthorizationTest.java +++ b/modules/core/src/test/java/org/apache/accumulo/access/impl/AuthorizationTest.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.tests; +package org.apache.accumulo.access.impl; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -31,10 +31,9 @@ import org.apache.accumulo.access.Access; import org.apache.accumulo.access.InvalidAccessExpressionException; import org.apache.accumulo.access.InvalidAuthorizationException; -import org.apache.accumulo.access.impl.Tokenizer; import org.junit.jupiter.api.Test; -public class AuthorizationTest { +class AuthorizationTest { @Test public void testAuthorizationValidation() { diff --git a/modules/core/src/test/java/org/apache/accumulo/access/tests/ParsedAccessExpressionTest.java b/modules/core/src/test/java/org/apache/accumulo/access/impl/ParsedAccessExpressionTest.java similarity index 98% rename from modules/core/src/test/java/org/apache/accumulo/access/tests/ParsedAccessExpressionTest.java rename to modules/core/src/test/java/org/apache/accumulo/access/impl/ParsedAccessExpressionTest.java index 0acbdce..efaad6f 100644 --- a/modules/core/src/test/java/org/apache/accumulo/access/tests/ParsedAccessExpressionTest.java +++ b/modules/core/src/test/java/org/apache/accumulo/access/impl/ParsedAccessExpressionTest.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.tests; +package org.apache.accumulo.access.impl; import static org.apache.accumulo.access.ParsedAccessExpression.ExpressionType.AND; import static org.apache.accumulo.access.ParsedAccessExpression.ExpressionType.AUTHORIZATION; @@ -36,7 +36,8 @@ import org.apache.accumulo.access.ParsedAccessExpression; import org.junit.jupiter.api.Test; -public class ParsedAccessExpressionTest { +class ParsedAccessExpressionTest { + @Test public void testParsing() { HashSet seenAuths = new HashSet<>(); diff --git a/modules/examples/pom.xml b/modules/examples/pom.xml index d22ce6c..8a17a21 100644 --- a/modules/examples/pom.xml +++ b/modules/examples/pom.xml @@ -38,5 +38,10 @@ junit-jupiter-api test + + org.junit.jupiter + junit-jupiter-engine + test + diff --git a/modules/examples/src/test/java/org/apache/accumulo/access/examples/test/AccessExampleTest.java b/modules/examples/src/test/java/org/apache/accumulo/access/examples/AccessExampleTest.java similarity index 92% rename from modules/examples/src/test/java/org/apache/accumulo/access/examples/test/AccessExampleTest.java rename to modules/examples/src/test/java/org/apache/accumulo/access/examples/AccessExampleTest.java index 7ec077b..3a023a8 100644 --- a/modules/examples/src/test/java/org/apache/accumulo/access/examples/test/AccessExampleTest.java +++ b/modules/examples/src/test/java/org/apache/accumulo/access/examples/AccessExampleTest.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.examples.test; +package org.apache.accumulo.access.examples; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -27,10 +27,9 @@ import java.io.PrintStream; import java.util.List; -import org.apache.accumulo.access.examples.AccessExample; import org.junit.jupiter.api.Test; -public class AccessExampleTest { +class AccessExampleTest { @Test public void testExampleCode() throws IOException { try (final var baos = new ByteArrayOutputStream(); diff --git a/modules/examples/src/test/java/org/apache/accumulo/access/examples/test/ParseExamplesTest.java b/modules/examples/src/test/java/org/apache/accumulo/access/examples/ParseExamplesTest.java similarity index 97% rename from modules/examples/src/test/java/org/apache/accumulo/access/examples/test/ParseExamplesTest.java rename to modules/examples/src/test/java/org/apache/accumulo/access/examples/ParseExamplesTest.java index c34d045..c2c0cf1 100644 --- a/modules/examples/src/test/java/org/apache/accumulo/access/examples/test/ParseExamplesTest.java +++ b/modules/examples/src/test/java/org/apache/accumulo/access/examples/ParseExamplesTest.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.examples.test; +package org.apache.accumulo.access.examples; import static org.apache.accumulo.access.examples.ParseExamples.ACCESS; import static org.apache.accumulo.access.examples.ParseExamples.replaceAuthorizations; @@ -26,11 +26,10 @@ import java.util.List; import java.util.Map; -import org.apache.accumulo.access.examples.ParseExamples; import org.junit.jupiter.api.Test; // In addition to testing the examples, these test also provide extensive testing of ParsedAccessExpression -public class ParseExamplesTest { +class ParseExamplesTest { @Test public void testNormalize() { diff --git a/modules/test-data/pom.xml b/modules/test-data/pom.xml new file mode 100644 index 0000000..68d88a4 --- /dev/null +++ b/modules/test-data/pom.xml @@ -0,0 +1,40 @@ + + + + 4.0.0 + + org.apache.accumulo + accumulo-access + 1.0.0-beta2-SNAPSHOT + ../../pom.xml + + accumulo-access-test-data + + releases + + + + com.google.code.gson + gson + + + diff --git a/modules/test-data/src/main/java/module-info.java b/modules/test-data/src/main/java/module-info.java new file mode 100644 index 0000000..276a992 --- /dev/null +++ b/modules/test-data/src/main/java/module-info.java @@ -0,0 +1,22 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +module org.apache.accumulo.access.testdata { + exports org.apache.accumulo.access.testdata; + requires com.google.gson; +} diff --git a/modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr/TestDataLoader.java b/modules/test-data/src/main/java/org/apache/accumulo/access/testdata/TestDataLoader.java similarity index 94% rename from modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr/TestDataLoader.java rename to modules/test-data/src/main/java/org/apache/accumulo/access/testdata/TestDataLoader.java index 6201b33..5a0bb37 100644 --- a/modules/antlr4-example/src/test/java/org/apache/accumulo/access/antlr/TestDataLoader.java +++ b/modules/test-data/src/main/java/org/apache/accumulo/access/testdata/TestDataLoader.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.accumulo.access.antlr; +package org.apache.accumulo.access.testdata; import static java.nio.charset.StandardCharsets.UTF_8; @@ -86,9 +86,10 @@ public List getTests() { } } - public static List readTestData() throws IOException, URISyntaxException { + private TestDataLoader() {} - try (var input = TestDataLoader.class.getResourceAsStream("/testdata.json")) { + public static List readTestData() throws IOException, URISyntaxException { + try (var input = TestDataLoader.class.getResourceAsStream("testdata.json")) { if (input == null) { throw new IllegalStateException("could not find resource : testdata.json"); } diff --git a/modules/core/src/test/resources/testdata.json b/modules/test-data/src/main/resources/org/apache/accumulo/access/testdata/testdata.json similarity index 100% rename from modules/core/src/test/resources/testdata.json rename to modules/test-data/src/main/resources/org/apache/accumulo/access/testdata/testdata.json diff --git a/pom.xml b/pom.xml index 1203d01..9c546f8 100644 --- a/pom.xml +++ b/pom.xml @@ -77,6 +77,7 @@ + modules/test-data modules/core modules/examples modules/antlr4-example @@ -142,11 +143,6 @@ under the License. - - com.github.spotbugs - spotbugs-annotations - 4.9.8 - com.google.code.gson gson @@ -162,6 +158,11 @@ under the License. accumulo-access-core ${project.version} + + org.apache.accumulo + accumulo-access-test-data + ${project.version} + org.apache.accumulo accumulo-core @@ -172,6 +173,11 @@ under the License. junit-jupiter-api ${version.junit} + + org.junit.jupiter + junit-jupiter-engine + ${version.junit} + org.openjdk.jmh jmh-core @@ -424,6 +430,10 @@ under the License. true + + + org.junit.jupiter:junit-jupiter-engine:jar:* + @@ -915,6 +925,19 @@ under the License. + + + com.github.spotbugs + spotbugs-maven-plugin + [0,) + + check + + + + + + diff --git a/src/build/spotbugs-exclude.xml b/src/build/spotbugs-exclude.xml index 756574e..15481e5 100644 --- a/src/build/spotbugs-exclude.xml +++ b/src/build/spotbugs-exclude.xml @@ -24,12 +24,7 @@ - + - - - - - From 428d619d5195a4f25ab1e2c75cdd8a6df80162c4 Mon Sep 17 00:00:00 2001 From: Christopher Tubbs Date: Mon, 16 Mar 2026 21:31:36 -0400 Subject: [PATCH 2/2] Fix benchmark profile and add test --- .github/workflows/maven.yaml | 1 + modules/antlr4-example/pom.xml | 2 +- modules/core/pom.xml | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/maven.yaml b/.github/workflows/maven.yaml index 1935366..23425da 100644 --- a/.github/workflows/maven.yaml +++ b/.github/workflows/maven.yaml @@ -62,6 +62,7 @@ jobs: - {name: 'unit-tests', javaver: 21, args: 'verify -PskipQA -DskipTests=false'} - {name: 'qa-checks', javaver: 21, args: 'verify javadoc:jar -DskipTests -Dspotbugs.timeout=3600000'} - {name: 'errorprone', javaver: 21, args: 'verify -Perrorprone,skipQA'} + - {name: 'benchmarks', javaver: 21, args: 'verify -DskipQA -Dbenchmark=none'} - {name: 'jdk25', javaver: 25, args: 'verify javadoc:jar -DskipTests'} fail-fast: false runs-on: ubuntu-latest diff --git a/modules/antlr4-example/pom.xml b/modules/antlr4-example/pom.xml index bcb0be1..f7542ab 100644 --- a/modules/antlr4-example/pom.xml +++ b/modules/antlr4-example/pom.xml @@ -137,7 +137,7 @@ -classpath - org.apache.accumulo.access.grammar.antlr.AccessExpressionAntlrBenchmark + org.apache.accumulo.access.antlr4.benchmark.AccessExpressionAntlrBenchmark ${benchmark} diff --git a/modules/core/pom.xml b/modules/core/pom.xml index 6ea5d5f..2e6eaea 100644 --- a/modules/core/pom.xml +++ b/modules/core/pom.xml @@ -120,7 +120,7 @@ -classpath - org.apache.accumulo.access.tests.AccessExpressionBenchmark + org.apache.accumulo.access.benchmark.AccessExpressionBenchmark ${benchmark}