Add more docs and tests

Author: Quinn-With-Two-Ns

gradle/jacoco.gradle

@@ -13,6 +13,7 @@ dependencies {
     jacocoAggregation project(':temporal-spring-boot-starter')
     jacocoAggregation project(':temporal-test-server')
     jacocoAggregation project(':temporal-testing')
+    jacocoAggregation project(':temporal-envconfig')
 }
 
 def jacocoExclusions = [

temporal-bom/build.gradle

@@ -16,5 +16,6 @@ dependencies {
         api project(':temporal-spring-boot-starter')
         api project(':temporal-test-server')
         api project(':temporal-testing')
+        api project(':temporal-envconfig')
     }
 }

temporal-envconfig/build.gradle

@@ -4,7 +4,6 @@ dependencies {
     // this module shouldn't carry temporal-sdk with it, especially for situations when users may be using a shaded artifact
     compileOnly project(':temporal-sdk')
 
-    implementation 'org.tomlj:tomlj:1.1.1'
     implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-toml:2.15.4'
     testImplementation project(":temporal-testing")
     testImplementation "junit:junit:${junitVersion}"

temporal-envconfig/src/main/java/io/temporal/envconfig/ClientConfig.java

@@ -1,21 +1,16 @@
 package io.temporal.envconfig;
 
-import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
-import com.fasterxml.jackson.annotation.JsonInclude;
-import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.databind.ObjectReader;
 import com.fasterxml.jackson.dataformat.toml.TomlMapper;
-import io.grpc.Metadata;
 import io.temporal.common.Experimental;
 import java.io.*;
-import java.util.Collections;
-import java.util.HashMap;
 import java.util.Map;
-import javax.annotation.Nullable;
 
+/** ClientConfig represents a client config file. */
 @Experimental
 public class ClientConfig {
 
+  /** Get the default config file path: $HOME/.config/temporal/temporal.toml */
   private static String getDefaultConfigFilePath() {
     String userDir = System.getProperty("user.home");
     if (userDir == null || userDir.isEmpty()) {
@@ -41,21 +36,29 @@ public static ClientConfig load() throws IOException {
    * <p>This does not apply environment variable overrides to the profiles, it only uses an
    * environment variable to find the default config file path (TEMPORAL_CONFIG_FILE). To get a
    * single profile with environment variables applied, use {@link ClientConfigProfile#load}.
+   *
+   * @param options options to control loading the config
+   * @throws IOException if the config file cannot be read or parsed
    */
   public static ClientConfig load(LoadClientConfigOptions options) throws IOException {
-    ObjectReader reader = new TomlMapper().readerFor(TomlClientConfig.class);
+    ObjectReader reader = new TomlMapper().readerFor(ClientConfigToml.TomlClientConfig.class);
     if (options.isStrictConfigFile()) {
       reader =
           reader.withFeatures(
               com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+    } else {
+      reader =
+          reader.withoutFeatures(
+              com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
     }
+
     if (options.getConfigFileData() != null && options.getConfigFileData().length > 0) {
       if (options.getConfigFilePath() != null && !options.getConfigFilePath().isEmpty()) {
         throw new IllegalArgumentException(
             "Cannot have both ConfigFileData and ConfigFilePath set");
       }
-      TomlClientConfig result = reader.readValue(options.getConfigFileData());
-      return new ClientConfig(result);
+      ClientConfigToml.TomlClientConfig result = reader.readValue(options.getConfigFileData());
+      return new ClientConfig(ClientConfigToml.getClientProfiles(result));
     } else {
       // Get file name which is either set value, env var, or default path
       String file = options.getConfigFilePath();
@@ -71,172 +74,19 @@ public static ClientConfig load(LoadClientConfigOptions options) throws IOExcept
       if (file == null || file.isEmpty()) {
         file = getDefaultConfigFilePath();
       }
-      TomlClientConfig result = reader.readValue(file);
-      return new ClientConfig(result);
+      ClientConfigToml.TomlClientConfig result = reader.readValue(new File(file));
+      return new ClientConfig(ClientConfigToml.getClientProfiles(result));
     }
   }
 
-  private final Map<String, ClientConfigProfile> profiles;
-
-  private ClientConfig() {
-    profiles = Collections.emptyMap();
-  }
-
-  private String normalizeGrpcMetaKey(String key) {
-    return key.toLowerCase().replace('_', '-');
-  }
-
-  private ClientConfig(TomlClientConfig clientConfig) {
-    profiles = new HashMap<>(clientConfig.profiles.size());
-    for (Map.Entry<String, TomlClientConfigProfile> entry : clientConfig.profiles.entrySet()) {
-      String profileName = entry.getKey();
-      TomlClientConfigProfile tomlProfile = entry.getValue();
-      ClientConfigTLS tls = getClientConfigTLS(tomlProfile);
-      Metadata metadata = null;
-      if (tomlProfile.grpcMeta != null) {
-        for (Map.Entry<String, String> metaEntry : tomlProfile.grpcMeta.entrySet()) {
-          if (metadata == null) {
-            metadata = new Metadata();
-          }
-          Metadata.Key<String> key =
-              Metadata.Key.of(
-                  normalizeGrpcMetaKey(metaEntry.getKey()), Metadata.ASCII_STRING_MARSHALLER);
-          metadata.put(key, metaEntry.getValue());
-        }
-      }
-      ClientConfigProfile profile =
-          ClientConfigProfile.newBuilder()
-              .setAddress(tomlProfile.address)
-              .setNamespace(tomlProfile.namespace)
-              .setApiKey(tomlProfile.apiKey)
-              .setMetadata(metadata)
-              .setTls(tls)
-              .build();
-      profiles.put(profileName, profile);
-    }
+  public ClientConfig(Map<String, ClientConfigProfile> profiles) {
+    this.profiles = profiles;
   }
 
-  @Nullable
-  private static ClientConfigTLS getClientConfigTLS(TomlClientConfigProfile tomlProfile) {
-    ClientConfigTLS tls = null;
-    if (tomlProfile.tls != null) {
-      tls =
-          new ClientConfigTLS(
-              tomlProfile.tls.disabled,
-              tomlProfile.tls.clientCertPath,
-              tomlProfile.tls.clientCertData.getBytes(),
-              tomlProfile.tls.clientKeyPath,
-              tomlProfile.tls.clientKeyData.getBytes(),
-              tomlProfile.tls.serverCACertPath,
-              tomlProfile.tls.serverCACertData.getBytes(),
-              tomlProfile.tls.serverName,
-              tomlProfile.tls.disableHostVerification);
-    }
-    return tls;
-  }
+  private final Map<String, ClientConfigProfile> profiles;
 
+  /** All profiles loaded from the config file, may be empty but never null. */
   public Map<String, ClientConfigProfile> getProfiles() {
     return profiles;
   }
-
-  @JsonIgnoreProperties(ignoreUnknown = true)
-  @JsonInclude(JsonInclude.Include.NON_EMPTY)
-  static class TomlClientConfig {
-    @JsonProperty("profile")
-    public Map<String, TomlClientConfigProfile> profiles;
-
-    protected TomlClientConfig() {}
-
-    TomlClientConfig(Map<String, TomlClientConfigProfile> profiles) {
-      this.profiles = profiles;
-    }
-  }
-
-  @JsonIgnoreProperties(ignoreUnknown = true)
-  @JsonInclude(JsonInclude.Include.NON_EMPTY)
-  static class TomlClientConfigProfile {
-    @JsonProperty("address")
-    public String address;
-
-    @JsonProperty("namespace")
-    public String namespace;
-
-    @JsonProperty("api_key")
-    public String apiKey;
-
-    @JsonProperty("tls")
-    public TomlClientConfigTLS tls;
-
-    @JsonProperty("grpc_meta")
-    public Map<String, String> grpcMeta;
-
-    protected TomlClientConfigProfile() {}
-
-    TomlClientConfigProfile(
-        String address,
-        String namespace,
-        String apiKey,
-        TomlClientConfigTLS tls,
-        Map<String, String> grpcMeta) {
-      this.address = address;
-      this.namespace = namespace;
-      this.apiKey = apiKey;
-      this.tls = tls;
-      this.grpcMeta = grpcMeta;
-    }
-  }
-
-  @JsonIgnoreProperties(ignoreUnknown = true)
-  @JsonInclude(JsonInclude.Include.NON_EMPTY)
-  static class TomlClientConfigTLS {
-    @JsonProperty("disabled")
-    public boolean disabled;
-
-    @JsonProperty("client_cert_path")
-    public String clientCertPath;
-
-    @JsonProperty("client_cert_data")
-    public String clientCertData;
-
-    @JsonProperty("client_key_path")
-    public String clientKeyPath;
-
-    @JsonProperty("client_key_data")
-    public String clientKeyData;
-
-    @JsonProperty("server_ca_cert_path")
-    public String serverCACertPath;
-
-    @JsonProperty("server_ca_cert_data")
-    public String serverCACertData;
-
-    @JsonProperty("server_name")
-    public String serverName;
-
-    @JsonProperty("disable_host_verification")
-    public boolean disableHostVerification;
-
-    protected TomlClientConfigTLS() {}
-
-    TomlClientConfigTLS(
-        boolean disabled,
-        String clientCertPath,
-        String clientCertData,
-        String clientKeyPath,
-        String clientKeyData,
-        String serverCACertPath,
-        String serverCACertData,
-        String serverName,
-        boolean disableHostVerification) {
-      this.disabled = disabled;
-      this.clientCertPath = clientCertPath;
-      this.clientCertData = clientCertData;
-      this.clientKeyPath = clientKeyPath;
-      this.clientKeyData = clientKeyData;
-      this.serverCACertPath = serverCACertPath;
-      this.serverCACertData = serverCACertData;
-      this.serverName = serverName;
-      this.disableHostVerification = disableHostVerification;
-    }
-  }
 }

temporal-envconfig/src/main/java/io/temporal/envconfig/ClientConfigProfile.java

@@ -15,10 +15,17 @@
 /** ClientConfigProfile is profile-level configuration for a client. */
 @Experimental
 public class ClientConfigProfile {
+  /** Creates a new builder to build a ClientConfigProfile. */
   public static Builder newBuilder() {
     return new Builder();
   }
 
+  /**
+   * Creates a new builder to build a ClientConfigProfile based on an existing profile.
+   *
+   * @param profile the existing profile to base the builder on
+   * @return a new Builder instance
+   */
   public static Builder newBuilder(ClientConfigProfile profile) {
     return new Builder(profile);
   }
@@ -46,6 +53,10 @@ public Builder toBuilder() {
     return new Builder(this);
   }
 
+  /**
+   * Converts this profile to WorkflowServiceStubsOptions. Note that not all fields are converted,
+   * only those relevant to service stubs.
+   */
   public WorkflowServiceStubsOptions toWorkflowServiceStubsOptions() {
     WorkflowServiceStubsOptions.Builder builder = WorkflowServiceStubsOptions.newBuilder();
     if (this.address != null && !this.address.isEmpty()) {
@@ -100,11 +111,18 @@ public WorkflowServiceStubsOptions toWorkflowServiceStubsOptions() {
       if (this.tls.getServerName() != null && !this.tls.getServerName().isEmpty()) {
         builder.setChannelInitializer(c -> c.overrideAuthority(this.tls.getServerName()));
       }
+    } else if (this.apiKey != null && !this.apiKey.isEmpty()) {
+      // If API key is set, TLS is required, so enable it with defaults
+      builder.setEnableHttps(true);
     }
 
     return builder.build();
   }
 
+  /**
+   * Converts this profile to WorkflowClientOptions. Note that not all fields are converted, only
+   * those relevant to client options.
+   */
   public WorkflowClientOptions toWorkflowClientOptions() {
     WorkflowClientOptions.Builder builder = WorkflowClientOptions.newBuilder();
     if (this.namespace != null && !this.namespace.isEmpty()) {
@@ -156,8 +174,7 @@ public static ClientConfigProfile load(LoadClientConfigProfileOptions options)
     }
     // If env is enabled, apply it on top of an empty profile
     if (!options.isDisableEnv()) {
-      Map<String, String> overrideEnvVars = null;
-      clientConfigProfile.applyEnvOverrides(overrideEnvVars);
+      clientConfigProfile.applyEnvOverrides(options.getEnvOverrides());
     }
     return clientConfigProfile;
   }
@@ -328,26 +345,42 @@ public Builder(ClientConfigProfile profile) {
       this.tls = profile.tls;
     }
 
+    /**
+     * Sets the namespace for the client. This is optional; if not set, the default namespace will
+     * be used.
+     */
     public Builder setNamespace(String namespace) {
       this.namespace = namespace;
       return this;
     }
 
+    /**
+     * Sets the address of the Temporal service endpoint. This is optional; if not set, the default
+     * address will be used.
+     */
     public Builder setAddress(String address) {
       this.address = address;
       return this;
     }
 
+    /** Sets the API key for the client. This is optional; if not set, no API key will be used. */
     public Builder setApiKey(String apiKey) {
       this.apiKey = apiKey;
       return this;
     }
 
+    /**
+     * Sets the gRPC metadata to be sent with each request. This is optional; if not set, no
+     * metadata will be sent.
+     */
     public Builder setMetadata(Metadata metadata) {
       this.metadata = metadata;
       return this;
     }
 
+    /**
+     * Sets the TLS configuration for the client. This is optional; if not set, no TLS will be used.
+     */
     public Builder setTls(ClientConfigTLS tls) {
       this.tls = tls;
       return this;