make enableHttps a Boolean

THardy98 · THardy98 · commit 1ce12f298daf · 2025-12-15T10:59:41.000-05:00
diff --git a/temporal-serviceclient/src/main/java/io/temporal/serviceclient/ServiceStubsOptions.java b/temporal-serviceclient/src/main/java/io/temporal/serviceclient/ServiceStubsOptions.java
@@ -39,8 +39,14 @@ public class ServiceStubsOptions {
 
   protected final @Nullable Consumer<ManagedChannelBuilder<?>> channelInitializer;
 
-  /** Indicates whether basic HTTPS/SSL/TLS should be enabled * */
-  protected final boolean enableHttps;
+  /**
+   * Indicates whether basic HTTPS/SSL/TLS should be enabled. Null means not explicitly set by the
+   * user, allowing runtime derivation of the effective value.
+   */
+  protected final Boolean enableHttps;
+
+  /** Indicates whether an API key was provided, used for automatic TLS enablement */
+  protected final boolean apiKeyProvided;
 
   /** The user provided context for SSL/TLS over gRPC * */
   protected final SslContext sslContext;
@@ -113,6 +119,7 @@ public class ServiceStubsOptions {
     this.target = that.target;
     this.channelInitializer = that.channelInitializer;
     this.enableHttps = that.enableHttps;
+    this.apiKeyProvided = that.apiKeyProvided;
     this.sslContext = that.sslContext;
     this.healthCheckAttemptTimeout = that.healthCheckAttemptTimeout;
     this.systemInfoTimeout = that.systemInfoTimeout;
@@ -134,7 +141,8 @@ public class ServiceStubsOptions {
       ManagedChannel channel,
       String target,
       @Nullable Consumer<ManagedChannelBuilder<?>> channelInitializer,
-      boolean enableHttps,
+      Boolean enableHttps,
+      boolean apiKeyProvided,
       SslContext sslContext,
       Duration healthCheckAttemptTimeout,
       Duration healthCheckTimeout,
@@ -154,6 +162,7 @@ public class ServiceStubsOptions {
     this.target = target;
     this.channelInitializer = channelInitializer;
     this.enableHttps = enableHttps;
+    this.apiKeyProvided = apiKeyProvided;
     this.sslContext = sslContext;
     this.healthCheckAttemptTimeout = healthCheckAttemptTimeout;
     this.healthCheckTimeout = healthCheckTimeout;
@@ -202,11 +211,23 @@ public Consumer<ManagedChannelBuilder<?>> getChannelInitializer() {
   }
 
   /**
-   * @return if gRPC should use SSL/TLS; Ignored and assumed {@code true} if {@link
-   *     #getSslContext()} is set
+   * Returns whether gRPC should use SSL/TLS. This method computes the effective value based on:
+   *
+   * <ul>
+   *   <li>If explicitly set via {@link Builder#setEnableHttps(boolean)}, returns that value
+   *   <li>If an API key was provided and no custom SSL context or channel is set, returns {@code
+   *       true}
+   *   <li>Otherwise returns {@code false}
+   * </ul>
+   *
+   * <p>Note: This is ignored and assumed {@code true} if {@link #getSslContext()} is set.
+   *
+   * @return if gRPC should use SSL/TLS
    */
   public boolean getEnableHttps() {
-    return enableHttps;
+    return (enableHttps != null)
+        ? enableHttps
+        : (apiKeyProvided && sslContext == null && channel == null);
   }
 
   /**
@@ -325,12 +346,13 @@ public boolean equals(Object o) {
     if (this == o) return true;
     if (o == null || getClass() != o.getClass()) return false;
     ServiceStubsOptions that = (ServiceStubsOptions) o;
-    return enableHttps == that.enableHttps
+    return apiKeyProvided == that.apiKeyProvided
         && enableKeepAlive == that.enableKeepAlive
         && keepAlivePermitWithoutStream == that.keepAlivePermitWithoutStream
         && Objects.equals(channel, that.channel)
         && Objects.equals(target, that.target)
         && Objects.equals(channelInitializer, that.channelInitializer)
+        && Objects.equals(enableHttps, that.enableHttps)
         && Objects.equals(sslContext, that.sslContext)
         && Objects.equals(healthCheckAttemptTimeout, that.healthCheckAttemptTimeout)
         && Objects.equals(healthCheckTimeout, that.healthCheckTimeout)
@@ -353,6 +375,7 @@ public int hashCode() {
         target,
         channelInitializer,
         enableHttps,
+        apiKeyProvided,
         sslContext,
         healthCheckAttemptTimeout,
         healthCheckTimeout,
@@ -382,6 +405,8 @@ public String toString() {
         + channelInitializer
         + ", enableHttps="
         + enableHttps
+        + ", apiKeyProvided="
+        + apiKeyProvided
         + ", sslContext="
         + sslContext
         + ", healthCheckAttemptTimeout="
@@ -444,6 +469,7 @@ protected Builder(ServiceStubsOptions options) {
       this.target = options.target;
       this.channelInitializer = options.channelInitializer;
       this.enableHttps = options.enableHttps;
+      this.apiKeyProvided = options.apiKeyProvided;
       this.sslContext = options.sslContext;
       this.healthCheckAttemptTimeout = options.healthCheckAttemptTimeout;
       this.healthCheckTimeout = options.healthCheckTimeout;
@@ -456,8 +482,15 @@ protected Builder(ServiceStubsOptions options) {
       this.connectionBackoffResetFrequency = options.connectionBackoffResetFrequency;
       this.grpcReconnectFrequency = options.grpcReconnectFrequency;
       this.headers = options.headers;
-      this.grpcMetadataProviders = options.grpcMetadataProviders;
-      this.grpcClientInterceptors = options.grpcClientInterceptors;
+      // Make mutable copies of collections to allow adding more items
+      this.grpcMetadataProviders =
+          options.grpcMetadataProviders != null && !options.grpcMetadataProviders.isEmpty()
+              ? new ArrayList<>(options.grpcMetadataProviders)
+              : null;
+      this.grpcClientInterceptors =
+          options.grpcClientInterceptors != null && !options.grpcClientInterceptors.isEmpty()
+              ? new ArrayList<>(options.grpcClientInterceptors)
+              : null;
       this.metricsScope = options.metricsScope;
     }
 
@@ -805,7 +838,8 @@ public ServiceStubsOptions build() {
           this.channel,
           this.target,
           this.channelInitializer,
-          this.enableHttps != null ? this.enableHttps : false,
+          this.enableHttps,
+          this.apiKeyProvided,
           this.sslContext,
           this.healthCheckAttemptTimeout,
           this.healthCheckTimeout,
@@ -853,14 +887,6 @@ public ServiceStubsOptions validateAndBuildWithDefaults() {
       Collection<ClientInterceptor> grpcClientInterceptors =
           MoreObjects.firstNonNull(this.grpcClientInterceptors, Collections.emptyList());
 
-      // Resolve enableHttps: explicit value, auto-enable with API key, or default false
-      boolean enableHttps = false;
-      if (this.enableHttps != null) {
-        enableHttps = this.enableHttps;
-      } else if (this.apiKeyProvided && this.sslContext == null && this.channel == null) {
-        enableHttps = true;
-      }
-
       Scope metricsScope = this.metricsScope != null ? this.metricsScope : new NoopScope();
       Duration healthCheckAttemptTimeout =
           this.healthCheckAttemptTimeout != null
@@ -875,7 +901,8 @@ public ServiceStubsOptions validateAndBuildWithDefaults() {
           this.channel,
           target,
           this.channelInitializer,
-          enableHttps,
+          this.enableHttps,
+          this.apiKeyProvided,
           this.sslContext,
           healthCheckAttemptTimeout,
           healthCheckTimeout,
diff --git a/temporal-serviceclient/src/test/java/io/temporal/serviceclient/ServiceStubsOptionsTest.java b/temporal-serviceclient/src/test/java/io/temporal/serviceclient/ServiceStubsOptionsTest.java
@@ -1,10 +1,7 @@
 package io.temporal.serviceclient;
 
 import static org.junit.Assert.*;
-import static org.mockito.Mockito.mock;
 
-import io.grpc.ManagedChannel;
-import io.grpc.netty.shaded.io.netty.handler.ssl.SslContext;
 import org.junit.Test;
 
 public class ServiceStubsOptionsTest {
@@ -68,33 +65,116 @@ public void testExplicitTLSEnableWithoutAPIKey() {
   }
 
   @Test
-  public void testTLSNotAutoEnabledWhenSslContextProvided() {
-    // When user provides custom sslContext, they're handling TLS themselves
-    // so enableHttps should not be auto-enabled
-    SslContext sslContext = mock(SslContext.class);
-    ServiceStubsOptions options =
+  public void testBuilderFromOptionsPreservesDefaultTLSBehavior() {
+    // Step 1: Create options with no API key and no TLS setting
+    ServiceStubsOptions options1 =
         WorkflowServiceStubsOptions.newBuilder()
             .setTarget("localhost:7233")
+            .validateAndBuildWithDefaults();
+
+    // Verify TLS is disabled (no API key)
+    assertFalse(options1.getEnableHttps());
+
+    // Step 2: Create new builder from existing options and add API key
+    ServiceStubsOptions options2 =
+        WorkflowServiceStubsOptions.newBuilder(options1)
             .addApiKey(() -> "test-api-key")
-            .setSslContext(sslContext)
             .validateAndBuildWithDefaults();
 
-    // enableHttps stays false because sslContext handles TLS
-    assertFalse(options.getEnableHttps());
-    assertNotNull(options.getSslContext());
+    // TLS should auto-enable because user never explicitly set TLS=false,
+    // even though options1 had enableHttps=false (derived, not explicit)
+    assertTrue(
+        "TLS should auto-enable when API key is added to builder from options that had default TLS behavior",
+        options2.getEnableHttps());
   }
 
   @Test
-  public void testTLSNotAutoEnabledWhenCustomChannelProvided() {
-    // When user provides custom channel, they're managing connection themselves
-    // so enableHttps should not be auto-enabled
-    ManagedChannel channel = mock(ManagedChannel.class);
-    ServiceStubsOptions options =
+  public void testBuilderFromOptionsWithExplicitTLSDisableStaysDisabled() {
+    // Step 1: Create options with explicit TLS=false
+    ServiceStubsOptions options1 =
         WorkflowServiceStubsOptions.newBuilder()
-            .setChannel(channel)
+            .setTarget("localhost:7233")
+            .setEnableHttps(false)
+            .validateAndBuildWithDefaults();
+
+    assertFalse(options1.getEnableHttps());
+
+    // Step 2: Create new builder from existing options and add API key
+    ServiceStubsOptions options2 =
+        WorkflowServiceStubsOptions.newBuilder(options1)
             .addApiKey(() -> "test-api-key")
             .validateAndBuildWithDefaults();
 
-    assertFalse(options.getEnableHttps());
+    // TLS should stay disabled because user explicitly set TLS=false
+    assertFalse(
+        "TLS should stay disabled when explicitly set to false, even with API key",
+        options2.getEnableHttps());
+  }
+
+  @Test
+  public void testBuilderFromOptionsWithExplicitTLSEnableStaysEnabled() {
+    // Step 1: Create options with explicit TLS=true
+    ServiceStubsOptions options1 =
+        WorkflowServiceStubsOptions.newBuilder()
+            .setTarget("localhost:7233")
+            .setEnableHttps(true)
+            .validateAndBuildWithDefaults();
+
+    assertTrue(options1.getEnableHttps());
+
+    // Step 2: Create new builder from existing options (no API key)
+    ServiceStubsOptions options2 =
+        WorkflowServiceStubsOptions.newBuilder(options1).validateAndBuildWithDefaults();
+
+    // TLS should stay enabled because user explicitly set TLS=true
+    assertTrue("TLS should stay enabled when explicitly set to true", options2.getEnableHttps());
+  }
+
+  @Test
+  public void testSpringBootStyleAutoTLSWithApiKey() {
+    ServiceStubsOptions options1 =
+        WorkflowServiceStubsOptions.newBuilder()
+            .setTarget("my-namespace.tmprl.cloud:7233")
+            .addApiKey(() -> "my-api-key")
+            .validateAndBuildWithDefaults();
+
+    assertTrue(
+        "TLS should auto-enable when API key is provided without explicit TLS setting",
+        options1.getEnableHttps());
+
+    // Scenario 2: API key provided, TLS explicitly enabled (should stay enabled)
+    ServiceStubsOptions options2 =
+        WorkflowServiceStubsOptions.newBuilder()
+            .setTarget("my-namespace.tmprl.cloud:7233")
+            .setEnableHttps(true)
+            .addApiKey(() -> "my-api-key")
+            .validateAndBuildWithDefaults();
+
+    assertTrue(
+        "TLS should be enabled when explicitly set to true with API key",
+        options2.getEnableHttps());
+
+    // Scenario 3: API key provided, TLS explicitly disabled (should stay disabled)
+    ServiceStubsOptions options3 =
+        WorkflowServiceStubsOptions.newBuilder()
+            .setTarget("localhost:7233")
+            .setEnableHttps(false)
+            .addApiKey(() -> "my-api-key")
+            .validateAndBuildWithDefaults();
+
+    assertFalse(
+        "TLS should stay disabled when explicitly set to false, even with API key",
+        options3.getEnableHttps());
+
+    // Scenario 4: No API key, no explicit TLS setting (should be disabled)
+    ServiceStubsOptions options4 =
+        WorkflowServiceStubsOptions.newBuilder()
+            .setTarget("localhost:7233")
+            // Note: No API key, no setEnableHttps()
+            .validateAndBuildWithDefaults();
+
+    assertFalse(
+        "TLS should be disabled when no API key and no explicit TLS setting",
+        options4.getEnableHttps());
   }
 }
