Amoratinos commented on code in PR #2212: URL: https://github.com/apache/jackrabbit-oak/pull/2212#discussion_r2032675189
########## oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncConfig.java: ########## @@ -18,10 +18,14 @@ import java.util.Collections; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.apache.jackrabbit.guava.common.collect.ImmutableSet; +import org.apache.jackrabbit.oak.commons.collections.SetUtils; Review Comment: These two are unused in the class could you please remove them? ########## oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/AbstractLoginModule.java: ########## @@ -212,9 +213,10 @@ public void initialize(Subject subject, CallbackHandler callbackHandler, Map<Str @Override public boolean logout() throws LoginException { boolean success = false; - Set<Object> creds = ImmutableSet.builder() - .addAll(subject.getPublicCredentials(Credentials.class)) - .addAll(subject.getPublicCredentials(AuthInfo.class)).build(); + Set<Object> builder = new LinkedHashSet<>(subject.getPublicCredentials(Credentials.class)); + builder.addAll(subject.getPublicCredentials(AuthInfo.class)); + Set<Object> creds = Collections.unmodifiableSet(builder); Review Comment: Could you review the imports as `import org.apache.jackrabbit.guava.common.collect.ImmutableSet;` is not longer used after the changes ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/composite/CompositeAccessControlManager.java: ########## @@ -18,7 +18,10 @@ import java.security.Principal; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; Review Comment: Unused import, could you please remove it from here? ########## oak-exercise/src/main/java/org/apache/jackrabbit/oak/exercise/security/authorization/models/simplifiedroles/Role.java: ########## @@ -44,7 +49,9 @@ private Role(long permissions, String... privilegeNames) { private Role(@NotNull Role base, long permissions, String... privilegeNames) { this.permissions = base.permissions|permissions; - this.privilegeNames = ImmutableSet.<String>builder().addAll(base.privilegeNames).add(privilegeNames).build(); + this.privilegeNames = Collections.unmodifiableSet( + (Set<String>) Stream.concat(base.privilegeNames.stream(), Arrays.stream(privilegeNames)) + .collect(Collectors.toCollection(LinkedHashSet::new))); Review Comment: There's a shortcut to create unmodifiable sets from Collectors class: ```suggestion this.privilegeNames = Stream.concat(base.privilegeNames.stream(), Arrays.stream(privilegeNames)) .collect(Collectors.toUnmodifiableSet()); ``` ########## oak-exercise/src/main/java/org/apache/jackrabbit/oak/exercise/security/authorization/models/simplifiedroles/Role.java: ########## @@ -16,8 +16,13 @@ */ package org.apache.jackrabbit.oak.exercise.security.authorization.models.simplifiedroles; +import java.util.Arrays; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + import org.apache.jackrabbit.guava.common.collect.ImmutableSet; Review Comment: With the changes this import is no longer used -- 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: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org