Skip to content
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

[improvement] Enable NullAway gradle checks #99

Merged
merged 4 commits into from
Jan 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I tried using IntelliJ's regenerate equals method and it gave us exactly this.

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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't change any behaviour because we don't expect people to make their own subclasses of Arg, but better safe than sorry!

}

@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