From 383ceb8737b40b986b2b506d6168baf84d095d70 Mon Sep 17 00:00:00 2001 From: Dom G Date: Wed, 21 Feb 2024 13:22:34 -0500 Subject: [PATCH 1/3] Use sorted set in Authorizations --- .../apache/accumulo/access/Authorizations.java | 18 +++++++++++------- .../accumulo/access/AccessExpressionTest.java | 8 +++----- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/apache/accumulo/access/Authorizations.java b/src/main/java/org/apache/accumulo/access/Authorizations.java index f1b1640..faa5c36 100644 --- a/src/main/java/org/apache/accumulo/access/Authorizations.java +++ b/src/main/java/org/apache/accumulo/access/Authorizations.java @@ -18,8 +18,11 @@ */ package org.apache.accumulo.access; +import java.util.Arrays; import java.util.Collection; -import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; +import java.util.stream.Collectors; /** * An immutable collection of authorization strings. @@ -33,13 +36,13 @@ * @since 1.0.0 */ public final class Authorizations { - private final Set authorizations; + private final SortedSet authorizations; - private Authorizations(Set authorizations) { - this.authorizations = Set.copyOf(authorizations); + private Authorizations(SortedSet authorizations) { + this.authorizations = authorizations; } - public Set asSet() { + public SortedSet asSet() { return authorizations; } @@ -64,11 +67,12 @@ public String toString() { } public static Authorizations of(String... authorizations) { - return new Authorizations(Set.of(authorizations)); + return new Authorizations( + Arrays.stream(authorizations).collect(Collectors.toCollection(TreeSet::new))); } public static Authorizations of(Collection authorizations) { - return new Authorizations(Set.copyOf(authorizations)); + return new Authorizations(new TreeSet<>(authorizations)); } } diff --git a/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java b/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java index c41fdd8..07a53e4 100644 --- a/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java +++ b/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java @@ -35,7 +35,6 @@ import java.util.ArrayList; import java.util.List; import java.util.function.Predicate; -import java.util.stream.Collectors; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.function.Executable; @@ -61,11 +60,10 @@ public void testGetAuthorizations() { assertEquals(2, testCase.size()); var expression = testCase.get(0); var expected = testCase.get(1); - var actual = AccessExpression.of(expression).getAuthorizations().asSet().stream().sorted() - .collect(Collectors.joining(",")); + var actual = String.join(",", AccessExpression.of(expression).getAuthorizations().asSet()); assertEquals(expected, actual); - actual = AccessExpression.of(expression.getBytes(UTF_8)).getAuthorizations().asSet().stream() - .sorted().collect(Collectors.joining(",")); + actual = String.join(",", + AccessExpression.of(expression.getBytes(UTF_8)).getAuthorizations().asSet()); assertEquals(expected, actual); } From 6aee3e158fa73d70bbbf61c6958c31d9f25de685 Mon Sep 17 00:00:00 2001 From: Dom G Date: Thu, 22 Feb 2024 11:16:30 -0500 Subject: [PATCH 2/3] Review suggestions --- .../accumulo/access/Authorizations.java | 22 +++++++++---------- .../accumulo/access/AccessExpressionTest.java | 15 ++++++++++--- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/apache/accumulo/access/Authorizations.java b/src/main/java/org/apache/accumulo/access/Authorizations.java index faa5c36..b13d6bb 100644 --- a/src/main/java/org/apache/accumulo/access/Authorizations.java +++ b/src/main/java/org/apache/accumulo/access/Authorizations.java @@ -18,10 +18,8 @@ */ package org.apache.accumulo.access; -import java.util.Arrays; import java.util.Collection; -import java.util.SortedSet; -import java.util.TreeSet; +import java.util.Set; import java.util.stream.Collectors; /** @@ -36,13 +34,13 @@ * @since 1.0.0 */ public final class Authorizations { - private final SortedSet authorizations; + private final Set authorizations; - private Authorizations(SortedSet authorizations) { - this.authorizations = authorizations; + private Authorizations(Set authorizations) { + this.authorizations = Set.copyOf(authorizations); } - public SortedSet asSet() { + public Set asSet() { return authorizations; } @@ -61,18 +59,20 @@ public int hashCode() { return authorizations.hashCode(); } + /** + * @return a String containing the sorted, comma-separated set of authorizations + */ @Override public String toString() { - return authorizations.toString(); + return authorizations.stream().sorted().collect(Collectors.joining(",")); } public static Authorizations of(String... authorizations) { - return new Authorizations( - Arrays.stream(authorizations).collect(Collectors.toCollection(TreeSet::new))); + return new Authorizations(Set.of(authorizations)); } public static Authorizations of(Collection authorizations) { - return new Authorizations(new TreeSet<>(authorizations)); + return new Authorizations(Set.copyOf(authorizations)); } } diff --git a/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java b/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java index 07a53e4..2980d43 100644 --- a/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java +++ b/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java @@ -60,10 +60,9 @@ public void testGetAuthorizations() { assertEquals(2, testCase.size()); var expression = testCase.get(0); var expected = testCase.get(1); - var actual = String.join(",", AccessExpression.of(expression).getAuthorizations().asSet()); + var actual = AccessExpression.of(expression).getAuthorizations().toString(); assertEquals(expected, actual); - actual = String.join(",", - AccessExpression.of(expression.getBytes(UTF_8)).getAuthorizations().asSet()); + actual = AccessExpression.of(expression.getBytes(UTF_8)).getAuthorizations().toString(); assertEquals(expected, actual); } @@ -222,4 +221,14 @@ public void testSpecificationDocumentation() throws IOException, URISyntaxExcept assertFalse(specLinesFromAbnfFile.isEmpty()); // make sure we didn't just compare nothing assertEquals(specLinesFromAbnfFile, specLinesFromMarkdownFile); } + + @Test + public void authSetIsImmutable() { + Authorizations auths = AccessExpression.of("A&B").getAuthorizations(); + var authSet = auths.asSet(); + assertThrows(UnsupportedOperationException.class, () -> authSet.add("C"), + "Set returned by asSet should be immutable"); + assertThrows(UnsupportedOperationException.class, () -> authSet.remove("A"), + "Set returned by asSet should be immutable"); + } } From 51e02f5427d0428935125cf54a5040b9c708d87d Mon Sep 17 00:00:00 2001 From: Dom G Date: Thu, 22 Feb 2024 13:06:17 -0500 Subject: [PATCH 3/3] Use TreeSet for toString() --- .../apache/accumulo/access/Authorizations.java | 4 ++-- .../accumulo/access/AccessExpressionTest.java | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/apache/accumulo/access/Authorizations.java b/src/main/java/org/apache/accumulo/access/Authorizations.java index b13d6bb..2695f25 100644 --- a/src/main/java/org/apache/accumulo/access/Authorizations.java +++ b/src/main/java/org/apache/accumulo/access/Authorizations.java @@ -20,7 +20,7 @@ import java.util.Collection; import java.util.Set; -import java.util.stream.Collectors; +import java.util.TreeSet; /** * An immutable collection of authorization strings. @@ -64,7 +64,7 @@ public int hashCode() { */ @Override public String toString() { - return authorizations.stream().sorted().collect(Collectors.joining(",")); + return new TreeSet<>(authorizations).toString(); } public static Authorizations of(String... authorizations) { diff --git a/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java b/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java index 2980d43..cd9b671 100644 --- a/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java +++ b/src/test/java/org/apache/accumulo/access/AccessExpressionTest.java @@ -47,14 +47,14 @@ public void testGetAuthorizations() { // is the expected authorization in the expression var testData = new ArrayList>(); - testData.add(List.of("", "")); - testData.add(List.of("a", "a")); - testData.add(List.of("(a)", "a")); - testData.add(List.of("Z|M|A", "A,M,Z")); - testData.add(List.of("Z&M&A", "A,M,Z")); - testData.add(List.of("(Y|B|Y)&(Z|A|Z)", "A,B,Y,Z")); - testData.add(List.of("(Y&B&Y)|(Z&A&Z)", "A,B,Y,Z")); - testData.add(List.of("(A1|B1)&((A1|B2)&(B2|C1))", "A1,B1,B2,C1")); + testData.add(List.of("", "[]")); + testData.add(List.of("a", "[a]")); + testData.add(List.of("(a)", "[a]")); + testData.add(List.of("Z|M|A", "[A, M, Z]")); + testData.add(List.of("Z&M&A", "[A, M, Z]")); + testData.add(List.of("(Y|B|Y)&(Z|A|Z)", "[A, B, Y, Z]")); + testData.add(List.of("(Y&B&Y)|(Z&A&Z)", "[A, B, Y, Z]")); + testData.add(List.of("(A1|B1)&((A1|B2)&(B2|C1))", "[A1, B1, B2, C1]")); for (var testCase : testData) { assertEquals(2, testCase.size());