[improvement] Enforce guava and objects calls with no parameters and constant strings use logsafe #517

dsd987 · 2019-01-21T20:51:12Z

Before this PR

There is no standardization in the use of basic calls to verify conditions like guava's Preconditions.check* or Objects.requireNonNull.

After this PR

Simple use cases of guava Preconditions.check* and Objects.requireNonNull are now required to be changed to use logsafe variants. This will help to standardize calls where it is clear there can be a direct one-for-one replacement made with logsafe calls.

dsd987 · 2019-01-21T20:52:31Z

To address #507

carterkozak · 2019-01-21T20:54:36Z

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsTests.java

@@ -62,8 +75,6 @@ public final void validGuava() throws Exception {
                "class Test {",
                "  private static final String compileTimeConstant = \"constant\";",
                "  void f(boolean bArg, int iArg, Object oArg) {",
-                "    Preconditions.checkArgument(bArg);",
-                "    Preconditions.checkArgument(bArg, \"message\");",


I don't think this should be modified, it is valid for the existing test, just not the new check

carterkozak · 2019-01-21T20:57:12Z

...rone/src/main/java/com/palantir/baseline/errorprone/GuavaPreconditionsEnforceLogSafeUse.java

+
+@AutoService(BugChecker.class)
+@BugPattern(
+        name = "GuavaPreconditionsEnforceLogSafeUse",


We're really checking one thing: functionality that can be replaced with safe-logging utilities without losing any information/utility. I'd prefer to have a single check for that.

Based on the existing PreferSafeLoggableExceptions check we should call it PreferSafeLoggingPreconditions.

carterkozak · 2019-01-21T20:58:22Z

...rone/src/main/java/com/palantir/baseline/errorprone/GuavaPreconditionsEnforceLogSafeUse.java

+@BugPattern(
+        name = "GuavaPreconditionsEnforceLogSafeUse",
+        category = BugPattern.Category.ONE_OFF,
+        severity = BugPattern.SeverityLevel.ERROR,


Based on PreferSafeLoggableExceptions we should use SeverityLevel.SUGGESTION by default, in certain large internal codebases we will override it to ERROR. Longer term we should set the new check and PreferSafeLoggableExceptions to error, but I'd like to decouple that from this change.

carterkozak · 2019-01-21T21:00:19Z

...line-error-prone/src/main/java/com/palantir/baseline/errorprone/EnforceLogSafeUseFormat.java

+
+        if (args.size() == 2) {
+            ExpressionTree messageArg = args.get(1);
+            if (!compileTimeConstExpressionMatcher.matches(messageArg, state)) {


This needs to check the type of args[1], otherwise we'll get false positives on Objects.requireNonNull(obj, () -> calculateExpensiveMessage()) using the supplier overload.

iamdanfox · 2019-01-22T11:23:26Z

...ine-error-prone/src/main/java/com/palantir/baseline/errorprone/ObjectsEnforceLogSafeUse.java

+        category = BugPattern.Category.ONE_OFF,
+        severity = BugPattern.SeverityLevel.ERROR,
+        summary = "Objects.requireNonNull(Object) and Objects.requireNonNull(Object, String) methods with a "
+                + "constant message and no parameters should use logsafe Preconditions.checkNotNull(Object, String)")


might as well give the FQN: com.palantir.logsafe.Preconditions#checkNotNull(Object, String)

Also I'd recommend giving a little hint as to why error-prone is recommending the change, e.g. the PreferSafeLoggableExceptions rule says "Throw SafeLoggable exceptions to ensure the exception message will not be redacted"

schlosna · 2019-01-22T12:06:57Z

...rone/src/main/java/com/palantir/baseline/errorprone/GuavaPreconditionsEnforceLogSafeUse.java

+            Matchers.anyOf(
+                    MethodMatchers.staticMethod()
+                            .onClass("com.google.common.base.Preconditions")
+                            .withNameMatching(Pattern.compile("checkArgument|checkState|checkNotNull")));


Probably a separate PR, but would be good to check Guava’s Verify.verify & Verify.verifyNotNull as well as commons-lang Validate.*?

That would be difficult -- guava Preconditions throws standard IllegalState/IllegalArg/NullPointer exceptions, but these methods throw library-specific exception types, so a migration could break existing catches.

I think we should suggest against using them in favor of safe-logging preconditions, but I'd prefer to do so in a different check.

👍 Makes sense for Guava's Verify and I expect much lower usage of that.

The commons-lang Validate is much more heavily used in some places, and those should be straightforward replacements by equivalent Safe preconditions

I had though lang3 validate used custom exception types, thanks for the correction! In that case it makes sense to add common methods to this check. Let's add it in a separate PR.

carterkozak · 2019-01-23T17:11:31Z

...ror-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLoggingPreconditions.java

+                            .replace(tree, String.format(
+                                    "com.palantir.logsafe.Preconditions.checkNotNull(%s)", tree.getArguments()))
+                            .build())
+                    .build();


Can we combine these? The message is different, but doesn't need to be because we output a suggested change which illustrates the current and proposed states. We can add a function which maps from a given method name to a safe-logging Preconditions method name instead.

carterkozak · 2019-01-23T17:12:24Z

...ror-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLoggingPreconditions.java

+
+        if (args.size() == 2) {
+            ExpressionTree messageArg = args.get(1);
+            if (!messageArg.getKind().equals(Tree.Kind.STRING_LITERAL)


Do we actually want to test that this is a string literal? Will that catch references to compile-time constant methods?

Good catch! Yeah, that won't work with compile-time constants. Will fix.

carterkozak · 2019-01-23T17:13:20Z

...ror-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLoggingPreconditions.java

+        if (args.size() == 2) {
+            ExpressionTree messageArg = args.get(1);
+            if (!messageArg.getKind().equals(Tree.Kind.STRING_LITERAL)
+                    || !compileTimeConstExpressionMatcher.matches(messageArg, state)) {


I think we should check the method matchers prior to this, I imagine the compile time constant matcher is more expensive than checking method names, and this will fire for all 2-arg methods with a string as the second argument.

carterkozak · 2019-01-23T18:51:42Z

This check should be added to the list of exclusions for gradle plugins, since those don't use safe-logging

…s and constant strings

dsd987 · 2019-01-23T19:16:01Z

README.md

+- `PreferSafeLoggingPreconditions`: Users should use the safe-logging versions of Precondition checks for standardization when there is equivalent functionality
+    ```diff
+    -com.google.common.base.Preconditions.checkNotNull(variable, "message");
+    -com.palantir.logsafe.Preconditions.checkNotNull(variable, "message"); // equivalent functionality is available in the safe-logging variant


Should be a "+"

carterkozak · 2019-01-23T19:20:50Z

Please don't force-push, that makes it hard to review diffs!

iamdanfox

Looks solid to me :) I'm looking forward to actually exposing those suggested fixes!

carterkozak

lgtm, thanks @dsd987!

dsd987 requested a review from a team as a code owner January 21, 2019 20:51

carterkozak requested changes Jan 21, 2019

View reviewed changes

iamdanfox reviewed Jan 22, 2019

View reviewed changes

schlosna reviewed Jan 22, 2019

View reviewed changes

dsd987 force-pushed the dd/enforceLogSafe branch 2 times, most recently from f484ec9 to 36f119a Compare January 23, 2019 16:04

carterkozak reviewed Jan 23, 2019

View reviewed changes

Suggest logsafe alternatives for precondition calls with no parameter…

8a31435

…s and constant strings

dsd987 force-pushed the dd/enforceLogSafe branch from 36f119a to 8a31435 Compare January 23, 2019 19:14

dsd987 commented Jan 23, 2019

View reviewed changes

David Dunn added 2 commits January 23, 2019 15:13

Fixing readme and matching logic.

d784e86

Addressing nits and adding some mixed tests.

56752b2

iamdanfox approved these changes Jan 23, 2019

View reviewed changes

carterkozak approved these changes Jan 24, 2019

View reviewed changes

carterkozak added the merge when ready label Jan 24, 2019

iamdanfox merged commit fade540 into palantir:develop Jan 24, 2019

pkoenig10 mentioned this pull request Jul 18, 2019

Promote safe logging checks from SUGGESTION -> WARNING #704

Merged

carterkozak mentioned this pull request Aug 16, 2019

FR: Add check that simple uses of Objects.requireNonNull and Preconditions.check* use safe-logging #507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improvement] Enforce guava and objects calls with no parameters and constant strings use logsafe #517

[improvement] Enforce guava and objects calls with no parameters and constant strings use logsafe #517

dsd987 commented Jan 21, 2019

dsd987 commented Jan 21, 2019

carterkozak Jan 21, 2019

carterkozak Jan 21, 2019

carterkozak Jan 21, 2019

carterkozak Jan 21, 2019

iamdanfox Jan 22, 2019

iamdanfox Jan 22, 2019

schlosna Jan 22, 2019

carterkozak Jan 22, 2019

schlosna Jan 22, 2019

carterkozak Jan 22, 2019

carterkozak Jan 23, 2019

dsd987 Jan 23, 2019

carterkozak Jan 23, 2019

dsd987 Jan 23, 2019

carterkozak Jan 23, 2019

dsd987 Jan 23, 2019

carterkozak commented Jan 23, 2019

dsd987 Jan 23, 2019

carterkozak commented Jan 23, 2019

iamdanfox left a comment

carterkozak left a comment

[improvement] Enforce guava and objects calls with no parameters and constant strings use logsafe #517

[improvement] Enforce guava and objects calls with no parameters and constant strings use logsafe #517

Conversation

dsd987 commented Jan 21, 2019

Before this PR

After this PR

dsd987 commented Jan 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carterkozak commented Jan 23, 2019

Choose a reason for hiding this comment

carterkozak commented Jan 23, 2019

iamdanfox left a comment

Choose a reason for hiding this comment

carterkozak left a comment

Choose a reason for hiding this comment