Skip to content

Commit

Permalink
[improvement] Enable NullAway gradle checks (#99)
Browse files Browse the repository at this point in the history
<!-- PR title should start with '[fix]', '[improvement]' or '[break]' if this PR would cause a patch, minor or major SemVer bump. Omit the prefix if this PR doesn't warrant a standalone release. -->

## Before this PR
<!-- Describe the problem you encountered with the current state of the world (or link to an issue) and why it's important to fix now. -->

The safe-logging library did not explicitly annotate `Arg` and its subtypes, so consumers of this library would receive false-positive warnings when using static analysis checks such as [NullAway](https://github.com/uber/NullAway) or IntelliJ nullability checks, for example the below as seen in palantir/tritium#152 where [use of `UnsafeArg.of` with a `@Nullable` value](https://github.com/palantir/tritium/blob/develop/tritium-core/src/main/java/com/palantir/tritium/event/CompositeInvocationEventHandler.java#L142) is mistakenly flagged:

```
warning: [NullAway] passing @nullable parameter 'context' where @nonnull is required
                    UnsafeArg.of("context", context),
                                            ^
```

## After this PR
<!-- Describe at a high-level why this approach is better. -->

Enables [NullAway](https://github.com/uber/NullAway) gradle build checks and appropriately annotates Arg and its subtypes to support proper nullability analysis.

<!-- Reference any existing GitHub issues, e.g. 'fixes #000' or 'relevant to #000' -->

Relevant to palantir/tritium#152 and supersedes #97

The NullAway checks identified one possible issue in safe-logging's `LoggableExceptionAssert`, and this has been corrected.

* 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
```

Another error-prone warning identified a potential issue with `Arg.equals` implementation

* 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
```
  • Loading branch information
schlosna authored and bulldozer-bot[bot] committed Jan 7, 2019
1 parent fbbaffa commit 6841f17
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 8 deletions.
10 changes: 10 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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']
}
})
}

Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -30,7 +31,8 @@ public class LoggableExceptionAssert<T extends Throwable & SafeLoggable>
public LoggableExceptionAssert(T actual) {
super(actual, LoggableExceptionAssert.class);

argsAssert = actual == null ? null : new ArgsAssert(actual.getArgs());
List<Arg<?>> args = actual == null ? Collections.emptyList() : actual.getArgs();
argsAssert = new ArgsAssert(args);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
14 changes: 11 additions & 3 deletions safe-logging/src/main/java/com/palantir/logsafe/Arg.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> 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;
}
Expand All @@ -52,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
Expand Down
6 changes: 4 additions & 2 deletions safe-logging/src/main/java/com/palantir/logsafe/SafeArg.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> extends Arg<T> {

private SafeArg(String name, T value) {
private SafeArg(String name, @Nullable T value) {
super(name, value);
}

public static <T> SafeArg<T> of(String name, T value) {
public static <T> SafeArg<T> of(String name, @Nullable T value) {
return new SafeArg<>(name, value);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> extends Arg<T> {

private UnsafeArg(String name, T value) {
private UnsafeArg(String name, @Nullable T value) {
super(name, value);
}

public static <T> UnsafeArg<T> of(String name, T value) {
public static <T> UnsafeArg<T> of(String name, @Nullable T value) {
return new UnsafeArg<>(name, value);
}

Expand Down
2 changes: 2 additions & 0 deletions versions.props
Original file line number Diff line number Diff line change
@@ -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

Expand Down

0 comments on commit 6841f17

Please sign in to comment.