Fix missing format strings in calls to DruidException.build (#15056)

* Fix the NPE bug in nonStrictFormat * using non null format string * using Assert.assertThrows
apache · Sep 30, 2023 · 07c28f1 · 07c28f1
1 parent 86087ce
commit 07c28f1
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 8 deletions.
diff --git a/...ti-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java b/...ti-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java
@@ -217,7 +217,7 @@ public Response doPost(final SqlQuery sqlQuery, @Context final HttpServletReques
       return buildNonOkResponse(
           DruidException.forPersona(DruidException.Persona.DEVELOPER)
                         .ofCategory(DruidException.Category.UNCATEGORIZED)
-                        .build(e.getMessage())
+                        .build("%s", e.getMessage())
       );
     }
     finally {

diff --git a/...e/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlTaskResource.java b/...e/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlTaskResource.java
@@ -170,7 +170,7 @@ public Response doPost(
           sqlQueryId,
           DruidException.forPersona(DruidException.Persona.DEVELOPER)
                         .ofCategory(DruidException.Category.UNCATEGORIZED)
-                        .build(e.getMessage())
+                        .build("%s", e.getMessage())
       );
     }
     finally {

diff --git a/...multi-stage-query/src/main/java/org/apache/druid/msq/util/SqlStatementResourceHelper.java b/...multi-stage-query/src/main/java/org/apache/druid/msq/util/SqlStatementResourceHelper.java
@@ -234,7 +234,7 @@ public static Optional<SqlStatementResult> getExceptionPayload(
           null,
           DruidException.forPersona(DruidException.Persona.DEVELOPER)
                         .ofCategory(DruidException.Category.UNCATEGORIZED)
-                        .build(taskResponse.getStatus().getErrorMsg()).toErrorResponse()
+                        .build("%s", taskResponse.getStatus().getErrorMsg()).toErrorResponse()
       ));
     }
 

diff --git a/processing/src/main/java/org/apache/druid/error/ErrorResponse.java b/processing/src/main/java/org/apache/druid/error/ErrorResponse.java
@@ -204,7 +204,7 @@ protected DruidException makeException(DruidException.DruidExceptionBuilder bob)
             {
               return bob.forPersona(cause.getTargetPersona())
                         .ofCategory(cause.getCategory())
-                        .build(cause, cause.getMessage())
+                        .build(cause, "%s", cause.getMessage())
                         .withContext(cause.getContext());
             }
           }

diff --git a/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java b/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java
@@ -47,7 +47,7 @@ protected DruidException makeException(DruidException.DruidExceptionBuilder bob)
   {
     return bob.forPersona(DruidException.Persona.OPERATOR)
               .ofCategory(convertFailType(exception.getFailType()))
-              .build(exception, exception.getMessage())
+              .build(exception, "%s", exception.getMessage())
               .withContext("host", exception.getHost())
               .withContext("errorClass", exception.getErrorClass())
               .withContext("legacyErrorCode", exception.getErrorCode());

diff --git a/processing/src/test/java/org/apache/druid/error/ErrorResponseTest.java b/processing/src/test/java/org/apache/druid/error/ErrorResponseTest.java
@@ -107,4 +107,32 @@ public void testQueryExceptionCompat()
             .expectMessageIs("Query did not complete within configured timeout period. You can increase query timeout or tune the performance of query.")
     );
   }
+  @Test
+  public void testQueryExceptionCompatWithNullMessage()
+  {
+    ErrorResponse response = new ErrorResponse(DruidException.fromFailure(new QueryExceptionCompat(new QueryTimeoutException(
+        null,
+        "hostname"
+    ))));
+    final Map<String, Object> asMap = response.getAsMap();
+    MatcherAssert.assertThat(
+        asMap,
+        DruidMatchers.mapMatcher(
+            "error",
+            "Query timeout",
+
+            "errorCode",
+            "legacyQueryException",
+
+            "persona",
+            "OPERATOR",
+
+            "category",
+            "TIMEOUT",
+
+            "errorMessage",
+            "null"
+        )
+    );
+  }
 }
diff --git a/processing/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java b/processing/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java
@@ -386,4 +386,19 @@ public void testJavaStringCompare()
       }
     }
   }
+
+  @Test()
+  public void testNonStrictFormatWithNullMessage()
+  {
+    Assert.assertThrows(NullPointerException.class, () -> StringUtils.nonStrictFormat(null, 1, 2));
+  }
+
+  @Test
+  public void testNonStrictFormatWithStringContainingPercent()
+  {
+    Assert.assertEquals(
+        "some string containing % %s %d %f",
+        StringUtils.nonStrictFormat("%s", "some string containing % %s %d %f")
+    );
+  }
 }
diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
@@ -345,7 +345,7 @@ public static DruidException translateException(Exception e)
           return DruidException.forPersona(DruidException.Persona.USER)
                                .ofCategory(DruidException.Category.INVALID_INPUT)
                                .withErrorCode("invalidInput")
-                               .build(inner, inner.getMessage()).withContext("sourceType", "sql");
+                               .build(inner, "%s", inner.getMessage()).withContext("sourceType", "sql");
         } else {
           final String theUnexpectedToken = getUnexpectedTokenString(parseException);
 
@@ -390,14 +390,14 @@ public static DruidException translateException(Exception e)
     catch (RelOptPlanner.CannotPlanException inner) {
       return DruidException.forPersona(DruidException.Persona.USER)
                            .ofCategory(DruidException.Category.INVALID_INPUT)
-                           .build(inner, inner.getMessage());
+                           .build(inner, "%s", inner.getMessage());
     }
     catch (Exception inner) {
       // Anything else. Should not get here. Anything else should already have
       // been translated to a DruidException unless it is an unexpected exception.
       return DruidException.forPersona(DruidException.Persona.ADMIN)
                            .ofCategory(DruidException.Category.UNCATEGORIZED)
-                           .build(inner, inner.getMessage());
+                           .build(inner, "%s", inner.getMessage());
     }
   }