Skip to content

OAK-11640 : removed usage of ImmutableSet.builder #2212

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

Merged
rishabhdaim merged 3 commits into from
Apr 9, 2025
Merged

OAK-11640 : removed usage of ImmutableSet.builder #2212

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
db933cf
OAK-11640 : removed usage of ImmutableSet.builder
Apr 7, 2025
28b39c7
OAK-11640 : incorporated review comments)
Apr 7, 2025
ad16d36
OAK-11640 : revert unintendent code change
Apr 7, 2025
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
10 changes: 7 additions & 3 deletions ...g/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
import org.jetbrains.annotations.NotNull;

/**
Expand Down Expand Up @@ -203,9 +207,9 @@ public Authorizable setAutoMembershipConfig(@NotNull AutoMembershipConfig autoMe
*/
@NotNull
public Set<String> getAutoMembership(@NotNull org.apache.jackrabbit.api.security.user.Authorizable authorizable) {
return ImmutableSet.<String>builder().
addAll(autoMembershipConfig.getAutoMembership(authorizable)).
addAll(getAutoMembership()).build();
return Collections.unmodifiableSet((Set<String>)
Stream.concat(autoMembershipConfig.getAutoMembership(authorizable).stream(), getAutoMembership().stream())
.collect(Collectors.toCollection(LinkedHashSet::new)));
Copy link
Contributor Author

@rishabhdaim rishabhdaim Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

getAutoMembership has been annotated with @NotNull

}

/**
Expand Down
10 changes: 6 additions & 4 deletions ...apache/jackrabbit/oak/security/authorization/composite/CompositeAccessControlManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@

import java.security.Principal;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import javax.jcr.AccessDeniedException;
Expand Down Expand Up @@ -71,12 +74,11 @@ public CompositeAccessControlManager(@NotNull Root root,
@NotNull
@Override
public Privilege[] getSupportedPrivileges(String absPath) throws RepositoryException {
ImmutableSet.Builder<Privilege> privs = ImmutableSet.builder();
Set<Privilege> privs = new LinkedHashSet<>();
for (AccessControlManager acMgr : acMgrs) {
privs.add(acMgr.getSupportedPrivileges(absPath));
privs.addAll(Arrays.asList(acMgr.getSupportedPrivileges(absPath)));
Copy link
Contributor Author

@rishabhdaim rishabhdaim Apr 8, 2025

Choose a reason for hiding this comment

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

Null not possible here as well since getSupportedPrivileges has been annotated as @NotNull

}
Set<Privilege> s = privs.build();
return s.toArray(new Privilege[0]);
return privs.toArray(new Privilege[0]);
}

@Override
Expand Down
9 changes: 8 additions & 1 deletion ...rg/apache/jackrabbit/oak/exercise/security/authorization/models/simplifiedroles/Role.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
import org.apache.jackrabbit.oak.commons.collections.SetUtils;
import org.apache.jackrabbit.oak.spi.security.authorization.permission.Permissions;
Expand All @@ -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(
Copy link
Contributor Author

@rishabhdaim rishabhdaim Apr 8, 2025

Choose a reason for hiding this comment

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

private constructor called from within the class, privilegeNames can't be null here.

(Set<String>) Stream.concat(base.privilegeNames.stream(), Arrays.stream(privilegeNames))
.collect(Collectors.toCollection(LinkedHashSet::new)));
}

boolean grants(long permissions) {
Expand Down
8 changes: 5 additions & 3 deletions .../main/java/org/apache/jackrabbit/oak/spi/security/authentication/AbstractLoginModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.security.PrivilegedExceptionAction;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import javax.jcr.Credentials;
Expand Down Expand Up @@ -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));
Copy link
Contributor Author

@rishabhdaim rishabhdaim Apr 8, 2025

Choose a reason for hiding this comment

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

same case as above, getPublicCredentials can't return null.

builder.addAll(subject.getPublicCredentials(AuthInfo.class));
Set<Object> creds = Collections.unmodifiableSet(builder);

if (!subject.getPrincipals().isEmpty() && !creds.isEmpty()) {
// clear subject if not readonly
if (!subject.isReadOnly()) {
Expand Down
8 changes: 3 additions & 5 deletions ...n/java/org/apache/jackrabbit/oak/spi/security/privilege/ImmutablePrivilegeDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@
*/
package org.apache.jackrabbit.oak.spi.security.privilege;

import java.util.Collections;
import java.util.Objects;
import java.util.Set;

import org.apache.jackrabbit.guava.common.collect.ImmutableSet;

import org.apache.jackrabbit.oak.commons.collections.SetUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

Expand All @@ -37,11 +39,7 @@ public final class ImmutablePrivilegeDefinition implements PrivilegeDefinition {
public ImmutablePrivilegeDefinition(@NotNull String name, boolean isAbstract, @Nullable Iterable<String> declaredAggregateNames) {
this.name = name;
this.isAbstract = isAbstract;
ImmutableSet.Builder<String> builder = ImmutableSet.builder();
if (declaredAggregateNames != null) {
builder.addAll(declaredAggregateNames);
}
this.declaredAggregateNames = builder.build();
this.declaredAggregateNames = declaredAggregateNames != null ? Collections.unmodifiableSet(SetUtils.toLinkedSet(declaredAggregateNames)) : Set.of();
Copy link
Contributor Author

@rishabhdaim rishabhdaim Apr 8, 2025

Choose a reason for hiding this comment

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

Null case already handled.

hashcode = Objects.hash(this.name, this.isAbstract, this.declaredAggregateNames);
}

Expand Down
11 changes: 6 additions & 5 deletions ...src/main/java/org/apache/jackrabbit/oak/spi/security/privilege/PrivilegeBitsProvider.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.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
Expand Down Expand Up @@ -268,15 +269,15 @@ private Iterable<String> extractAggregatedPrivileges(@NotNull Iterable<String> p

@NotNull
private Set<String> resolveBuiltInAggregation(@NotNull String privilegeName) {
ImmutableSet.Builder<String> builder = ImmutableSet.builder();
Set<String> builder = new LinkedHashSet<>();
Copy link
Contributor Author

@rishabhdaim rishabhdaim Apr 8, 2025

Choose a reason for hiding this comment

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

Only called when AGGREGATE_PRIVILEGES already has privilegeName, so null value not possible here as well.

for (String name : AGGREGATE_PRIVILEGES.get(privilegeName)) {
if (!AGGREGATE_PRIVILEGES.containsKey(name)) {
builder.add(name);
} else {
builder.addAll(resolveBuiltInAggregation(name));
}
}
Set<String> set = builder.build();
Set<String> set = Collections.unmodifiableSet(builder);
aggregation.put(privilegeName, set);
return set;
}
Expand All @@ -295,10 +296,10 @@ public Iterable<String> apply(@Nullable String privName) {
} else if (AGGREGATE_PRIVILEGES.containsKey(privName)) {
return resolveBuiltInAggregation(privName);
} else {
ImmutableSet.Builder<String> builder = ImmutableSet.builder();
Set<String> builder = new LinkedHashSet<>();
fillAggregation(getPrivilegesTree().getChild(privName), builder);

Set<String> aggregates = builder.build();
Set<String> aggregates = Collections.unmodifiableSet(builder);
if (!JCR_ALL.equals(privName) && !aggregates.isEmpty()) {
aggregation.put(privName, aggregates);
}
Expand All @@ -307,7 +308,7 @@ public Iterable<String> apply(@Nullable String privName) {
}
}

private void fillAggregation(@NotNull Tree privTree, @NotNull ImmutableSet.Builder<String> builder) {
private void fillAggregation(@NotNull Tree privTree, @NotNull Set<String> builder) {
if (!privTree.exists()) {
return;
}
Expand Down
9 changes: 7 additions & 2 deletions ...ure/src/test/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreV8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@
import java.net.URISyntaxException;
import java.time.Duration;
import java.time.Instant;
import java.util.Collections;
import java.util.Date;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Set;
import java.util.stream.StreamSupport;

import org.apache.jackrabbit.oak.blob.cloud.azure.blobstorage.AzuriteDockerRule;
import org.apache.jackrabbit.oak.commons.collections.SetUtils;
import org.apache.jackrabbit.oak.segment.azure.AzureSegmentStoreService;
import org.apache.jackrabbit.oak.segment.azure.Configuration;
import org.apache.jackrabbit.oak.segment.azure.util.Environment;
Expand Down Expand Up @@ -244,8 +246,11 @@ private static Instant yesterday() {
return Instant.now().minus(Duration.ofDays(1));
}

private static ImmutableSet<String> concat(ImmutableSet<String> blobs, String element) {
return ImmutableSet.<String>builder().addAll(blobs).add(element).build();
private static Set<String> concat(Set<String> blobs, String element) {

final Set<String> set = SetUtils.toLinkedSet(blobs);
set.add(element);
return Collections.unmodifiableSet(set);
}

private static Configuration getConfigurationWithSharedAccessSignature(String sasToken) {
Expand Down
6 changes: 4 additions & 2 deletions ...store-composite/src/main/java/org/apache/jackrabbit/oak/composite/CompositionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -67,10 +69,10 @@ class CompositionContext {
this.prefetchNodeStore = globalStore instanceof PrefetchNodeStore ? (PrefetchNodeStore) globalStore : PrefetchNodeStore.NOOP;
this.nonDefaultStores = nonDefaultStores;

ImmutableSet.Builder<MountedNodeStore> b = ImmutableSet.builder();
Set<MountedNodeStore> b = new LinkedHashSet<>();
b.add(this.globalStore);
b.addAll(this.nonDefaultStores);
Copy link
Contributor Author

@rishabhdaim rishabhdaim Apr 8, 2025

Choose a reason for hiding this comment

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

Both values globalStore & nonDefaultStores can't be null else the code would have already failed in the higher tier.

allStores = b.build();
allStores = Collections.unmodifiableSet(b);

this.nodeStoresByMount = allStores.stream().collect(Collectors.toMap(MountedNodeStore::getMount, Function.identity()));
this.nodeStateMonitor = nodeStateMonitor;
Expand Down
6 changes: 2 additions & 4 deletions ...-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,15 @@

import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public class MongoStatus implements ServerMonitorListener {

private static final Logger LOG = LoggerFactory.getLogger(MongoStatus.class);

private static final ImmutableSet<String> SERVER_DETAIL_FIELD_NAMES
= ImmutableSet.<String>builder()
.add("host", "process", "connections", "repl", "storageEngine", "mem")
.build();
private static final Set<String> SERVER_DETAIL_FIELD_NAMES = Set.of("host", "process", "connections", "repl", "storageEngine", "mem");

private final MongoClient client;

Expand Down
6 changes: 3 additions & 3 deletions oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositorySidegrade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.Calendar;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -465,8 +466,7 @@ private boolean isCompleteMigration() {

private void copyWorkspace(NodeState sourceRoot, NodeBuilder targetRoot) {
final Set<String> includes = calculateEffectiveIncludePaths(includePaths, sourceRoot);
ImmutableSet.Builder<String> excludes = new ImmutableSet.Builder<>();
excludes.addAll(excludePaths);
Set<String> excludes = new LinkedHashSet<>(excludePaths);
if (!versionCopyConfiguration.isCopyAll()) {
excludes.add("/jcr:system/jcr:versionStorage");
}
Expand All @@ -477,7 +477,7 @@ private void copyWorkspace(NodeState sourceRoot, NodeBuilder targetRoot) {

NodeStateCopier.builder()
.include(includes)
.exclude(excludes.build())
.exclude(Collections.unmodifiableSet(excludes))
Copy link
Contributor Author

@rishabhdaim rishabhdaim Apr 8, 2025

Choose a reason for hiding this comment

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

excludes can't be null.

.merge(merges)
.copy(sourceRoot, targetRoot);

Expand Down
Loading