Updates description/updatedBy/createdBy fields to empty from null.

There was unhealthy mixing and matching of null vs. empty strings for these non-critical fields. SanitizedSecret#fromSecretSeriesAndContent in particular was causing a nasty CLI bug where it was the singular place createdBy and updatedBy were coexerced to null instead of empty string. Some places used Optional<String> for description, but it really isn't necessary. Constructors now accept null but will only store and emit empty string for these fields. Also converts SecretSeries to using AutoValue. It needed to be changed and moving it to an AutoValue object removed a bunch of code.
square · Jun 8, 2015 · 2bdd2a0 · 2bdd2a0
1 parent 37abd25
commit 2bdd2a0
Show file tree

Hide file tree

Showing 20 changed files with 136 additions and 208 deletions.
diff --git a/api/src/main/java/keywhiz/api/SecretDetailResponse.java b/api/src/main/java/keywhiz/api/SecretDetailResponse.java
@@ -92,7 +92,7 @@ public static SecretDetailResponse fromSecret(Secret secret, ImmutableList<Group
       ImmutableList<Client> clients) {
     return new SecretDetailResponse(secret.getId(),
         secret.getName(),
-        secret.getDescription().orElse(""),
+        secret.getDescription(),
         secret.getCreatedAt(),
         secret.getCreatedBy(),
         secret.getUpdatedAt(),

diff --git a/api/src/main/java/keywhiz/api/model/Client.java b/api/src/main/java/keywhiz/api/model/Client.java
@@ -20,8 +20,10 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Objects;
 import java.time.OffsetDateTime;
+import javax.annotation.Nullable;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Strings.nullToEmpty;
 
 /** Clients table entry for a client-cert authenticated client. */
 public class Client {
@@ -56,20 +58,20 @@ public class Client {
 
   public Client(@JsonProperty("id") long id,
       @JsonProperty("name") String name,
-      @JsonProperty("description") String description,
+      @JsonProperty("description") @Nullable String description,
       @JsonProperty("createdAt") OffsetDateTime createdAt,
-      @JsonProperty("createdBy") String createdBy,
+      @JsonProperty("createdBy") @Nullable String createdBy,
       @JsonProperty("updatedAt") OffsetDateTime updatedAt,
-      @JsonProperty("updatedBy") String updatedBy,
+      @JsonProperty("updatedBy") @Nullable String updatedBy,
       @JsonProperty("enabled") boolean enabled,
       @JsonProperty("automationAllowed") boolean automationAllowed) {
     this.id = id;
     this.name = checkNotNull(name);
-    this.description = description;
+    this.description = nullToEmpty(description);
     this.createdAt = createdAt;
-    this.createdBy = createdBy;
+    this.createdBy = nullToEmpty(createdBy);
     this.updatedAt = updatedAt;
-    this.updatedBy = updatedBy;
+    this.updatedBy = nullToEmpty(updatedBy);
     this.enabled = enabled;
     this.automationAllowed = automationAllowed;
   }

diff --git a/api/src/main/java/keywhiz/api/model/Group.java b/api/src/main/java/keywhiz/api/model/Group.java
@@ -19,8 +19,10 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Objects;
 import java.time.OffsetDateTime;
+import javax.annotation.Nullable;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Strings.nullToEmpty;
 
 /**
  * Groups entry for collecting a set of {@link Client}s, via membership, and/or {@link Secret}s, via
@@ -50,18 +52,18 @@ public class Group {
 
   public Group(@JsonProperty("id") long id,
       @JsonProperty("name") String name,
-      @JsonProperty("description") String description,
+      @JsonProperty("description") @Nullable String description,
       @JsonProperty("createdAt") OffsetDateTime createdAt,
-      @JsonProperty("createdBy") String createdBy,
+      @JsonProperty("createdBy") @Nullable String createdBy,
       @JsonProperty("updatedAt") OffsetDateTime updatedAt,
-      @JsonProperty("updatedBy") String updatedBy) {
+      @JsonProperty("updatedBy") @Nullable String updatedBy) {
     this.id = id;
     this.name = checkNotNull(name);
-    this.description = description;
+    this.description = nullToEmpty(description);
     this.createdAt = createdAt;
-    this.createdBy = createdBy;
+    this.createdBy = nullToEmpty(createdBy);
     this.updatedAt = updatedAt;
-    this.updatedBy = updatedBy;
+    this.updatedBy = nullToEmpty(updatedBy);
   }
 
   public long getId() {

diff --git a/api/src/main/java/keywhiz/api/model/SanitizedSecret.java b/api/src/main/java/keywhiz/api/model/SanitizedSecret.java
@@ -28,14 +28,15 @@
 import javax.annotation.Nullable;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Strings.nullToEmpty;
 
 /**
  * {@link Secret} object, but without the secret content.
  */
-// TODO(justin): would love to backfill and make createdBy and updatedBy no longer nullable.
 @AutoValue
 public abstract class SanitizedSecret {
-  @JsonCreator public static SanitizedSecret of(@JsonProperty("id") long id,
+  @JsonCreator public static SanitizedSecret of(
+      @JsonProperty("id") long id,
       @JsonProperty("name") String name,
       @JsonProperty("version") String version,
       @JsonProperty("description") @Nullable String description,
@@ -50,25 +51,26 @@ public abstract class SanitizedSecret {
         (metadata == null) ? ImmutableMap.of() : ImmutableMap.copyOf(metadata);
     ImmutableMap<String, String> genOptions =
         (generationOptions == null) ? ImmutableMap.of() : ImmutableMap.copyOf(generationOptions);
-    return new AutoValue_SanitizedSecret(id, name, version, Optional.ofNullable(description),
-        createdAt, createdBy, updatedAt, updatedBy, meta, Optional.ofNullable(type), genOptions);
+    return new AutoValue_SanitizedSecret(id, name, version, nullToEmpty(description), createdAt,
+        nullToEmpty(createdBy), updatedAt, nullToEmpty(updatedBy), meta, Optional.ofNullable(type),
+        genOptions);
   }
 
   public static SanitizedSecret fromSecretSeriesAndContent(SecretSeriesAndContent seriesAndContent) {
     SecretSeries series = seriesAndContent.series();
     SecretContent content = seriesAndContent.content();
     return SanitizedSecret.of(
-        series.getId(),
-        series.getName(),
+        series.id(),
+        series.name(),
         content.version().orElse(""),
-        series.getDescription().orElse(null),
+        series.description(),
         content.createdAt(),
         content.createdBy(),
         content.updatedAt(),
         content.updatedBy(),
         content.metadata(),
-        series.getType().orElse(null),
-        series.getGenerationOptions());
+        series.type().orElse(null),
+        series.generationOptions());
   }
 
   /**
@@ -83,7 +85,7 @@ public static SanitizedSecret fromSecret(Secret secret) {
         secret.getId(),
         secret.getName(),
         secret.getVersion(),
-        secret.getDescription().orElse(""),
+        secret.getDescription(),
         secret.getCreatedAt(),
         secret.getCreatedBy(),
         secret.getUpdatedAt(),
@@ -108,11 +110,11 @@ public static List<SanitizedSecret> fromSecrets(List<Secret> secrets) {
   @JsonProperty public abstract long id();
   @JsonProperty public abstract String name();
   @JsonProperty public abstract String version();
-  @JsonProperty public abstract Optional<String> description();
+  @JsonProperty public abstract String description();
   @JsonProperty public abstract OffsetDateTime createdAt();
-  @JsonProperty @Nullable public abstract String createdBy();
+  @JsonProperty public abstract String createdBy();
   @JsonProperty public abstract OffsetDateTime updatedAt();
-  @JsonProperty @Nullable public abstract String updatedBy();
+  @JsonProperty public abstract String updatedBy();
   @JsonProperty public abstract ImmutableMap<String, String> metadata();
   @JsonProperty public abstract Optional<String> type();
   @JsonProperty public abstract ImmutableMap<String, String> generationOptions();

diff --git a/api/src/main/java/keywhiz/api/model/Secret.java b/api/src/main/java/keywhiz/api/model/Secret.java
@@ -17,7 +17,6 @@
 
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Objects;
-import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import java.text.ParseException;
 import java.time.OffsetDateTime;
@@ -27,6 +26,7 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Strings.nullToEmpty;
 import static java.util.regex.Pattern.quote;
 
 /**
@@ -79,13 +79,13 @@ public Secret(long id,
     checkArgument(!name.isEmpty());
     this.id = id;
     this.name = name;
-    this.version = Strings.nullToEmpty(version);
-    this.description = description;
+    this.version = nullToEmpty(version);
+    this.description = nullToEmpty(description);
     this.secret = checkNotNull(secret); /* Expected empty when sanitized. */
     this.createdAt = checkNotNull(createdAt);
-    this.createdBy = Strings.nullToEmpty(createdBy);
+    this.createdBy = nullToEmpty(createdBy);
     this.updatedAt = checkNotNull(updatedAt);
-    this.updatedBy = Strings.nullToEmpty(updatedBy);
+    this.updatedBy = nullToEmpty(updatedBy);
     this.metadata = (metadata == null) ?
         ImmutableMap.of() : ImmutableMap.copyOf(metadata);
     this.type = type;
@@ -114,8 +114,8 @@ public String getVersion() {
     return version;
   }
 
-  public Optional<String> getDescription() {
-    return Optional.ofNullable(description);
+  public String getDescription() {
+    return description;
   }
 
   public String getSecret() {

diff --git a/api/src/main/java/keywhiz/api/model/SecretContent.java b/api/src/main/java/keywhiz/api/model/SecretContent.java
@@ -24,6 +24,8 @@
 import java.util.Optional;
 import javax.annotation.Nullable;
 
+import static com.google.common.base.Strings.nullToEmpty;
+
 /**
  * Maps to entity from secrets_content table. Contains content and related information on a specific
  * version of a secret. Many-to-one mapping with a {@link SecretSeries}.
@@ -34,17 +36,18 @@ public static SecretContent of(long id, long secretSeriesId, String encryptedCon
       @Nullable String version, OffsetDateTime createdAt, @Nullable String createdBy,
       OffsetDateTime updatedAt, @Nullable String updatedBy, ImmutableMap<String, String> metadata) {
     return new AutoValue_SecretContent(id, secretSeriesId, encryptedContent,
-        Optional.ofNullable(version), createdAt, createdBy, updatedAt, updatedBy, metadata);
+        Optional.ofNullable(version), createdAt, nullToEmpty(createdBy), updatedAt,
+        nullToEmpty(updatedBy), metadata);
   }
 
   public abstract long id();
   public abstract long secretSeriesId();
   public abstract String encryptedContent();
   public abstract Optional<String> version();
   public abstract OffsetDateTime createdAt();
-  @Nullable public abstract String createdBy();
+  public abstract String createdBy();
   public abstract OffsetDateTime updatedAt();
-  @Nullable public abstract String updatedBy();
+  public abstract String updatedBy();
   @JsonAnyGetter public abstract  ImmutableMap<String, String> metadata();
 
   @Override public String toString() {

diff --git a/api/src/main/java/keywhiz/api/model/SecretSeries.java b/api/src/main/java/keywhiz/api/model/SecretSeries.java
@@ -16,125 +16,46 @@
 
 package keywhiz.api.model;
 
-import com.google.common.base.MoreObjects;
-import com.google.common.base.Objects;
+import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableMap;
 import java.time.OffsetDateTime;
 import java.util.Map;
 import java.util.Optional;
 import javax.annotation.Nullable;
 
+import static com.google.common.base.Strings.nullToEmpty;
+
 /**
  * Maps to entity from secrets table. A secret may have many versions, each with different content,
  * but the versions share common information in a SecretSeries, such as name, description, and
  * metadata. In particular, access control is granted to a SecretSeries and not a specific version
  * in that series. New secret versions in the series will "inherit" the same group associations.
  * One-to-many mapping to {@link SecretContent}s.
  */
-public class SecretSeries {
-  private final long id;
-  private final String name;
-  private final String description;
-  private final OffsetDateTime createdAt;
-  private final String createdBy;
-  private final OffsetDateTime updatedAt;
-  private final String updatedBy;
-  private final String type;
-  private final ImmutableMap<String, String> generationOptions;
-
-  public SecretSeries(long id,
+@AutoValue
+public abstract class SecretSeries {
+  public static SecretSeries of(
+      long id,
       String name,
       @Nullable String description,
       OffsetDateTime createdAt,
-      String createdBy,
+      @Nullable String createdBy,
       OffsetDateTime updatedAt,
-      String updatedBy,
+      @Nullable String updatedBy,
       @Nullable String type,
       @Nullable Map<String, String> generationOptions) {
-    this.id = id;
-    this.name = name;
-    this.description = description;
-    this.createdAt = createdAt;
-    this.createdBy = createdBy;
-    this.updatedAt = updatedAt;
-    this.updatedBy = updatedBy;
-    this.type = type;
-    this.generationOptions = (generationOptions == null) ?
+    ImmutableMap<String, String> options = (generationOptions == null) ?
         ImmutableMap.of() : ImmutableMap.copyOf(generationOptions);
-  }
-
-  public long getId() {
-    return id;
-  }
-
-  public String getName() {
-    return name;
-  }
-
-  public Optional<String> getDescription() {
-    return Optional.ofNullable(description);
-  }
-
-  public OffsetDateTime getCreatedAt() {
-    return createdAt;
-  }
-
-  public String getCreatedBy() {
-    return createdBy;
-  }
-
-  public OffsetDateTime getUpdatedAt() {
-    return updatedAt;
-  }
-
-  public String getUpdatedBy() {
-    return updatedBy;
-  }
-
-  public Optional<String> getType() {
-    return Optional.ofNullable(type);
-  }
-
-  public ImmutableMap<String, String> getGenerationOptions() {
-    return generationOptions;
-  }
-
-  @Override public int hashCode() {
-    return Objects.hashCode(id, name, description, createdAt, createdBy, updatedAt, updatedBy,
-        type, generationOptions);
-  }
-
-  @Override public boolean equals(Object o) {
-    if (this == o) return true;
-    if (o instanceof SecretSeries) {
-      SecretSeries that = (SecretSeries) o;
-      if (this.id == that.id &&
-          Objects.equal(this.name, that.name) &&
-          Objects.equal(this.description, that.description) &&
-          Objects.equal(this.createdAt, that.createdAt) &&
-          Objects.equal(this.createdBy, that.createdBy) &&
-          Objects.equal(this.updatedAt, that.updatedAt) &&
-          Objects.equal(this.updatedBy, that.updatedBy) &&
-          Objects.equal(this.type, that.type) &&
-          Objects.equal(this.generationOptions, that.generationOptions)) {
-        return true;
-      }
-    }
-    return false;
-  }
-
-  @Override public String toString() {
-    return MoreObjects.toStringHelper(this)
-        .add("id", id)
-        .add("name", name)
-        .add("description", description)
-        .add("createdAt", createdAt)
-        .add("createdBy", createdBy)
-        .add("updatedAt", updatedAt)
-        .add("updatedBy", updatedBy)
-        .add("type", type)
-        .add("generationOptions", generationOptions)
-        .omitNullValues()
-        .toString();
-  }
+    return new AutoValue_SecretSeries(id, name, nullToEmpty(description), createdAt, nullToEmpty(createdBy), updatedAt, nullToEmpty(updatedBy), Optional.ofNullable(type), options);
+  }
+
+  public abstract long id();
+  public abstract String name();
+  public abstract String description();
+  public abstract OffsetDateTime createdAt();
+  public abstract String createdBy();
+  public abstract OffsetDateTime updatedAt();
+  public abstract String updatedBy();
+  public abstract Optional<String> type();
+  public abstract ImmutableMap<String, String> generationOptions();
 }