Skip to content

fix(csharp): prevent percent-encoding of '?' in HTTP path query parameters#26

Merged
birschick-bq merged 12 commits into
adbc-drivers:mainfrom
eric-wang-1990:fix/http-path-query-encoding
Mar 12, 2026
Merged

fix(csharp): prevent percent-encoding of '?' in HTTP path query parameters#26
birschick-bq merged 12 commits into
adbc-drivers:mainfrom
eric-wang-1990:fix/http-path-query-encoding

Conversation

@eric-wang-1990

Copy link
Copy Markdown
Contributor

Summary

  • UriBuilder percent-encodes ? as %3F when a query string is embedded in the path argument (e.g. Databricks warehouse paths like /sql/1.0/warehouses/<id>?o=<orgId>)
  • Fix by splitting the path on ? and assigning the query portion to UriBuilder.Query separately, using Substring for net472/netstandard2.0 compatibility
  • Adds unit tests covering paths with query parameters, plain paths, and the full-URI passthrough case

Test plan

  • GetBaseAddress_PathWithQueryParam_DoesNotPercentEncodeQuestionMark — verifies ? is not encoded as %3F and path/query are correctly separated
  • GetBaseAddress_PathWithoutQueryParam_ReturnsExpectedUri — verifies plain paths are unaffected
  • GetBaseAddress_FullUri_ReturnsUriDirectly — verifies full URI passthrough is unchanged

🤖 Generated with Claude Code

eric-wang-1990 and others added 2 commits March 12, 2026 02:26
UriBuilder encodes '?' as '%3F' when a query string is embedded in the
path argument (e.g. Databricks warehouse paths like
/sql/1.0/warehouses/<id>?o=<orgId>). Fix by splitting path on '?' and
assigning the query portion to UriBuilder.Query separately.

Adds unit tests covering paths with and without query parameters, and
the full-URI passthrough case.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@eric-wang-1990 eric-wang-1990 changed the title fix: prevent percent-encoding of '?' in HTTP path query parameters csharp: prevent percent-encoding of '?' in HTTP path query parameters Mar 12, 2026
@eric-wang-1990 eric-wang-1990 changed the title csharp: prevent percent-encoding of '?' in HTTP path query parameters fix(csharp): prevent percent-encoding of '?' in HTTP path query parameters Mar 12, 2026
Comment thread csharp/test/Hive2/HiveServer2ConnectionGetBaseAddressTest.cs Outdated
Comment thread csharp/test/Hive2/HiveServer2ConnectionGetBaseAddressTest.cs
@eric-wang-1990

Copy link
Copy Markdown
Contributor Author

Code review

Found 1 issue:

  1. Reflection flags will cause all tests to throw at runtimeBindingFlags.NonPublic | BindingFlags.Static | BindingFlags.FlattenHierarchy does not surface protected static members from base classes. FlattenHierarchy only works with BindingFlags.Public, not NonPublic. Since GetBaseAddress is protected static on HiveServer2Connection, the GetMethod call returns null and every test throws InvalidOperationException("GetBaseAddress method not found") instead of running. The fix is to use BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy (since protected is treated as public for FlattenHierarchy purposes).

var connection = new SparkHttpConnection(new Dictionary<string, string>());
MethodInfo? method = connection.GetType().GetMethod(
"GetBaseAddress",
BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.FlattenHierarchy,
null,
new[] { typeof(string), typeof(string), typeof(string), typeof(string), typeof(string), typeof(bool) },
null);
if (method == null)

🤖 Generated with Claude Code

If this code review was useful, please react with 👍. Otherwise, react with 👎.

eric-wang-1990 and others added 2 commits March 12, 2026 02:39
…lection

BindingFlags.FlattenHierarchy only surfaces protected static members
when combined with BindingFlags.Public, not NonPublic. The previous
flags caused GetMethod to return null and all tests to throw at runtime.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread csharp/test/Hive2/HiveServer2ConnectionGetBaseAddressTest.cs Outdated
eric-wang-1990 and others added 3 commits March 12, 2026 02:41
Instantiating SparkHttpConnection triggered auth validation in the
constructor, failing all tests. Reflect on typeof(HiveServer2Connection)
directly instead — no instance needed for a static method, and the test
assembly has InternalsVisibleTo access.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s internal

Mark GetBaseAddress as protected internal so the test assembly (which has
InternalsVisibleTo access) can call it directly. This avoids brittle
reflection that would silently fail at runtime on signature changes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@birschick-bq birschick-bq left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There is an UriBuilder overload that takes the query.

Comment thread csharp/src/Hive2/HiveServer2Connection.cs Outdated
@birschick-bq

Copy link
Copy Markdown
Collaborator

License text requirement for new test file ...

Apache RAT Check.........................................................Failed
- hook id: rat
- duration: 3.96s
- exit code: 1

Checking licenses for /home/runner/work/hiveserver2/hiveserver2
Using Apache RAT: /home/runner/.cache/adbc-drivers-dev/apache-rat-0.16.1.jar
Files that should not have 'This file has been modified' header:
- csharp/test/Hive2/HiveServer2ConnectionGetBaseAddressTest.cs

Error: Process completed with exit code 1.

eric-wang-1990 and others added 2 commits March 12, 2026 09:20
Use the 5-parameter constructor that accepts the query string (extraValue)
directly, instead of setting .Query after construction.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove the 'This file has been modified' preamble which is only for
files derived from existing Apache-licensed sources. This file is new.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread csharp/test/Hive2/HiveServer2ConnectionGetBaseAddressTest.cs Outdated

@birschick-bq birschick-bq left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good.

@birschick-bq birschick-bq merged commit 825f9b9 into adbc-drivers:main Mar 12, 2026
9 checks passed
eric-wang-1990 added a commit to adbc-drivers/databricks that referenced this pull request Mar 12, 2026
UriBuilder was percent-encoding '?' as '%3F' in HTTP paths containing
query parameters (e.g. /sql/1.0/warehouses/<id>?o=<orgId>). This caused
connection failures for warehouse paths with org ID suffixes.

Upstream fix: adbc-drivers/hiveserver2#26

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-merge-queue Bot pushed a commit to adbc-drivers/databricks that referenced this pull request Mar 12, 2026
…ding (#341)

## Summary

- Updates the `hiveserver2` submodule to the latest `main` which
includes the fix for `UriBuilder` percent-encoding `?` as `%3F` in HTTP
paths containing query parameters (e.g.
`/sql/1.0/warehouses/<id>?o=<orgId>`)
- This caused connection failures for Databricks warehouse paths that
include an org ID suffix

## Upstream fix

adbc-drivers/hiveserver2#26

## Test plan

- [ ] Verify connection succeeds with HTTP path containing `?o=<orgId>`
query parameter

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
github-merge-queue Bot pushed a commit to adbc-drivers/databricks that referenced this pull request Mar 12, 2026
…ding (#341)

## Summary

- Updates the `hiveserver2` submodule to the latest `main` which
includes the fix for `UriBuilder` percent-encoding `?` as `%3F` in HTTP
paths containing query parameters (e.g.
`/sql/1.0/warehouses/<id>?o=<orgId>`)
- This caused connection failures for Databricks warehouse paths that
include an org ID suffix

## Upstream fix

adbc-drivers/hiveserver2#26

## Test plan

- [ ] Verify connection succeeds with HTTP path containing `?o=<orgId>`
query parameter

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants