OAK-11640 : removed usage of ImmutableSet.builder by rishabhdaim · Pull Request #2212 · apache/jackrabbit-oak

rishabhdaim · 2025-04-07T06:23:12Z

No description provided.

github-actions · 2025-04-07T06:23:40Z

Commit-Check ✔️

thomasmueller

It looks OK as far as I see, but I'm not sure about the security part.

Possibly memory usage could be much higher than with the current ImmutableSet.

reschke · 2025-04-07T07:19:28Z

...src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/AbstractLoginModule.java

-        Set<Object> creds = ImmutableSet.builder()
-                .addAll(subject.getPublicCredentials(Credentials.class))
-                .addAll(subject.getPublicCredentials(AuthInfo.class)).build();
+        Set<Object> creds = Stream.concat(subject.getPublicCredentials(Credentials.class).stream(), subject.getPublicCredentials(AuthInfo.class).stream())


Did you check that it's ok not to make this immutable?

Yes, it is only iterated in the same class to destroy these credentials, but I would say that we should play safe here as well. let me update the ticket to use Collections.unmodifiableSet

addressed in 1c2d8a0

reschke

...but see comments.

reschke · 2025-04-07T07:23:19Z

Possibly memory usage could be much higher than with the current ImmutableSet.

...because?

rishabhdaim · 2025-04-07T08:00:45Z

It looks OK as far as I see, but I'm not sure about the security part.

Possibly memory usage could be much higher than with the current ImmutableSet.

We have already done the micro benchmarks for SetUtils.toLinkedSet vs ImmutableSet and the results are here: #2178 (comment)

it seems that using Java's LinkedHashSet uses lesser memory because it doesn't uses another data structure (i.e. ArrayList in case of ImmutableSet) to store insertion order.

...-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreV8Test.java

mbaedke · 2025-04-07T08:32:09Z

It looks OK as far as I see, but I'm not sure about the security part.

Possibly memory usage could be much higher than with the current ImmutableSet.

AFAICT all the sets in question are pretty small (also I don't quite understand how the memory usage would increase). Could you elaborate on your security concerns?

thomasmueller · 2025-04-07T14:21:16Z

Possibly memory usage could be much higher than with the current ImmutableSet.

...because?

Because the hash table implementation is very different. Guava seems to use an open-addressing hash table (power of 2 sized) with linear probing, where entries are very small. Java internally (inside of LinkedHashSet) uses a HashMap that uses separate chaining....

Did someone test memory usage?

thomasmueller

More investigation is needed:

Verify memory usage
Verify behavior of null entries
Verify with someone that knows the security part

sonarqubecloud · 2025-04-07T14:33:55Z

Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

rishabhdaim · 2025-04-08T03:44:07Z

.../org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncConfig.java

-                    addAll(getAutoMembership()).build();
+            return Collections.unmodifiableSet((Set<String>)
+                    Stream.concat(autoMembershipConfig.getAutoMembership(authorizable).stream(), getAutoMembership().stream())
+                            .collect(Collectors.toCollection(LinkedHashSet::new)));


getAutoMembership has been annotated with @NotNull

rishabhdaim · 2025-04-08T03:45:48Z

...rg/apache/jackrabbit/oak/security/authorization/composite/CompositeAccessControlManager.java

+        Set<Privilege> privs = new LinkedHashSet<>();
        for (AccessControlManager acMgr : acMgrs) {
-            privs.add(acMgr.getSupportedPrivileges(absPath));
+            privs.addAll(Arrays.asList(acMgr.getSupportedPrivileges(absPath)));


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

rishabhdaim · 2025-04-08T03:47:47Z

...a/org/apache/jackrabbit/oak/exercise/security/authorization/models/simplifiedroles/Role.java

    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(


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

rishabhdaim · 2025-04-08T03:49:26Z

...src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/AbstractLoginModule.java

-        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));


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

rishabhdaim · 2025-04-08T03:49:42Z

...main/java/org/apache/jackrabbit/oak/spi/security/privilege/ImmutablePrivilegeDefinition.java

-            builder.addAll(declaredAggregateNames);
-        }
-        this.declaredAggregateNames = builder.build();
+        this.declaredAggregateNames = declaredAggregateNames != null ? Collections.unmodifiableSet(SetUtils.toLinkedSet(declaredAggregateNames)) : Set.of();


Null case already handled.

rishabhdaim · 2025-04-08T03:51:47Z

...pi/src/main/java/org/apache/jackrabbit/oak/spi/security/privilege/PrivilegeBitsProvider.java

    @NotNull
    private Set<String> resolveBuiltInAggregation(@NotNull String privilegeName) {
-        ImmutableSet.Builder<String> builder = ImmutableSet.builder();
+        Set<String> builder = new LinkedHashSet<>();


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

rishabhdaim · 2025-04-08T03:55:37Z

oak-store-composite/src/main/java/org/apache/jackrabbit/oak/composite/CompositionContext.java

-        ImmutableSet.Builder<MountedNodeStore> b = ImmutableSet.builder();
+        Set<MountedNodeStore> b = new LinkedHashSet<>();
        b.add(this.globalStore);
        b.addAll(this.nonDefaultStores);


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

rishabhdaim · 2025-04-08T03:56:22Z

oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositorySidegrade.java

        NodeStateCopier.builder()
            .include(includes)
-            .exclude(excludes.build())
+            .exclude(Collections.unmodifiableSet(excludes))


excludes can't be null.

rishabhdaim · 2025-04-08T03:59:35Z

More investigation is needed:

* Verify memory usage

* Verify behavior of null entries

* Verify with someone that knows the security part

@thomasmueller please find reply as below:

Memory usage had already been tested along with performance and both Java's LinkedHashSet & ImmutableSet provides similar performance and memory usage.
ImmutableSet also throws NPE in case we put null values for addAll method, which is again same behaviour of LinkedHashSet.addAll. Also, I have added comments for each method explaining whether null value is possible or not.
@anchela @Amoratinos could you please review from security point of view.

Amoratinos

to me looks good, there are a couple of trivial things like removing imports but nothing blocker

.../org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncConfig.java

...rg/apache/jackrabbit/oak/security/authorization/composite/CompositeAccessControlManager.java

...a/org/apache/jackrabbit/oak/exercise/security/authorization/models/simplifiedroles/Role.java

...src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/AbstractLoginModule.java

...a/org/apache/jackrabbit/oak/exercise/security/authorization/models/simplifiedroles/Role.java

thomasmueller

It seems my concerns are addressed then! Thanks a lot!

rishabhdaim requested review from anchela, mbaedke, nfsantos, reschke and thomasmueller April 7, 2025 06:23

thomasmueller reviewed Apr 7, 2025

View reviewed changes

reschke reviewed Apr 7, 2025

View reviewed changes

reschke approved these changes Apr 7, 2025

View reviewed changes

mbaedke reviewed Apr 7, 2025

View reviewed changes

...-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreV8Test.java Outdated Show resolved Hide resolved

rishabhdaim requested a review from mbaedke April 7, 2025 08:51

Rishabh Kumar added 3 commits April 7, 2025 18:13

OAK-11640 : removed usage of ImmutableSet.builder

db933cf

OAK-11640 : incorporated review comments)

28b39c7

OAK-11640 : revert unintendent code change

ad16d36

rishabhdaim force-pushed the OAK-11640 branch from 89ac490 to ad16d36 Compare April 7, 2025 12:43

mbaedke approved these changes Apr 7, 2025

View reviewed changes

thomasmueller requested changes Apr 7, 2025

View reviewed changes

rishabhdaim commented Apr 8, 2025

View reviewed changes

rishabhdaim requested a review from thomasmueller April 8, 2025 03:59

Amoratinos approved these changes Apr 8, 2025

View reviewed changes

thomasmueller reviewed Apr 8, 2025

View reviewed changes

thomasmueller self-requested a review April 8, 2025 12:20

rishabhdaim merged commit 3216a1d into trunk Apr 9, 2025
5 checks passed

rishabhdaim deleted the OAK-11640 branch April 9, 2025 01:51

Conversation

rishabhdaim commented Apr 7, 2025

Uh oh!

github-actions bot commented Apr 7, 2025

Commit-Check ✔️

Uh oh!

thomasmueller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reschke left a comment

Choose a reason for hiding this comment

Uh oh!

reschke commented Apr 7, 2025

Uh oh!

rishabhdaim commented Apr 7, 2025

Uh oh!

Uh oh!

mbaedke commented Apr 7, 2025

Uh oh!

thomasmueller commented Apr 7, 2025

Uh oh!

thomasmueller left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 7, 2025

Quality Gate passed

Uh oh!

rishabhdaim Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishabhdaim commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amoratinos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasmueller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rishabhdaim Apr 8, 2025 •

edited

Loading

rishabhdaim commented Apr 8, 2025 •

edited

Loading