[Multi-Database Support][pg] Make JdbcUserDetailsManager compat with postgre #4790
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shoothzj
changed the title
shoothzj
force-pushed
the
jdbc-user-manager-postgre
branch
from
cd23991 to
e9276db
Compare
Codecov Report
@@ Coverage Diff @@
## master #4790 +/- ##
============================================
+ Coverage 47.90% 47.97% +0.07%
- Complexity 1701 1703 +2
============================================
Files 346 346
Lines 10694 10697 +3
Branches 1066 1066
============================================
+ Hits 5123 5132 +9
+ Misses 5255 5249 -6
Partials 316 316
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
A new error occurred when I tried to create the app. |
|
@nobodyiam It's a hibernate error. I think it's no relationship with this patch. That's make JdbcUserDetailsManager compat with postgre. |
|
The sql provided in #4782 is wrong, after I corrected it, another error occurred after adding an item |
nobodyiam
reviewed
Mar 15, 2023
| return jdbcUserDetailsManager; | ||
| } | ||
|
|
||
| @Profile("postgre") |
There was a problem hiding this comment.
This duplicated the logic which doesn't look elegant as we may need to duplicate one for other database like h2, is there a better way to handle the sql difference?
There was a problem hiding this comment.
I didn't find a better way since we are direct using jdbc. Maybe we can refract this to use jpa. But I have no idea how to achieve this.
There was a problem hiding this comment.
It looks like the only difference is the quoted identifier, MySQL is ` while pg and h2 is ".
We may retrieve the Dialect and get the identifier from it?
@Bean
public static JdbcUserDetailsManager jdbcUserDetailsManager(PasswordEncoder passwordEncoder,
AuthenticationManagerBuilder auth, DataSource datasource,
EntityManagerFactory entityManagerFactory) throws Exception {
char openQuote = '`';
char closeQuote = '`';
try {
SessionFactoryImplementor sessionFactory = entityManagerFactory.unwrap(
SessionFactoryImplementor.class);
Dialect dialect = sessionFactory.getJdbcServices().getDialect();
openQuote = dialect.openQuote();
closeQuote = dialect.closeQuote();
} catch (Throwable ex) {
//ignore
}
...
}
|
@nobodyiam Thanks for your feedback. I think I will resolve it later out of this pr :) |
shoothzj
force-pushed
the
jdbc-user-manager-postgre
branch
from
e9276db to
339ed41
Compare
nobodyiam
reviewed
Mar 20, 2023
| "select Username,Authority from `Authorities` where Username = ?") | ||
| .getUserDetailsService(); | ||
| .passwordEncoder(passwordEncoder).dataSource(datasource) | ||
| .usersByUsernameQuery("SELECT " + openQuote + "Username" + closeQuote + "," + openQuote + "Password" + closeQuote + "," + openQuote + "Enabled" + closeQuote + " FROM " + openQuote + "Users" + closeQuote + " where " + openQuote + "Username" + closeQuote + " = ?") |
There was a problem hiding this comment.
This reduces the code readability a lot, maybe we could embed some placeholders in the SQL and replace the placeholders with openQuote and closeQuote? e.g.
"select ^Username$, ^Password$, ^Enabled$ from ^Users$ where ^Username$ = ?".replace('^', openQuote).replace('$', closeQuote);There was a problem hiding this comment.
it sounds like a specific trick, I can't say it's better. I think hibernate uses 'openQuote' way. We can rename the variable to openQ, closeQ to reduce the line length. I prefer no to change, but I can change if you insist.
Please let me know what you think
There was a problem hiding this comment.
Or we may extract some method to do the job? The main purpose is to improve the code readability.
There was a problem hiding this comment.
How about we use the built-in MessageFormat to format the SQL?
.usersByUsernameQuery(MessageFormat.format("select {0}Username{1},{0}Password{1},{0}Enabled{1} from {0}Users{1} where {0}Username{1} = ?", openQuote, closeQuote));
shoothzj
force-pushed
the
jdbc-user-manager-postgre
branch
from
f97f6f9 to
a10494b
Compare
nobodyiam
reviewed
Mar 26, 2023
| .authoritiesByUsernameQuery(authoritiesQuerySql(openQuote, closeQuote)) | ||
| .getUserDetailsService(); | ||
|
|
||
| jdbcUserDetailsManager.setUserExistsSql(usersExistsSql(openQuote, closeQuote)); |
There was a problem hiding this comment.
How about we simply inline the statement here as the method is not used elsewhere.
nobodyiam
reviewed
Mar 26, 2023
| jdbcUserDetailsManager | ||
| .setChangePasswordSql("update `Users` set Password = ? where id = (select u.id from (select id from `Users` where Username = ?) as u)"); | ||
| .passwordEncoder(passwordEncoder).dataSource(datasource) | ||
| .usersByUsernameQuery(usersQuerySql(openQuote, closeQuote)) |
There was a problem hiding this comment.
Shall we also inline these 2?
|
|
||
| jdbcUserDetailsManager.setUserExistsSql(MessageFormat.format("SELECT {0}Username{1} FROM {0}Users{1} WHERE {0}Username{1} = ?", openQuote, closeQuote)); | ||
| jdbcUserDetailsManager.setCreateUserSql(MessageFormat.format("INSERT INTO {0}Users{1} ({0}Username{1}, {0}Password{1}, {0}Enabled{1}) values (?,?,?)", openQuote, closeQuote)); | ||
| jdbcUserDetailsManager.setUpdateUserSql(MessageFormat.format("UPDATE {0}Users{1} SET {0}Password{1} = ?, {0}Enabled{1} = ? WHERE id = (SELECT u.id FROM (SELECT id FROM {0}Users{1} WHERE {0}Username{1} = ?) AS u)", openQuote, closeQuote)); |
There was a problem hiding this comment.
do we need to quote id? btw, is it necessary to use the correct case like Id?
nobodyiam
reviewed
Mar 26, 2023
...tal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java
Outdated
Show resolved
Hide resolved
...tal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java
Outdated
Show resolved
Hide resolved
...tal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java
Outdated
Show resolved
Hide resolved
...tal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java
Outdated
Show resolved
Hide resolved
shoothzj
force-pushed
the
jdbc-user-manager-postgre
branch
from
ff68b70 to
bc6a93e
Compare
|
@nobodyiam PTAL again. Thanks |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Brief changelog
Use profile to make JdbcUserDetailsManager compat with postgre
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.