Skip to content

feat(csharp/src/Drivers/Databricks): capture x-thriftserver-error-message header #3558

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
Merged
CurtHagenlocher merged 2 commits into from
Oct 14, 2025
Merged

feat(csharp/src/Drivers/Databricks): capture x-thriftserver-error-message header #3558

CurtHagenlocher merged 2 commits into from
Oct 14, 2025

Conversation

@eric-wang-1990
Copy link
Contributor

@eric-wang-1990 eric-wang-1990 commented Oct 13, 2025

Rationale for this change

When the Databricks ADBC C# driver encounters HTTP errors during Thrift operations (e.g., 401 Unauthorized, 403 Forbidden), the specific error message from the Databricks server is lost. The server includes detailed error information in the x-thriftserver-error-message HTTP response header, but currently only generic HTTP status messages reach users.

This makes debugging authentication and authorization issues difficult, as users cannot distinguish between different failure causes (expired token vs. invalid token vs. insufficient permissions).

What changes are included in this PR?

  • Add ThriftErrorMessageHandler as a new DelegatingHandler that intercepts HTTP error responses
  • Extract x-thriftserver-error-message header and include it in exception messages
  • Integrate handler into DatabricksConnection HTTP handler chain as the innermost handler
  • Add comprehensive unit tests covering 11 test scenarios
  • Compatible with .NET Framework 4.7.2, .NET Standard 2.0, and .NET 8.0

Are these changes tested?

Yes. Added ThriftErrorMessageHandlerTest.cs with 11 unit tests covering:

  • HTTP 401/403 with Thrift error messages
  • Success responses (pass through unchanged)
  • Error responses without header (pass through unchanged)
  • Empty header values (ignored)
  • Multiple HTTP status codes (400, 401, 403, 500, 503)
  • Multiple header values (joined with commas)

All tests pass:

Test Run Successful.
Total tests: 11
     Passed: 11
 Total time: 0.6654 Seconds

Build verification also passed for all target frameworks.

Are there any user-facing changes?

Yes - users will now see detailed error messages from Databricks instead of generic HTTP status codes:

Before:

An unexpected error occurred while opening the session. 'Response status code does not indicate success: 401 (Unauthorized).'

After:

An unexpected error occurred while opening the session. 'Thrift server error: Invalid personal access token (HTTP 401 Unauthorized)'

This is a backward-compatible enhancement - if the header is not present, behavior is unchanged.

Closes #3557

@github-actions github-actions bot added this to the ADBC Libraries 21 milestone Oct 13, 2025
@eric-wang-1990 eric-wang-1990 changed the title feat(csharp/driver/databricks): capture x-thriftserver-error-message header feat(csharp/src/drivers/databricks): capture x-thriftserver-error-message header Oct 13, 2025
@eric-wang-1990
feat(csharp/driver/databricks): capture x-thriftserver-error-message …
cc9f2c5 
…header

Add ThriftErrorMessageHandler to capture and surface detailed error messages
from the x-thriftserver-error-message HTTP response header when Thrift
operations fail with HTTP error codes (401, 403, etc.).

Previously, when authentication or authorization failed, users would only see
generic HTTP status messages like 'Response status code does not indicate
success: 401 (Unauthorized)', losing the specific error message from the
Databricks Thrift server.

Now, users see detailed error messages like:
- 'Thrift server error: Invalid personal access token (HTTP 401 Unauthorized)'
- 'Thrift server error: Token has expired (HTTP 401 Unauthorized)'
- 'Thrift server error: User does not have permission to access catalog (HTTP 403 Forbidden)'

Changes:
- Add ThriftErrorMessageHandler as a DelegatingHandler
- Integrate handler into DatabricksConnection HTTP handler chain
- Add comprehensive unit tests (11 test cases, all passing)
- Handler is compatible with .NET Framework 4.7.2, .NET Standard 2.0, and .NET 8.0

The handler is positioned as the innermost handler in the chain to capture
response headers before other handlers or the Thrift transport can dispose
of the response.
@eric-wang-1990 eric-wang-1990 force-pushed the feat/csharp-capture-thrift-error-message branch from bcdf670 to cc9f2c5 Compare October 13, 2025 12:56
@CurtHagenlocher CurtHagenlocher changed the title feat(csharp/src/drivers/databricks): capture x-thriftserver-error-message header feat(csharp/src/Drivers/Databricks): capture x-thriftserver-error-message header Oct 13, 2025
@CurtHagenlocher
Copy link
Contributor

CurtHagenlocher commented Oct 13, 2025

If the response has e.g. a 401 status with a x-thriftserver-error-message then this will now throw an exception instead of returning the HttpResponseMessage with a status code. Is the consuming code still able to translate this exception into an ADBC status code 'Unauthenticated`? Or is my premise wrong because there could never be such a response?

@eric-wang-1990
Copy link
Contributor Author

eric-wang-1990 commented Oct 14, 2025

If the response has e.g. a 401 status with a x-thriftserver-error-message then this will now throw an exception instead of returning the HttpResponseMessage with a status code. Is the consuming code still able to translate this exception into an ADBC status code 'Unauthenticated`? Or is my premise wrong because there could never be such a response?

Before the change:

The exception flow is:

  1. _httpClient.PostAsync() returns an HttpResponseMessage with status 401
  2. The HttpResponseMessage object exists and contains the x-thriftserver-error-message header
  3. .EnsureSuccessStatusCode() is called on that response object
  4. .EnsureSuccessStatusCode() throws HttpRequestException with a generic message like "Response status code does not indicate success: 401 (Unauthorized)"
  5. The x-thriftserver-error-message header is lost because EnsureSuccessStatusCode() doesn't include custom headers in the exception message
  6. The catch (HttpRequestException ex2) block catches it and wraps it in TTransportException

After the change (with ThriftErrorMessageHandler):

The exception flow is:

  1. _httpClient.PostAsync() calls through the handler chain
  2. ThriftErrorMessageHandler intercepts the response BEFORE it returns to THttpTransport
  3. ThriftErrorMessageHandler sees status 401 with x-thriftserver-error-message header
  4. ThriftErrorMessageHandler throws HttpRequestException with detailed message: "Thrift server error: Invalid personal access token (HTTP 401 Unauthorized)"
  5. This exception bubbles up through the HttpClient pipeline
  6. From THttpTransport's perspective, PostAsync() itself throws the exception (not EnsureSuccessStatusCode())
  7. The catch (HttpRequestException ex2) block catches it and wraps it in TTransportException

It should still work for 401 mapping to Adbc authenticated exception since this block is checking on the error message.
I'll fix for NET 5.0+ path though since that can check on the status directly.

@CurtHagenlocher CurtHagenlocher merged commit 7a1d18e into apache:main Oct 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CurtHagenlocher CurtHagenlocher CurtHagenlocher approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

ADBC Libraries 21

Development

Successfully merging this pull request may close these issues.

[C#][Databricks] Capture x-thriftserver-error-message header for better error messages

2 participants

@eric-wang-1990 @CurtHagenlocher