Skip to content

Conversation

ngocnhan-tran1996
Copy link
Contributor

Closes #15805

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 16, 2024
Copy link
Contributor

@jzheaux jzheaux left a 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.

@jzheaux jzheaux self-assigned this Sep 16, 2024
@jzheaux jzheaux added in: config An issue in spring-security-config type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 16, 2024
@ngocnhan-tran1996
Copy link
Contributor Author

ngocnhan-tran1996 commented Sep 17, 2024

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.

@jzheaux

I found the reason, if change to use getBeanProvider in ServerHttpSecurity, we should fix test in CorsSpecTests. Should I change or preserve this and create a new PR?

@jzheaux
Copy link
Contributor

jzheaux commented Sep 18, 2024

Good point. I'm not sure that I'm a fan of mocking ApplicationContext as it can make tests like these a bit brittle. Would you consider updating that test to initialize the context in the following way, instead of using @Mock:

@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 context.getObjectProvider, the tests will not be affected.

If you agree, would you please make that change as a separate commit preceding the commit for updating to ObjectProvider? And then, when you believe the reactive updates are ready, please squash the other commits together so that you have two: One for the updates to CorsSpecTests and one for the ObjectProvider changes.

@ngocnhan-tran1996
Copy link
Contributor Author

@jzheaux

I updated unit test and changed commit. Please review

Copy link
Contributor

@jzheaux jzheaux left a 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

Use concrete ApplicationContext to simplify future maintenance.

Issue gh-4832
@ngocnhan-tran1996
Copy link
Contributor Author

@jzheaux

I updated unit test and changed commit. Please review

@jzheaux jzheaux added this to the 6.4.0-RC1 milestone Sep 23, 2024
@jzheaux jzheaux merged commit e618fc4 into spring-projects:main Sep 23, 2024
6 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Sep 23, 2024

Thanks, @ngocnhan-tran1996! This has now been merged into main

@ngocnhan-tran1996 ngocnhan-tran1996 deleted the enhance-main branch September 24, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Favor ObjectProvider over custom getBeanOrNull method
3 participants