Introduce StringFormattingCheck checkstyle rule (#81603)

The new rule Checks for calls to `String#formatted(Object...)` that
include format specifiers that are not locale-safe. This method always
uses the default `Locale`, and so for our purposes it is safer to use
`String#format(Locale, String, Object...)`.

Note that this rule can currently only detect violations when calling
`formatted()` on a string literal or text block. In theory, it could be
extended to detect violations in local variables or statics.

Note that this change also forbids `.formatted()` in server code, so
we are only permitted to use `.formatted()` in test code.

build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/StringFormattingCheck.java

@@ -0,0 +1,89 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.gradle.internal.checkstyle;
+
+import com.puppycrawl.tools.checkstyle.StatelessCheck;
+import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
+import com.puppycrawl.tools.checkstyle.api.DetailAST;
+import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+
+import java.util.Locale;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Checks for calls to {@link String#formatted(Object...)} that include format specifiers that
+ * are not locale-safe. This method always uses the default {@link Locale}, and so for our
+ * purposes it is safer to use {@link String#format(Locale, String, Object...)}.
+ * <p>
+ * Note that this rule can currently only detect violations when calling <code>formatted()</code>
+ * on a string literal or text block. In theory, it could be extended to detect violations in
+ * local variables or statics.
+ */
+@StatelessCheck
+public class StringFormattingCheck extends AbstractCheck {
+
+    public static final String FORMATTED_MSG_KEY = "forbidden.formatted";
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] { TokenTypes.METHOD_CALL };
+    }
+
+    @Override
+    public void visitToken(DetailAST ast) {
+        checkFormattedMethod(ast);
+    }
+
+    // Originally pinched from java/util/Formatter.java but then modified.
+    // %[argument_index$][flags][width][.precision][t]conversion
+    private static final Pattern formatSpecifier = Pattern.compile("%(?:\\d+\\$)?(?:[-#+ 0,\\(<]*)?(?:\\d+)?(?:\\.\\d+)?([tT]?[a-zA-Z%])");
+
+    private void checkFormattedMethod(DetailAST ast) {
+        final DetailAST dotAst = ast.findFirstToken(TokenTypes.DOT);
+        if (dotAst == null) {
+            return;
+        }
+
+        final String methodName = dotAst.findFirstToken(TokenTypes.IDENT).getText();
+        if (methodName.equals("formatted") == false) {
+            return;
+        }
+
+        final DetailAST subjectAst = dotAst.getFirstChild();
+
+        String stringContent = null;
+        if (subjectAst.getType() == TokenTypes.TEXT_BLOCK_LITERAL_BEGIN) {
+            stringContent = subjectAst.findFirstToken(TokenTypes.TEXT_BLOCK_CONTENT).getText();
+        } else if (subjectAst.getType() == TokenTypes.STRING_LITERAL) {
+            stringContent = subjectAst.getText();
+        }
+
+        if (stringContent != null) {
+            final Matcher m = formatSpecifier.matcher(stringContent);
+            while (m.find()) {
+                char specifier = m.group(1).toLowerCase(Locale.ROOT).charAt(0);
+
+                if (specifier == 'd' || specifier == 'e' || specifier == 'f' || specifier == 'g' || specifier == 't') {
+                    log(ast, FORMATTED_MSG_KEY, m.group());
+                }
+            }
+        }
+    }
+}

build-tools-internal/src/main/resources/checkstyle.xml

@@ -130,6 +130,12 @@
     </module>
     -->
 
+    <module name="org.elasticsearch.gradle.internal.checkstyle.StringFormattingCheck">
+        <message
+            key="forbidden.formatted"
+            value="''{0}'' format specifier is unsafe inside ''.formatted'' calls, as it uses the default locale. Use ''String.format'' for numeric formatting with ''Locale.ROOT'' instead." />
+    </module>
+
     <!-- We don't use Java's builtin serialization and we suppress all warning
       about it. The flip side of that coin is that we shouldn't _try_ to use
       it. We can't outright ban it with ForbiddenApis because it complain about

build-tools-internal/src/main/resources/forbidden/es-server-signatures.txt

@@ -130,3 +130,7 @@ org.apache.logging.log4j.Logger#error(java.lang.Object)
 org.apache.logging.log4j.Logger#error(java.lang.Object, java.lang.Throwable)
 org.apache.logging.log4j.Logger#fatal(java.lang.Object)
 org.apache.logging.log4j.Logger#fatal(java.lang.Object, java.lang.Throwable)
+
+# This is permitted in test code, where we have a Checkstyle rule to guard
+# against unsafe uses. This leniency does not extend to server code.
+java.lang.String#formatted(java.lang.Object[]) @ Uses default locale - use String#format(Locale, String, Object...) instead

server/src/internalClusterTest/java/org/elasticsearch/indices/IndicesLifecycleListenerIT.java

@@ -35,6 +35,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
@@ -244,11 +245,11 @@ private static void assertShardStatesMatch(
         try {
             assertBusy(waitPredicate, 1, TimeUnit.MINUTES);
         } catch (AssertionError ae) {
-            fail("""
+            fail(String.format(Locale.ROOT, """
                 failed to observe expect shard states
                 expected: [%d] shards with states: %s
                 observed:
-                %s""".formatted(numShards, Strings.arrayToCommaDelimitedString(shardStates), stateChangeListener));
+                %s""", numShards, Strings.arrayToCommaDelimitedString(shardStates), stateChangeListener));
         }
 
         stateChangeListener.shardStates.clear();

server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java

@@ -49,6 +49,7 @@
 import java.io.UncheckedIOException;
 import java.util.Collections;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.function.Supplier;
@@ -638,17 +639,17 @@ private static String normalizeValue(NamedAnalyzer normalizer, String field, Str
             final CharTermAttribute termAtt = ts.addAttribute(CharTermAttribute.class);
             ts.reset();
             if (ts.incrementToken() == false) {
-                throw new IllegalStateException("""
+                throw new IllegalStateException(String.format(Locale.ROOT, """
                     The normalization token stream is expected to produce exactly 1 token, \
                     but got 0 for analyzer %s and input "%s"
-                    """.formatted(normalizer, value));
+                    """, normalizer, value));
             }
             final String newValue = termAtt.toString();
             if (ts.incrementToken()) {
-                throw new IllegalStateException("""
+                throw new IllegalStateException(String.format(Locale.ROOT, """
                     The normalization token stream is expected to produce exactly 1 token, \
                     but got 2+ for analyzer %s and input "%s"
-                    """.formatted(normalizer, value));
+                    """, normalizer, value));
             }
             ts.end();
             return newValue;

server/src/main/java/org/elasticsearch/indices/SystemIndices.java

@@ -40,6 +40,7 @@
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
@@ -644,7 +645,9 @@ Collection<SystemIndexDescriptor> getSystemIndexDescriptors() {
     public static void validateFeatureName(String name, String plugin) {
         if (SnapshotsService.NO_FEATURE_STATES_VALUE.equalsIgnoreCase(name)) {
             throw new IllegalArgumentException(
-                "feature name cannot be reserved name [\"%s\"], but was for plugin [%s]".formatted(
+                String.format(
+                    Locale.ROOT,
+                    "feature name cannot be reserved name [\"%s\"], but was for plugin [%s]",
                     SnapshotsService.NO_FEATURE_STATES_VALUE,
                     plugin
                 )

server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java

@@ -30,6 +30,7 @@
 import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.function.BiConsumer;
 import java.util.function.Function;
@@ -406,9 +407,9 @@ public BucketComparator bucketComparator(String key, SortOrder order) {
         if (key == null || "doc_count".equals(key)) {
             return (lhs, rhs) -> order.reverseMul() * Long.compare(bucketDocCount(lhs), bucketDocCount(rhs));
         }
-        throw new IllegalArgumentException("""
+        throw new IllegalArgumentException(String.format(Locale.ROOT, """
             Ordering on a single-bucket aggregation can only be done on its doc_count. \
-            Either drop the key (a la "%s") or change it to "doc_count" (a la "%s.doc_count") or "key".""".formatted(name(), name()));
+            Either drop the key (a la "%s") or change it to "doc_count" (a la "%s.doc_count") or "key".""", name(), name()));
     }
 
     public static boolean descendsFromGlobalAggregator(Aggregator parent) {

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/heuristic/NXYSignificanceHeuristic.java

@@ -15,6 +15,7 @@
 import org.elasticsearch.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.util.Locale;
 import java.util.function.BiFunction;
 import java.util.function.Function;
 
@@ -26,9 +27,9 @@ public abstract class NXYSignificanceHeuristic extends SignificanceHeuristic {
 
     protected static final ParseField INCLUDE_NEGATIVES_FIELD = new ParseField("include_negatives");
 
-    protected static final String SCORE_ERROR_MESSAGE = """
+    protected static final String SCORE_ERROR_MESSAGE = String.format(Locale.ROOT, """
         , does your background filter not include all documents in the bucket? If so and it is intentional, set "%s": false
-        """.formatted(BACKGROUND_IS_SUPERSET.getPreferredName());
+        """, BACKGROUND_IS_SUPERSET.getPreferredName());
 
     protected final boolean backgroundIsSuperset;
 

server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregator.java

@@ -13,6 +13,7 @@
 import org.elasticsearch.search.sort.SortOrder;
 
 import java.io.IOException;
+import java.util.Locale;
 import java.util.Map;
 
 public abstract class NumericMetricsAggregator extends MetricsAggregator {
@@ -33,9 +34,9 @@ protected SingleValue(String name, AggregationContext context, Aggregator parent
         @Override
         public BucketComparator bucketComparator(String key, SortOrder order) {
             if (key != null && false == "value".equals(key)) {
-                throw new IllegalArgumentException("""
+                throw new IllegalArgumentException(String.format(Locale.ROOT, """
                     Ordering on a single-value metrics aggregation can only be done on its value. \
-                    Either drop the key (a la "%s") or change it to "value" (a la "%s.value")""".formatted(name(), name()));
+                    Either drop the key (a la "%s") or change it to "value" (a la "%s.value")""", name(), name()));
             }
             return (lhs, rhs) -> Comparators.compareDiscardNaN(metric(lhs), metric(rhs), order == SortOrder.ASC);
         }

server/src/main/java/org/elasticsearch/transport/RemoteConnectionStrategy.java

@@ -431,7 +431,9 @@ public void validate(T value, Map<Setting<?>, Object> settings, boolean isPresen
             ConnectionStrategy modeType = (ConnectionStrategy) settings.get(concrete);
             if (isPresent && modeType.equals(expectedStrategy) == false) {
                 throw new IllegalArgumentException(
-                    "Setting \"%s\" cannot be used with the configured \"%s\" [required=%s, configured=%s]".formatted(
+                    String.format(
+                        Locale.ROOT,
+                        "Setting \"%s\" cannot be used with the configured \"%s\" [required=%s, configured=%s]",
                         key,
                         concrete.getKey(),
                         expectedStrategy.name(),

server/src/test/java/org/elasticsearch/ingest/PipelineConfigurationTests.java

@@ -24,6 +24,7 @@
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
+import java.util.Locale;
 import java.util.function.Predicate;
 
 import static org.hamcrest.Matchers.equalTo;
@@ -98,9 +99,9 @@ public void testGetVersion() {
         {
             // null version
             int version = randomInt();
-            String configJson = """
+            String configJson = String.format(Locale.ROOT, """
                 {"version": %d, "description": "blah", "_meta" : {"foo": "bar"}}
-                """.formatted(version);
+                """, version);
             PipelineConfiguration configuration = new PipelineConfiguration(
                 "1",
                 new BytesArray(configJson.getBytes(StandardCharsets.UTF_8)),

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSuite.java

@@ -24,6 +24,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Locale;
 import java.util.Objects;
 import java.util.Set;
 import java.util.TreeSet;
@@ -167,10 +168,10 @@ private static Stream<String> validateExecutableSections(
             .map(section -> (DoSection) section)
             .filter(section -> false == section.getExpectedWarningHeaders().isEmpty())
             .filter(section -> false == hasSkipFeature("warnings", testSection, setupSection, teardownSection))
-            .map(section -> """
+            .map(section -> String.format(Locale.ROOT, """
                 attempted to add a [do] with a [warnings] section without a corresponding ["skip": "features": "warnings"] \
                 so runners that do not support the [warnings] section can skip the test at line [%d]\
-                """.formatted(section.getLocation().lineNumber));
+                """, section.getLocation().lineNumber));
 
         errors = Stream.concat(
             errors,
@@ -179,11 +180,11 @@ private static Stream<String> validateExecutableSections(
                 .map(section -> (DoSection) section)
                 .filter(section -> false == section.getExpectedWarningHeadersRegex().isEmpty())
                 .filter(section -> false == hasSkipFeature("warnings_regex", testSection, setupSection, teardownSection))
-                .map(section -> """
+                .map(section -> String.format(Locale.ROOT, """
                     attempted to add a [do] with a [warnings_regex] section without a corresponding \
                     ["skip": "features": "warnings_regex"] so runners that do not support the [warnings_regex] \
                     section can skip the test at line [%d]\
-                    """.formatted(section.getLocation().lineNumber))
+                    """, section.getLocation().lineNumber))
         );
 
         errors = Stream.concat(
@@ -193,11 +194,11 @@ private static Stream<String> validateExecutableSections(
                 .map(section -> (DoSection) section)
                 .filter(section -> false == section.getAllowedWarningHeaders().isEmpty())
                 .filter(section -> false == hasSkipFeature("allowed_warnings", testSection, setupSection, teardownSection))
-                .map(section -> """
+                .map(section -> String.format(Locale.ROOT, """
                     attempted to add a [do] with a [allowed_warnings] section without a corresponding \
                     ["skip": "features": "allowed_warnings"] so runners that do not support the [allowed_warnings] \
                     section can skip the test at line [%d]\
-                    """.formatted(section.getLocation().lineNumber))
+                    """, section.getLocation().lineNumber))
         );
 
         errors = Stream.concat(
@@ -207,11 +208,11 @@ private static Stream<String> validateExecutableSections(
                 .map(section -> (DoSection) section)
                 .filter(section -> false == section.getAllowedWarningHeadersRegex().isEmpty())
                 .filter(section -> false == hasSkipFeature("allowed_warnings_regex", testSection, setupSection, teardownSection))
-                .map(section -> """
+                .map(section -> String.format(Locale.ROOT, """
                     attempted to add a [do] with a [allowed_warnings_regex] section without a corresponding \
                     ["skip": "features": "allowed_warnings_regex"] so runners that do not support the [allowed_warnings_regex] \
                     section can skip the test at line [%d]\
-                    """.formatted(section.getLocation().lineNumber))
+                    """, section.getLocation().lineNumber))
         );
 
         errors = Stream.concat(
@@ -221,22 +222,22 @@ private static Stream<String> validateExecutableSections(
                 .map(section -> (DoSection) section)
                 .filter(section -> NodeSelector.ANY != section.getApiCallSection().getNodeSelector())
                 .filter(section -> false == hasSkipFeature("node_selector", testSection, setupSection, teardownSection))
-                .map(section -> """
+                .map(section -> String.format(Locale.ROOT, """
                     attempted to add a [do] with a [node_selector] section without a corresponding \
                     ["skip": "features": "node_selector"] so runners that do not support the [node_selector] section \
                     can skip the test at line [%d]\
-                    """.formatted(section.getLocation().lineNumber))
+                    """, section.getLocation().lineNumber))
         );
 
         errors = Stream.concat(
             errors,
             sections.stream()
                 .filter(section -> section instanceof ContainsAssertion)
                 .filter(section -> false == hasSkipFeature("contains", testSection, setupSection, teardownSection))
-                .map(section -> """
+                .map(section -> String.format(Locale.ROOT, """
                     attempted to add a [contains] assertion without a corresponding ["skip": "features": "contains"] \
                     so runners that do not support the [contains] assertion can skip the test at line [%d]\
-                    """.formatted(section.getLocation().lineNumber))
+                    """, section.getLocation().lineNumber))
         );
 
         errors = Stream.concat(
@@ -246,21 +247,21 @@ private static Stream<String> validateExecutableSections(
                 .map(section -> (DoSection) section)
                 .filter(section -> false == section.getApiCallSection().getHeaders().isEmpty())
                 .filter(section -> false == hasSkipFeature("headers", testSection, setupSection, teardownSection))
-                .map(section -> """
+                .map(section -> String.format(Locale.ROOT, """
                     attempted to add a [do] with a [headers] section without a corresponding ["skip": "features": "headers"] \
                     so runners that do not support the [headers] section can skip the test at line [%d]\
-                    """.formatted(section.getLocation().lineNumber))
+                    """, section.getLocation().lineNumber))
         );
 
         errors = Stream.concat(
             errors,
             sections.stream()
                 .filter(section -> section instanceof CloseToAssertion)
                 .filter(section -> false == hasSkipFeature("close_to", testSection, setupSection, teardownSection))
-                .map(section -> """
+                .map(section -> String.format(Locale.ROOT, """
                     attempted to add a [close_to] assertion without a corresponding ["skip": "features": "close_to"] \
                     so runners that do not support the [close_to] assertion can skip the test at line [%d]\
-                    """.formatted(section.getLocation().lineNumber))
+                    """, section.getLocation().lineNumber))
         );
 
         return errors;