-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing format strings in calls to DruidException.build #15056
Conversation
@@ -373,16 +373,24 @@ public static String nonStrictFormat(String message, Object... formatArgs) | |||
if (formatArgs == null || formatArgs.length == 0) { | |||
return message; | |||
} | |||
if (message == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of these changes, it would be simpler to just do a Preconditions.checkNotNull(message)
at the start of this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preconditions check will throw an exception again. Some of the exceptions are not having message at all and we are calling nonStrictFormat while making DruidException, esp exception from third party library. As the name says, expectation from nonStrictFormat in this case is that it should just give back some non null string. Best case effort is to append the args to string and give it back.
If we add precondition then we need to safeguard the invoker lines with not null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on comments, I found the instances where we could potentially pass null as formatString. I fixed all of them I could find in this PR. I am still doubtful about adding precondition check to nonStrictFormat as it is used at tons of places. I removed the change here and fixing the invokers.
Anything that is calling IMO, better to fix the above bug than to work around it in Btw, looking at other calls to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how null
could have been passed as message
; but maybe it would be good to mark that arg as @NonNull
in case something could process and uncover its source in the future
or possibly enable:
https://errorprone.info/api/latest/com/google/errorprone/annotations/FormatString.html
so that -DstrictCompile
could possibly catch formatting issues
not sure if it that would have worked in this case...
processing/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java
Fixed
Show fixed
Hide fixed
2813a5e
to
45739b4
Compare
Thank you, I was able to find the potential null at multiple places, fixed all of them I could find. Added test to StringUtilsTest to ensure formatting with string containing |
I've played around a bit with adding https://github.com/kgyrtkirk/druid/tree/formatmethod-formatstring |
great, it would be helpful to open another followup PR after this with annotation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find ! LGTM
Neat, nice idea! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, although the title of the PR is no longer correct. Please update it to something like "Fix missing format strings in calls to DruidException.build"
@@ -386,4 +386,19 @@ public void testJavaStringCompare() | |||
} | |||
} | |||
} | |||
|
|||
@Test(expected = NullPointerException.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe use Assert.assertThrows
instead of expected exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
…5056) * Fix the NPE bug in nonStrictFormat * using non null format string * using Assert.assertThrows
Fixes #15055
Description
PR fix exception eating duck and helps to surface the original exception,
String.format(Locale.ENGLISH, null, 1); or String.format(Locale.ENGLISH, null);
throws NPE and I safeguarding the nonStrictFormat function with null check.
Fixed the bug ...
Renamed the class ...
Added a forbidden-apis entry ...
This PR has: