Skip to content

Commit f9a12b6

Browse files
visumickeyvisumickey
committed
Address comments for code clarity.
1 parent 5f2f9e2 commit f9a12b6

File tree

6 files changed

+60
-51
lines changed

6 files changed

+60
-51
lines changed

‎firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerformance.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -364,10 +364,7 @@ private void checkAttribute(@Nullable String key, @Nullable String value) {
364364
Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
365365
}
366366

367-
String err = PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
368-
if (err != null) {
369-
throw new IllegalArgumentException(err);
370-
}
367+
PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
371368
}
372369

373370
/**

‎firebase-perf/src/main/java/com/google/firebase/perf/metrics/HttpMetric.java

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ public void putAttribute(@NonNull String attribute, @NonNull String value) {
181181
}
182182
}
183183

184-
private void checkAttribute(@Nullable String key, @Nullable String value) {
184+
private void checkAttribute(@NonNull String key, @NonNull String value) {
185185
if (isStopped) {
186186
throw new IllegalArgumentException(
187187
"HttpMetric has been logged already so unable to modify attributes");
@@ -195,10 +195,7 @@ private void checkAttribute(@Nullable String key, @Nullable String value) {
195195
"Exceeds max limit of number of attributes - %d",
196196
Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
197197
}
198-
String err = PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
199-
if (err != null) {
200-
throw new IllegalArgumentException(err);
201-
}
198+
PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
202199
}
203200

204201
/**

‎firebase-perf/src/main/java/com/google/firebase/perf/metrics/Trace.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -639,10 +639,7 @@ private void checkAttribute(@NonNull String key, @NonNull String value) {
639639
"Exceeds max limit of number of attributes - %d",
640640
Constants.MAX_TRACE_CUSTOM_ATTRIBUTES));
641641
}
642-
String err = PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
643-
if (err != null) {
644-
throw new IllegalArgumentException(err);
645-
}
642+
PerfMetricValidator.validateAttribute(new AbstractMap.SimpleEntry<>(key, value));
646643
}
647644

648645
/**

‎firebase-perf/src/main/java/com/google/firebase/perf/metrics/validator/FirebasePerfTraceValidator.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -180,9 +180,10 @@ private boolean isValidCounterId(@Nullable String counterId) {
180180

181181
private boolean hasValidAttributes(Map<String, String> customAttributes) {
182182
for (Map.Entry<String, String> entry : customAttributes.entrySet()) {
183-
String err = PerfMetricValidator.validateAttribute(entry);
184-
if (err != null) {
185-
logger.warn(err);
183+
try {
184+
PerfMetricValidator.validateAttribute(entry);
185+
} catch (IllegalArgumentException exception) {
186+
logger.warn(exception.getLocalizedMessage());
186187
return false;
187188
}
188189
}

‎firebase-perf/src/main/java/com/google/firebase/perf/metrics/validator/PerfMetricValidator.java

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -140,28 +140,27 @@ public static String validateMetricName(@Nullable String str) {
140140
* it can't be used.
141141
*/
142142
@Nullable
143-
public static String validateAttribute(@NonNull Map.Entry<String, String> attribute) {
143+
public static void validateAttribute(@NonNull Map.Entry<String, String> attribute) {
144144
String key = attribute.getKey();
145145
String value = attribute.getValue();
146146
if (key == null || key.length() == 0) {
147-
return "Attribute key must not be null or empty";
147+
throw new IllegalArgumentException("Attribute key must not be null or empty");
148148
} else if (value == null || value.length() == 0) {
149-
return "Attribute value must not be null or empty";
149+
throw new IllegalArgumentException("Attribute value must not be null or empty");
150150
} else if (key.length() > Constants.MAX_ATTRIBUTE_KEY_LENGTH) {
151-
return String.format(
151+
throw new IllegalArgumentException(String.format(
152152
Locale.US,
153153
"Attribute key length must not exceed %d characters",
154-
Constants.MAX_ATTRIBUTE_KEY_LENGTH);
154+
Constants.MAX_ATTRIBUTE_KEY_LENGTH));
155155
} else if (value.length() > Constants.MAX_ATTRIBUTE_VALUE_LENGTH) {
156-
return String.format(
156+
throw new IllegalArgumentException(String.format(
157157
Locale.US,
158158
"Attribute value length must not exceed %d characters",
159-
Constants.MAX_ATTRIBUTE_VALUE_LENGTH);
159+
Constants.MAX_ATTRIBUTE_VALUE_LENGTH));
160160
} else if (!key.matches("^(?!(firebase_|google_|ga_))[A-Za-z][A-Za-z_0-9]*")) {
161-
return "Attribute key must start with letter, must only contain alphanumeric characters and"
162-
+ " underscore and must not start with \"firebase_\", \"google_\" and \"ga_";
161+
throw new IllegalArgumentException("Attribute key must start with letter, must only contain alphanumeric characters and"
162+
+ " underscore and must not start with \"firebase_\", \"google_\" and \"ga_");
163163
}
164-
return null;
165164
}
166165

167166
public abstract boolean isValidPerfMetric();

‎firebase-perf/src/test/java/com/google/firebase/perf/metrics/validator/FirebasePerfTraceValidatorTest.java

Lines changed: 43 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -169,57 +169,75 @@ public void screenTrace_shouldNotAllowNonPositiveTotalFrames() {
169169
}
170170

171171
@Test
172-
public void traceValidator_invalidCustomAttribute_marksPerfMetricInvalid() {
172+
public void traceValidator_customAttributeWithUnderscorePrefix_marksPerfMetricInvalid() {
173173
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("_test", "value");
174174
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
175+
}
175176

176-
trace = createValidTraceMetric();
177-
trace.clearCustomAttributes().putCustomAttributes("0_test", "value");
177+
@Test
178+
public void traceValidator_customAttributeWithNumberPrefix_marksPerfMetricInvalid() {
179+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("0_test", "value");
178180
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
181+
}
182+
183+
@Test
184+
public void traceValidator_customAttributeWithGooglePrefix_marksPerfMetricInvalid() {
185+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("google_test", "value");
186+
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
187+
}
179188

180-
trace = trace.clone();
181-
trace.clearCustomAttributes().putCustomAttributes("google_test", "value");
189+
@Test
190+
public void traceValidator_customAttributeWithFirebasePrefix_marksPerfMetricInvalid() {
191+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("firebase_test", "value");
182192
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
193+
}
183194

184-
trace = trace.clone();
185-
trace.clearCustomAttributes().putCustomAttributes("firebase_test", "value");
195+
@Test
196+
public void traceValidator_customAttributeWithGAPrefix_marksPerfMetricInvalid() {
197+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("ga_test", "value");
186198
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
199+
}
187200

188-
trace = trace.clone();
189-
trace.clearCustomAttributes().putCustomAttributes("ga_test", "value");
201+
@Test
202+
public void traceValidator_customAttributeEmptyValue_marksPerfMetricInvalid() {
203+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("key", "");
190204
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
205+
}
191206

192-
trace = trace.clone();
193-
trace.clearCustomAttributes().putCustomAttributes("key", "");
207+
@Test
208+
public void traceValidator_customAttributeEmptyKey_marksPerfMetricInvalid() {
209+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("", "value");
194210
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
211+
}
195212

196-
trace = trace.clone();
197-
trace.clearCustomAttributes().putCustomAttributes("", "value");
213+
@Test
214+
public void traceValidator_customAttributeEmptyKeyAndValue_marksPerfMetricInvalid() {
215+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("", "");
198216
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
217+
}
218+
219+
@Test
220+
public void traceValidator_customAttributeWithLongKey_marksPerfMetricInvalid() {
199221

200222
StringBuilder longString = new StringBuilder();
201223
for (int i = 0; i <= Constants.MAX_ATTRIBUTE_KEY_LENGTH; i++) {
202224
longString.append("a");
203225
}
204-
205-
trace = trace.clone();
206-
trace.clearCustomAttributes().putCustomAttributes(longString.toString(), "value");
226+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes(longString.toString(), "value");
207227
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
228+
}
208229

209-
longString = new StringBuilder();
230+
@Test
231+
public void traceValidator_customAttributeWithLongValue_marksPerfMetricInvalid() {
232+
233+
StringBuilder longString = new StringBuilder();
210234
for (int i = 0; i <= Constants.MAX_ATTRIBUTE_VALUE_LENGTH; i++) {
211235
longString.append("a");
212236
}
213-
214-
trace = trace.clone();
215-
trace.clearCustomAttributes().putCustomAttributes("key", longString.toString());
237+
TraceMetric.Builder trace = createValidTraceMetric().putCustomAttributes("key", longString.toString());
216238
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isFalse();
217-
218-
trace = trace.clone();
219-
trace.clearCustomAttributes().putCustomAttributes("test", "value");
220-
assertThat(new FirebasePerfTraceValidator(trace.build()).isValidPerfMetric()).isTrue();
221239
}
222-
240+
223241
@Test
224242
public void testIsValid() {
225243
TraceMetric.Builder trace = createValidTraceMetric().putCounters("counter", 2);

0 commit comments

Comments
 (0)