[KYUUBI-7316] Fix parsePropertyFromUrl to remove query string from parsed values#7317
Closed
wangyum wants to merge 4 commits intoapache:masterfrom
Closed
[KYUUBI-7316] Fix parsePropertyFromUrl to remove query string from parsed values#7317wangyum wants to merge 4 commits intoapache:masterfrom
wangyum wants to merge 4 commits intoapache:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7317 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 698 698
Lines 43654 43658 +4
Branches 5896 5897 +1
======================================
- Misses 43654 43658 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an inconsistency in JDBC URL property parsing where parsePropertyFromUrl was incorrectly including query string parameters in property values while extractURLComponents correctly excluded them. This could cause authentication failures when using JWT auth with additional query parameters.
Changes:
- Modified
parsePropertyFromUrlto strip query string portions (everything after?) from parsed property values - Added comprehensive test coverage for both query string and non-query string scenarios
- Ensured both parsing methods return consistent results
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Utils.java | Updated parsePropertyFromUrl to remove query string parameters from property values |
| kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/UtilsTest.java | Added tests verifying consistency between parsing methods and correct handling of query strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
turboFei
reviewed
Jan 29, 2026
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
turboFei
reviewed
Feb 12, 2026
| String clientProperties = clientPropertiesSection.substring(semiColonIndex + 1); | ||
| Matcher matcher = | ||
| Pattern.compile("(?:^|;)" + Pattern.quote(key) + "=([^;]*)").matcher(clientProperties); | ||
| return matcher.find() ? matcher.group(1) : null; |
Member
There was a problem hiding this comment.
- per the code, I can see that, this method is only used to parse clientPropertiesSection just likes this PR
we can add some comments to make this method clear
- we can make this PR simple (two lines change)
String urlClientPropertiesSection = url.split("[?#]")[0];
String[] tokens = urlClientPropertiesSection.split(";");
turboFei
approved these changes
Feb 12, 2026
pan3793
reviewed
Feb 13, 2026
Member
|
CI starts to fail with "Could not find a valid Docker environment", it should be irrelevant, not sure what happened in infra. |
pan3793
approved these changes
Feb 13, 2026
turboFei
pushed a commit
that referenced
this pull request
Feb 13, 2026
… string from parsed values ### Why are the changes needed? Fixed the `parsePropertyFromUrl` method in `Utils.java` to properly remove query string parts (everything after ?) from parsed JDBC URL property values, making it consistent with the `extractURLComponents` method. When parsing JDBC URLs with query strings like: `jdbc:hive2://host:10012/db;auth=JWT?kyuubi.session.cluster=clusterA`. The `parsePropertyFromUrl` method was incorrectly returning `JWT?kyuubi.session.cluster=clusterA` for the `auth` property, while `extractURLComponents` correctly returned only `JWT`. This inconsistency could cause authentication failures in JWT auth scenarios with additional query parameters. ### How was this patch tested? 1. URLs with query strings parse properties correctly without the query string 2. URLs without query strings continue to work as expected 3. Both parsing methods return consistent results ### Was this patch authored or co-authored using generative AI tooling? No. Closes #7317 from wangyum/KYUUBI-7316. Closes #7317 6b8d3b5 [Yuming Wang] f 88a3c5a [Yuming Wang] fix e89260c [Yuming Wang] fix 206f28d [Yuming Wang] fix Authored-by: Yuming Wang <yumwang@ebay.com> Signed-off-by: Fei Wang <feiwang@apache.org> (cherry picked from commit c5acf54) Signed-off-by: Fei Wang <feiwang@apache.org>
turboFei
pushed a commit
that referenced
this pull request
Feb 13, 2026
… string from parsed values ### Why are the changes needed? Fixed the `parsePropertyFromUrl` method in `Utils.java` to properly remove query string parts (everything after ?) from parsed JDBC URL property values, making it consistent with the `extractURLComponents` method. When parsing JDBC URLs with query strings like: `jdbc:hive2://host:10012/db;auth=JWT?kyuubi.session.cluster=clusterA`. The `parsePropertyFromUrl` method was incorrectly returning `JWT?kyuubi.session.cluster=clusterA` for the `auth` property, while `extractURLComponents` correctly returned only `JWT`. This inconsistency could cause authentication failures in JWT auth scenarios with additional query parameters. ### How was this patch tested? 1. URLs with query strings parse properties correctly without the query string 2. URLs without query strings continue to work as expected 3. Both parsing methods return consistent results ### Was this patch authored or co-authored using generative AI tooling? No. Closes #7317 from wangyum/KYUUBI-7316. Closes #7317 6b8d3b5 [Yuming Wang] f 88a3c5a [Yuming Wang] fix e89260c [Yuming Wang] fix 206f28d [Yuming Wang] fix Authored-by: Yuming Wang <yumwang@ebay.com> Signed-off-by: Fei Wang <feiwang@apache.org> (cherry picked from commit c5acf54) Signed-off-by: Fei Wang <feiwang@apache.org>
Member
|
thanks, merged to 1.10.4 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why are the changes needed?
Fixed the
parsePropertyFromUrlmethod inUtils.javato properly remove query string parts (everything after ?) from parsed JDBC URL property values, making it consistent with theextractURLComponentsmethod.When parsing JDBC URLs with query strings like:
jdbc:hive2://host:10012/db;auth=JWT?kyuubi.session.cluster=clusterA.The
parsePropertyFromUrlmethod was incorrectly returningJWT?kyuubi.session.cluster=clusterAfor theauthproperty, whileextractURLComponentscorrectly returned onlyJWT. This inconsistency could cause authentication failures in JWT auth scenarios with additional query parameters.How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No.