Skip to content

Replace internal Stream usage with for loops #7154

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

Closed
rwinch opened this issue Jul 26, 2019 · 6 comments
Closed

Replace internal Stream usage with for loops #7154

rwinch opened this issue Jul 26, 2019 · 6 comments
Labels
status: ideal-for-contribution An issue that we actively are looking for someone to help us with
Milestone

Comments

@rwinch
Copy link
Member

rwinch commented Jul 26, 2019

Summary

While some prefer the readability of using the Stream APIs, the GC overhead of creating additional objects (including intermediate objects) can cause significant decrease in performance There are some simple benchmarks that illustrate the problem.

We should replace Stream usage with for loops throughout Spring Security's code base.

Related gh-7155

@rwinch rwinch added the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Jul 26, 2019
@eddumelendez
Copy link
Contributor

Hi @rwinch, do you think this is good for first-timers-only?

@kostya05983
Copy link
Contributor

Hello @rwinch, I have an experience with stream api and others streams such as rxjava, I can take it in work. Where can I start? is stream api used in all modules?

@rwinch
Copy link
Member Author

rwinch commented Aug 6, 2019

Thanks for the offer @kostya05983!

A search shows the following locations contain an import to stream APIs. Note that we should only change private internal code. Please note:

  • We should not change any public APIs only internal implementation code
  • We do not need to change test code
  • We do not need to change sample code
samples/boot/oauth2authorizationserver/src/main/java/sample/AuthorizationServerConfiguration.java
core/src/main/java/org/springframework/security/authorization/AuthorityReactiveAuthorizationManager.java
core/src/main/java/org/springframework/security/converter/RsaKeyConverters.java
core/src/main/java/org/springframework/security/core/userdetails/MapReactiveUserDetailsService.java
config/src/test/java/org/springframework/security/config/http/MiscHttpConfigTests.java
config/src/test/java/org/springframework/security/config/http/SecurityFiltersAssertions.java
config/src/test/java/org/springframework/security/config/doc/SpringSecurityXsdParser.java
config/src/test/java/org/springframework/security/config/doc/XsdDocumentedTests.java
config/src/test/java/org/springframework/security/config/doc/XmlNode.java
web/src/main/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriter.java
web/src/test/java/org/springframework/security/web/server/header/ClearSiteDataServerHttpHeadersWriterTests.java
web/src/main/java/org/springframework/security/web/server/header/CompositeServerHttpHeadersWriter.java
web/src/main/java/org/springframework/security/web/server/header/ClearSiteDataServerHttpHeadersWriter.java
web/src/main/java/org/springframework/security/web/server/authentication/DelegatingServerAuthenticationSuccessHandler.java
samples/boot/oauth2resourceserver-opaque/src/main/java/org/springframework/boot/env/MockWebServerPropertySource.java
samples/boot/oauth2login/src/integration-test/java/org/springframework/security/samples/OAuth2LoginApplicationTests.java
samples/boot/oauth2resourceserver-multitenancy/src/main/java/org/springframework/boot/env/MockWebServerPropertySource.java
core/src/test/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandlerTests.java
core/src/main/java/org/springframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java
config/src/test/java/org/springframework/security/config/web/server/ServerHttpSecurityTests.java
config/src/test/java/org/springframework/security/config/web/server/OAuth2ResourceServerSpecTests.java
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverter.java
oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/JwtTimestampValidatorTests.java
oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/OAuth2AuthorizedClientProviderBuilder.java
web/src/test/java/org/springframework/security/web/server/authentication/logout/LogoutWebFilterTests.java
web/src/main/java/org/springframework/security/web/server/authentication/logout/DelegatingServerLogoutHandler.java
config/src/test/java/org/springframework/security/config/annotation/web/configurers/NamespaceHttpCustomFilterTests.java
config/src/test/java/org/springframework/security/config/annotation/web/configurers/NamespaceHttpJeeTests.java
config/src/main/java/org/springframework/security/config/annotation/authentication/configuration/AuthenticationConfiguration.java
oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/user/DefaultOAuth2User.java
oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationRequest.java
oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToListStringConverter.java
oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/registration/ClientRegistrationTests.java
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jose/jws/SignatureAlgorithm.java
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jose/jws/MacAlgorithm.java
oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryReactiveClientRegistrationRepository.java
oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepository.java
oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/http/converter/OAuth2AccessTokenResponseHttpMessageConverter.java
oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/authentication/ReactiveJwtGrantedAuthoritiesConverterAdapterTests.java
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/BearerTokenAuthenticationEntryPoint.java
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/authentication/OAuth2IntrospectionReactiveAuthenticationManager.java
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOAuth2TokenIntrospectionClient.java
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/authentication/OAuth2IntrospectionAuthenticationProvider.java
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOAuth2TokenIntrospectionClient.java
oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/access/BearerTokenAccessDeniedHandler.java
config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/access/server/BearerTokenServerAccessDeniedHandler.java
oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/BearerTokenServerAuthenticationEntryPoint.java

@kostya05983
Copy link
Contributor

Thanks for a detailed guide.

  • We should not change any public APIs only internal implementation code
    One question @rwinch :
    If I understand correctly, I shouldn't change any public methods, should I?

kostya05983 added a commit to kostya05983/spring-security that referenced this issue Aug 8, 2019
@rwinch
Copy link
Member Author

rwinch commented Aug 9, 2019

Don't change the method signatures of public methods. Changing how the method is implemented is fine.

@kostya05983
Copy link
Contributor

Ok, @rwinch , I didn't change the signature. I think, the request is ready for review, please see it, when you have a time.

kostya05983 added a commit to kostya05983/spring-security that referenced this issue Aug 17, 2019
kostya05983 added a commit to kostya05983/spring-security that referenced this issue Aug 26, 2019
First version of replacing streams

Fix wwwAuthenticate and codestyle

Fix errors in implementation to pass tests
kostya05983 added a commit to kostya05983/spring-security that referenced this issue Aug 31, 2019
kostya05983 added a commit to kostya05983/spring-security that referenced this issue Aug 31, 2019
First version of replacing streams

fix wwwAuthenticate and codestyle

fix errors in implementation to pass tests

Fix review notes

Remove uneccessary final to align with cb

Short circuit way to authorize

Simplify error message, make code readably

Return error while duplicate key found

Delete check for duplicate, checkstyle issues

Return duplicate error
@jzheaux jzheaux closed this as completed in f6c650d Sep 2, 2019
@eleftherias eleftherias added this to the 5.2.0.RC1 milestone Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ideal-for-contribution An issue that we actively are looking for someone to help us with
Projects
None yet
Development

No branches or pull requests

4 participants