Prohibit String.replace() and String.replaceAll(), fix and prohibit some toString()-related redundancies

benchmarks/src/main/java/org/apache/druid/benchmark/DimensionPredicateFilterBenchmark.java

@@ -79,7 +79,7 @@ public boolean apply(String input)
               if (input == null) {
                 return false;
               }
-              return Integer.parseInt(input.toString()) % 2 == 0;
+              return Integer.parseInt(input) % 2 == 0;
             }
           };
         }

benchmarks/src/main/java/org/apache/druid/benchmark/FloatCompressionBenchmarkFileGenerator.java

@@ -139,7 +139,7 @@ public static void main(String[] args) throws IOException
     // create compressed files using all combinations of CompressionStrategy and FloatEncoding provided
     for (Map.Entry<String, BenchmarkColumnValueGenerator> entry : generators.entrySet()) {
       for (CompressionStrategy compression : compressions) {
-        String name = entry.getKey() + "-" + compression.toString();
+        String name = entry.getKey() + "-" + compression;
         log.info("%s: ", name);
         File compFile = new File(dir, name);
         compFile.delete();

benchmarks/src/main/java/org/apache/druid/benchmark/LongCompressionBenchmarkFileGenerator.java

@@ -132,7 +132,7 @@ public static void main(String[] args) throws IOException
     for (Map.Entry<String, BenchmarkColumnValueGenerator> entry : generators.entrySet()) {
       for (CompressionStrategy compression : compressions) {
         for (CompressionFactory.LongEncodingStrategy encoding : encodings) {
-          String name = entry.getKey() + "-" + compression.toString() + "-" + encoding.toString();
+          String name = entry.getKey() + "-" + compression + "-" + encoding;
           log.info("%s: ", name);
           File compFile = new File(dir, name);
           compFile.delete();

benchmarks/src/test/java/org/apache/druid/benchmark/BenchmarkDataGeneratorTest.java

@@ -443,7 +443,7 @@ public void printStuff()
         Collections.sort(valList);
 
         for (Comparable val : valList) {
-          System.out.println(" VAL: " + val.toString() + " CNT: " + valueMap.get(val));
+          System.out.println(" VAL: " + val + " CNT: " + valueMap.get(val));
         }
         System.out.println();
       }

codestyle/druid-forbidden-apis.txt

@@ -22,6 +22,11 @@ com.google.common.collect.Sets#newTreeSet() @ Create java.util.TreeSet directly
 com.google.common.collect.Sets#newTreeSet(java.util.Comparator) @ Create java.util.TreeSet directly
 com.google.common.util.concurrent.Futures#transform(com.google.common.util.concurrent.ListenableFuture, com.google.common.util.concurrent.AsyncFunction) @ Use org.apache.druid.java.util.common.concurrent.ListenableFutures#transformAsync
 java.io.File#toURL() @ Use java.io.File#toURI() and java.net.URI#toURL() instead
+java.lang.String#matches(java.lang.String) @ Use startsWith(), endsWith(), contains(), or compile and cache a Pattern explicitly
+java.lang.String#replace(java.lang.CharSequence,java.lang.CharSequence) @ Use one of the appropriate methods in StringUtils instead
+java.lang.String#replaceAll(java.lang.String,java.lang.String) @ Use one of the appropriate methods in StringUtils instead, or compile and cache a Pattern explicitly
+java.lang.String#replaceFirst(java.lang.String,java.lang.String) @ Use String.indexOf() and substring methods, or compile and cache a Pattern explicitly
 java.util.LinkedList @ Use ArrayList or ArrayDeque instead
 java.util.Random#<init>() @ Use ThreadLocalRandom.current() or the constructor with a seed (the latter in tests only!)
+java.util.regex.Pattern#matches(java.lang.String,java.lang.CharSequence) @ Use String.startsWith(), endsWith(), contains(), or compile and cache a Pattern explicitly
 org.apache.commons.io.FileUtils#getTempDirectory() @ Use org.junit.rules.TemporaryFolder for tests instead

core/src/main/java/org/apache/druid/java/util/common/StringUtils.java

@@ -153,16 +153,6 @@ public static String toUpperCase(String s)
     return s.toUpperCase(Locale.ENGLISH);
   }
 
-  public static String removeChar(String s, char c)
-  {
-    for (int i = 0; i < s.length(); i++) {
-      if (s.charAt(i) == c) {
-        return removeChar(s, c, i);
-      }
-    }
-    return s;
-  }
-
   public static String urlEncode(String s)
   {
     try {
@@ -173,16 +163,81 @@ public static String urlEncode(String s)
     }
   }
 
-  private static String removeChar(String s, char c, int firstOccurranceIndex)
+  /**
+   * Removes all occurrences of the given char from the given string. This method is an optimal version of
+   * {@link String#replace(CharSequence, CharSequence) s.replace("c", "")}.
+   */
+  public static String removeChar(String s, char c)
   {
+    int pos = s.indexOf(c);
+    if (pos < 0) {
+      return s;
+    }
     StringBuilder sb = new StringBuilder(s.length() - 1);
-    sb.append(s, 0, firstOccurranceIndex);
-    for (int i = firstOccurranceIndex + 1; i < s.length(); i++) {
-      char charOfString = s.charAt(i);
-      if (charOfString != c) {
-        sb.append(charOfString);
-      }
+    int prevPos = 0;
+    do {
+      sb.append(s, prevPos, pos);
+      prevPos = pos + 1;
+      pos = s.indexOf(c, pos + 1);
+    } while (pos > 0);
+    sb.append(s, prevPos, s.length());
+    return sb.toString();
+  }
+
+  /**
+   * Replaces all occurrences of the given char in the given string with the given replacement string. This method is an
+   * optimal version of {@link String#replace(CharSequence, CharSequence) s.replace("c", replacement)}.
+   */
+  public static String replaceChar(String s, char c, String replacement)
+  {
+    int pos = s.indexOf(c);
+    if (pos < 0) {
+      return s;
+    }
+    StringBuilder sb = new StringBuilder(s.length() - 1 + replacement.length());
+    int prevPos = 0;
+    do {
+      sb.append(s, prevPos, pos);
+      sb.append(replacement);
+      prevPos = pos + 1;
+      pos = s.indexOf(c, pos + 1);
+    } while (pos > 0);
+    sb.append(s, prevPos, s.length());
+    return sb.toString();
+  }
+
+  /**
+   * Replaces all occurrences of the given target substring in the given string with the given replacement string. This
+   * method is an optimal version of {@link String#replace(CharSequence, CharSequence) s.replace(target, replacement)}.
+   */
+  public static String replace(String s, String target, String replacement)
+  {
+    // String.replace() is suboptimal in JDK8, but is fixed in JDK9+. When the minimal JDK version supported by Druid is
+    // JDK9+, the implementation of this method should be replaced with simple delegation to String.replace(). However,
+    // the method should still be prohibited to use in all other places except this method body, because it's easy to
+    // suboptimally call String.replace("a", "b"), String.replace("a", ""), String.replace("a", "abc"), which have
+    // better alternatives String.replace('a', 'b'), removeChar() and replaceChar() respectively.
+    int pos = s.indexOf(target);
+    if (pos < 0) {
+      return s;
     }
+    int sLength = s.length();
+    int targetLength = target.length();
+    // This is needed to work correctly with empty target string and mimic String.replace() behavior
+    int searchSkip = Math.max(targetLength, 1);
+    StringBuilder sb = new StringBuilder(sLength - targetLength + replacement.length());
+    int prevPos = 0;
+    do {
+      sb.append(s, prevPos, pos);
+      sb.append(replacement);
+      prevPos = pos + targetLength;
+      // Break from the loop if the target is empty
+      if (pos == sLength) {
+        break;
+      }
+      pos = s.indexOf(target, pos + searchSkip);
+    } while (pos > 0);
+    sb.append(s, prevPos, sLength);
     return sb.toString();
   }
 

core/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java

@@ -139,8 +139,7 @@ public DateTime toDate(String filePath, Formatter formatter)
   @Override
   public byte[] getCacheKey()
   {
-    return StringUtils.toUtf8(getPeriod().toString() + ":" +
-                              getTimeZone().toString() + ":" + getOrigin());
+    return StringUtils.toUtf8(getPeriod() + ":" + getTimeZone() + ":" + getOrigin());
   }
 
   @Override
@@ -420,8 +419,7 @@ private long truncateMillisPeriod(final long t)
       return t - offset;
     } else {
       throw new UnsupportedOperationException(
-          "Period cannot be converted to milliseconds as some fields mays vary in length with chronology "
-          + chronology.toString()
+          "Period cannot be converted to milliseconds as some fields mays vary in length with chronology " + chronology
       );
     }
   }

core/src/main/java/org/apache/druid/java/util/emitter/core/ParametrizedUriEmitter.java

@@ -22,6 +22,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableSet;
+import org.apache.druid.java.util.common.StringUtils;
 import org.apache.druid.java.util.common.lifecycle.Lifecycle;
 import org.apache.druid.java.util.common.lifecycle.LifecycleStart;
 import org.apache.druid.java.util.common.lifecycle.LifecycleStop;
@@ -48,7 +49,7 @@ private static UriExtractor makeUriExtractor(ParametrizedUriEmitterConfig config
     final ParametrizedUriExtractor parametrizedUriExtractor = new ParametrizedUriExtractor(baseUri);
     UriExtractor uriExtractor = parametrizedUriExtractor;
     if (ONLY_FEED_PARAM.equals(parametrizedUriExtractor.getParams())) {
-      uriExtractor = new FeedUriExtractor(baseUri.replace("{feed}", "%s"));
+      uriExtractor = new FeedUriExtractor(StringUtils.replace(baseUri, "{feed}", "%s"));
     }
     return uriExtractor;
   }

core/src/main/java/org/apache/druid/java/util/emitter/core/ParametrizedUriExtractor.java

@@ -65,7 +65,7 @@ public URI apply(Event event) throws URISyntaxException
             eventMap
         );
       }
-      processedUri = processedUri.replace(StringUtils.format("{%s}", key), paramValue.toString());
+      processedUri = StringUtils.replace(processedUri, StringUtils.format("{%s}", key), paramValue.toString());
     }
     return new URI(processedUri);
   }

core/src/main/java/org/apache/druid/math/expr/Expr.java

@@ -294,7 +294,7 @@ public void visit(Visitor visitor)
   @Override
   public String toString()
   {
-    return "-" + expr.toString();
+    return "-" + expr;
   }
 }
 
@@ -321,7 +321,7 @@ public ExprEval eval(ObjectBinding bindings)
   @Override
   public String toString()
   {
-    return "!" + expr.toString();
+    return "!" + expr;
   }
 }
 

core/src/main/java/org/apache/druid/math/expr/Function.java

@@ -1024,14 +1024,12 @@ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
       }
 
       final String arg = args.get(0).eval(bindings).asString();
-      final String pattern = args.get(1).eval(bindings).asString();
-      final String replacement = args.get(2).eval(bindings).asString();
+      final String pattern = NullHandling.nullToEmptyIfNeeded(args.get(1).eval(bindings).asString());
+      final String replacement = NullHandling.nullToEmptyIfNeeded(args.get(2).eval(bindings).asString());
       if (arg == null) {
         return ExprEval.of(NullHandling.defaultStringValue());
       }
-      return ExprEval.of(
-          arg.replace(NullHandling.nullToEmptyIfNeeded(pattern), NullHandling.nullToEmptyIfNeeded(replacement))
-      );
+      return ExprEval.of(StringUtils.replace(arg, pattern, replacement));
     }
   }
 

core/src/test/java/org/apache/druid/common/utils/UUIDUtilsTest.java

@@ -24,6 +24,7 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.Lists;
+import org.apache.druid.java.util.common.StringUtils;
 import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -102,7 +103,7 @@ public static void validateIsStandardUUID(
     strings.add(uuidString.substring(16, 20));
     strings.add(uuidString.substring(20, 32));
     UUID uuid = UUID.fromString(Joiner.on('-').join(strings));
-    Assert.assertEquals(uuid.toString().replace("-", ""), uuidString);
+    Assert.assertEquals(StringUtils.removeChar(uuid.toString(), '-'), uuidString);
   }
 
   @Test

core/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java

@@ -118,11 +118,34 @@ public void testNonStrictFormat()
   }
 
   @Test
-  public void testRemoveCharacter()
+  public void testRemoveChar()
   {
     Assert.assertEquals("123", StringUtils.removeChar("123", ','));
     Assert.assertEquals("123", StringUtils.removeChar("123,", ','));
     Assert.assertEquals("123", StringUtils.removeChar(",1,,2,3,", ','));
     Assert.assertEquals("", StringUtils.removeChar(",,", ','));
   }
+
+  @Test
+  public void testReplaceChar()
+  {
+    Assert.assertEquals("123", StringUtils.replaceChar("123", ',', "x"));
+    Assert.assertEquals("12345", StringUtils.replaceChar("123,", ',', "45"));
+    Assert.assertEquals("", StringUtils.replaceChar("", 'a', "bb"));
+    Assert.assertEquals("bb", StringUtils.replaceChar("a", 'a', "bb"));
+    Assert.assertEquals("bbbb", StringUtils.replaceChar("aa", 'a', "bb"));
+  }
+
+  @Test
+  public void testReplace()
+  {
+    Assert.assertEquals("x1x2x3x", StringUtils.replace("123", "", "x"));
+    Assert.assertEquals("12345", StringUtils.replace("123,", ",", "45"));
+    Assert.assertEquals("", StringUtils.replace("", "a", "bb"));
+    Assert.assertEquals("bb", StringUtils.replace("a", "a", "bb"));
+    Assert.assertEquals("bba", StringUtils.replace("aaa", "aa", "bb"));
+    Assert.assertEquals("bcb", StringUtils.replace("aacaa", "aa", "b"));
+    Assert.assertEquals("bb", StringUtils.replace("aaaa", "aa", "b"));
+    Assert.assertEquals("", StringUtils.replace("aaaa", "aa", ""));
+  }
 }

core/src/test/java/org/apache/druid/java/util/emitter/core/ParametrizedUriEmitterTest.java

@@ -137,7 +137,7 @@ public void testEmitterWithMultipleFeeds() throws Exception
           protected ListenableFuture<Response> go(Request request)
           {
             results.put(
-                request.getUrl().toString(),
+                request.getUrl(),
                 StandardCharsets.UTF_8.decode(request.getByteBufferData().slice()).toString()
             );
             return GoHandlers.immediateFuture(okResponse());

core/src/test/java/org/apache/druid/java/util/http/client/FriendlyServersTest.java

@@ -238,7 +238,7 @@ public void testFriendlySelfSignedHttpsServer() throws Exception
         }
 
         Assert.assertTrue("ChannelException thrown by 'get'", ea instanceof ChannelException);
-        Assert.assertTrue("Expected error message", ea.getCause().getMessage().matches(".*Failed to handshake.*"));
+        Assert.assertTrue("Expected error message", ea.getCause().getMessage().contains("Failed to handshake"));
       }
 
       {

core/src/test/java/org/apache/druid/java/util/http/client/JankyServersTest.java

@@ -298,7 +298,7 @@ public void testHttpsConnectionClosingServer() throws Throwable
   public boolean isChannelClosedException(Throwable e)
   {
     return e instanceof ChannelException ||
-           (e instanceof IOException && e.getMessage().matches(".*Connection reset by peer.*"));
+           (e instanceof IOException && e.getMessage().contains("Connection reset by peer"));
   }
 
   @Test

core/src/test/java/org/apache/druid/java/util/metrics/cgroups/TestUtils.java

@@ -25,7 +25,6 @@
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
-import java.util.regex.Pattern;
 
 public class TestUtils
 {
@@ -41,10 +40,7 @@ public static void setUpCgroups(
     final String procMountsString = StringUtils.fromUtf8(Files.readAllBytes(procMountsTemplate.toPath()));
     Files.write(
         procMounts.toPath(),
-        StringUtils.toUtf8(procMountsString.replaceAll(
-            Pattern.quote("/sys/fs/cgroup"),
-            cgroupDir.getAbsolutePath()
-        ))
+        StringUtils.toUtf8(StringUtils.replace(procMountsString, "/sys/fs/cgroup", cgroupDir.getAbsolutePath()))
     );
 
     Assert.assertTrue(new File(