Skip to content
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

Polish OAuth2ClientConfiguration #15857

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Polish OAuth2ClientConfiguration #15857

merged 1 commit into from
Sep 30, 2024

Conversation

kse-music
Copy link
Contributor

@kse-music kse-music commented Sep 26, 2024

Use constructor inject and use ObjectProvider.getIfUnique instead of list.size == 1

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

jzheaux commented Sep 26, 2024

Thanks for the cleanup, @kse-music! Will you please add some more detail to your Git commit so that it is clear from reading the title what is going on? In the absence of a ticket that already has this detail, it would also be helpful if the commit had a description as to the benefit of this commit.

Second, I imagine that this pattern of taking a list and checking the size is common in Spring Security. Can you update the PR to clean up other places as well (e.g. OAuth2ClientConfiguration, AbstractSecurityWebSocketMessageBrokerConfigurer, etc.).

Finally, I think it would be better to do the Arrays.asList -> Collections.singleton maneuver in a separate PR since there are likely many places where this appears as well.

I can help with additional commits for anything you feel you don't have time for, just let me know.

@kse-music kse-music marked this pull request as draft September 28, 2024 00:19
@kse-music kse-music changed the title Polish Polish OAuth2ClientConfiguration Sep 28, 2024
@kse-music
Copy link
Contributor Author

@jzheaux After inspection, only OAuth2ClientConfiguration, OAuth2ClientWebFluxSecurityConfiguration, and AbstractSecurityWebSocketMessageBrokerConfigurer were found to have the only bean judgment in the spring-config module. All except AbstractSecurityWebSocketMessageBrokerConfigurer have been modified because it tag deprecated.

If I have overlooked anything or other needs to be revised, please feel free to add it.

@kse-music kse-music marked this pull request as ready for review September 28, 2024 01:15
@rwinch
Copy link
Member

rwinch commented Sep 30, 2024

Finally, I think it would be better to do the Arrays.asList -> Collections.singleton maneuver in a separate PR since there are likely many places where this appears as well.

Note that Arrays.asList is mutable and Collections.singleton is not. Switching to an immutable Collection would be considered as a breaking change.

@jzheaux
Copy link
Contributor

jzheaux commented Sep 30, 2024

@rwinch, I learned something new today. :) I had always thought Arrays.asList was immutable since I couldn't add or remove from it. However, you can use set(index, value). Good catch.

@jzheaux jzheaux self-assigned this Sep 30, 2024
@jzheaux jzheaux added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 30, 2024
@jzheaux jzheaux added this to the 6.4.0-RC1 milestone Sep 30, 2024
@jzheaux jzheaux merged commit 8b97fdd into spring-projects:main Sep 30, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants