[KYUUBI #3222][FOLLOWUP] Fixing placeholder and config of user in JDBC Authentication Provider by bowenliang123 · Pull Request #3288 · apache/kyuubi

bowenliang123 · 2022-08-20T14:43:09Z

Why are the changes needed?

To fix the config name and placeholder with username introduced in #3235 violate this convention as in JDBC driver use user keyword used for connection user rather than username,

change config name from kyuubi.authentication.jdbc.username to kyuubi.authentication.jdbc.user
change placeholder from ${username} to ${user}
update docs and config description related to above changes, and sync the update in jdbc auth docs statement details to config docs.
fix error in throwing AuthenticationException with auth db password. ut added for the fix.
other minor update in docs of custom auth

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request

…ig name `kyuubi.authentication.jdbc.user`, 2. use ${user} placeholder instead of ${username}

…add ut for the fix.

bowenliang123 · 2022-08-20T14:44:18Z

@pan3793 Please have a look and I suggest to merge it to v1.6.0 and master.

pan3793

LGTM

codecov-commenter · 2022-08-20T16:22:24Z

Codecov Report

Merging #3288 (502703f) into master (4ee8171) will increase coverage by 0.05%.
The diff coverage is 92.10%.

@@             Coverage Diff              @@
##             master    #3288      +/-   ##
============================================
+ Coverage     51.37%   51.42%   +0.05%     
  Complexity       13       13              
============================================
  Files           469      469              
  Lines         26229    26237       +8     
  Branches       3630     3629       -1     
============================================
+ Hits          13474    13493      +19     
+ Misses        11469    11464       -5     
+ Partials       1286     1280       -6

Impacted Files	Coverage Δ
...uthentication/JdbcAuthenticationProviderImpl.scala	`90.16% <90.32%> (+16.94%)`	⬆️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala	`97.05% <100.00%> (ø)`
.../main/scala/org/apache/kyuubi/util/JdbcUtils.scala	`88.57% <100.00%> (+1.07%)`	⬆️
...ache/kyuubi/operation/KyuubiOperationManager.scala	`80.82% <0.00%> (-1.37%)`	⬇️
...g/apache/kyuubi/operation/BatchJobSubmission.scala	`74.40% <0.00%> (ø)`
...yuubi/server/metadata/jdbc/JDBCMetadataStore.scala	`89.96% <0.00%> (+0.69%)`	⬆️
...apache/kyuubi/engine/JpsApplicationOperation.scala	`80.64% <0.00%> (+3.22%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…le times with "i+1"

…t or authenticate

bowenliang123 · 2022-08-21T07:32:15Z

@pan3793 UT and docs improved and CI passed. Ready for the merge. Please take a look again.

pan3793 · 2022-08-22T05:36:03Z

Thanks, merging to master/1.6

…C Authentication Provider ### _Why are the changes needed?_ To fix the config name and placeholder with `username` introduced in #3235 violate this convention as in JDBC driver use `user` keyword used for connection user rather than `username`, 1. change config name from `kyuubi.authentication.jdbc.username` to `kyuubi.authentication.jdbc.user` 2. change placeholder from `${username}` to `${user}` 3. update docs and config description related to above changes, and sync the update in jdbc auth docs statement details to config docs. 4. fix error in throwing AuthenticationException with auth db password. ut added for the fix. 5. other minor update in docs of custom auth ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3288 from bowenliang123/jdbc-auth-config-update. Closes #3222 502703f [Bowen Liang] skip map for placeholder value lookup 3733be4 [liangbowen] nit ab00525 [liangbowen] nit 2301c4f [liangbowen] fix ut of jdbc auth with wrong_password 06f0c1b [liangbowen] remove redundant docs ec4565b [liangbowen] remove redundant docs ae1cce2 [liangbowen] fix compilation error of configLog 5d14103 [liangbowen] simplify configLog 6678e65 [liangbowen] reformat 52c1038 [liangbowen] simplify placeholder checking 21c2d5e [liangbowen] check whether placeholders in supported list before conn establishment or authenticate 7db0adf [liangbowen] ut for unknown placeholder 657de6a [liangbowen] nit 736b3f2 [liangbowen] refactoring placeholder value lookup, for preventing setString multiple times with "i+1" 86c8912 [liangbowen] setMaxRows after prepare placeholder, to postpone operation on jdbc conn 115fae5 [liangbowen] increase test code coverage b45b28c [liangbowen] resultSet returned by executeQuery is never null e1c0727 [liangbowen] update ut for redactPassword in JdbcUtils b4a52e2 [liangbowen] fix typo in docs of custom auth 371c2c6 [liangbowen] move redactPassword method to JdbcUtils and add ut. a4973c5 [liangbowen] reformat code 486e150 [liangbowen] fix error in throwing AuthenticationException with auth db password. add ut for the fix. efced90 [liangbowen] update settings.md ef97e35 [liangbowen] add SELECT prefix hint for doc of kyuubi.authentication.jdbc.query 025f94c [liangbowen] fix username to user in JdbcAuthenticationProviderImpl by 1. use config name `kyuubi.authentication.jdbc.user`, 2. use ${user} placeholder instead of ${username} Lead-authored-by: liangbowen <liangbowen@gf.com.cn> Co-authored-by: Bowen Liang <liangbowen@gf.com.cn> Signed-off-by: Cheng Pan <chengpan@apache.org>

bowenliang123 · 2022-08-22T06:02:40Z

@zhaomin1423 FYI. This PR has been merged to master/1.6.
The doc changes suggested in #3270 is also applied with it.

bowenliang123 added 4 commits August 20, 2022 21:54

fix username to user in JdbcAuthenticationProviderImpl by 1. use conf…

025f94c

…ig name `kyuubi.authentication.jdbc.user`, 2. use ${user} placeholder instead of ${username}

add SELECT prefix hint for doc of kyuubi.authentication.jdbc.query

ef97e35

update settings.md

efced90

fix error in throwing AuthenticationException with auth db password. …

486e150

…add ut for the fix.

github-actions Bot added kind:documentation Documentation is a feature! module:common labels Aug 20, 2022

bowenliang123 mentioned this pull request Aug 20, 2022

[KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code #3278

Closed

3 tasks

reformat code

a4973c5

pan3793 approved these changes Aug 20, 2022

View reviewed changes

pan3793 assigned bowenliang123 Aug 20, 2022

pan3793 added this to the v1.6.0 milestone Aug 20, 2022

move redactPassword method to JdbcUtils and add ut.

371c2c6

bowenliang123 commented Aug 20, 2022

View reviewed changes

Comment thread kyuubi-common/src/main/scala/org/apache/kyuubi/util/JdbcUtils.scala

bowenliang123 mentioned this pull request Aug 20, 2022

Change the first letter to uppercase #3270

Closed

3 tasks

fix typo in docs of custom auth

b4a52e2

bowenliang123 added 3 commits August 21, 2022 00:35

update ut for redactPassword in JdbcUtils

e1c0727

resultSet returned by executeQuery is never null

b45b28c

increase test code coverage

115fae5

bowenliang123 changed the title ~~[KYUUBI #3222][FOLLOWUP] Fixing placeholder and config with user keyword for JDBC Authentication Provider~~ [KYUUBI #3222][FOLLOWUP] Fixing placeholder and config of user in JDBC Authentication Provider Aug 20, 2022

bowenliang123 added 9 commits August 21, 2022 01:32

setMaxRows after prepare placeholder, to postpone operation on jdbc conn

86c8912

refactoring placeholder value lookup, for preventing setString multip…

736b3f2

…le times with "i+1"

nit

657de6a

ut for unknown placeholder

7db0adf

check whether placeholders in supported list before conn establishmen…

21c2d5e

…t or authenticate

simplify placeholder checking

52c1038

reformat

6678e65

simplify configLog

5d14103

fix compilation error of configLog

ae1cce2

bowenliang123 added 5 commits August 21, 2022 08:14

remove redundant docs

ec4565b

remove redundant docs

06f0c1b

fix ut of jdbc auth with wrong_password

2301c4f

nit

ab00525

nit

3733be4

pan3793 reviewed Aug 22, 2022

View reviewed changes

Comment thread ...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala Outdated

pan3793 reviewed Aug 22, 2022

View reviewed changes

Comment thread ...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala Outdated

skip map for placeholder value lookup

502703f

pan3793 closed this in 2b67ab6 Aug 22, 2022

bowenliang123 deleted the jdbc-auth-config-update branch August 31, 2022 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #3222][FOLLOWUP] Fixing placeholder and config of user in JDBC Authentication Provider#3288

[KYUUBI #3222][FOLLOWUP] Fixing placeholder and config of user in JDBC Authentication Provider#3288
bowenliang123 wants to merge 25 commits into
apache:masterfrom
bowenliang123:jdbc-auth-config-update

bowenliang123 commented Aug 20, 2022 •

edited

Loading

Uh oh!

bowenliang123 commented Aug 20, 2022 •

edited

Loading

Uh oh!

pan3793 left a comment

Uh oh!

Uh oh!

codecov-commenter commented Aug 20, 2022 •

edited

Loading

Uh oh!

bowenliang123 commented Aug 21, 2022

Uh oh!

Uh oh!

Uh oh!

pan3793 commented Aug 22, 2022

Uh oh!

bowenliang123 commented Aug 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bowenliang123 commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

How was this patch tested?

Uh oh!

bowenliang123 commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan3793 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bowenliang123 commented Aug 21, 2022

Uh oh!

Uh oh!

Uh oh!

pan3793 commented Aug 22, 2022

Uh oh!

bowenliang123 commented Aug 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bowenliang123 commented Aug 20, 2022 •

edited

Loading

bowenliang123 commented Aug 20, 2022 •

edited

Loading

codecov-commenter commented Aug 20, 2022 •

edited

Loading