refactor(artifacts): Make ArtifactCredentials null-safe (#4586)

* refactor(artifacts): Make artifact accounts immutable and null-safe

There are a lot of config properties that can be set for artifacts.
In general, they don't care about the difference between unset and
null values, but this is enforced at various points of rather than
at the API boundary. In order to make it more clear that this is
the case, update the configuration classes to be immutable and to
translate null strings to empty strings upon creation. This removes
the ability of any consuming code to be able to see an null value
in these string properties.

In particular, we make all fields on artifact accounts immutable,
and replace the setters with a constructor. The ConstructorBinding
annotation is what tells Spring to bind properties using the
constructor (without it, it will look for setters and fail).
We also provide a Builder for convenience (primarily for testing)
to avoid needing to use the many-arg-constructor.

As the constructor will be called by Spring, which does allow null
properties we need to treat all input as potentially null and
convert to non-null in the constructor.

* refactor(artifacts): Make ArtifactCredentials null-safe

Now that the artifact accounts are immutable, the artifact credentials
that read from them can be made NonNullByDefault. This commit also
removes a number of now-redundant null-checks on its fields, replacing
them with .isEmpty() on a now guaranteed to be non-null string.

This commit also updates one of the interface methods to return an
ImmutableList (which all but one implementation was effectively doing
by returning Collections.singletonList), and makes the interface
NonnullByDefault.

There was a single implementation of download() that was not null-safe.
I've updated it to eagerly throw an exception; all callers of that method
that was updated immediately pass the output to IOUtils.copy which will
throw a null pointer exception. This change will simply provide more
context around the exception.

* refactor(artifacts): Make auth fields optional

The username, password, and usernamePasswordFile fields are
really optional, so let's store them as an Optional<String> (where
the contents of the optional is non-null and non-empty) instead of
using the empty string to mean optional.

* style(aritfacts): Remove guava from API boundary

This commit removes the ImmutableList from the API boundary of
ArtifactCredentials.  The list should still be immutable, but
we'll add a comment to do what type checking won't here.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

clouddriver-artifacts/clouddriver-artifacts.gradle

@@ -14,6 +14,7 @@ dependencies {
   implementation 'com.google.auth:google-auth-library-oauth2-http'
   implementation "com.netflix.frigga:frigga"
   implementation "com.netflix.spinnaker.kork:kork-artifacts"
+  implementation "com.netflix.spinnaker.kork:kork-annotations"
   implementation "com.netflix.spinnaker.kork:kork-exceptions"
   implementation "com.oracle.oci.sdk:oci-java-sdk-core"
   implementation "com.squareup.okhttp:okhttp"

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/ArtifactCredentialsRepository.java

@@ -17,14 +17,14 @@
 
 package com.netflix.spinnaker.clouddriver.artifacts;
 
+import com.google.common.base.Strings;
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactCredentials;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.stream.Collectors;
 import lombok.Getter;
-import org.apache.commons.lang3.StringUtils;
 import org.springframework.stereotype.Component;
 
 @Component
@@ -51,7 +51,7 @@ private ArtifactCredentials getCredentials(String accountName) {
   }
 
   public ArtifactCredentials getCredentials(String accountName, String type) {
-    if (StringUtils.isEmpty(accountName)) {
+    if (Strings.isNullOrEmpty(accountName)) {
       throw new IllegalArgumentException(
           "An artifact account must be supplied to download this artifact: " + accountName);
     }

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/bitbucket/BitbucketArtifactAccount.java

@@ -17,14 +17,32 @@
 
 package com.netflix.spinnaker.clouddriver.artifacts.bitbucket;
 
+import com.google.common.base.Strings;
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactAccount;
 import com.netflix.spinnaker.clouddriver.artifacts.config.BasicAuth;
-import lombok.Data;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
+import java.util.Optional;
+import javax.annotation.ParametersAreNullableByDefault;
+import lombok.Builder;
+import lombok.Value;
+import org.springframework.boot.context.properties.ConstructorBinding;
 
-@Data
+@NonnullByDefault
+@Value
 final class BitbucketArtifactAccount implements ArtifactAccount, BasicAuth {
-  private String name;
-  private String username;
-  private String password;
-  private String usernamePasswordFile;
+  private final String name;
+  private final Optional<String> username;
+  private final Optional<String> password;
+  private final Optional<String> usernamePasswordFile;
+
+  @Builder
+  @ConstructorBinding
+  @ParametersAreNullableByDefault
+  BitbucketArtifactAccount(
+      String name, String username, String password, String usernamePasswordFile) {
+    this.name = Strings.nullToEmpty(name);
+    this.username = Optional.ofNullable(Strings.emptyToNull(username));
+    this.password = Optional.ofNullable(Strings.emptyToNull(password));
+    this.usernamePasswordFile = Optional.ofNullable(Strings.emptyToNull(usernamePasswordFile));
+  }
 }

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/bitbucket/BitbucketArtifactCredentials.java

@@ -17,19 +17,20 @@
 
 package com.netflix.spinnaker.clouddriver.artifacts.bitbucket;
 
+import com.google.common.collect.ImmutableList;
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactCredentials;
 import com.netflix.spinnaker.clouddriver.artifacts.config.SimpleHttpArtifactCredentials;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.squareup.okhttp.OkHttpClient;
-import java.util.Collections;
-import java.util.List;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 
+@NonnullByDefault
 @Slf4j
 final class BitbucketArtifactCredentials
     extends SimpleHttpArtifactCredentials<BitbucketArtifactAccount> implements ArtifactCredentials {
   @Getter private final String name;
-  @Getter private final List<String> types = Collections.singletonList("bitbucket/file");
+  @Getter private final ImmutableList<String> types = ImmutableList.of("bitbucket/file");
 
   BitbucketArtifactCredentials(BitbucketArtifactAccount account, OkHttpClient okHttpClient) {
     super(okHttpClient, account);

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/config/ArtifactCredentials.java

@@ -18,16 +18,23 @@
 package com.netflix.spinnaker.clouddriver.artifacts.config;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.List;
 import java.util.Optional;
 import org.apache.commons.lang3.NotImplementedException;
 
+@NonnullByDefault
 public interface ArtifactCredentials {
   String getName();
 
+  /**
+   * Returns the artifact types that are handled by these credentials.
+   *
+   * @return A list of artifact types, which should be immutable.
+   */
   List<String> getTypes();
 
   InputStream download(Artifact artifact) throws IOException;
@@ -57,6 +64,6 @@ default List<String> getArtifactVersions(String artifactName) {
   }
 
   default boolean handlesType(String type) {
-    return getTypes().stream().anyMatch(it -> it.equals(type));
+    return getTypes().contains(type);
   }
 }

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/config/BasicAuth.java

@@ -17,23 +17,24 @@
 package com.netflix.spinnaker.clouddriver.artifacts.config;
 
 import com.netflix.spinnaker.clouddriver.artifacts.CredentialReader;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import java.util.Optional;
 import org.apache.commons.codec.binary.Base64;
-import org.apache.commons.lang3.StringUtils;
 
+@NonnullByDefault
 public interface BasicAuth {
-  String getUsername();
+  Optional<String> getUsername();
 
-  String getPassword();
+  Optional<String> getPassword();
 
-  String getUsernamePasswordFile();
+  Optional<String> getUsernamePasswordFile();
 
   default Optional<String> getBasicAuthHeader() {
     String usernamePassword = null;
-    if (StringUtils.isNotEmpty(getUsernamePasswordFile())) {
-      usernamePassword = CredentialReader.credentialsFromFile(getUsernamePasswordFile());
-    } else if (StringUtils.isNotEmpty(getUsername()) && StringUtils.isNotEmpty(getPassword())) {
-      usernamePassword = getUsername() + ":" + getPassword();
+    if (getUsernamePasswordFile().isPresent()) {
+      usernamePassword = CredentialReader.credentialsFromFile(getUsernamePasswordFile().get());
+    } else if (getUsername().isPresent() && getPassword().isPresent()) {
+      usernamePassword = getUsername().get() + ":" + getPassword().get();
     }
 
     return Optional.ofNullable(usernamePassword)

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/config/SimpleHttpArtifactCredentials.java

@@ -17,12 +17,14 @@
 package com.netflix.spinnaker.clouddriver.artifacts.config;
 
 import com.netflix.spinnaker.clouddriver.artifacts.exceptions.FailedDownloadException;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import com.squareup.okhttp.HttpUrl;
 import com.squareup.okhttp.OkHttpClient;
 import java.io.IOException;
 import java.io.InputStream;
 
+@NonnullByDefault
 public abstract class SimpleHttpArtifactCredentials<T extends ArtifactAccount>
     extends BaseHttpArtifactCredentials<T> {
   protected SimpleHttpArtifactCredentials(OkHttpClient okHttpClient, T account) {

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/config/TokenAuth.java

@@ -17,26 +17,24 @@
 package com.netflix.spinnaker.clouddriver.artifacts.config;
 
 import com.netflix.spinnaker.clouddriver.artifacts.CredentialReader;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import java.util.Optional;
-import org.apache.commons.lang3.StringUtils;
 
+@NonnullByDefault
 public interface TokenAuth {
-  String getToken();
+  Optional<String> getToken();
 
-  String getTokenFile();
+  Optional<String> getTokenFile();
 
   default Optional<String> getTokenAuthHeader() {
     return getTokenAsString().map(t -> "token " + t);
   }
 
   default Optional<String> getTokenAsString() {
-    String token = null;
-    if (StringUtils.isNotEmpty(getTokenFile())) {
-      token = CredentialReader.credentialsFromFile(getTokenFile());
-    } else if (StringUtils.isNotEmpty(getToken())) {
-      token = getToken();
+    Optional<String> result = getTokenFile().map(CredentialReader::credentialsFromFile);
+    if (result.isPresent()) {
+      return result;
     }
-
-    return Optional.ofNullable(token);
+    return getToken();
   }
 }

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/custom/CustomArtifactAccount.java

@@ -18,9 +18,11 @@
 package com.netflix.spinnaker.clouddriver.artifacts.custom;
 
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactAccount;
-import lombok.Data;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
+import lombok.Value;
 
-@Data
+@NonnullByDefault
+@Value
 final class CustomArtifactAccount implements ArtifactAccount {
-  private String name = "custom-artifact";
+  private final String name = "custom-artifact";
 }

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/custom/CustomArtifactCredentials.java

@@ -17,18 +17,19 @@
 
 package com.netflix.spinnaker.clouddriver.artifacts.custom;
 
+import com.google.common.collect.ImmutableList;
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactCredentials;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import java.io.InputStream;
-import java.util.Collections;
-import java.util.List;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 
+@NonnullByDefault
 @Slf4j
 final class CustomArtifactCredentials implements ArtifactCredentials {
   @Getter private final String name;
-  @Getter private final List<String> types = Collections.singletonList("custom/object");
+  @Getter private final ImmutableList<String> types = ImmutableList.of("custom/object");
 
   CustomArtifactCredentials(CustomArtifactAccount account) {
     name = account.getName();

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/docker/DockerArtifactAccount.java

@@ -18,7 +18,11 @@
 package com.netflix.spinnaker.clouddriver.artifacts.docker;
 
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactAccount;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
+import lombok.Value;
 
+@NonnullByDefault
+@Value
 final class DockerArtifactAccount implements ArtifactAccount {
   @Override
   public String getName() {

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/docker/DockerArtifactCredentials.java

@@ -17,21 +17,22 @@
 
 package com.netflix.spinnaker.clouddriver.artifacts.docker;
 
+import com.google.common.collect.ImmutableList;
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactCredentials;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import java.io.InputStream;
-import java.util.Collections;
-import java.util.List;
-import lombok.Data;
+import lombok.Value;
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
-@Data
+@NonnullByDefault
+@Value
 final class DockerArtifactCredentials implements ArtifactCredentials {
   public static final String TYPE = "docker/image";
 
   private final String name;
-  private final List<String> types = Collections.singletonList(TYPE);
+  private final ImmutableList<String> types = ImmutableList.of(TYPE);
 
   DockerArtifactCredentials(DockerArtifactAccount account) {
     this.name = account.getName();

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/embedded/EmbeddedArtifactAccount.java

@@ -18,9 +18,11 @@
 package com.netflix.spinnaker.clouddriver.artifacts.embedded;
 
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactAccount;
-import lombok.Data;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
+import lombok.Value;
 
-@Data
+@NonnullByDefault
+@Value
 final class EmbeddedArtifactAccount implements ArtifactAccount {
-  private String name = "embedded-artifact";
+  private final String name = "embedded-artifact";
 }

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/embedded/EmbeddedArtifactCredentials.java

@@ -18,21 +18,22 @@
 package com.netflix.spinnaker.clouddriver.artifacts.embedded;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.google.common.collect.ImmutableList;
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactCredentials;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.util.Base64;
-import java.util.Collections;
-import java.util.List;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang3.NotImplementedException;
 
+@NonnullByDefault
 @Slf4j
 final class EmbeddedArtifactCredentials implements ArtifactCredentials {
   @Getter private final String name;
-  @Getter private final List<String> types = Collections.singletonList("embedded/base64");
+  @Getter private final ImmutableList<String> types = ImmutableList.of("embedded/base64");
 
   @JsonIgnore private final Base64.Decoder base64Decoder;
 

clouddriver-artifacts/src/main/java/com/netflix/spinnaker/clouddriver/artifacts/front50/Front50ArtifactCredentials.java

@@ -18,8 +18,11 @@
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
 import com.netflix.spinnaker.clouddriver.artifacts.config.ArtifactCredentials;
 import com.netflix.spinnaker.clouddriver.core.services.Front50Service;
+import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
@@ -32,15 +35,15 @@
 import lombok.Data;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.lang3.StringUtils;
 
+@NonnullByDefault
 @Slf4j
 final class Front50ArtifactCredentials implements ArtifactCredentials {
   private static final String ACCOUNT_NAME = "front50ArtifactCredentials";
   private static final String URL_PREFIX = "spinnaker://";
 
   @Getter private final String name = ACCOUNT_NAME;
-  @Getter private final List<String> types = Collections.singletonList("front50/pipelineTemplate");
+  @Getter private final ImmutableList<String> types = ImmutableList.of("front50/pipelineTemplate");
 
   @JsonIgnore private final Front50Service front50Service;
   @JsonIgnore private final ObjectMapper objectMapper;
@@ -52,8 +55,8 @@ final class Front50ArtifactCredentials implements ArtifactCredentials {
 
   @Override
   public InputStream download(Artifact artifact) throws IOException {
-    String reference = artifact.getReference();
-    if (StringUtils.isEmpty(reference) || !reference.startsWith(URL_PREFIX)) {
+    String reference = Strings.nullToEmpty(artifact.getReference());
+    if (!reference.startsWith(URL_PREFIX)) {
       throw new IllegalArgumentException(
           String.format(
               "'front50/pipelineTemplate' artifacts must be specified with a "