Skip to content

Sort set in Authorizations.toString() and add immutability test #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DomGarguilo wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Sort set in Authorizations.toString() and add immutability test #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
383ceb8
Use sorted set in Authorizations
DomGarguilo Feb 21, 2024
6aee3e1
Review suggestions
DomGarguilo Feb 22, 2024
51e02f5
Use TreeSet for toString()
DomGarguilo Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/main/java/org/apache/accumulo/access/Authorizations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.util.Collection;
import java.util.Set;
import java.util.TreeSet;

/**
* An immutable collection of authorization strings.
Expand Down Expand Up @@ -58,9 +59,12 @@ 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 new TreeSet<>(authorizations).toString();
Copy link
Contributor

@dlmarion dlmarion Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what the penalty is here when the user doesn't care about sorting? Consider the example:

  LOG.info("Authorizations: {}", authorizations);

I wonder if we should leave toString() the way that it was, and add a new method with this implementation called toSortedString or something. i'm usually on the fence regarding changes like this because we are baking in a performance penalty 100% of the time for something that we think is a good idea. I'd rather let the user choose if they want that penalty.

Copy link
Member Author

@DomGarguilo DomGarguilo Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with that. Although in that case do you think its worth even creating a new method like toSortedString or should we just let the user use toSet and sort and create the string on their own?

Copy link
Contributor

@dlmarion dlmarion Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with letting users do it on their own. We should get the issue originator to weigh in on this.

Copy link
Member Author

@DomGarguilo DomGarguilo Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ctubbsii do you have any input on this as the creator of the issue associated with this PR?

Copy link
Member

@ctubbsii ctubbsii Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think now that Authorizations.of() takes a set (that was done in #68), we could just use the set that the user passed in, preserving whatever order it already had. If it's an immutable set already, we can just keep a reference to the same object instead of doing an unnecessary copy. If it's not, we can preserve the order when we do the copy (maybe using LinkedHashSet.addAll() wrapped with Collections.unmodifiableSet?). The only issue with this approach is that asSet() won't return something that has the SortedSet interface if it originally was a mutable sorted set, like TreeSet, but that might be an acceptable compromise).

Copy link
Contributor

@dlmarion dlmarion Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ctubbsii - are you saying we can close this and the associated issue?

}

public static Authorizations of(String... authorizations) {
Expand Down
33 changes: 20 additions & 13 deletions src/test/java/org/apache/accumulo/access/AccessExpressionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -48,24 +47,22 @@ public void testGetAuthorizations() {
// is the expected authorization in the expression
var testData = new ArrayList<List<String>>();

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());
var expression = testCase.get(0);
var expected = testCase.get(1);
var actual = AccessExpression.of(expression).getAuthorizations().asSet().stream().sorted()
.collect(Collectors.joining(","));
var actual = AccessExpression.of(expression).getAuthorizations().toString();
assertEquals(expected, actual);
actual = AccessExpression.of(expression.getBytes(UTF_8)).getAuthorizations().asSet().stream()
.sorted().collect(Collectors.joining(","));
actual = AccessExpression.of(expression.getBytes(UTF_8)).getAuthorizations().toString();
assertEquals(expected, actual);
}

Expand Down Expand Up @@ -224,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");
}
}