Fix StringIndexOutOfBoundsException (#1827)

* Fix StringIndexOutOfBoundsException

* spotless

* Fix test logging

Author: trask

agent/agent-tooling/src/main/java/com/microsoft/applicationinsights/agent/internal/common/Strings.java

@@ -31,6 +31,14 @@ public static boolean isNullOrEmpty(@Nullable String string) {
     return string == null || string.isEmpty();
   }
 
+  public static String trimAndEmptyToNull(String str) {
+    if (str == null) {
+      return null;
+    }
+    String trimmed = str.trim();
+    return trimmed.isEmpty() ? null : trimmed;
+  }
+
   public static Map<String, String> splitToMap(String str) {
     Map<String, String> map = new HashMap<>();
     for (String part : str.split(";")) {

agent/agent-tooling/src/main/java/com/microsoft/applicationinsights/agent/internal/configuration/ConfigurationBuilder.java

@@ -445,7 +445,7 @@ private static String getConfigPath() {
     }
     // intentionally not checking system properties for other system properties
     // with the intention to keep configuration paths minimal to help with supportability
-    return trimAndEmptyToNull(System.getProperty("applicationinsights.configuration.file"));
+    return Strings.trimAndEmptyToNull(System.getProperty("applicationinsights.configuration.file"));
   }
 
   private static String getWebsiteSiteNameEnvVar() {
@@ -500,21 +500,12 @@ static boolean overlayWithEnvVar(String name, boolean defaultValue) {
 
   // never returns empty string (empty string is normalized to null)
   protected static String getSystemProperty(String name) {
-    return trimAndEmptyToNull(System.getProperty(name));
+    return Strings.trimAndEmptyToNull(System.getProperty(name));
   }
 
   // never returns empty string (empty string is normalized to null)
   protected static String getEnvVar(String name) {
-    return trimAndEmptyToNull(System.getenv(name));
-  }
-
-  // visible for testing
-  static String trimAndEmptyToNull(String str) {
-    if (str == null) {
-      return null;
-    }
-    String trimmed = str.trim();
-    return trimmed.isEmpty() ? null : trimmed;
+    return Strings.trimAndEmptyToNull(System.getenv(name));
   }
 
   private static boolean isTrimEmpty(String value) {

agent/agent-tooling/src/main/java/com/microsoft/applicationinsights/agent/internal/exporter/Exceptions.java

@@ -23,6 +23,7 @@
 
 import static java.util.Collections.singletonList;
 
+import com.microsoft.applicationinsights.agent.internal.common.Strings;
 import com.microsoft.applicationinsights.agent.internal.exporter.models.TelemetryExceptionDetails;
 import java.util.ArrayList;
 import java.util.List;
@@ -45,7 +46,7 @@ public static List<TelemetryExceptionDetails> minimalParse(String str) {
     // at the end of the loop, current will be end of the first line
     if (separator != -1) {
       details.setTypeName(str.substring(0, separator));
-      details.setMessage(str.substring(separator + 2, current));
+      details.setMessage(Strings.trimAndEmptyToNull(str.substring(separator + 1, current)));
     } else {
       details.setTypeName(str.substring(0, current));
     }
@@ -80,10 +81,10 @@ void process(String line) {
           line = line.substring("Caused by: ".length());
         }
         current = new TelemetryExceptionDetails();
-        int index = line.indexOf(": ");
+        int index = line.indexOf(':');
         if (index != -1) {
           current.setTypeName(line.substring(0, index));
-          current.setMessage(line.substring(index + 2));
+          current.setMessage(Strings.trimAndEmptyToNull(line.substring(index + 1)));
         } else {
           current.setTypeName(line);
         }

agent/agent-tooling/src/test/java/com/microsoft/applicationinsights/agent/internal/common/StringsTest.java

@@ -0,0 +1,39 @@
+/*
+ * ApplicationInsights-Java
+ * Copyright (c) Microsoft Corporation
+ * All rights reserved.
+ *
+ * MIT License
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of this
+ * software and associated documentation files (the ""Software""), to deal in the Software
+ * without restriction, including without limitation the rights to use, copy, modify, merge,
+ * publish, distribute, sublicense, and/or sell copies of the Software, and to permit
+ * persons to whom the Software is furnished to do so, subject to the following conditions:
+ * The above copyright notice and this permission notice shall be included in all copies or
+ * substantial portions of the Software.
+ * THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
+ * INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
+ * PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
+ * FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+package com.microsoft.applicationinsights.agent.internal.common;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.jupiter.api.Test;
+
+public class StringsTest {
+
+  @Test
+  void testEmptyToNull() {
+    assertThat(Strings.trimAndEmptyToNull("   ")).isNull();
+    assertThat(Strings.trimAndEmptyToNull("")).isNull();
+    assertThat(Strings.trimAndEmptyToNull(null)).isNull();
+    assertThat(Strings.trimAndEmptyToNull("a")).isEqualTo("a");
+    assertThat(Strings.trimAndEmptyToNull("  a  ")).isEqualTo("a");
+    assertThat(Strings.trimAndEmptyToNull("\t")).isNull();
+  }
+}

agent/agent-tooling/src/test/java/com/microsoft/applicationinsights/agent/internal/configuration/ConfigurationBuilderTest.java

@@ -21,7 +21,6 @@
 
 package com.microsoft.applicationinsights.agent.internal.configuration;
 
-import static com.microsoft.applicationinsights.agent.internal.configuration.ConfigurationBuilder.trimAndEmptyToNull;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
@@ -39,16 +38,6 @@ Path getConfigFilePath(String resourceName) {
     return file.toPath();
   }
 
-  @Test
-  void testEmptyToNull() {
-    assertThat(trimAndEmptyToNull("   ")).isNull();
-    assertThat(trimAndEmptyToNull("")).isNull();
-    assertThat(trimAndEmptyToNull(null)).isNull();
-    assertThat(trimAndEmptyToNull("a")).isEqualTo("a");
-    assertThat(trimAndEmptyToNull("  a  ")).isEqualTo("a");
-    assertThat(trimAndEmptyToNull("\t")).isNull();
-  }
-
   @Test
   void testGetConfigFilePath() {
     Path path = getConfigFilePath("applicationinsights.json");

agent/agent-tooling/src/test/java/com/microsoft/applicationinsights/agent/internal/exporter/ExceptionsTest.java

@@ -32,7 +32,55 @@
 class ExceptionsTest {
 
   @Test
-  void test() {
+  void testMinimalParse() {
+    // given
+    String str = toString(new IllegalStateException("test"));
+
+    // when
+    List<TelemetryExceptionDetails> list = Exceptions.minimalParse(str);
+
+    // then
+    assertThat(list.size()).isEqualTo(1);
+
+    TelemetryExceptionDetails details = list.get(0);
+    assertThat(details.getTypeName()).isEqualTo(IllegalStateException.class.getName());
+    assertThat(details.getMessage()).isEqualTo("test");
+  }
+
+  @Test
+  void testMinimalParseWithNoMessage() {
+    // given
+    String str = toString(new IllegalStateException());
+
+    // when
+    List<TelemetryExceptionDetails> list = Exceptions.minimalParse(str);
+
+    // then
+    assertThat(list.size()).isEqualTo(1);
+
+    TelemetryExceptionDetails details = list.get(0);
+    assertThat(details.getTypeName()).isEqualTo(IllegalStateException.class.getName());
+    assertThat(details.getMessage()).isNull();
+  }
+
+  @Test
+  void testMinimalParseWithProblematicMessage() {
+    // given
+    String str = toString(new ProblematicException());
+
+    // when
+    List<TelemetryExceptionDetails> list = Exceptions.minimalParse(str);
+
+    // then
+    assertThat(list.size()).isEqualTo(1);
+
+    TelemetryExceptionDetails details = list.get(0);
+    assertThat(details.getTypeName()).isEqualTo(ProblematicException.class.getName());
+    assertThat(details.getMessage()).isNull();
+  }
+
+  @Test
+  void testFullParse() {
     // given
     String str = toString(new IllegalStateException("test"));
 
@@ -48,7 +96,7 @@ void test() {
   }
 
   @Test
-  void testWithNoMessage() {
+  void testFullParseWithNoMessage() {
     // given
     String str = toString(new IllegalStateException());
 
@@ -63,6 +111,22 @@ void testWithNoMessage() {
     assertThat(details.getMessage()).isNull();
   }
 
+  @Test
+  void testFullParseWithProblematicMessage() {
+    // given
+    String str = toString(new ProblematicException());
+
+    // when
+    List<TelemetryExceptionDetails> list = Exceptions.fullParse(str);
+
+    // then
+    assertThat(list.size()).isEqualTo(1);
+
+    TelemetryExceptionDetails details = list.get(0);
+    assertThat(details.getTypeName()).isEqualTo(ProblematicException.class.getName());
+    assertThat(details.getMessage()).isNull();
+  }
+
   @Test
   void testWithCausedBy() {
     // given
@@ -108,4 +172,12 @@ private static String toString(Throwable t) {
     t.printStackTrace(new PrintWriter(out));
     return out.toString();
   }
+
+  @SuppressWarnings("OverrideThrowableToString")
+  private static class ProblematicException extends Exception {
+    @Override
+    public String toString() {
+      return ProblematicException.class.getName() + ":";
+    }
+  }
 }

buildSrc/src/main/kotlin/ai.java-conventions.gradle.kts

@@ -109,6 +109,7 @@ tasks.withType<Test>().configureEach {
   }
 
   testLogging {
+    showStandardStreams = true
     exceptionFormat = TestExceptionFormat.FULL
   }
 }

buildSrc/src/main/kotlin/ai.smoke-test.gradle.kts

@@ -1,5 +1,6 @@
 import com.microsoft.applicationinsights.gradle.AiSmokeTestExtension
 import gradle.kotlin.dsl.accessors._94a23f0be9141f27f052e4809bb3869b.java
+import org.gradle.api.tasks.testing.logging.TestExceptionFormat
 
 plugins {
   `java-library`
@@ -110,6 +111,11 @@ tasks {
       jvmArgs("-Dio.opentelemetry.javaagent.shaded.io.opentelemetry.context.enableStrictContext=true")
     }
 
+    testLogging {
+      showStandardStreams = true
+      exceptionFormat = TestExceptionFormat.FULL
+    }
+
     // TODO (trask) is this still a problem?
     //outputs.upToDateWhen { false }
   }