-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
ObjectProvider over custom getBeanOrNull method #15816
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @ngocnhan-tran1996! I've left some feedback inline.
Also, will you please ensure that there are unit tests that confirm the pre-existing behavior? I'm concerned that some of the changes you made, though they change the behavior, did not cause any tests to fail.
.../springframework/security/config/annotation/web/configuration/HttpSecurityConfiguration.java
Show resolved
Hide resolved
...ava/org/springframework/security/config/annotation/web/configurers/RememberMeConfigurer.java
Show resolved
Hide resolved
...a/org/springframework/security/config/annotation/web/configurers/RequestCacheConfigurer.java
Show resolved
Hide resolved
.../springframework/security/config/annotation/web/configurers/SessionManagementConfigurer.java
Show resolved
Hide resolved
...main/java/org/springframework/security/config/annotation/web/configurers/X509Configurer.java
Show resolved
Hide resolved
...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java
Show resolved
Hide resolved
.../springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurer.java
Outdated
Show resolved
Hide resolved
...pringframework/security/config/annotation/web/configurers/saml2/Saml2MetadataConfigurer.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/security/config/authentication/AuthenticationManagerFactoryBean.java
Show resolved
Hide resolved
...ava/org/springframework/security/config/authentication/AuthenticationManagerFactoryBean.java
Show resolved
Hide resolved
I found the reason, if change to use |
Good point. I'm not sure that I'm a fan of mocking @Test
public void testXXX() {
GenericApplicationContext context = new GenericApplicationContext();
context.registerBean(CorsConfigurationSource.class, () -> this.source) // etc. as needed
context.refresh();
// ... remove context mocking
} In this way, when you change to If you agree, would you please make that change as a separate commit preceding the commit for updating to |
I updated unit test and changed commit. Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for another update, @ngocnhan-tran1996. This is really coming together. I've left just a bit more feedback, which I missed last time.
Additionally, would you please make sure that your commits have a format that includes the associated tickets, like so:
Favor ObjectProvider
Closes gh-15805
Polish CorsSpecTests
Use concrete ApplicationContext to simplify future maintenance.
Issue gh-4832
...amework/security/config/annotation/web/configurers/ExpressionUrlAuthorizationConfigurer.java
Outdated
Show resolved
Hide resolved
.../springframework/security/config/annotation/web/configuration/HttpSecurityConfiguration.java
Outdated
Show resolved
Hide resolved
...a/org/springframework/security/config/annotation/web/configurers/RequestCacheConfigurer.java
Outdated
Show resolved
Hide resolved
I updated unit test and changed commit. Please review |
Thanks, @ngocnhan-tran1996! This has now been merged into |
Closes #15805