Add Additional Tests for CliTokenSource #462
Merged
Conversation
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
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
changed the title
emmyzhou-db
changed the title
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
changed the title
parthban-db
reviewed
Jun 12, 2025
databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
Outdated
Show resolved
Hide resolved
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
parthban-db
reviewed
Jun 13, 2025
databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
Outdated
Show resolved
Hide resolved
parthban-db
approved these changes
Jun 13, 2025
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
renaudhartert-db
approved these changes
Jun 13, 2025
databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
Outdated
Show resolved
Hide resolved
databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
Show resolved
Hide resolved
databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
Outdated
Show resolved
Hide resolved
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
emmyzhou-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 16, 2025
## What changes are proposed in this pull request? This pull request refactors the Databricks Java SDK to replace all usages of LocalDateTime with Instant for representing date-time values. The primary motivation is to ensure that all date-time handling is unambiguous, always in UTC, and consistent across the SDK. ## How does time currently operate in the SDK? Currently, tokens in the SDK use LocalDateTime for expiry timestamps, which by itself is timezone-agnostic and could be ambiguous. Thus, it is not clear how time is handled from the type alone and we need to look at the implementation details of the ClockSupplier used. The SDK currently uses system time through the `SystemClockSupplier` class as defined [here](https://github.com/databricks/databricks-sdk-java/blob/main/databricks-sdk-java/src/main/java/com/databricks/sdk/core/utils/SystemClockSupplier.java). This implementation of the ClockSupplier interface returns `Clock.systemDefaultZone()`, meaning it uses the actual system time from the operating system. The `SystemClockSupplier` is the used throughout the SDK to conduct OAuth token expiration checks as seen [here](https://github.com/databricks/databricks-sdk-java/blob/main/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/Token.java) in the `isExpired()` method of the Token class. While `LocalDateTime` doesn't explicitly store timezone information, the system's timezone is being implicitly used for these comparisons through `Clock.systemDefaultZone()`. In other words, token expiry times are implicitly measured in the system's timezone. ## Converting to UTC To convert these expiry times to UTC while maintaining the SDK's current behavior, we need to handle two distinct scenarios: ### 1. Tokens with `expires_in` - The response includes the time until expiry. - **Conversion:** Add the `expires_in` value to the current UTC time. - **Result:** This approach maintains the original expiration behavior while using UTC internally. --- ### 2. Tokens with `expires_on` (e.g., Azure CLI, Databricks CLI) #### With Time Zone Indicator - Tokens (such as those from the Databricks CLI) provide expiry times in RFC 3339 format, including a time zone indicator. - **Conversion:** These can be easily converted to UTC using standard library methods. #### Without Time Zone Indicator - Example: The `expiresOn` field from Azure CLI [Reference](https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/_profile.py#L384) - Tokens provide expiry timestamps without explicit time zone information. - The timestamp is implicitly in the system’s local time zone. - **Conversion steps:** 1. Interpret the timestamp as local time. 2. Add the system timezone. 3. Convert this to UTC. We do this with **`LocalDateTime.atZone(systemDefault).toInstant()`** ## How is this tested? - Existing unit tests to ensure no regressions. - Added new tests in #462 which verifies that this change does not change overall behavior of CilTokenSource. ## Future Work - The SDK currently parses expiry strings from external sources like Azure CLI - Azure CLI provides two timestamp fields: - `expiresOn`: Local datetime without timezone (maintained for backward compatibility) - `expires_on`: POSIX timestamp (seconds since epoch) - preferred format ### Current Implementation - The Java SDK currently uses the deprecated `expiresOn` field - `LocalDateTime` creates timezone interpretation challenges ### To Do 1. **Phase out `expiresOn`** This legacy field should be deprecated in favor of `expires_on` 2. **Update parsing logic** Future PRs should prioritize using `expires_on` (POSIX timestamp) which: - Provides unambiguous UTC-based expiration time - Avoids timezone conversion complexities NO_CHANGELOG=true --------- Co-authored-by: Parth Bansal <parth.bansal@databricks.com>
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.
What changes are proposed in this pull request?
This PR adds additional tests for the
CliTokenSourceclass, specifically focusing on token expiration handling across different timezones and date formats.Key Changes
This test verifies the behavior of the 'parseExpiry()' method in CliTokenSource by systematically testing tokens with varying expiration times (5, 30, 60, and 120 minutes before and after current system time) across all timezones. For each combination, it creates a mock token with the specified expiry time, temporarily sets the system timezone, and verifies that the token's expired status matches the expected value regardless of the timezone or date format used.
Important Note: This test assumes that all expiry strings provided to
parseExpiry()are in the system's local timezone. It does not cover scenarios where the expiry string is in a different timezone than where the SDK is running, as this would require additional timezone conversion logic that is not part of the current implementation.How is this tested?
N/A
NO_CHANGELOG=true