Interface for testing and error for invalid grant - #3
Closed
jasonlin45 wants to merge 25 commits into
Closed
Conversation
This introduces a flexible TokenProvider interface that allows custom authentication implementations: - TokenProvider interface with static, external function support - Token struct with expiration handling - Authenticator wrapper for integration with existing auth system - Connector functions: WithTokenProvider, WithExternalToken, WithStaticToken This foundation enables custom token management strategies without requiring changes to the core driver. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Reduce token expiry buffer from 5 minutes to 30 seconds (matches SDK standard) - Add detailed documentation to TokenProviderAuthenticator explaining flow - Add ctx.Err() check in ExternalTokenProvider for cancellation support - Rename tokenFunc to tokenSource for better clarity - Remove duplicate empty token validation from ExternalTokenProvider - Update tests to reflect changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implemented per-host feature flag caching system with the following capabilities: - Singleton pattern for global feature flag cache management - Per-host caching with 15-minute TTL to prevent rate limiting - Reference counting tied to connection lifecycle - Thread-safe operations using sync.RWMutex for concurrent access - Graceful error handling with cached value fallback - HTTP integration to fetch feature flags from Databricks API Key Features: - featureFlagCache: Manages per-host feature flag contexts - featureFlagContext: Holds cached state, timestamp, and ref count - getOrCreateContext: Creates context and increments reference count - releaseContext: Decrements ref count and cleans up when zero - isTelemetryEnabled: Returns cached value or fetches fresh - fetchFeatureFlag: HTTP call to Databricks feature flag API Testing: - Comprehensive unit tests with 100% code coverage - Tests for singleton pattern, reference counting, caching behavior - Thread-safety tests with concurrent access - Mock HTTP server tests for API integration - Error handling and fallback scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
MST design doc in Go SQL
Fixes databricks#293 and provides some context where the message is coming from. The OAuth2 M2M authenticator currently logs token fetch operations at the Info level in auth/oauth/m2m/m2m.go at line 60. When running applications with Info-level logging enabled, this generates log entries every time a token is fetched or refreshed, which pollutes application logs with operational noise. Its also messing with my Ginkgo tests in CI: onsi/ginkgo#1614 (comment). Libraries should generally avoid logging at Info level during normal operations unless there's actionable information for the application operator. I think this is a pretty standard practice.
Addressed PR review comments from databricks#304: 1. Fixed race condition when reading flagCtx fields - Added proper locking with flagCtx.mu for enabled, lastFetched, fetching - Previously accessed without correct lock causing data races 2. Fixed concurrent fetch issue - Implemented fetching flag to prevent simultaneous HTTP requests - First goroutine sets fetching=true, others use cached value - Prevents rate limiting from concurrent fetches when cache expires 3. Added HTTP request timeout - Added featureFlagHTTPTimeout = 10s constant - Wraps context with timeout if none exists - Prevents indefinite hangs (Go's default has no timeout) All tests pass. Thread-safe concurrent access verified.
The linter requires explicit error handling. Since we're in an error path and only draining the response body for connection reuse, we explicitly ignore the error with blank identifiers.
Implements token provider support for the go driver
- We can live with owner approval like in our other repos. https://github.com/databricks/databricks-jdbc/tree/main/.github - Note : we already have a require approval from owners in GH ruleset
…atabricks#304) ## Summary Implements per-host feature flag caching system with reference counting as part of the telemetry infrastructure (parent ticket PECOBLR-1143). This is the first component of Phase 2: Per-Host Management. ## What Changed - **New File**: `telemetry/featureflag.go` - Feature flag cache implementation - **New File**: `telemetry/featureflag_test.go` - Comprehensive unit tests - **Updated**: `telemetry/DESIGN.md` - Updated implementation checklist ## Implementation Details ### Core Components 1. **featureFlagCache** - Singleton managing per-host feature flag contexts - Thread-safe using `sync.RWMutex` - Maps host → featureFlagContext 2. **featureFlagContext** - Per-host state holder - Cached feature flag value with 15-minute TTL - Reference counting for connection lifecycle management - Automatic cleanup when ref count reaches zero ### Key Features - ✅ Per-host caching to prevent rate limiting - ✅ 15-minute TTL with automatic cache expiration - ✅ Reference counting tied to connection lifecycle - ✅ Thread-safe for concurrent access - ✅ Graceful error handling with cached value fallback - ✅ HTTP integration with Databricks feature flag API ### Methods Implemented - `getFeatureFlagCache()` - Singleton accessor - `getOrCreateContext(host)` - Creates context and increments ref count - `releaseContext(host)` - Decrements ref count and cleans up - `isTelemetryEnabled(ctx, host, httpClient)` - Returns cached or fetches fresh - `fetchFeatureFlag(ctx, host, httpClient)` - HTTP call to Databricks API ## Test Coverage - ✅ Singleton pattern verification - ✅ Reference counting (increment/decrement/cleanup) - ✅ Cache expiration and refresh logic - ✅ Thread-safety under concurrent access (100 goroutines) - ✅ HTTP fetching with mock server - ✅ Error handling and fallback scenarios - ✅ Context cancellation - ✅ All tests passing with 100% code coverage ## Test Results \`\`\` === RUN TestGetFeatureFlagCache_Singleton --- PASS: TestGetFeatureFlagCache_Singleton (0.00s) ... (all 17 tests passing) PASS ok github.com/databricks/databricks-sql-go/telemetry 0.008s \`\`\` ## Design Alignment Implementation follows the design document (telemetry/DESIGN.md, section 3.1) exactly. The only addition is flexible URL construction in \`fetchFeatureFlag\` to support both production (hostname without protocol) and testing (httptest with protocol) scenarios. ## Testing Instructions \`\`\`bash go test -v ./telemetry -run TestFeatureFlag go test -v ./telemetry # Run all telemetry tests go build ./telemetry # Verify build \`\`\` ## Related Links - Parent Ticket: [PECOBLR-1143](https://databricks.atlassian.net/browse/PECOBLR-1143) - This Ticket: [PECOBLR-1146](https://databricks.atlassian.net/browse/PECOBLR-1146) - Design Doc: \`telemetry/DESIGN.md\` ## Next Steps After this PR: - PECOBLR-1147: Client Manager for Per-Host Clients - PECOBLR-1148: Circuit Breaker Implementation 🤖 Generated with [Claude Code](https://claude.com/claude-code) [PECOBLR-1143]: https://databricks.atlassian.net/browse/PECOBLR-1143?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
…cks#305) ## 🥞 Stacked PR Use this [link](https://github.com/databricks/databricks-sql-go/pull/305/files?w=1) to review incremental changes. - [databricks#304 - Feature Flag Cache (PECOBLR-1146)](databricks#304) [[Files changed](https://github.com/databricks/databricks-sql-go/pull/304/files)] - [**databricks#305 - Client Manager (PECOBLR-1147)**](databricks#305) [[Files changed](https://github.com/databricks/databricks-sql-go/pull/305/files)] ← This PR --------- ## Summary Implements per-host client management system with reference counting as part of the telemetry infrastructure (parent ticket PECOBLR-1143). This is the second component of Phase 2: Per-Host Management. ## What Changed - **New File**: `telemetry/client.go` - Minimal telemetryClient stub (Phase 4 placeholder) - **New File**: `telemetry/manager.go` - Client manager implementation - **New File**: `telemetry/manager_test.go` - Comprehensive unit tests - **Updated**: `telemetry/DESIGN.md` - Updated implementation checklist ## Implementation Details ### Core Components 1. **clientManager** - Singleton managing per-host telemetry clients - Thread-safe using `sync.RWMutex` - Maps host → clientHolder 2. **clientHolder** - Per-host state holder - Holds telemetry client reference - Reference count for active connections - Automatic cleanup when ref count reaches zero 3. **telemetryClient** (stub) - Minimal implementation - Placeholder for Phase 4 (Export) - Provides `start()` and `close()` methods - Will be fully implemented later ### Key Features - ✅ Singleton pattern for global client management - ✅ One client per host to prevent rate limiting - ✅ Reference counting tied to connection lifecycle - ✅ Thread-safe for concurrent access - ✅ Automatic client cleanup when last connection closes - ✅ Client start() called on creation - ✅ Client close() called on removal ### Methods Implemented - `getClientManager()` - Returns singleton instance - `getOrCreateClient(host, httpClient, cfg)` - Creates or reuses client, increments ref count - `releaseClient(host)` - Decrements ref count, removes when zero ## Test Coverage - ✅ Singleton pattern verification - ✅ Reference counting (increment/decrement/cleanup) - ✅ Multiple hosts management - ✅ Partial releases - ✅ Thread-safety under concurrent access (100+ goroutines) - ✅ Client lifecycle (start/close) verification - ✅ Non-existent host handling - ✅ All tests passing with 100% code coverage ## Test Results \`\`\` === RUN TestGetClientManager_Singleton --- PASS: TestGetClientManager_Singleton (0.00s) ... (all 11 tests passing) PASS ok github.com/databricks/databricks-sql-go/telemetry 0.005s \`\`\` ## Design Alignment Implementation follows the design document (telemetry/DESIGN.md, section 3.2) exactly. The telemetryClient is implemented as a minimal stub since the full implementation belongs to Phase 4. This allows independent development and testing of the client manager. ## Testing Instructions \`\`\`bash go test -v ./telemetry -run "TestGetClientManager|TestClientManager" go test -v ./telemetry # Run all telemetry tests go build ./telemetry # Verify build \`\`\` ## Related Links - Parent Ticket: [PECOBLR-1143](https://databricks.atlassian.net/browse/PECOBLR-1143) - This Ticket: [PECOBLR-1147](https://databricks.atlassian.net/browse/PECOBLR-1147) - Previous: [PECOBLR-1146](https://databricks.atlassian.net/browse/PECOBLR-1146) - Feature Flag Cache (databricks#304) - Design Doc: \`telemetry/DESIGN.md\` ## Next Steps After this PR: - PECOBLR-1148: Circuit Breaker Implementation [PECOBLR-1143]: https://databricks.atlassian.net/browse/PECOBLR-1143?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
…cks#308) [issues](databricks#307) Hello, as per issue looking for ways to modify the transport layer of the httpclient that cloudfetch uses. Happy to go another way to solving this, this just seamed like the simplest. Thanks for your work on the driver, its been very useful 👍 --------- Authored-by: Tim Mulqueen <tim.mulqueen@gmail.com> Co-authored-by: Samikshya Chand <148681192+samikshya-db@users.noreply.github.com>
…o include more details on circuit breaker and config (databricks#311) ### Changes : The changes in this PR : 1. Add config params for telemetry similar to oss jdbc 2. Add circuit breaker logic code (Note : this will be integrated at a later point in time) 3. Keep the lowest level of logging for telemetry (to keep telemetry as silent as possible) The original design doc was changed to : 1. Keep config names similar to the ones used in JDBC. 2. Add more details to circuit breaker logic. (This will be useful from a spec driven development perspective )
Adds token federation for databricks sql go driver
…tabricks#316) ## Summary Fixes type inference bugs for numeric parameters: - **int64/uint64**: Were incorrectly mapped to `SqlInteger` instead of `SqlBigInt` (fixes databricks#250) - **float64**: Was incorrectly mapped to `SqlFloat` instead of `SqlDouble` (fixes databricks#314) ## Problems Fixed ### 1. int64/uint64 → BIGINT When inserting int64/uint64 values into BIGINT columns, the driver was sending them with type `INTEGER` instead of `BIGINT`, causing the server to reject large values with error: ``` [INVALID_PARAMETER_MARKER_VALUE.INVALID_VALUE_FOR_DATA_TYPE] An invalid parameter mapping was provided: the value '1311768467463790320' for parameter 'null' cannot be cast to INT because it is malformed. ``` Additionally, int64 was using `strconv.Itoa(int(value))` which truncates values larger than int32. ### 2. float64 → DOUBLE When inserting float64 values into DOUBLE columns, the driver was sending them with type `FLOAT` (32-bit) instead of `DOUBLE` (64-bit), causing: - Precision loss for high-precision float64 values - Potential overflow for values beyond float32 range (~3.4e38) ### 3. Panic with explicit Parameter type When using `Parameter{Type: SqlBigInt, Value: int64(...)}` with a non-string value, the driver panicked at `convertNamedValuesToSparkParams` due to unsafe type assertion. ## Changes - `parameters.go`: - int64 now uses `strconv.FormatInt()` and maps to `SqlBigInt` - uint64 now maps to `SqlBigInt` - float64 now maps to `SqlDouble` instead of `SqlFloat` - Added safe type assertion with fallback in `convertNamedValuesToSparkParams` ## Test plan - [x] Added unit tests for int64/uint64 type inference (`TestParameter_BigInt`) - [x] Added unit tests for float64/float32 type inference (`TestParameter_Float`) - [x] Verified large int64 values are correctly inserted and retrieved from BIGINT columns - [x] Verified float64 values with high precision are correctly inserted and retrieved - [x] All existing parameter tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
VersusFacit
approved these changes
Mar 3, 2026
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
9 participants
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.
No description provided.