From 827acdc29c119c9a0285a82f3eb040141b8e1931 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 2 Jan 2019 13:24:04 -0500 Subject: [PATCH 1/4] Enable nullaway checks --- build.gradle | 10 ++++++++++ versions.props | 2 ++ 2 files changed, 12 insertions(+) diff --git a/build.gradle b/build.gradle index a82748c9..6fccbf4e 100644 --- a/build.gradle +++ b/build.gradle @@ -50,5 +50,15 @@ subprojects { } } tasks.check.dependsOn(javadoc) + + plugins.withId('com.palantir.baseline-error-prone', { + dependencies { + compileOnly 'com.google.code.findbugs:jsr305' + annotationProcessor 'com.uber.nullaway:nullaway' + } + tasks.withType(JavaCompile) { + options.errorprone.errorproneArgs += ['-XepOpt:NullAway:AnnotatedPackages=com.palantir'] + } + }) } diff --git a/versions.props b/versions.props index 9b0f7f49..83e77aba 100644 --- a/versions.props +++ b/versions.props @@ -1,4 +1,6 @@ com.google.code.findbugs:jsr305 = 3.0.2 +com.google.guava:guava = 23.5-jre +com.uber.nullaway:nullaway = 0.6.4 junit:junit = 4.12 org.assertj:assertj-core = 3.11.1 From 7ba9c47453d306df78af4cdae08ca23e36faa88f Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 2 Jan 2019 13:30:29 -0500 Subject: [PATCH 2/4] Add @Nullable Arg value Include appropriate @Nullable and @Nonnull annotations on Arg and its subtypes to properly support NullAway checks. --- safe-logging/src/main/java/com/palantir/logsafe/Arg.java | 9 ++++++++- .../src/main/java/com/palantir/logsafe/SafeArg.java | 6 ++++-- .../src/main/java/com/palantir/logsafe/UnsafeArg.java | 6 ++++-- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/safe-logging/src/main/java/com/palantir/logsafe/Arg.java b/safe-logging/src/main/java/com/palantir/logsafe/Arg.java index 35e0ea5b..4ea89a61 100644 --- a/safe-logging/src/main/java/com/palantir/logsafe/Arg.java +++ b/safe-logging/src/main/java/com/palantir/logsafe/Arg.java @@ -18,24 +18,31 @@ import java.io.Serializable; import java.util.Objects; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; /** A wrapper around an argument used to build a formatted message. */ public abstract class Arg implements Serializable { + @Nonnull private final String name; + + @Nullable private final T value; - protected Arg(String name, T value) { + protected Arg(String name, @Nullable T value) { this.name = Objects.requireNonNull(name, "name may not be null"); this.value = value; } /** A name describing this argument. */ + @Nonnull public final String getName() { return name; } /** The value of this argument (which may be {@code null}). */ + @Nullable public final T getValue() { return value; } diff --git a/safe-logging/src/main/java/com/palantir/logsafe/SafeArg.java b/safe-logging/src/main/java/com/palantir/logsafe/SafeArg.java index fe77d1c7..5262fff4 100644 --- a/safe-logging/src/main/java/com/palantir/logsafe/SafeArg.java +++ b/safe-logging/src/main/java/com/palantir/logsafe/SafeArg.java @@ -16,14 +16,16 @@ package com.palantir.logsafe; +import javax.annotation.Nullable; + /** A wrapper around an argument known to be safe for logging. */ public final class SafeArg extends Arg { - private SafeArg(String name, T value) { + private SafeArg(String name, @Nullable T value) { super(name, value); } - public static SafeArg of(String name, T value) { + public static SafeArg of(String name, @Nullable T value) { return new SafeArg<>(name, value); } diff --git a/safe-logging/src/main/java/com/palantir/logsafe/UnsafeArg.java b/safe-logging/src/main/java/com/palantir/logsafe/UnsafeArg.java index e0cb2195..5c3145bd 100644 --- a/safe-logging/src/main/java/com/palantir/logsafe/UnsafeArg.java +++ b/safe-logging/src/main/java/com/palantir/logsafe/UnsafeArg.java @@ -16,14 +16,16 @@ package com.palantir.logsafe; +import javax.annotation.Nullable; + /** A wrapper around an argument that is not safe for logging. */ public final class UnsafeArg extends Arg { - private UnsafeArg(String name, T value) { + private UnsafeArg(String name, @Nullable T value) { super(name, value); } - public static UnsafeArg of(String name, T value) { + public static UnsafeArg of(String name, @Nullable T value) { return new UnsafeArg<>(name, value); } From f9286e61ca5a5890890fbc77d6224debe34b59a6 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 2 Jan 2019 13:29:39 -0500 Subject: [PATCH 3/4] Handle null LoggableExceptionAssert arg Addresses NullAway warnings: > Task :preconditions-assertj:compileJava /Volumes/git/safe-logging/preconditions-assertj/src/main/java/com/palantir/logsafe/testing/LoggableExceptionAssert.java:30: warning: [NullAway] initializer method does not guarantee @NonNull field argsAssert is initialized along all control-flow paths (remember to check for exceptions or early returns). public LoggableExceptionAssert(T actual) { ^ (see http://t.uber.com/nullaway ) /Volumes/git/safe-logging/preconditions-assertj/src/main/java/com/palantir/logsafe/testing/LoggableExceptionAssert.java:33: warning: [NullAway] assigning @Nullable expression to @NonNull field argsAssert = actual == null ? null : new ArgsAssert(actual.getArgs()); ^ (see http://t.uber.com/nullaway ) 2 warnings --- .../palantir/logsafe/testing/LoggableExceptionAssert.java | 4 +++- .../logsafe/testing/LoggableExceptionAssertionsTest.java | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/preconditions-assertj/src/main/java/com/palantir/logsafe/testing/LoggableExceptionAssert.java b/preconditions-assertj/src/main/java/com/palantir/logsafe/testing/LoggableExceptionAssert.java index a8fc5a24..21ba351b 100755 --- a/preconditions-assertj/src/main/java/com/palantir/logsafe/testing/LoggableExceptionAssert.java +++ b/preconditions-assertj/src/main/java/com/palantir/logsafe/testing/LoggableExceptionAssert.java @@ -18,6 +18,7 @@ import com.palantir.logsafe.Arg; import com.palantir.logsafe.SafeLoggable; +import java.util.Collections; import java.util.List; import org.assertj.core.api.AbstractThrowableAssert; import org.assertj.core.api.ListAssert; @@ -30,7 +31,8 @@ public class LoggableExceptionAssert public LoggableExceptionAssert(T actual) { super(actual, LoggableExceptionAssert.class); - argsAssert = actual == null ? null : new ArgsAssert(actual.getArgs()); + List> args = actual == null ? Collections.emptyList() : actual.getArgs(); + argsAssert = new ArgsAssert(args); } /** diff --git a/preconditions-assertj/src/test/java/com/palantir/logsafe/testing/LoggableExceptionAssertionsTest.java b/preconditions-assertj/src/test/java/com/palantir/logsafe/testing/LoggableExceptionAssertionsTest.java index 318f0d31..e43e51a7 100755 --- a/preconditions-assertj/src/test/java/com/palantir/logsafe/testing/LoggableExceptionAssertionsTest.java +++ b/preconditions-assertj/src/test/java/com/palantir/logsafe/testing/LoggableExceptionAssertionsTest.java @@ -156,6 +156,13 @@ public void failIncorrectAssertThatLoggableExceptionThrownBy() { })); } + @Test + public void nullArgsAssert() { + assertThatThrownBy(() -> new LoggableExceptionAssert<>(null).isNotNull()) + .isInstanceOf(AssertionError.class) + .hasMessageContaining("Expecting actual not to be null"); + } + private static class LoggableException extends Throwable implements SafeLoggable { LoggableException() { super("test message"); From 06eb1233d0481b619d07b9ead0ed78fc17c29a89 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 2 Jan 2019 13:31:38 -0500 Subject: [PATCH 4/4] Arg.equals checks isSafeForLogging Addresses EqualsGetClass error-prone finding: > Task :safe-logging:compileJava /Volumes/git/safe-logging/safe-logging/src/main/java/com/palantir/logsafe/Arg.java:51: warning: [EqualsGetClass] Overriding Object#equals in a non-final class by using getClass rather than instanceof breaks substitutability of subclasses. public final boolean equals(Object other) { ^ (see https://errorprone.info/bugpattern/EqualsGetClass) Did you mean 'if (!(other instanceof Arg)) {'? 1 warning --- safe-logging/src/main/java/com/palantir/logsafe/Arg.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/safe-logging/src/main/java/com/palantir/logsafe/Arg.java b/safe-logging/src/main/java/com/palantir/logsafe/Arg.java index 4ea89a61..83175a4a 100644 --- a/safe-logging/src/main/java/com/palantir/logsafe/Arg.java +++ b/safe-logging/src/main/java/com/palantir/logsafe/Arg.java @@ -59,12 +59,13 @@ public final boolean equals(Object other) { if (this == other) { return true; } - if (other == null || getClass() != other.getClass()) { + if (!(other instanceof Arg)) { return false; } Arg arg = (Arg) other; return Objects.equals(name, arg.name) - && Objects.equals(value, arg.value); + && Objects.equals(value, arg.value) + && (isSafeForLogging() == arg.isSafeForLogging()); } @Override