Refactor CliTokenSource to use an ordered attempt chain#752
Refactor CliTokenSource to use an ordered attempt chain#752mihaimitrea-db wants to merge 2 commits intomainfrom
Conversation
628f509 to
09cdbc4
Compare
Range-diff: stack/cli-force-refresh (628f509 -> 09cdbc4)
Reproduce locally: |
09cdbc4 to
f88c52a
Compare
Range-diff: stack/cli-force-refresh (09cdbc4 -> f88c52a)
Reproduce locally: |
f88c52a to
25a3779
Compare
Range-diff: stack/cli-force-refresh (f88c52a -> 25a3779)
Reproduce locally: |
25a3779 to
694521d
Compare
Range-diff: stack/cli-force-refresh (25a3779 -> 694521d)
Reproduce locally: |
694521d to
0f6baf6
Compare
Range-diff: stack/cli-force-refresh (694521d -> 0f6baf6)
Reproduce locally: |
Try `--force-refresh` before the regular CLI command so the SDK can bypass the CLI's own token cache when the SDK considers its token stale. If the CLI is too old to recognise `--force-refresh` (or `--profile`), gracefully fall back to the next command in the chain. Chain order: - with profile: forceCmd (--profile --force-refresh) -> profileCmd (--profile) -> fallbackCmd (--host) - without profile: forceCmd (--host --force-refresh) -> profileCmd (--host) Azure CLI callers are unchanged; they use constructors that leave forceCmd null, preserving existing behavior. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
0f6baf6 to
0ef99dc
Compare
0ef99dc to
0dd48d1
Compare
Range-diff: stack/cli-force-refresh (0ef99dc -> 0dd48d1)
Reproduce locally: |
|
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. |
Stacked PR
Use this link to review incremental changes.
Summary
Generalize
CliTokenSourcefrom three explicit command fields (cmd,fallbackCmd,secondFallbackCmd) into aList<CliCommand>command chain with anactiveCommandIndexthat caches which command works, so that adding future CLI flags is straightforward and subsequent token fetches skip probing.Why
The parent PR (#751) introduced
--force-refreshsupport by adding a third command field and hand-writing each fallback block ingetToken(). This works, but every new flag would require adding another field, anotherifblock, another error check, and another test — the pattern doesn't scale.We expect future flags like
--scopes(forwarding custom OAuth scopes to the CLI). Rather than growing the class linearly with each flag, this PR extracts the repeating pattern into a loop over a command list.Why try-and-retry over version detection or
--helpparsingThree approaches were evaluated for resolving which flags the installed CLI supports:
databricks version+ static version table) was rejected because it creates a maintenance burden and a second source of truth. Every SDK (Go, Python, Java) would need to independently maintain a table mapping flags to the CLI version that introduced them. If any SDK's table falls out of sync with the CLI's actual releases, users silently get degraded commands.--helpflag parsing (databricks auth token --help+contains) was rejected because it depends on the output format of--help— which is not a stable API. Cobra format changes could break detection, and naive substring matching is fragile.getToken()call, each command is tried in order; when the CLI responds with"unknown flag:", the next simpler command is tried. The working command index is cached so subsequent calls skip probing entirely. This approach has zero maintenance burden, zero overhead on the happy path (newest CLI succeeds on the first command), and requires no signature changes.What changed
Interface changes
None.
CliTokenSourceis not part of the public API surface.Behavioral changes
activeCommandIndex. SubsequentgetToken()calls execute that command directly without re-probing the fallback chain — a pure performance improvement.isUnknownFlagErrornow matches against specificusedFlagsper command rather than a blanket"unknown flag:"check, preventing false-positive fallbacks from unexpected unknown-flag errors.Internal changes
CliCommandinner class: replaces the three separateList<String>fields. Each entry holdscmd(the full CLI command),usedFlags(flags in this command, used for error matching), andfallbackMessage(logged when falling back from this command).CliTokenSource: now holdsList<CliCommand> commandsandAtomicInteger activeCommandIndex(initialized to -1 = unresolved) instead ofcmd,fallbackCmd,secondFallbackCmd. The 6-arg and 7-arg public constructors are removed; only the 5-arg constructor (Azure CLI) and thefromCommandsfactory remain.activeCommandIndex: usesAtomicIntegerrather thansynchronizedor aCompletableFuture-based once-pattern because probing must be retryable on transient errors (network failures should not permanently lock in a failure). Concurrent callers may redundantly probe but all converge to the same index.getToken(): checksactiveCommandIndexfirst — if resolved (>= 0), calls the cached command directly. Otherwise delegates toprobeAndExec().probeAndExec(): walkscommandsfrom index 0, falls back on unknown flag errors, storesactiveCommandIndexon success.DatabricksCliCredentialsProvider.buildCommands: constructs command variants inline — the most-featured command (--profile+--force-refresh) first, then the plain--profilecommand, then--hostif available. Profile and host commands are built independently. Adding a future flag means adding one moreCliCommandliteral here.commandsnorcmdis provided, failing fast instead of at token-fetch time. Ifcommandsis an empty list butcmdis set, a warning is logged andcmdis used as the sole command.How is this tested?
Unit tests in
DatabricksCliCredentialsProviderTest:testBuildCommands_WithProfileAndHost— verifies 3 commands with correctcmd,usedFlags, andfallbackMessage.testBuildCommands_WithProfileOnly— verifies 2 commands (no host fallback).testBuildCommands_WithHostOnly— verifies single--hostcommand, no fallback chain.testBuildCommands_WithAccountHost— verifies--host+--account-idfor account-level hosts.Unit tests in
CliTokenSourceTest:testActiveCommandIndexPersists— first call falls back (2 ProcessBuilders), second call uses cached index (1 ProcessBuilder), verifying probing is skipped.getToken()/probeAndExec()split.