Skip to content

Remove internal Optional usage in favor of null checks #7295

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

Conversation

krisztian-toth
Copy link
Contributor

Fixes gh-7155

I did not modify public APIs, tests, examples and samples. Optional usage in the codebase after this PR:

config\src\main\java\org\springframework\security\config\annotation\web\configuration\OAuth2ClientConfiguration.java
config\src\test\java\org\springframework\security\config\annotation\web\configurers\NamespaceHttpAnonymousTests.java
config\src\test\java\org\springframework\security\config\annotation\web\configurers\NamespaceHttpLogoutTests.java
config\src\test\java\org\springframework\security\config\doc\XmlNode.java
config\src\test\java\org\springframework\security\config\web\server\OAuth2ResourceServerSpecTests.java
config\src\test\java\org\springframework\security\config\web\server\ServerHttpSecurityTests.java
oauth2\oauth2-client\src\main\java\org\springframework\security\oauth2\client\web\reactive\function\client\OAuth2AuthorizedClientResolver.java
oauth2\oauth2-client\src\main\java\org\springframework\security\oauth2\client\web\reactive\result\method\annotation\OAuth2AuthorizedClientResolver.java
oauth2\oauth2-client\src\test\java\org\springframework\security\oauth2\client\web\reactive\function\client\ServerOAuth2AuthorizedClientExchangeFilterFunctionTests.java
oauth2\oauth2-client\src\test\java\org\springframework\security\oauth2\client\web\reactive\function\client\ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java
oauth2\oauth2-core\src\test\java\org\springframework\security\oauth2\core\web\reactive\function\OAuth2BodyExtractorsTests.java
oauth2\oauth2-jose\src\test\java\org\springframework\security\oauth2\jwt\JwtDecodersTests.java
oauth2\oauth2-jose\src\test\java\org\springframework\security\oauth2\jwt\ReactiveJwtDecodersTests.java
oauth2\oauth2-resource-server\src\test\java\org\springframework\security\oauth2\server\resource\introspection\NimbusOpaqueTokenIntrospectorTests.java
oauth2\oauth2-resource-server\src\test\java\org\springframework\security\oauth2\server\resource\introspection\NimbusReactiveOpaqueTokenIntrospectorTests.java
samples\boot\oauth2resourceserver-multitenancy\src\main\java\org\springframework\boot\env\MockWebServerPropertySource.java
samples\boot\oauth2resourceserver-multitenancy\src\main\java\sample\OAuth2ResourceServerSecurityConfiguration.java
samples\boot\oauth2resourceserver-opaque\src\main\java\org\springframework\boot\env\MockWebServerPropertySource.java
samples\javaconfig\hellojs\src\main\java\org\springframework\security\samples\mvc\MessageJsonController.java

Note that Optional is still used in

  • org.springframework.security.oauth2.client.web.reactive.function.client.OAuth2AuthorizedClientResolver
  • org.springframework.security.oauth2.client.web.reactive.result.method.annotation.OAuth2AuthorizedClientResolver

As I couldn't think of a nice way to resolve it.

…ster' of https://github.com/watsta/spring-security into spring-projectsgh-7155-remove-optional-usage

# Conflicts:
#	oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunction.java
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 22, 2019
@rwinch rwinch requested a review from eleftherias August 22, 2019 14:42
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @watsta. I've left some feedback inline.

.map(configurationMetadata -> configurationMetadata.get("end_session_endpoint"))
.map(Object::toString)
.map(URI::create);
ClientRegistration clientRegistration = this.clientRegistrationRepository.findByRegistrationId(registrationId);
Copy link
Contributor

Choose a reason for hiding this comment

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

clientRegistration may be null. Please add a null check here.

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 6a4425c

@@ -67,7 +67,6 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to change this file since it is a test. Please revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted in f6f4378

if (this.accessTokenResponseClient != null) {
authorizedClientProviderBuilder.clientCredentials(configurer ->
configurer.accessTokenResponseClient(this.accessTokenResponseClient));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This code changes the existing functionality. If accessTokenResponseClient is null, clientCredentials should still be enabled. Please add an else statement containing authorizedClientProviderBuilder.clientCredentials() to ensure clientCredentials is enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 3fef167

@krisztian-toth
Copy link
Contributor Author

Hi @eleftherias, Thank you for the comments! I updated the PR with the requested changes.

@eleftherias eleftherias merged commit 2c2e8e5 into spring-projects:master Aug 26, 2019
@eleftherias
Copy link
Contributor

Thanks for the PR @watsta! This is now merged into master.

@rwinch rwinch added type: task A general task status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 18, 2019
@rwinch rwinch modified the milestones: 5.2.0, 5.2.0.RC1 Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove internal Optional usage in favor of null checks
4 participants