Skip to content

Fix CLI token source --profile fallback with version detection#751

Open
mihaimitrea-db wants to merge 1 commit intomainfrom
mihaimitrea-db/stack/cli-force-refresh
Open

Fix CLI token source --profile fallback with version detection#751
mihaimitrea-db wants to merge 1 commit intomainfrom
mihaimitrea-db/stack/cli-force-refresh

Conversation

@mihaimitrea-db
Copy link
Copy Markdown
Contributor

@mihaimitrea-db mihaimitrea-db commented Mar 31, 2026

🥞 Stacked PR

Use this link to review incremental changes.


Summary

Replace the broken error-based --profile fallback in CliTokenSource with version-based CLI detection at init time. Mirrors databricks/databricks-sdk-go#1605 and databricks/databricks-sdk-py#1377.

Why

--profile on databricks auth token is a global flag, so old CLIs (< v0.207.1) silently accept it and then fail with "cannot fetch credentials" instead of "unknown flag: --profile". The existing retry check was matching on the latter and never fired — the --host fallback it gated was effectively dead code. Switching to databricks version + a minimum-version constant makes the fallback reliable and sets up future capability-gated flags (e.g. --force-refresh in #752) without additional subprocess calls.

What changed

Interface changes

None. CliTokenSource is not part of the public API surface.

Behavioral changes

  • cfg.profile + CLI < v0.207.1 now correctly falls back to --host (previously broken).
  • databricks version failures log a WARNING and fall back to the most conservative command. Successful detections are cached per CLI path; failures are not cached and will be retried on the next call.
  • A default dev build (v0.0.0-dev) logs an INFO explaining why feature gates are conservative.

AzureCliCredentialsProvider is untouched.

Internal changes

  • New DatabricksCliVersion class with a (major, minor, patch) triple, an UNKNOWN sentinel, an atLeast() comparator, and an isDefaultDevBuild() helper.
  • CliTokenSource simplified to a single cmd; the fallbackCmd parameter and its retry logic are removed.
  • DatabricksCliCredentialsProvider gains getCliVersion, probeCliVersion, parseCliVersion, resolveCliCommand, and buildCliCommand helpers.

How is this tested?

Unit tests in DatabricksCliVersionTest cover version comparison (across patch/minor/major), the UNKNOWN sentinel, dev-build detection, and toString formatting.

Unit tests in DatabricksCliCredentialsProviderTest cover JSON parsing of databricks version --output json (standard, dev build, missing fields, malformed JSON, empty string) and command assembly for every profile/host/version combination (host-only, account host, profile + new CLI, profile + old CLI, unknown version, dev build).

CliTokenSourceTest retains its parsing and timezone tests; the obsolete fallback tests are dropped.

@mihaimitrea-db
Copy link
Copy Markdown
Contributor Author

Range-diff: main (048a903 -> 5e8f476)
NEXT_CHANGELOG.md
@@ -0,0 +1,10 @@
+diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md
+--- a/NEXT_CHANGELOG.md
++++ b/NEXT_CHANGELOG.md
+ 
+ ### New Features and Improvements
+ * Added automatic detection of AI coding agents (Antigravity, Claude Code, Cline, Codex, Copilot CLI, Cursor, Gemini CLI, OpenCode) in the user-agent string. The SDK now appends `agent/<name>` to HTTP request headers when running inside a known AI agent environment.
++* Pass `--force-refresh` to Databricks CLI `auth token` command so the SDK always receives a fresh token instead of a potentially stale one from the CLI's internal cache. Falls back gracefully on older CLIs that do not support this flag.
+ 
+ ### Bug Fixes
+ * Fixed Databricks CLI authentication to detect when the cached token's scopes don't match the SDK's configured scopes. Previously, a scope mismatch was silently ignored, causing requests to use wrong permissions. The SDK now raises an error with instructions to re-authenticate.
\ No newline at end of file
databricks-sdk-java/src/main/java/com/databricks/sdk/core/CliTokenSource.java
@@ -57,8 +57,7 @@
      this.env = env;
      this.fallbackCmd =
          fallbackCmd != null ? OSUtils.get(env).getCliExecutableCommand(fallbackCmd) : null;
-+    this.forceCmd =
-+        forceCmd != null ? OSUtils.get(env).getCliExecutableCommand(forceCmd) : null;
++    this.forceCmd = forceCmd != null ? OSUtils.get(env).getCliExecutableCommand(forceCmd) : null;
    }
  
    /**
databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
@@ -50,8 +50,7 @@
 +            (pb, context) -> {
 +              Process successProcess = mock(Process.class);
 +              when(successProcess.getInputStream())
-+                  .thenReturn(
-+                      new ByteArrayInputStream(validTokenJson("forced-token").getBytes()));
++                  .thenReturn(new ByteArrayInputStream(validTokenJson("forced-token").getBytes()));
 +              when(successProcess.getErrorStream())
 +                  .thenReturn(new ByteArrayInputStream(new byte[0]));
 +              when(successProcess.waitFor()).thenReturn(0);
@@ -136,15 +135,13 @@
 +                    .thenReturn(new ByteArrayInputStream(new byte[0]));
 +                when(failProcess.getErrorStream())
 +                    .thenReturn(
-+                        new ByteArrayInputStream(
-+                            "Error: unknown flag: --profile".getBytes()));
++                        new ByteArrayInputStream("Error: unknown flag: --profile".getBytes()));
 +                when(failProcess.waitFor()).thenReturn(1);
 +                when(pb.start()).thenReturn(failProcess);
 +              } else {
 +                Process successProcess = mock(Process.class);
 +                when(successProcess.getInputStream())
-+                    .thenReturn(
-+                        new ByteArrayInputStream(validTokenJson("host-token").getBytes()));
++                    .thenReturn(new ByteArrayInputStream(validTokenJson("host-token").getBytes()));
 +                when(successProcess.getErrorStream())
 +                    .thenReturn(new ByteArrayInputStream(new byte[0]));
 +                when(successProcess.waitFor()).thenReturn(0);
@@ -164,8 +161,7 @@
 +
 +    List<String> forceCmd =
 +        Arrays.asList("databricks", "auth", "token", "--host", "https://h", "--force-refresh");
-+    List<String> profileCmd =
-+        Arrays.asList("databricks", "auth", "token", "--host", "https://h");
++    List<String> profileCmd = Arrays.asList("databricks", "auth", "token", "--host", "https://h");
 +
 +    CliTokenSource tokenSource = makeTokenSource(env, profileCmd, null, forceCmd);
 +

Reproduce locally: git range-diff f28430b..048a903 f28430b..5e8f476 | Disable: git config gitstack.push-range-diff false

@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 5e8f476 to 6b8a57f Compare March 31, 2026 14:04
@mihaimitrea-db
Copy link
Copy Markdown
Contributor Author

Range-diff: main (5e8f476 -> 6b8a57f)
databricks-sdk-java/src/main/java/com/databricks/sdk/core/CliTokenSource.java
@@ -5,9 +5,6 @@
    private static final Logger LOG = LoggerFactory.getLogger(CliTokenSource.class);
  
 -  private List<String> cmd;
-+  private static final String UNKNOWN_PROFILE_FLAG = "unknown flag: --profile";
-+  private static final String UNKNOWN_FORCE_REFRESH_FLAG = "unknown flag: --force-refresh";
-+
 +  // forceCmd is tried before profileCmd when non-null. If the CLI rejects
 +  // --force-refresh or --profile, execution falls through to profileCmd.
 +  private List<String> forceCmd;
@@ -73,7 +70,7 @@
 +  }
 +
 +  private boolean isUnknownFlagError(String errorText, String flag) {
-+    return errorText != null && errorText.contains(flag);
++    return errorText != null && errorText.contains("unknown flag: " + flag);
 +  }
 +
 +  private Token execProfileCmdWithFallback() {
@@ -89,7 +86,7 @@
 -          && textToCheck != null
 -          && textToCheck.contains("unknown flag: --profile")) {
 +      String textToCheck = getErrorText(e);
-+      if (fallbackCmd != null && isUnknownFlagError(textToCheck, UNKNOWN_PROFILE_FLAG)) {
++      if (fallbackCmd != null && isUnknownFlagError(textToCheck, "--profile")) {
          LOG.warn(
              "Databricks CLI does not support --profile flag. Falling back to --host. "
                  + "Please upgrade your CLI to the latest version.");
@@ -107,8 +104,8 @@
 +      return execCliCommand(this.forceCmd);
 +    } catch (IOException e) {
 +      String textToCheck = getErrorText(e);
-+      if (isUnknownFlagError(textToCheck, UNKNOWN_FORCE_REFRESH_FLAG)
-+          || isUnknownFlagError(textToCheck, UNKNOWN_PROFILE_FLAG)) {
++      if (isUnknownFlagError(textToCheck, "--force-refresh")
++          || isUnknownFlagError(textToCheck, "--profile")) {
 +        LOG.warn(
 +            "Databricks CLI does not support --force-refresh flag. "
 +                + "Falling back to regular token fetch. "

Reproduce locally: git range-diff f28430b..5e8f476 f28430b..6b8a57f | Disable: git config gitstack.push-range-diff false

@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 6b8a57f to 61686da Compare March 31, 2026 16:03
@mihaimitrea-db
Copy link
Copy Markdown
Contributor Author

Range-diff: main (6b8a57f -> 61686da)
databricks-sdk-java/src/main/java/com/databricks/sdk/core/CliTokenSource.java
@@ -1,15 +1,11 @@
 diff --git a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/CliTokenSource.java b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/CliTokenSource.java
 --- a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/CliTokenSource.java
 +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/CliTokenSource.java
- public class CliTokenSource implements TokenSource {
    private static final Logger LOG = LoggerFactory.getLogger(CliTokenSource.class);
  
--  private List<String> cmd;
-+  // forceCmd is tried before profileCmd when non-null. If the CLI rejects
-+  // --force-refresh or --profile, execution falls through to profileCmd.
-+  private List<String> forceCmd;
-+
-+  private List<String> profileCmd;
+   private List<String> cmd;
++  private List<String> fallbackCmd;
++  private List<String> secondFallbackCmd;
    private String tokenTypeField;
    private String accessTokenField;
    private String expiryField;
@@ -18,11 +14,10 @@
 -  // indicating the CLI is too old to support --profile. Can be removed once support
 -  // for CLI versions predating --profile is dropped.
 -  // See: https://github.com/databricks/databricks-sdk-go/pull/1497
-+  // fallbackCmd is tried when profileCmd fails with "unknown flag: --profile",
-+  // indicating the CLI is too old to support --profile.
-   private List<String> fallbackCmd;
+-  private List<String> fallbackCmd;
  
    /**
+    * Internal exception that carries the clean stderr message but exposes full output for checks.
        String accessTokenField,
        String expiryField,
        Environment env) {
@@ -31,52 +26,43 @@
    }
  
    public CliTokenSource(
+       String accessTokenField,
        String expiryField,
        Environment env,
-       List<String> fallbackCmd) {
-+    this(cmd, tokenTypeField, accessTokenField, expiryField, env, fallbackCmd, null);
-+  }
-+
-+  public CliTokenSource(
-+      List<String> cmd,
-+      String tokenTypeField,
-+      String accessTokenField,
-+      String expiryField,
-+      Environment env,
+-      List<String> fallbackCmd) {
+-    super();
 +      List<String> fallbackCmd,
-+      List<String> forceCmd) {
-     super();
--    this.cmd = OSUtils.get(env).getCliExecutableCommand(cmd);
-+    this.profileCmd = OSUtils.get(env).getCliExecutableCommand(cmd);
++      List<String> secondFallbackCmd) {
+     this.cmd = OSUtils.get(env).getCliExecutableCommand(cmd);
      this.tokenTypeField = tokenTypeField;
      this.accessTokenField = accessTokenField;
-     this.expiryField = expiryField;
      this.env = env;
      this.fallbackCmd =
          fallbackCmd != null ? OSUtils.get(env).getCliExecutableCommand(fallbackCmd) : null;
-+    this.forceCmd = forceCmd != null ? OSUtils.get(env).getCliExecutableCommand(forceCmd) : null;
++    this.secondFallbackCmd =
++        secondFallbackCmd != null
++            ? OSUtils.get(env).getCliExecutableCommand(secondFallbackCmd)
++            : null;
    }
  
    /**
      }
    }
  
--  @Override
--  public Token getToken() {
 +  private String getErrorText(IOException e) {
 +    return e instanceof CliCommandException
 +        ? ((CliCommandException) e).getFullOutput()
 +        : e.getMessage();
 +  }
 +
-+  private boolean isUnknownFlagError(String errorText, String flag) {
-+    return errorText != null && errorText.contains("unknown flag: " + flag);
++  private boolean isUnknownFlagError(String errorText) {
++    return errorText != null && errorText.contains("unknown flag:");
 +  }
 +
-+  private Token execProfileCmdWithFallback() {
+   @Override
+   public Token getToken() {
      try {
--      return execCliCommand(this.cmd);
-+      return execCliCommand(this.profileCmd);
+       return execCliCommand(this.cmd);
      } catch (IOException e) {
 -      String textToCheck =
 -          e instanceof CliCommandException
@@ -85,34 +71,38 @@
 -      if (fallbackCmd != null
 -          && textToCheck != null
 -          && textToCheck.contains("unknown flag: --profile")) {
-+      String textToCheck = getErrorText(e);
-+      if (fallbackCmd != null && isUnknownFlagError(textToCheck, "--profile")) {
++      if (fallbackCmd != null && isUnknownFlagError(getErrorText(e))) {
          LOG.warn(
-             "Databricks CLI does not support --profile flag. Falling back to --host. "
+-            "Databricks CLI does not support --profile flag. Falling back to --host. "
++            "CLI does not support some flags used by this SDK. "
++                + "Falling back to a compatible command. "
                  + "Please upgrade your CLI to the latest version.");
-       throw new DatabricksException(e.getMessage(), e);
-     }
-   }
-+
-+  @Override
-+  public Token getToken() {
-+    if (forceCmd == null) {
-+      return execProfileCmdWithFallback();
+-        try {
+-          return execCliCommand(this.fallbackCmd);
+-        } catch (IOException fallbackException) {
+-          throw new DatabricksException(fallbackException.getMessage(), fallbackException);
+-        }
++      } else {
++        throw new DatabricksException(e.getMessage(), e);
++      }
 +    }
 +
 +    try {
-+      return execCliCommand(this.forceCmd);
++      return execCliCommand(this.fallbackCmd);
 +    } catch (IOException e) {
-+      String textToCheck = getErrorText(e);
-+      if (isUnknownFlagError(textToCheck, "--force-refresh")
-+          || isUnknownFlagError(textToCheck, "--profile")) {
++      if (secondFallbackCmd != null && isUnknownFlagError(getErrorText(e))) {
 +        LOG.warn(
-+            "Databricks CLI does not support --force-refresh flag. "
-+                + "Falling back to regular token fetch. "
++            "CLI does not support some flags used by this SDK. "
++                + "Falling back to a compatible command. "
 +                + "Please upgrade your CLI to the latest version.");
-+        return execProfileCmdWithFallback();
-+      }
-+      throw new DatabricksException(e.getMessage(), e);
++      } else {
++        throw new DatabricksException(e.getMessage(), e);
+       }
 +    }
-+  }
- }
\ No newline at end of file
++
++    try {
++      return execCliCommand(this.secondFallbackCmd);
++    } catch (IOException e) {
+       throw new DatabricksException(e.getMessage(), e);
+     }
+   }
\ No newline at end of file
databricks-sdk-java/src/main/java/com/databricks/sdk/core/DatabricksCliCredentialsProvider.java
@@ -18,13 +18,10 @@
    private CliTokenSource getDatabricksCliTokenSource(DatabricksConfig config) {
      String cliPath = config.getDatabricksCliPath();
      if (cliPath == null) {
-       return null;
-     }
  
--    List<String> cmd;
-+    List<String> profileCmd;
+     List<String> cmd;
      List<String> fallbackCmd = null;
-+    List<String> forceCmd;
++    List<String> secondFallbackCmd = null;
  
      if (config.getProfile() != null) {
 -      // When profile is set, use --profile as the primary command.
@@ -33,20 +30,26 @@
 -          new ArrayList<>(
 -              Arrays.asList(cliPath, "auth", "token", "--profile", config.getProfile()));
 -      // Build a --host fallback for older CLIs that don't support --profile.
-+      profileCmd = buildProfileArgs(cliPath, config);
-+      forceCmd = withForceRefresh(profileCmd);
++      List<String> profileArgs = buildProfileArgs(cliPath, config);
++      cmd = withForceRefresh(profileArgs);
++      fallbackCmd = profileArgs;
        if (config.getHost() != null) {
-         fallbackCmd = buildHostArgs(cliPath, config);
+-        fallbackCmd = buildHostArgs(cliPath, config);
++        secondFallbackCmd = buildHostArgs(cliPath, config);
        }
      } else {
--      cmd = buildHostArgs(cliPath, config);
-+      profileCmd = buildHostArgs(cliPath, config);
-+      forceCmd = withForceRefresh(profileCmd);
+       cmd = buildHostArgs(cliPath, config);
      }
  
      return new CliTokenSource(
 -        cmd, "token_type", "access_token", "expiry", config.getEnv(), fallbackCmd);
-+        profileCmd, "token_type", "access_token", "expiry", config.getEnv(), fallbackCmd, forceCmd);
++        cmd,
++        "token_type",
++        "access_token",
++        "expiry",
++        config.getEnv(),
++        fallbackCmd,
++        secondFallbackCmd);
    }
  
    @Override
\ No newline at end of file
databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
@@ -8,19 +8,20 @@
 +  // ---- Fallback tests for --profile and --force-refresh flag handling ----
  
    private CliTokenSource makeTokenSource(
-       Environment env, List<String> primaryCmd, List<String> fallbackCmd) {
-+    return makeTokenSource(env, primaryCmd, fallbackCmd, null);
+-      Environment env, List<String> primaryCmd, List<String> fallbackCmd) {
++      Environment env, List<String> cmd, List<String> fallbackCmd) {
++    return makeTokenSource(env, cmd, fallbackCmd, null);
 +  }
 +
 +  private CliTokenSource makeTokenSource(
-+      Environment env, List<String> primaryCmd, List<String> fallbackCmd, List<String> forceCmd) {
++      Environment env, List<String> cmd, List<String> fallbackCmd, List<String> secondFallbackCmd) {
      OSUtilities osUtils = mock(OSUtilities.class);
      when(osUtils.getCliExecutableCommand(any())).thenAnswer(inv -> inv.getArgument(0));
      try (MockedStatic<OSUtils> mockedOSUtils = mockStatic(OSUtils.class)) {
        mockedOSUtils.when(() -> OSUtils.get(any())).thenReturn(osUtils);
        return new CliTokenSource(
 -          primaryCmd, "token_type", "access_token", "expiry", env, fallbackCmd);
-+          primaryCmd, "token_type", "access_token", "expiry", env, fallbackCmd, forceCmd);
++          cmd, "token_type", "access_token", "expiry", env, fallbackCmd, secondFallbackCmd);
      }
    }
  
@@ -31,18 +32,18 @@
 +  // ---- Force-refresh tests ----
 +
 +  @Test
-+  public void testForceCmdSucceedsAndProfileCmdNotRun() {
++  public void testForceCmdSucceedsAndFallbacksNotRun() {
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> forceCmd =
++    List<String> cmd =
 +        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
-+    List<String> profileCmd =
++    List<String> fallbackCmdList =
 +        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
-+    List<String> fallbackCmd =
++    List<String> secondFallbackCmdList =
 +        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
 +
-+    CliTokenSource tokenSource = makeTokenSource(env, profileCmd, fallbackCmd, forceCmd);
++    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList, secondFallbackCmdList);
 +
 +    try (MockedConstruction<ProcessBuilder> mocked =
 +        mockConstruction(
@@ -63,16 +64,16 @@
 +  }
 +
 +  @Test
-+  public void testForceCmdFailsWithUnknownForceRefreshFallsBackToProfileCmd() {
++  public void testCmdFailsWithUnknownFlagFallsBackToFallbackCmd() {
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> forceCmd =
++    List<String> cmd =
 +        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
-+    List<String> profileCmd =
++    List<String> fallbackCmdList =
 +        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
 +
-+    CliTokenSource tokenSource = makeTokenSource(env, profileCmd, null, forceCmd);
++    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList);
 +
 +    AtomicInteger callCount = new AtomicInteger(0);
 +    try (MockedConstruction<ProcessBuilder> mocked =
@@ -107,20 +108,20 @@
 +  }
 +
 +  @Test
-+  public void testForceCmdFailsWithUnknownProfileFallsThroughToFallbackCmd() {
-+    // Very old CLI: forceCmd has --profile --force-refresh but CLI doesn't know --profile.
-+    // Should fall through to profileCmd (which also fails), then to fallbackCmd (--host).
++  public void testCmdAndFallbackBothFailFallsThroughToSecondFallback() {
++    // Very old CLI: cmd has --profile --force-refresh but CLI doesn't know --profile.
++    // Should fall through to fallbackCmd (which also fails), then to secondFallbackCmd (--host).
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> forceCmd =
++    List<String> cmd =
 +        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
-+    List<String> profileCmd =
++    List<String> fallbackCmdList =
 +        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
-+    List<String> fallbackCmd =
++    List<String> secondFallbackCmdList =
 +        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
 +
-+    CliTokenSource tokenSource = makeTokenSource(env, profileCmd, fallbackCmd, forceCmd);
++    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList, secondFallbackCmdList);
 +
 +    AtomicInteger callCount = new AtomicInteger(0);
 +    try (MockedConstruction<ProcessBuilder> mocked =
@@ -155,15 +156,16 @@
 +  }
 +
 +  @Test
-+  public void testForceCmdRealAuthErrorDoesNotFallBack() {
++  public void testRealAuthErrorDoesNotFallBack() {
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> forceCmd =
-+        Arrays.asList("databricks", "auth", "token", "--host", "https://h", "--force-refresh");
-+    List<String> profileCmd = Arrays.asList("databricks", "auth", "token", "--host", "https://h");
++    List<String> cmd =
++        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
++    List<String> fallbackCmdList =
++        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
 +
-+    CliTokenSource tokenSource = makeTokenSource(env, profileCmd, null, forceCmd);
++    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList);
 +
 +    try (MockedConstruction<ProcessBuilder> mocked =
 +        mockConstruction(
@@ -185,17 +187,16 @@
 +  }
 +
 +  @Test
-+  public void testNullForceCmdPreservesExistingBehavior() {
-+    // When forceCmd is null, behaves exactly like before: profileCmd -> fallbackCmd.
++  public void testTwoLevelFallbackWithNoSecondFallback() {
++    // cmd -> fallbackCmd chain with no secondFallbackCmd.
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> profileCmd =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
-+    List<String> fallbackCmd =
++    List<String> cmd = Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
++    List<String> fallbackCmdList =
 +        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
 +
-+    CliTokenSource tokenSource = makeTokenSource(env, profileCmd, fallbackCmd, null);
++    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList);
 +
 +    AtomicInteger callCount = new AtomicInteger(0);
 +    try (MockedConstruction<ProcessBuilder> mocked =

Reproduce locally: git range-diff f28430b..6b8a57f f28430b..61686da | Disable: git config gitstack.push-range-diff false

@mihaimitrea-db mihaimitrea-db self-assigned this Apr 1, 2026
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 61686da to 1c68e85 Compare April 1, 2026 07:59
@mihaimitrea-db
Copy link
Copy Markdown
Contributor Author

Range-diff: main (61686da -> 1c68e85)
databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
@@ -1,6 +1,19 @@
 diff --git a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
 --- a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
 +++ b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/CliTokenSourceTest.java
+ import org.mockito.MockedStatic;
+ 
+ public class CliTokenSourceTest {
++  private static final List<String> FORCE_CMD =
++      Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
++  private static final List<String> PROFILE_CMD =
++      Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
++  private static final List<String> HOST_CMD =
++      Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
++
+   String getExpiryStr(String dateFormat, Duration offset) {
+     ZonedDateTime futureExpiry = ZonedDateTime.now().plus(offset);
+     return futureExpiry.format(DateTimeFormatter.ofPattern(dateFormat));
      }
    }
  
@@ -25,6 +38,68 @@
      }
    }
  
+     Environment env = mock(Environment.class);
+     when(env.getEnv()).thenReturn(new HashMap<>());
+ 
+-    List<String> primaryCmd =
+-        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
+-    List<String> fallbackCmdList =
+-        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
+-
+-    CliTokenSource tokenSource = makeTokenSource(env, primaryCmd, fallbackCmdList);
++    CliTokenSource tokenSource = makeTokenSource(env, PROFILE_CMD, HOST_CMD);
+ 
+     AtomicInteger callCount = new AtomicInteger(0);
+     try (MockedConstruction<ProcessBuilder> mocked =
+ 
+   @Test
+   public void testFallbackTriggeredWhenUnknownFlagInStdout() {
+-    // Fallback triggers even when "unknown flag" appears in stdout rather than stderr.
+     Environment env = mock(Environment.class);
+     when(env.getEnv()).thenReturn(new HashMap<>());
+ 
+-    List<String> primaryCmd =
+-        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
+-    List<String> fallbackCmdList =
+-        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
+-
+-    CliTokenSource tokenSource = makeTokenSource(env, primaryCmd, fallbackCmdList);
++    CliTokenSource tokenSource = makeTokenSource(env, PROFILE_CMD, HOST_CMD);
+ 
+     AtomicInteger callCount = new AtomicInteger(0);
+     try (MockedConstruction<ProcessBuilder> mocked =
+ 
+   @Test
+   public void testNoFallbackOnRealAuthError() {
+-    // When the primary fails with a real error (not unknown flag), no fallback is attempted.
+     Environment env = mock(Environment.class);
+     when(env.getEnv()).thenReturn(new HashMap<>());
+ 
+-    List<String> primaryCmd =
+-        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
+-    List<String> fallbackCmdList =
+-        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
+-
+-    CliTokenSource tokenSource = makeTokenSource(env, primaryCmd, fallbackCmdList);
++    CliTokenSource tokenSource = makeTokenSource(env, PROFILE_CMD, HOST_CMD);
+ 
+     try (MockedConstruction<ProcessBuilder> mocked =
+         mockConstruction(
+ 
+   @Test
+   public void testNoFallbackWhenFallbackCmdNotSet() {
+-    // When fallbackCmd is null and the primary fails with unknown flag, original error propagates.
+     Environment env = mock(Environment.class);
+     when(env.getEnv()).thenReturn(new HashMap<>());
+ 
+-    List<String> primaryCmd =
+-        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
+-
+-    CliTokenSource tokenSource = makeTokenSource(env, primaryCmd, null);
++    CliTokenSource tokenSource = makeTokenSource(env, PROFILE_CMD, null);
+ 
+     try (MockedConstruction<ProcessBuilder> mocked =
+         mockConstruction(
        assertEquals(1, mocked.constructed().size());
      }
    }
@@ -36,14 +111,7 @@
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> cmd =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
-+    List<String> fallbackCmdList =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
-+    List<String> secondFallbackCmdList =
-+        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
-+
-+    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList, secondFallbackCmdList);
++    CliTokenSource tokenSource = makeTokenSource(env, FORCE_CMD, PROFILE_CMD, HOST_CMD);
 +
 +    try (MockedConstruction<ProcessBuilder> mocked =
 +        mockConstruction(
@@ -68,12 +136,7 @@
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> cmd =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
-+    List<String> fallbackCmdList =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
-+
-+    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList);
++    CliTokenSource tokenSource = makeTokenSource(env, FORCE_CMD, PROFILE_CMD);
 +
 +    AtomicInteger callCount = new AtomicInteger(0);
 +    try (MockedConstruction<ProcessBuilder> mocked =
@@ -109,19 +172,10 @@
 +
 +  @Test
 +  public void testCmdAndFallbackBothFailFallsThroughToSecondFallback() {
-+    // Very old CLI: cmd has --profile --force-refresh but CLI doesn't know --profile.
-+    // Should fall through to fallbackCmd (which also fails), then to secondFallbackCmd (--host).
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> cmd =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
-+    List<String> fallbackCmdList =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
-+    List<String> secondFallbackCmdList =
-+        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
-+
-+    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList, secondFallbackCmdList);
++    CliTokenSource tokenSource = makeTokenSource(env, FORCE_CMD, PROFILE_CMD, HOST_CMD);
 +
 +    AtomicInteger callCount = new AtomicInteger(0);
 +    try (MockedConstruction<ProcessBuilder> mocked =
@@ -160,12 +214,7 @@
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> cmd =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile", "--force-refresh");
-+    List<String> fallbackCmdList =
-+        Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
-+
-+    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList);
++    CliTokenSource tokenSource = makeTokenSource(env, FORCE_CMD, PROFILE_CMD);
 +
 +    try (MockedConstruction<ProcessBuilder> mocked =
 +        mockConstruction(
@@ -188,15 +237,10 @@
 +
 +  @Test
 +  public void testTwoLevelFallbackWithNoSecondFallback() {
-+    // cmd -> fallbackCmd chain with no secondFallbackCmd.
 +    Environment env = mock(Environment.class);
 +    when(env.getEnv()).thenReturn(new HashMap<>());
 +
-+    List<String> cmd = Arrays.asList("databricks", "auth", "token", "--profile", "my-profile");
-+    List<String> fallbackCmdList =
-+        Arrays.asList("databricks", "auth", "token", "--host", "https://workspace.databricks.com");
-+
-+    CliTokenSource tokenSource = makeTokenSource(env, cmd, fallbackCmdList);
++    CliTokenSource tokenSource = makeTokenSource(env, PROFILE_CMD, HOST_CMD);
 +
 +    AtomicInteger callCount = new AtomicInteger(0);
 +    try (MockedConstruction<ProcessBuilder> mocked =

Reproduce locally: git range-diff f28430b..61686da f28430b..1c68e85 | Disable: git config gitstack.push-range-diff false

@mihaimitrea-db mihaimitrea-db removed the request for review from parthban-db April 14, 2026 13:59
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 1c68e85 to 0de2614 Compare April 30, 2026 13:28
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 0de2614 to 6d775fc Compare April 30, 2026 13:37
@mihaimitrea-db mihaimitrea-db changed the title Add --force-refresh support for Databricks CLI token fetching Fix CLI token source --profile fallback with version detection Apr 30, 2026
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 6d775fc to c4e12c1 Compare April 30, 2026 14:51
Copy link
Copy Markdown
Member

@simonfaltum simonfaltum left a comment

Choose a reason for hiding this comment

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

Review Swarm (Isaac + Cursor, 2 rounds, deep review)

Verdict: Not ready yet

0 Critical | 2 Major | 1 Gap (Minor) | 8 Nit | 6 Suggestion

Both reviewers independently proposed the same architecture the PR already uses (probe + cache + typed semver + UNKNOWN sentinel). The two Major findings are about subprocess and JSON-parsing robustness, not the design. Everything else is polish.

See inline comments for specific findings, and a follow-up general PR comment for findings that reference lines outside the diff hunks.

Verifications (both reviewers agreed, no issues)

  • CliTokenSource carries @InternalApi; removing the 6-arg constructor is not a breaking change.
  • AzureCliCredentialsProvider is untouched (still uses the 5-arg constructor).
  • atLeast/compareTo ordering is correct across patch/minor/major; UNKNOWN(-1,-1,-1) sorts below all real versions including (0,0,0).
  • Account-host vs workspace-host behavior is preserved (buildHostArgs still appends --account-id only when ClientType.ACCOUNT).
  • The core bug is fixed: cfg.profile set with CLI < v0.207.1 now correctly falls back to --host.

Automated review swarm. Classifications are advisory; final judgement is yours.

Comment on lines +40 to +44

// Successful version probes keyed by cliPath. Failures are deliberately not cached, so a
// transient error (timeout, AV scan) does not pin every later token source to the conservative
// fallback for the rest of the process lifetime.
private static final Map<String, DatabricksCliVersion> VERSION_CACHE = new ConcurrentHashMap<>();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[Suggestion] Negative-result caching trade-off could bite SDK consumers that build many WorkspaceClients

Found by: Cursor, confirmed by Isaac

Failing-fast is fine when the CLI is truly unavailable (fast exception). But a CLI that genuinely hangs (corporate AV scan on first launch, blocked exec) incurs 5s on every configure(...) call for the JVM lifetime. For SDK consumers that build many WorkspaceClients this is death by a thousand cuts. Worth confirming parity with the Go/Python siblings.

Suggestion: Either (a) document the trade-off explicitly in the cache comment ("we accept up to N×5s startup latency under sustained CLI failure"), or (b) cache UNKNOWN with a small TTL (e.g. 60s) to bound the damage. Not a blocker.

@simonfaltum
Copy link
Copy Markdown
Member

Review Swarm: Additional findings

The findings below reference code locations outside the PR's diff hunks (pre-existing lines that weren't touched, or test-coverage gaps that don't have a single pin-point) so they're posted here rather than as inline comments.


[Nit] Dead CliCommandException inner class + misleading comment after fallback removal

File: databricks-sdk-java/src/main/java/com/databricks/sdk/core/CliTokenSource.java (class at lines 37-48, comment at lines 121-122)
Found by: Cursor, confirmed by Isaac

The PR removes the only consumer of CliCommandException.getFullOutput() (the "unknown flag: --profile" text check in getToken). The class is still thrown from execCliCommand, but fullOutput is never read — it's effectively a renamed IOException with a dead field. Worse, the surrounding comment ("getFullOutput() exposes both streams so the caller can check for unknown flag: --profile") is now actively misleading about why the class exists.

Suggestion: Either inline to throw new IOException("cannot get access token: " + stderr) or, if the dual-stream message is still useful for diagnostics, drop the getFullOutput() accessor and update the comment. Cleanup atomic with the obsolescence keeps the diff self-contained.


[Nit] "CLI not installed" path now emits two WARN logs instead of one

File: databricks-sdk-java/src/main/java/com/databricks/sdk/core/DatabricksCliCredentialsProvider.java (existing ~lines 117-122 + new line 235-237)
Found by: Isaac (cross-review)

When the CLI binary doesn't exist at cliPath, probeCliVersion fails first (ProcessBuilder.start() throws "error=2, No such file or directory"), getCliVersion swallows and emits WARN "Failed to detect Databricks CLI version...". Then configure proceeds, cachedTokenSource.getToken() runs the same nonexistent binary again, and the existing configure catch block emits WARN "Most likely databricks CLI is not installed". Pre-PR: one WARN. Post-PR: two WARN lines for the same condition.

Suggestion: Downgrade the probe-failure log to DEBUG, or check IOException.getMessage() for "No such file"/"error=2" before warning, so the existing "not installed" WARN remains the canonical signal.


[Nit] Comparable.compareTo is not directly exercised by tests

File: databricks-sdk-java/src/test/java/com/databricks/sdk/core/DatabricksCliVersionTest.java
Found by: Cursor, confirmed by Isaac

atLeast is tested thoroughly; compareTo is only covered transitively. A future refactor that splits the two could regress silently. No test asserts the correct sign of compareTo (only the boolean result via atLeast).

Suggestion: Add one parameterized test:

assertEquals(expectedSign, Integer.signum(a.compareTo(b)));

covering: equal, lesser-by-major, lesser-by-minor, lesser-by-patch, UNKNOWN-vs-real, dev-vs-real.


[Nit] Wildcard java.util.* mixed with explicit java.util.concurrent.* imports

File: databricks-sdk-java/src/main/java/com/databricks/sdk/core/DatabricksCliCredentialsProvider.java (line 18)
Found by: Isaac (cross-review)

Pre-existing import java.util.*; was kept; new imports (ConcurrentHashMap, TimeUnit) were added explicitly. Minor inconsistency — the wildcard predates this PR. Defer to repo lint settings; skip if wildcard imports are allowed.


[Suggestion] buildCliCommand silently emits --host null when host isn't set

File: databricks-sdk-java/src/main/java/com/databricks/sdk/core/DatabricksCliCredentialsProvider.java:171-198
Found by: Cursor; Isaac downgraded to Nit in cross-review

buildCliCommand no longer guards config.getHost() != null before emitting --host. In practice the path is unreachable: configure(...) short-circuits with return null when host == null before getDatabricksCliTokenSource is called, and getDatabricksCliTokenSource is private. Defensive-only. The same latent bug existed pre-PR in the else branch.

Suggestion: Optional — guard with Objects.requireNonNull(config.getHost(), ...) in buildCliCommand, or throw a clear DatabricksException, so the code documents its own assumption. Keep as-is if relying on configure's short-circuit is fine.

@simonfaltum
Copy link
Copy Markdown
Member

Process resource leak in probeCliVersion on InterruptedException path

  • Priority: P2
  • Location: DatabricksCliCredentialsProvider.java: InterruptedException catch block inside probeCliVersion, after process.waitFor(VERSION_PROBE_TIMEOUT_SECONDS, TimeUnit.SECONDS)
  • Scenario: Thread is interrupted while waiting for the databricks version subprocess. The catch block re-interrupts the thread and throws IOException but never calls process.destroy() or process.destroyForcibly(), leaving the subprocess running. The timeout branch in the same method already calls process.destroyForcibly() correctly.
  • Potential solution: Wrap the post-pb.start() block in a try/finally that calls process.destroyForcibly() (guarded by process.isAlive()), or at minimum add process.destroyForcibly() to the InterruptedException catch before rethrowing, mirroring the existing timeout branch.

Duplicate WARN log emitted when CLI binary is missing

  • Priority: P3
  • Location: DatabricksCliCredentialsProvider.java: getCliVersion catch block (WARN "Failed to detect Databricks CLI version…") and configure catch block (WARN "Most likely databricks CLI is not installed")
  • Scenario: cliPath is non-null but the binary does not exist. probeCliVersion throws (e.g. "No such file or directory"); getCliVersion catches it and logs WARN before returning UNKNOWN. getDatabricksCliTokenSource still constructs a CliTokenSource; configure calls getToken(), the same missing binary fails again, and the existing handler logs the second WARN when the message contains "not found". Pre-PR only one WARN was emitted for this condition.
  • Potential solution: Downgrade the version-probe failure log in getCliVersion to DEBUG, or detect missing-binary errors specifically and suppress the WARN there, letting the existing "not installed" WARN in configure remain the canonical signal.

🔍 Reviewed by nitpicker

@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from c4e12c1 to 228ea4d Compare May 4, 2026 13:30
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 228ea4d to 5525326 Compare May 4, 2026 13:57
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 5525326 to 6a66c24 Compare May 4, 2026 14:18
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from 6a66c24 to c968abd Compare May 4, 2026 14:28
`--profile` on `databricks auth token` is a global Cobra flag, so old
CLIs (< v0.207.1) silently accept it and fail later with `cannot fetch
credentials` instead of `unknown flag: --profile`. The previous
error-based fallback never matched, leaving the `--host` fallback as
dead code.

This commit replaces the runtime fallback chain with version-based
capability detection:

* `CliVersion` carries a (major, minor, patch) triple plus an
  `UNKNOWN` sentinel and a default-dev-build (0,0,0) check.
* `DatabricksCliCredentialsProvider` runs `databricks version --output
  json` once per CLI path (cached on success only, with a 5s timeout)
  and gates `--profile` on >= v0.207.1; everything else falls back to
  `--host` with a precise warning.
* `CliTokenSource` is simplified to a single `cmd`; the
  `fallbackCmd` parameter and the runtime "unknown flag" retry loop are
  removed.

Mirrors the equivalent refactors in the Go and Python SDKs:
* databricks/databricks-sdk-go#1605
* databricks/databricks-sdk-py#1377

Co-authored-by: Isaac
@mihaimitrea-db mihaimitrea-db force-pushed the mihaimitrea-db/stack/cli-force-refresh branch from c968abd to fdd7383 Compare May 4, 2026 14:47
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-java

Inputs:

  • PR number: 751
  • Commit SHA: fdd73835893b0532c20058b0acf1267b58cbf01c

Checks will be approved automatically on success.

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