ProviderManager Should Use CollectionUtils#contains #8695
Conversation
|
@yukihane Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@yukihane Thank you for signing the Contributor License Agreement! |
spring-projects-issues
added
the
status: waiting-for-triage
| @@ -145,7 +145,7 @@ private void checkState() { | |||
| throw new IllegalArgumentException( | |||
| "A parent AuthenticationManager or a list " | |||
| + "of AuthenticationProviders is required"); | |||
| } else if (providers.contains(null)) { | |||
| } else if (providers.stream().anyMatch(Objects::isNull)) { | |||
There was a problem hiding this comment.
At the framework level, we find that the Stream API is not fast enough. Would you please use a for loop instead?
There was a problem hiding this comment.
In particular we avoid the Stream APIs because they create extra objects which leads to excessive Garbage Collection. This can cause the JDK to pause and deteriorate the performance. See gh-7154
I believe we could use Spring's CollectionUtils.contains
There was a problem hiding this comment.
I've understood it and amended to use CollectionUtils.contains.
| @@ -102,6 +102,26 @@ public void testStartupFailsIfProvidersNotSetAsVarargs() { | |||
| new ProviderManager((AuthenticationProvider) null); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testUsingNullNotPermittedList() { | |||
There was a problem hiding this comment.
I think this test has the right idea. It seems like the list ought to have a null element, though, to fully describe the use case.
There was a problem hiding this comment.
I assumed it already existed.
I've created additional test case.
jzheaux
added
in: core
yukihane
force-pushed
the
alternative-null-elements-check
branch
from
3b7fe27
to
a8d910e
Compare
jzheaux
changed the title
jzheaux
changed the title
Use
stream().anyMatch(Objects::isNull)insead ofcontains(null), which may throwNullPointerException.Fixes #8689