From 07c28f17ca6020c1cc0fa954d19b9b0cc8d32c1b Mon Sep 17 00:00:00 2001 From: Pranav Date: Fri, 29 Sep 2023 17:00:36 -0700 Subject: [PATCH] Fix missing format strings in calls to DruidException.build (#15056) * Fix the NPE bug in nonStrictFormat * using non null format string * using Assert.assertThrows --- .../sql/resources/SqlStatementResource.java | 2 +- .../msq/sql/resources/SqlTaskResource.java | 2 +- .../msq/util/SqlStatementResourceHelper.java | 2 +- .../org/apache/druid/error/ErrorResponse.java | 2 +- .../druid/error/QueryExceptionCompat.java | 2 +- .../apache/druid/error/ErrorResponseTest.java | 28 +++++++++++++++++++ .../java/util/common/StringUtilsTest.java | 15 ++++++++++ .../sql/calcite/planner/DruidPlanner.java | 6 ++-- 8 files changed, 51 insertions(+), 8 deletions(-) diff --git a/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java b/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java index 97be6dbc3bb9..dd4e08403006 100644 --- a/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java +++ b/extensions-core/multi-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/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlTaskResource.java b/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlTaskResource.java index 187601cea0e6..e9b4c61cef23 100644 --- a/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlTaskResource.java +++ b/extensions-core/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/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/SqlStatementResourceHelper.java b/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/SqlStatementResourceHelper.java index 08bc3dc54d93..86aed98f063e 100644 --- a/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/SqlStatementResourceHelper.java +++ b/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/SqlStatementResourceHelper.java @@ -234,7 +234,7 @@ public static Optional 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 index 7b571cca2719..0c6edf25c10a 100644 --- 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 index 163c7b04cd0b..5782ca55ee0f 100644 --- 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 index 2ddd39aa7dae..16a1ccecec0f 100644 --- 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 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 index 3f2d5713c2f8..5927207ba08c 100644 --- 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 index b77a5fdd498f..e47411361b2e 100644 --- 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()); } }