Avoid doing string formatting when calling checkArgument. (#1394)

9 files changed, 151 insertions, 35 deletions

diff --git a/api/src/main/java/io/opencensus/internal/Utils.java b/api/src/main/java/io/opencensus/internal/Utils.java
index a2c09c82..df5c9840 100644
--- a/api/src/main/java/io/opencensus/internal/Utils.java
+++ b/api/src/main/java/io/opencensus/internal/Utils.java
@@ -17,7 +17,6 @@
 package io.opencensus.internal;
 
 import java.util.List;
-import javax.annotation.Nullable;
 
 /*>>>
 import org.checkerframework.checker.nullness.qual.NonNull;
@@ -33,11 +32,38 @@ public final class Utils {
    * {@code Preconditions.checkArgument(boolean, Object)} from Guava.
    *
    * @param isValid whether the argument check passed.
-   * @param message the message to use for the exception.
+   * @param errorMessage the message to use for the exception. Will be converted to a string using
+   *     {@link String#valueOf(Object)}.
    */
-  public static void checkArgument(boolean isValid, String message) {
+  public static void checkArgument(
+      boolean isValid, @javax.annotation.Nullable Object errorMessage) {
     if (!isValid) {
-      throw new IllegalArgumentException(message);
+      throw new IllegalArgumentException(String.valueOf(errorMessage));
+    }
+  }
+
+  /**
+   * Throws an {@link IllegalArgumentException} if the argument is false. This method is similar to
+   * {@code Preconditions.checkArgument(boolean, Object)} from Guava.
+   *
+   * @param expression a boolean expression
+   * @param errorMessageTemplate a template for the exception message should the check fail. The
+   *     message is formed by replacing each {@code %s} placeholder in the template with an
+   *     argument. These are matched by position - the first {@code %s} gets {@code
+   *     errorMessageArgs[0]}, etc. Unmatched arguments will be appended to the formatted message in
+   *     square braces. Unmatched placeholders will be left as-is.
+   * @param errorMessageArgs the arguments to be substituted into the message template. Arguments
+   *     are converted to strings using {@link String#valueOf(Object)}.
+   * @throws IllegalArgumentException if {@code expression} is false
+   * @throws NullPointerException if the check fails and either {@code errorMessageTemplate} or
+   *     {@code errorMessageArgs} is null (don't let this happen)
+   */
+  public static void checkArgument(
+      boolean expression,
+      String errorMessageTemplate,
+      @javax.annotation.Nullable Object... errorMessageArgs) {
+    if (!expression) {
+      throw new IllegalArgumentException(format(errorMessageTemplate, errorMessageArgs));
     }
   }
 
@@ -46,11 +72,12 @@ public final class Utils {
    * {@code Preconditions.checkState(boolean, Object)} from Guava.
    *
    * @param isValid whether the state check passed.
-   * @param message the message to use for the exception.
+   * @param errorMessage the message to use for the exception. Will be converted to a string using
+   *     {@link String#valueOf(Object)}.
    */
-  public static void checkState(boolean isValid, String message) {
+  public static void checkState(boolean isValid, @javax.annotation.Nullable Object errorMessage) {
     if (!isValid) {
-      throw new IllegalStateException(message);
+      throw new IllegalStateException(String.valueOf(errorMessage));
     }
   }
 
@@ -77,12 +104,14 @@ public final class Utils {
    * Preconditions.checkNotNull(Object, Object)} from Guava.
    *
    * @param arg the argument to check for null.
-   * @param message the message to use for the exception.
+   * @param errorMessage the message to use for the exception. Will be converted to a string using
+   *     {@link String#valueOf(Object)}.
    * @return the argument, if it passes the null check.
    */
-  public static <T /*>>> extends @NonNull Object*/> T checkNotNull(T arg, String message) {
+  public static <T /*>>> extends @NonNull Object*/> T checkNotNull(
+      T arg, @javax.annotation.Nullable Object errorMessage) {
     if (arg == null) {
-      throw new NullPointerException(message);
+      throw new NullPointerException(String.valueOf(errorMessage));
     }
     return arg;
   }
@@ -91,13 +120,14 @@ public final class Utils {
    * Throws a {@link NullPointerException} if any of the list elements is null.
    *
    * @param list the argument list to check for null.
-   * @param message the message to use for the exception.
+   * @param errorMessage the message to use for the exception. Will be converted to a string using
+   *     {@link String#valueOf(Object)}.
    */
   public static <T /*>>> extends @NonNull Object*/> void checkListElementNotNull(
-      List<T> list, String message) {
+      List<T> list, @javax.annotation.Nullable Object errorMessage) {
     for (T element : list) {
       if (element == null) {
-        throw new NullPointerException(message);
+        throw new NullPointerException(String.valueOf(errorMessage));
       }
     }
   }
@@ -106,7 +136,56 @@ public final class Utils {
    * Compares two Objects for equality. This functionality is provided by {@code
    * Objects.equal(Object, Object)} in Java 7.
    */
-  public static boolean equalsObjects(@Nullable Object x, @Nullable Object y) {
+  public static boolean equalsObjects(
+      @javax.annotation.Nullable Object x, @javax.annotation.Nullable Object y) {
     return x == null ? y == null : x.equals(y);
   }
+
+  /**
+   * Substitutes each {@code %s} in {@code template} with an argument. These are matched by
+   * position: the first {@code %s} gets {@code args[0]}, etc. If there are more arguments than
+   * placeholders, the unmatched arguments will be appended to the end of the formatted message in
+   * square braces.
+   *
+   * <p>Copied from {@code Preconditions.format(String, Object...)} from Guava
+   *
+   * @param template a non-null string containing 0 or more {@code %s} placeholders.
+   * @param args the arguments to be substituted into the message template. Arguments are converted
+   *     to strings using {@link String#valueOf(Object)}. Arguments can be null.
+   */
+  // Note that this is somewhat-improperly used from Verify.java as well.
+  private static String format(String template, @javax.annotation.Nullable Object... args) {
+    // If no arguments return the template.
+    if (args == null) {
+      return template;
+    }
+
+    // start substituting the arguments into the '%s' placeholders
+    StringBuilder builder = new StringBuilder(template.length() + 16 * args.length);
+    int templateStart = 0;
+    int i = 0;
+    while (i < args.length) {
+      int placeholderStart = template.indexOf("%s", templateStart);
+      if (placeholderStart == -1) {
+        break;
+      }
+      builder.append(template, templateStart, placeholderStart);
+      builder.append(args[i++]);
+      templateStart = placeholderStart + 2;
+    }
+    builder.append(template, templateStart, template.length());
+
+    // if we run out of placeholders, append the extra args in square braces
+    if (i < args.length) {
+      builder.append(" [");
+      builder.append(args[i++]);
+      while (i < args.length) {
+        builder.append(", ");
+        builder.append(args[i++]);
+      }
+      builder.append(']');
+    }
+
+    return builder.toString();
+  }
 }
diff --git a/api/src/main/java/io/opencensus/stats/Measure.java b/api/src/main/java/io/opencensus/stats/Measure.java
index 4b47cd79..2de7fd70 100644
--- a/api/src/main/java/io/opencensus/stats/Measure.java
+++ b/api/src/main/java/io/opencensus/stats/Measure.java
@@ -30,8 +30,11 @@ import javax.annotation.concurrent.Immutable;
  */
 @Immutable
 public abstract class Measure {
-
   @DefaultVisibilityForTesting static final int NAME_MAX_LENGTH = 255;
+  private static final String ERROR_MESSAGE_INVALID_NAME =
+      "Name should be a ASCII string with a length no greater than "
+          + NAME_MAX_LENGTH
+          + " characters.";
 
   /**
    * Applies the given match function to the underlying data type.
@@ -105,9 +108,7 @@ public abstract class Measure {
     public static MeasureDouble create(String name, String description, String unit) {
       Utils.checkArgument(
           StringUtils.isPrintableString(name) && name.length() <= NAME_MAX_LENGTH,
-          "Name should be a ASCII string with a length no greater than "
-              + NAME_MAX_LENGTH
-              + " characters.");
+          ERROR_MESSAGE_INVALID_NAME);
       return new AutoValue_Measure_MeasureDouble(name, description, unit);
     }
 
@@ -152,9 +153,7 @@ public abstract class Measure {
     public static MeasureLong create(String name, String description, String unit) {
       Utils.checkArgument(
           StringUtils.isPrintableString(name) && name.length() <= NAME_MAX_LENGTH,
-          "Name should be a ASCII string with a length no greater than "
-              + NAME_MAX_LENGTH
-              + " characters.");
+          ERROR_MESSAGE_INVALID_NAME);
       return new AutoValue_Measure_MeasureLong(name, description, unit);
     }
 
diff --git a/api/src/main/java/io/opencensus/tags/TagKey.java b/api/src/main/java/io/opencensus/tags/TagKey.java
index 615c2d46..ca4582bd 100644
--- a/api/src/main/java/io/opencensus/tags/TagKey.java
+++ b/api/src/main/java/io/opencensus/tags/TagKey.java
@@ -60,7 +60,7 @@ public abstract class TagKey {
    * @since 0.8
    */
   public static TagKey create(String name) {
-    Utils.checkArgument(isValid(name), "Invalid TagKey name: " + name);
+    Utils.checkArgument(isValid(name), "Invalid TagKey name: %s", name);
     return new AutoValue_TagKey(name);
   }
 
diff --git a/api/src/main/java/io/opencensus/tags/TagValue.java b/api/src/main/java/io/opencensus/tags/TagValue.java
index 5c1019ff..9111ca28 100644
--- a/api/src/main/java/io/opencensus/tags/TagValue.java
+++ b/api/src/main/java/io/opencensus/tags/TagValue.java
@@ -55,7 +55,7 @@ public abstract class TagValue {
    * @since 0.8
    */
   public static TagValue create(String value) {
-    Utils.checkArgument(isValid(value), "Invalid TagValue: " + value);
+    Utils.checkArgument(isValid(value), "Invalid TagValue: %s", value);
     return new AutoValue_TagValue(value);
   }
 
diff --git a/api/src/main/java/io/opencensus/trace/SpanId.java b/api/src/main/java/io/opencensus/trace/SpanId.java
index 1556bde0..06ff0458 100644
--- a/api/src/main/java/io/opencensus/trace/SpanId.java
+++ b/api/src/main/java/io/opencensus/trace/SpanId.java
@@ -38,6 +38,8 @@ public final class SpanId implements Comparable<SpanId> {
    */
   public static final int SIZE = 8;
 
+  private static final int HEX_SIZE = 2 * SIZE;
+
   /**
    * The invalid {@code SpanId}. All bytes are 0.
    *
@@ -70,8 +72,7 @@ public final class SpanId implements Comparable<SpanId> {
   public static SpanId fromBytes(byte[] buffer) {
     Utils.checkNotNull(buffer, "buffer");
     Utils.checkArgument(
-        buffer.length == SIZE,
-        String.format("Invalid size: expected %s, got %s", SIZE, buffer.length));
+        buffer.length == SIZE, "Invalid size: expected %s, got %s", SIZE, buffer.length);
     byte[] bytesCopied = Arrays.copyOf(buffer, SIZE);
     return new SpanId(bytesCopied);
   }
@@ -107,8 +108,7 @@ public final class SpanId implements Comparable<SpanId> {
    */
   public static SpanId fromLowerBase16(CharSequence src) {
     Utils.checkArgument(
-        src.length() == 2 * SIZE,
-        String.format("Invalid size: expected %s, got %s", 2 * SIZE, src.length()));
+        src.length() == HEX_SIZE, "Invalid size: expected %s, got %s", HEX_SIZE, src.length());
     byte[] bytes = BaseEncoding.base16().lowerCase().decode(src);
     return new SpanId(bytes);
   }
diff --git a/api/src/main/java/io/opencensus/trace/TraceId.java b/api/src/main/java/io/opencensus/trace/TraceId.java
index a02a4e05..c7aa48f0 100644
--- a/api/src/main/java/io/opencensus/trace/TraceId.java
+++ b/api/src/main/java/io/opencensus/trace/TraceId.java
@@ -39,6 +39,8 @@ public final class TraceId implements Comparable<TraceId> {
    */
   public static final int SIZE = 16;
 
+  private static final int HEX_SIZE = 32;
+
   /**
    * The invalid {@code TraceId}. All bytes are '\0'.
    *
@@ -71,8 +73,7 @@ public final class TraceId implements Comparable<TraceId> {
   public static TraceId fromBytes(byte[] buffer) {
     Utils.checkNotNull(buffer, "buffer");
     Utils.checkArgument(
-        buffer.length == SIZE,
-        String.format("Invalid size: expected %s, got %s", SIZE, buffer.length));
+        buffer.length == SIZE, "Invalid size: expected %s, got %s", SIZE, buffer.length);
     byte[] bytesCopied = Arrays.copyOf(buffer, SIZE);
     return new TraceId(bytesCopied);
   }
@@ -108,8 +109,7 @@ public final class TraceId implements Comparable<TraceId> {
    */
   public static TraceId fromLowerBase16(CharSequence src) {
     Utils.checkArgument(
-        src.length() == 2 * SIZE,
-        String.format("Invalid size: expected %s, got %s", 2 * SIZE, src.length()));
+        src.length() == HEX_SIZE, "Invalid size: expected %s, got %s", HEX_SIZE, src.length());
     byte[] bytes = BaseEncoding.base16().lowerCase().decode(src);
     return new TraceId(bytes);
   }
diff --git a/api/src/main/java/io/opencensus/trace/TraceOptions.java b/api/src/main/java/io/opencensus/trace/TraceOptions.java
index 86379f1e..218f4dab 100644
--- a/api/src/main/java/io/opencensus/trace/TraceOptions.java
+++ b/api/src/main/java/io/opencensus/trace/TraceOptions.java
@@ -78,8 +78,7 @@ public final class TraceOptions {
   public static TraceOptions fromBytes(byte[] buffer) {
     Utils.checkNotNull(buffer, "buffer");
     Utils.checkArgument(
-        buffer.length == SIZE,
-        String.format("Invalid size: expected %s, got %s", SIZE, buffer.length));
+        buffer.length == SIZE, "Invalid size: expected %s, got %s", SIZE, buffer.length);
     return fromByte(buffer[0]);
   }
 
diff --git a/api/src/main/java/io/opencensus/trace/Tracestate.java b/api/src/main/java/io/opencensus/trace/Tracestate.java
index f88d3bd3..dae587c8 100644
--- a/api/src/main/java/io/opencensus/trace/Tracestate.java
+++ b/api/src/main/java/io/opencensus/trace/Tracestate.java
@@ -201,8 +201,8 @@ public abstract class Tracestate {
     public static Entry create(String key, String value) {
       Utils.checkNotNull(key, "key");
       Utils.checkNotNull(value, "value");
-      Utils.checkArgument(validateKey(key), "Invalid key " + key);
-      Utils.checkArgument(validateValue(value), "Invalid value " + value);
+      Utils.checkArgument(validateKey(key), "Invalid key %s", key);
+      Utils.checkArgument(validateValue(value), "Invalid value %s", value);
       return new AutoValue_Tracestate_Entry(key, value);
     }
 
diff --git a/api/src/test/java/io/opencensus/internal/UtilsTest.java b/api/src/test/java/io/opencensus/internal/UtilsTest.java
index 983264f6..608a8fe0 100644
--- a/api/src/test/java/io/opencensus/internal/UtilsTest.java
+++ b/api/src/test/java/io/opencensus/internal/UtilsTest.java
@@ -30,6 +30,10 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public final class UtilsTest {
   private static final String TEST_MESSAGE = "test message";
+  private static final String TEST_MESSAGE_TEMPLATE = "I ate %s eggs.";
+  private static final int TEST_MESSAGE_VALUE = 2;
+  private static final String FORMATED_SIMPLE_TEST_MESSAGE = "I ate 2 eggs.";
+  private static final String FORMATED_COMPLEX_TEST_MESSAGE = "I ate 2 eggs. [2]";
 
   @Rule public ExpectedException thrown = ExpectedException.none();
 
@@ -42,6 +46,27 @@ public final class UtilsTest {
   }
 
   @Test
+  public void checkArgument_NullErrorMessage() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("null");
+    Utils.checkArgument(false, null);
+  }
+
+  @Test
+  public void checkArgument_WithSimpleFormat() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage(FORMATED_SIMPLE_TEST_MESSAGE);
+    Utils.checkArgument(false, TEST_MESSAGE_TEMPLATE, TEST_MESSAGE_VALUE);
+  }
+
+  @Test
+  public void checkArgument_WithComplexFormat() {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage(FORMATED_COMPLEX_TEST_MESSAGE);
+    Utils.checkArgument(false, TEST_MESSAGE_TEMPLATE, TEST_MESSAGE_VALUE, TEST_MESSAGE_VALUE);
+  }
+
+  @Test
   public void checkState() {
     Utils.checkNotNull(true, TEST_MESSAGE);
     thrown.expect(IllegalStateException.class);
@@ -50,6 +75,13 @@ public final class UtilsTest {
   }
 
   @Test
+  public void checkState_NullErrorMessage() {
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("null");
+    Utils.checkState(false, null);
+  }
+
+  @Test
   public void checkNotNull() {
     Utils.checkNotNull(new Object(), TEST_MESSAGE);
     thrown.expect(NullPointerException.class);
@@ -58,6 +90,13 @@ public final class UtilsTest {
   }
 
   @Test
+  public void checkNotNull_NullErrorMessage() {
+    thrown.expect(NullPointerException.class);
+    thrown.expectMessage("null");
+    Utils.checkNotNull(null, null);
+  }
+
+  @Test
   public void checkIndex_Valid() {
     Utils.checkIndex(1, 2);
   }