From 4662eb44cce0b18e493c74b142790fef0c769956 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Wed, 25 May 2022 19:11:08 +0200 Subject: [PATCH 1/3] Catch an exception when formatting a string fails Strings.format method, which is used heavily in logging with Supplier should handle exceptions when a format is incorrect. This will prevent a hard to catch mistakes to blow up in server. Those mistakes are especially hard to detect in logging when a code to create a message might be only executed when logger is debug or trace. Which is not always the case in CI. relates https://github.com/elastic/elasticsearch/pull/87077#discussion_r881794589 relates #86549 --- .../src/main/java/org/elasticsearch/core/Strings.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libs/core/src/main/java/org/elasticsearch/core/Strings.java b/libs/core/src/main/java/org/elasticsearch/core/Strings.java index 6b323c5a2a370..1d27b94ee7837 100644 --- a/libs/core/src/main/java/org/elasticsearch/core/Strings.java +++ b/libs/core/src/main/java/org/elasticsearch/core/Strings.java @@ -21,8 +21,14 @@ public class Strings { * * This method calls {@link String#format(Locale, String, Object...)} * with Locale.ROOT + * If format is incorrect the function will return format without populating + * its variable placeholders. */ public static String format(String format, Object... args) { - return String.format(Locale.ROOT, format, args); + try { + return String.format(Locale.ROOT, format, args); + } catch (Throwable e) { + return format; + } } } From da694539b6a092d37154a9e043a15a6916ecc87a Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Wed, 25 May 2022 19:19:08 +0200 Subject: [PATCH 2/3] Update docs/changelog/87132.yaml --- docs/changelog/87132.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/87132.yaml diff --git a/docs/changelog/87132.yaml b/docs/changelog/87132.yaml new file mode 100644 index 0000000000000..349fa62494885 --- /dev/null +++ b/docs/changelog/87132.yaml @@ -0,0 +1,5 @@ +pr: 87132 +summary: Catch an exception when formatting a string fails +area: Infra/Logging +type: enhancement +issues: [] From a1a168f24dee1a60ce511846bb5be238f74ab0e5 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Wed, 25 May 2022 19:34:10 +0200 Subject: [PATCH 3/3] assert and catch exception --- .../java/org/elasticsearch/core/Strings.java | 5 ++-- .../org/elasticsearch/core/StringsTests.java | 25 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 libs/core/src/test/java/org/elasticsearch/core/StringsTests.java diff --git a/libs/core/src/main/java/org/elasticsearch/core/Strings.java b/libs/core/src/main/java/org/elasticsearch/core/Strings.java index 1d27b94ee7837..4ea0c64ce9d52 100644 --- a/libs/core/src/main/java/org/elasticsearch/core/Strings.java +++ b/libs/core/src/main/java/org/elasticsearch/core/Strings.java @@ -18,7 +18,7 @@ public class Strings { /** * Returns a formatted string using the specified format string and * arguments. - * + *

* This method calls {@link String#format(Locale, String, Object...)} * with Locale.ROOT * If format is incorrect the function will return format without populating @@ -27,7 +27,8 @@ public class Strings { public static String format(String format, Object... args) { try { return String.format(Locale.ROOT, format, args); - } catch (Throwable e) { + } catch (Exception e) { + assert false : "Exception thrown when formatting [" + format + "]. " + e.getClass().getCanonicalName() + ". " + e.getMessage(); return format; } } diff --git a/libs/core/src/test/java/org/elasticsearch/core/StringsTests.java b/libs/core/src/test/java/org/elasticsearch/core/StringsTests.java new file mode 100644 index 0000000000000..74838643ad032 --- /dev/null +++ b/libs/core/src/test/java/org/elasticsearch/core/StringsTests.java @@ -0,0 +1,25 @@ +/* + * 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.core; + +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.equalTo; + +public class StringsTests extends ESTestCase { + + public void testIncorrectPattern() { + AssertionError assertionError = expectThrows(AssertionError.class, () -> Strings.format("%s %s", 1)); + assertThat( + assertionError.getMessage(), + equalTo("Exception thrown when formatting [%s %s]. java.util.MissingFormatArgumentException. Format specifier '%s'") + ); + } + +}