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.

Sign up for GitHub

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

Jump to bottom

Make mapToUser and mapToGrantedAuthority protected in JdbcUserDetails… #16561

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Make mapToUser and mapToGrantedAuthority protected in JdbcUserDetails… #16561

wants to merge 5 commits into from
+80 −4

Conversation

big-cir
Copy link
Contributor

@big-cir big-cir commented Feb 10, 2025

Changed

  • Changed mapToUser method to protected
  • Changed mapToGrantedAuthority method to protected

Related

@big-cir big-cir force-pushed the refactor/jdbc-user-details-manager-private-to-protected-methods branch from 6b6a462 to 548be22 Compare February 10, 2025 06:43
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 10, 2025
jzheaux
jzheaux requested changes Feb 14, 2025
View reviewed changes
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, @big-cir! Can we try composition first? I've left some feedback inline for details on how that could be done.

core/src/main/java/org/springframework/security/provisioning/JdbcUserDetailsManager.java Outdated
@@ -181,7 +181,7 @@ protected List<UserDetails> loadUsersByUsername(String username) {
return getJdbcTemplate().query(getUsersByUsernameQuery(), this::mapToUser, username);
}

private UserDetails mapToUser(ResultSet rs, int rowNum) throws SQLException {
protected UserDetails mapToUser(ResultSet rs, int rowNum) throws SQLException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of opening these methods up, could we first try using RowMapper<UserDetails>? I think it could work like this:

  1. add a new member called RowMapper<UserDetails> userDetailsMapper = this::mapToUser
  2. use this mapper in the JdbcTemplate invocation
  3. add setUserDetailsMapper(RowMapper) to the class

This reflects what newer Jdbc-based Spring Security components have been doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jzheaux
Thank you for your feedback! I've implemented the changes as per your suggestion. Could you please confirm if I've correctly understood and applied your recommendations?

@big-cir big-cir force-pushed the refactor/jdbc-user-details-manager-private-to-protected-methods branch 2 times, most recently from 78af6dc to 52faf76 Compare February 14, 2025 01:00
jzheaux
jzheaux requested changes Feb 14, 2025
View reviewed changes
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.

Hi, @big-cir, yes, that's what I'm thinking. Please see some additional feedback about the method and then let's also proceed with doing the same for mapToGrantedAuthority.

core/src/main/java/org/springframework/security/provisioning/JdbcUserDetailsManager.java
public JdbcUserDetailsManager() {
}

public JdbcUserDetailsManager(DataSource dataSource) {
setDataSource(dataSource);
}

public void setUserDetailsMapper(RowMapper<UserDetails> mapper) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's please add a JavaDoc description. It should include the following:

  1. The instruction this setter represents (for example, "Use this {@code RowMapper} to convert each user result into a {@link UserDetails}"
  2. What the default row mapper does
  3. When the method was introduced (for example, "@SInCE 6.5"

core/src/main/java/org/springframework/security/provisioning/JdbcUserDetailsManager.java Outdated
@@ -178,7 +186,7 @@ protected void initDao() throws ApplicationContextException {
*/
@Override
protected List<UserDetails> loadUsersByUsername(String username) {
return getJdbcTemplate().query(getUsersByUsernameQuery(), this::mapToUser, username);
return getJdbcTemplate().query(getUsersByUsernameQuery(), userDetailsMapper, username);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add two unit tests:

  1. One that confirms that setting a null value for setUserDetailsMapper will throw an exception
  2. One that confirms that setting a mock value gets used when querying (call setUserDetailMapper with a Mockito mock, search for a user, and verify that the mock row mapper was used)

@big-cir big-cir force-pushed the refactor/jdbc-user-details-manager-private-to-protected-methods branch from 52faf76 to d506c1a Compare February 15, 2025 13:26
@big-cir
Copy link
Contributor Author

big-cir commented Feb 15, 2025 •
edited
Loading

@jzheaux,
Thank you very much for your detailed feedback on the setUserDetailsMapper method in JdbcUserDetailsManager.
I've implemented all the changes you suggested. Would you kindly review the updates when you have a moment? I'm eager to ensure the modifications meet the project's standards and your expectations.

Thank you again for your time and expertise.

@big-cir big-cir force-pushed the refactor/jdbc-user-details-manager-private-to-protected-methods branch 4 times, most recently from 7ee7de6 to 50fd7c0 Compare February 15, 2025 14:55
@big-cir
Add grantedAuthorityMapper as a class member
f06c979 
- Add unit tests for setGrantedAuthorityMapper method

Signed-off-by: dae won <[email protected]>
@big-cir big-cir force-pushed the refactor/jdbc-user-details-manager-private-to-protected-methods branch from 50fd7c0 to f06c979 Compare February 15, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux jzheaux requested changes

Assignees
No one assigned
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mapToUser/mapToGrantedAuthority in JdbcUserDetailsManager to become protected
3 participants
@big-cir @jzheaux @spring-projects-issues