Skip to content
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.

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

OAK-11317 : removed usage of ImmutableSet from guava #1914

Merged
merged 4 commits into from
Dec 21, 2024
Merged

Conversation

rishabhdaim
Copy link
Contributor

No description provided.

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

I think it would be good to analyze the context about whether null can be present; in that case we could avoid the use of filter(). Here, this seems to be the case.

This probably applies to more places.

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

I have reviewed a small part and left a few comments where I can assume that things coulf be simplified; maybe even after a somehat mechanical check (like looking for uses of "unmodifiable" close to an assert).

Other than that, if test are passing,,,

try {
return new PrincipalImpl(idp.getIdentity(externalIdentityRef).getPrincipalName());
} catch (ExternalIdentityException e) {
throw new RuntimeException(e);
}
}).collect(Collectors.toSet());
}).collect(Collectors.toUnmodifiableSet());
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be two conversions to Set here (inherited from earlier).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 412cad0


ServiceReference ref3 = mock(ServiceReference.class);
when(ref3.getProperty(PARAM_SYNC_HANDLER_NAME)).thenReturn("testSH");
when(ref3.getProperty(PARAM_IDP_NAME)).thenReturn("testIDP-3");
tracker.addingService(ref3);

assertEquals(Set.of("testIDP", "testIDP-3"), ImmutableSet.copyOf(tracker.getIdpNames("testSH")));
assertEquals(Set.of("testIDP-2"), ImmutableSet.copyOf(tracker.getIdpNames("testSH-2")));
assertEquals(Set.of("testIDP", "testIDP-3"), Collections.unmodifiableSet(CollectionUtils.toSet(tracker.getIdpNames("testSH"))));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need conversions to immutable when the set id only used in a subsequent use of "assert"? Maybe we can check the diff for uses of unmodifiableSet in the context of asserts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch @reschke. I would fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 412cad0 along with other uses of Collections.unmodifiableSet in test cases.

@rishabhdaim
Copy link
Contributor Author

I think it would be good to analyze the context about whether null can be present; in that case we could avoid the use of filter(). Here, this seems to be the case.

This probably applies to more places.

I tried to use filter only where we get data from external sources or where I can't confirm whether null is possible. In case I have missed something, could you please help in pointing that out.

@reschke
Copy link
Contributor

reschke commented Dec 20, 2024

I tried to use filter only where we get data from external sources or where I can't confirm whether null is possible. In case I have missed something, could you please help in pointing that out.

That of course will be a lot of work... Maybe we could check for all uses of "::nonNull" in test cases first; but only later on.

@@ -17,6 +17,8 @@
package org.apache.jackrabbit.oak.spi.security.principal;

import java.security.Principal;
import java.util.ArrayList;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, fixed in 4707355

@rishabhdaim
Copy link
Contributor Author

rishabhdaim commented Dec 20, 2024

That of course will be a lot of work... Maybe we could check for all uses of "::nonNull" in test cases first; but only later on.

There is no usage in test files, only 3 usages in code files where we ingest data from outside.

@rishabhdaim rishabhdaim changed the title OAK-11317 :removed usage of ImmutableSet from guava OAK-11317 : removed usage of ImmutableSet from guava Dec 21, 2024
@rishabhdaim rishabhdaim merged commit 81abf85 into trunk Dec 21, 2024
4 of 5 checks passed
@rishabhdaim rishabhdaim deleted the OAK-11317 branch December 21, 2024 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants