Skip to content
This repository was archived by the owner on Dec 23, 2023. It is now read-only.

Commit eabc800

Browse files
author
Bogdan Drutu
authored
Avoid doing string formatting when calling checkArgument. (#1394)
1 parent 884015c commit eabc800

File tree

11 files changed

+163
-35
lines changed

11 files changed

+163
-35
lines changed

‎api/src/main/java/io/opencensus/internal/Utils.java

+93-14
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package io.opencensus.internal;
1818

1919
import java.util.List;
20-
import javax.annotation.Nullable;
2120

2221
/*>>>
2322
import org.checkerframework.checker.nullness.qual.NonNull;
@@ -33,11 +32,38 @@ private Utils() {}
3332
* {@code Preconditions.checkArgument(boolean, Object)} from Guava.
3433
*
3534
* @param isValid whether the argument check passed.
36-
* @param message the message to use for the exception.
35+
* @param errorMessage the message to use for the exception. Will be converted to a string using
36+
* {@link String#valueOf(Object)}.
3737
*/
38-
public static void checkArgument(boolean isValid, String message) {
38+
public static void checkArgument(
39+
boolean isValid, @javax.annotation.Nullable Object errorMessage) {
3940
if (!isValid) {
40-
throw new IllegalArgumentException(message);
41+
throw new IllegalArgumentException(String.valueOf(errorMessage));
42+
}
43+
}
44+
45+
/**
46+
* Throws an {@link IllegalArgumentException} if the argument is false. This method is similar to
47+
* {@code Preconditions.checkArgument(boolean, Object)} from Guava.
48+
*
49+
* @param expression a boolean expression
50+
* @param errorMessageTemplate a template for the exception message should the check fail. The
51+
* message is formed by replacing each {@code %s} placeholder in the template with an
52+
* argument. These are matched by position - the first {@code %s} gets {@code
53+
* errorMessageArgs[0]}, etc. Unmatched arguments will be appended to the formatted message in
54+
* square braces. Unmatched placeholders will be left as-is.
55+
* @param errorMessageArgs the arguments to be substituted into the message template. Arguments
56+
* are converted to strings using {@link String#valueOf(Object)}.
57+
* @throws IllegalArgumentException if {@code expression} is false
58+
* @throws NullPointerException if the check fails and either {@code errorMessageTemplate} or
59+
* {@code errorMessageArgs} is null (don't let this happen)
60+
*/
61+
public static void checkArgument(
62+
boolean expression,
63+
String errorMessageTemplate,
64+
@javax.annotation.Nullable Object... errorMessageArgs) {
65+
if (!expression) {
66+
throw new IllegalArgumentException(format(errorMessageTemplate, errorMessageArgs));
4167
}
4268
}
4369

@@ -46,11 +72,12 @@ public static void checkArgument(boolean isValid, String message) {
4672
* {@code Preconditions.checkState(boolean, Object)} from Guava.
4773
*
4874
* @param isValid whether the state check passed.
49-
* @param message the message to use for the exception.
75+
* @param errorMessage the message to use for the exception. Will be converted to a string using
76+
* {@link String#valueOf(Object)}.
5077
*/
51-
public static void checkState(boolean isValid, String message) {
78+
public static void checkState(boolean isValid, @javax.annotation.Nullable Object errorMessage) {
5279
if (!isValid) {
53-
throw new IllegalStateException(message);
80+
throw new IllegalStateException(String.valueOf(errorMessage));
5481
}
5582
}
5683

@@ -77,12 +104,14 @@ public static void checkIndex(int index, int size) {
77104
* Preconditions.checkNotNull(Object, Object)} from Guava.
78105
*
79106
* @param arg the argument to check for null.
80-
* @param message the message to use for the exception.
107+
* @param errorMessage the message to use for the exception. Will be converted to a string using
108+
* {@link String#valueOf(Object)}.
81109
* @return the argument, if it passes the null check.
82110
*/
83-
public static <T /*>>> extends @NonNull Object*/> T checkNotNull(T arg, String message) {
111+
public static <T /*>>> extends @NonNull Object*/> T checkNotNull(
112+
T arg, @javax.annotation.Nullable Object errorMessage) {
84113
if (arg == null) {
85-
throw new NullPointerException(message);
114+
throw new NullPointerException(String.valueOf(errorMessage));
86115
}
87116
return arg;
88117
}
@@ -91,13 +120,14 @@ public static void checkIndex(int index, int size) {
91120
* Throws a {@link NullPointerException} if any of the list elements is null.
92121
*
93122
* @param list the argument list to check for null.
94-
* @param message the message to use for the exception.
123+
* @param errorMessage the message to use for the exception. Will be converted to a string using
124+
* {@link String#valueOf(Object)}.
95125
*/
96126
public static <T /*>>> extends @NonNull Object*/> void checkListElementNotNull(
97-
List<T> list, String message) {
127+
List<T> list, @javax.annotation.Nullable Object errorMessage) {
98128
for (T element : list) {
99129
if (element == null) {
100-
throw new NullPointerException(message);
130+
throw new NullPointerException(String.valueOf(errorMessage));
101131
}
102132
}
103133
}
@@ -106,7 +136,56 @@ public static void checkIndex(int index, int size) {
106136
* Compares two Objects for equality. This functionality is provided by {@code
107137
* Objects.equal(Object, Object)} in Java 7.
108138
*/
109-
public static boolean equalsObjects(@Nullable Object x, @Nullable Object y) {
139+
public static boolean equalsObjects(
140+
@javax.annotation.Nullable Object x, @javax.annotation.Nullable Object y) {
110141
return x == null ? y == null : x.equals(y);
111142
}
143+
144+
/**
145+
* Substitutes each {@code %s} in {@code template} with an argument. These are matched by
146+
* position: the first {@code %s} gets {@code args[0]}, etc. If there are more arguments than
147+
* placeholders, the unmatched arguments will be appended to the end of the formatted message in
148+
* square braces.
149+
*
150+
* <p>Copied from {@code Preconditions.format(String, Object...)} from Guava
151+
*
152+
* @param template a non-null string containing 0 or more {@code %s} placeholders.
153+
* @param args the arguments to be substituted into the message template. Arguments are converted
154+
* to strings using {@link String#valueOf(Object)}. Arguments can be null.
155+
*/
156+
// Note that this is somewhat-improperly used from Verify.java as well.
157+
private static String format(String template, @javax.annotation.Nullable Object... args) {
158+
// If no arguments return the template.
159+
if (args == null) {
160+
return template;
161+
}
162+
163+
// start substituting the arguments into the '%s' placeholders
164+
StringBuilder builder = new StringBuilder(template.length() + 16 * args.length);
165+
int templateStart = 0;
166+
int i = 0;
167+
while (i < args.length) {
168+
int placeholderStart = template.indexOf("%s", templateStart);
169+
if (placeholderStart == -1) {
170+
break;
171+
}
172+
builder.append(template, templateStart, placeholderStart);
173+
builder.append(args[i++]);
174+
templateStart = placeholderStart + 2;
175+
}
176+
builder.append(template, templateStart, template.length());
177+
178+
// if we run out of placeholders, append the extra args in square braces
179+
if (i < args.length) {
180+
builder.append(" [");
181+
builder.append(args[i++]);
182+
while (i < args.length) {
183+
builder.append(", ");
184+
builder.append(args[i++]);
185+
}
186+
builder.append(']');
187+
}
188+
189+
return builder.toString();
190+
}
112191
}

‎api/src/main/java/io/opencensus/stats/Measure.java

+6-7
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,11 @@
3030
*/
3131
@Immutable
3232
public abstract class Measure {
33-
3433
@DefaultVisibilityForTesting static final int NAME_MAX_LENGTH = 255;
34+
private static final String ERROR_MESSAGE_INVALID_NAME =
35+
"Name should be a ASCII string with a length no greater than "
36+
+ NAME_MAX_LENGTH
37+
+ " characters.";
3538

3639
/**
3740
* Applies the given match function to the underlying data type.
@@ -105,9 +108,7 @@ public abstract static class MeasureDouble extends Measure {
105108
public static MeasureDouble create(String name, String description, String unit) {
106109
Utils.checkArgument(
107110
StringUtils.isPrintableString(name) && name.length() <= NAME_MAX_LENGTH,
108-
"Name should be a ASCII string with a length no greater than "
109-
+ NAME_MAX_LENGTH
110-
+ " characters.");
111+
ERROR_MESSAGE_INVALID_NAME);
111112
return new AutoValue_Measure_MeasureDouble(name, description, unit);
112113
}
113114

@@ -152,9 +153,7 @@ public abstract static class MeasureLong extends Measure {
152153
public static MeasureLong create(String name, String description, String unit) {
153154
Utils.checkArgument(
154155
StringUtils.isPrintableString(name) && name.length() <= NAME_MAX_LENGTH,
155-
"Name should be a ASCII string with a length no greater than "
156-
+ NAME_MAX_LENGTH
157-
+ " characters.");
156+
ERROR_MESSAGE_INVALID_NAME);
158157
return new AutoValue_Measure_MeasureLong(name, description, unit);
159158
}
160159

‎api/src/main/java/io/opencensus/tags/TagKey.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public abstract class TagKey {
6060
* @since 0.8
6161
*/
6262
public static TagKey create(String name) {
63-
Utils.checkArgument(isValid(name), "Invalid TagKey name: " + name);
63+
Utils.checkArgument(isValid(name), "Invalid TagKey name: %s", name);
6464
return new AutoValue_TagKey(name);
6565
}
6666

‎api/src/main/java/io/opencensus/tags/TagValue.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public abstract class TagValue {
5555
* @since 0.8
5656
*/
5757
public static TagValue create(String value) {
58-
Utils.checkArgument(isValid(value), "Invalid TagValue: " + value);
58+
Utils.checkArgument(isValid(value), "Invalid TagValue: %s", value);
5959
return new AutoValue_TagValue(value);
6060
}
6161

‎api/src/main/java/io/opencensus/trace/SpanId.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ public final class SpanId implements Comparable<SpanId> {
3838
*/
3939
public static final int SIZE = 8;
4040

41+
private static final int HEX_SIZE = 2 * SIZE;
42+
4143
/**
4244
* The invalid {@code SpanId}. All bytes are 0.
4345
*
@@ -70,8 +72,7 @@ private SpanId(byte[] bytes) {
7072
public static SpanId fromBytes(byte[] buffer) {
7173
Utils.checkNotNull(buffer, "buffer");
7274
Utils.checkArgument(
73-
buffer.length == SIZE,
74-
String.format("Invalid size: expected %s, got %s", SIZE, buffer.length));
75+
buffer.length == SIZE, "Invalid size: expected %s, got %s", SIZE, buffer.length);
7576
byte[] bytesCopied = Arrays.copyOf(buffer, SIZE);
7677
return new SpanId(bytesCopied);
7778
}
@@ -107,8 +108,7 @@ public static SpanId fromBytes(byte[] src, int srcOffset) {
107108
*/
108109
public static SpanId fromLowerBase16(CharSequence src) {
109110
Utils.checkArgument(
110-
src.length() == 2 * SIZE,
111-
String.format("Invalid size: expected %s, got %s", 2 * SIZE, src.length()));
111+
src.length() == HEX_SIZE, "Invalid size: expected %s, got %s", HEX_SIZE, src.length());
112112
byte[] bytes = BaseEncoding.base16().lowerCase().decode(src);
113113
return new SpanId(bytes);
114114
}

‎api/src/main/java/io/opencensus/trace/TraceId.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ public final class TraceId implements Comparable<TraceId> {
3939
*/
4040
public static final int SIZE = 16;
4141

42+
private static final int HEX_SIZE = 32;
43+
4244
/**
4345
* The invalid {@code TraceId}. All bytes are '\0'.
4446
*
@@ -71,8 +73,7 @@ private TraceId(byte[] bytes) {
7173
public static TraceId fromBytes(byte[] buffer) {
7274
Utils.checkNotNull(buffer, "buffer");
7375
Utils.checkArgument(
74-
buffer.length == SIZE,
75-
String.format("Invalid size: expected %s, got %s", SIZE, buffer.length));
76+
buffer.length == SIZE, "Invalid size: expected %s, got %s", SIZE, buffer.length);
7677
byte[] bytesCopied = Arrays.copyOf(buffer, SIZE);
7778
return new TraceId(bytesCopied);
7879
}
@@ -108,8 +109,7 @@ public static TraceId fromBytes(byte[] src, int srcOffset) {
108109
*/
109110
public static TraceId fromLowerBase16(CharSequence src) {
110111
Utils.checkArgument(
111-
src.length() == 2 * SIZE,
112-
String.format("Invalid size: expected %s, got %s", 2 * SIZE, src.length()));
112+
src.length() == HEX_SIZE, "Invalid size: expected %s, got %s", HEX_SIZE, src.length());
113113
byte[] bytes = BaseEncoding.base16().lowerCase().decode(src);
114114
return new TraceId(bytes);
115115
}

‎api/src/main/java/io/opencensus/trace/TraceOptions.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,7 @@ private TraceOptions(byte options) {
7878
public static TraceOptions fromBytes(byte[] buffer) {
7979
Utils.checkNotNull(buffer, "buffer");
8080
Utils.checkArgument(
81-
buffer.length == SIZE,
82-
String.format("Invalid size: expected %s, got %s", SIZE, buffer.length));
81+
buffer.length == SIZE, "Invalid size: expected %s, got %s", SIZE, buffer.length);
8382
return fromByte(buffer[0]);
8483
}
8584

‎api/src/main/java/io/opencensus/trace/Tracestate.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -201,8 +201,8 @@ public abstract static class Entry {
201201
public static Entry create(String key, String value) {
202202
Utils.checkNotNull(key, "key");
203203
Utils.checkNotNull(value, "value");
204-
Utils.checkArgument(validateKey(key), "Invalid key " + key);
205-
Utils.checkArgument(validateValue(value), "Invalid value " + value);
204+
Utils.checkArgument(validateKey(key), "Invalid key %s", key);
205+
Utils.checkArgument(validateValue(value), "Invalid value %s", value);
206206
return new AutoValue_Tracestate_Entry(key, value);
207207
}
208208

‎api/src/test/java/io/opencensus/internal/UtilsTest.java

+39
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@
3030
@RunWith(JUnit4.class)
3131
public final class UtilsTest {
3232
private static final String TEST_MESSAGE = "test message";
33+
private static final String TEST_MESSAGE_TEMPLATE = "I ate %s eggs.";
34+
private static final int TEST_MESSAGE_VALUE = 2;
35+
private static final String FORMATED_SIMPLE_TEST_MESSAGE = "I ate 2 eggs.";
36+
private static final String FORMATED_COMPLEX_TEST_MESSAGE = "I ate 2 eggs. [2]";
3337

3438
@Rule public ExpectedException thrown = ExpectedException.none();
3539

@@ -41,6 +45,27 @@ public void checkArgument() {
4145
Utils.checkArgument(false, TEST_MESSAGE);
4246
}
4347

48+
@Test
49+
public void checkArgument_NullErrorMessage() {
50+
thrown.expect(IllegalArgumentException.class);
51+
thrown.expectMessage("null");
52+
Utils.checkArgument(false, null);
53+
}
54+
55+
@Test
56+
public void checkArgument_WithSimpleFormat() {
57+
thrown.expect(IllegalArgumentException.class);
58+
thrown.expectMessage(FORMATED_SIMPLE_TEST_MESSAGE);
59+
Utils.checkArgument(false, TEST_MESSAGE_TEMPLATE, TEST_MESSAGE_VALUE);
60+
}
61+
62+
@Test
63+
public void checkArgument_WithComplexFormat() {
64+
thrown.expect(IllegalArgumentException.class);
65+
thrown.expectMessage(FORMATED_COMPLEX_TEST_MESSAGE);
66+
Utils.checkArgument(false, TEST_MESSAGE_TEMPLATE, TEST_MESSAGE_VALUE, TEST_MESSAGE_VALUE);
67+
}
68+
4469
@Test
4570
public void checkState() {
4671
Utils.checkNotNull(true, TEST_MESSAGE);
@@ -49,6 +74,13 @@ public void checkState() {
4974
Utils.checkState(false, TEST_MESSAGE);
5075
}
5176

77+
@Test
78+
public void checkState_NullErrorMessage() {
79+
thrown.expect(IllegalStateException.class);
80+
thrown.expectMessage("null");
81+
Utils.checkState(false, null);
82+
}
83+
5284
@Test
5385
public void checkNotNull() {
5486
Utils.checkNotNull(new Object(), TEST_MESSAGE);
@@ -57,6 +89,13 @@ public void checkNotNull() {
5789
Utils.checkNotNull(null, TEST_MESSAGE);
5890
}
5991

92+
@Test
93+
public void checkNotNull_NullErrorMessage() {
94+
thrown.expect(NullPointerException.class);
95+
thrown.expectMessage("null");
96+
Utils.checkNotNull(null, null);
97+
}
98+
6099
@Test
61100
public void checkIndex_Valid() {
62101
Utils.checkIndex(1, 2);

‎build.gradle

+6
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,12 @@ subprojects {
261261
html.enabled = true
262262
}
263263
}
264+
findbugsTest {
265+
reports {
266+
xml.enabled = false
267+
html.enabled = true
268+
}
269+
}
264270

265271
checkstyle {
266272
configFile = file("$rootDir/buildscripts/checkstyle.xml")

‎findbugs-exclude.xml

+6
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,12 @@
2828
<Class name="io.opencensus.internal.UtilsTest"/>
2929
<Method name="checkNotNull"/>
3030
</Match>
31+
<Match>
32+
<!-- Reason: This test is testing for a NPE. -->
33+
<Bug pattern="NP_NONNULL_PARAM_VIOLATION"/>
34+
<Class name="io.opencensus.internal.UtilsTest"/>
35+
<Method name="checkNotNull_NullErrorMessage"/>
36+
</Match>
3137
<Match>
3238
<!-- Reason: It seems like FindBugs incorrectly assumes that all -->
3339
<!-- Throwables are subclasses of Error or Exception. -->

0 commit comments

Comments
 (0)