Enable NullAway for all non-test sources

Motivation: This is a follow-up PR for line#5820 It would be nice if we can fail our build if there is any potential `null` dereference, so our users have much less chance of getting `NullPointerException`. Modifications: - Enabled NullAway for all projects - Added `assert` sentences and `@Nullable` annotations wherever applicable - Fixed a few minor potential `null` dereferences Result: - Theoretically, there should be no chance of `NullPointerException` anymore. - However, note that some NullPointerExceptions might have become `AssertionError`s, since we added a bunch of assertions in this PR, so please review carefully :-) - Closes line#1680 - Closes line#5184
trustin · Jul 25, 2024 · d564251 · d564251
1 parent cc400f3
commit d564251
Show file tree

Hide file tree

Showing 111 changed files with 371 additions and 118 deletions.
diff --git a/...rc/main/java/com/linecorp/armeria/server/annotation/processor/DocumentationProcessor.java b/...rc/main/java/com/linecorp/armeria/server/annotation/processor/DocumentationProcessor.java
@@ -38,6 +38,7 @@
 import javax.lang.model.SourceVersion;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.QualifiedNameable;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.element.VariableElement;
 import javax.tools.Diagnostic.Kind;
@@ -148,7 +149,9 @@ private void processAnnotation(TypeElement annotationElement, RoundEnvironment r
     }
 
     private void processMethod(ExecutableElement method) throws IOException {
-        final String className = ((TypeElement) method.getEnclosingElement()).getQualifiedName().toString();
+        final QualifiedNameable enclosingElement = (QualifiedNameable) method.getEnclosingElement();
+        assert enclosingElement != null;
+        final String className = enclosingElement.getQualifiedName().toString();
         final Properties properties = readProperties(className);
         final String docComment = processingEnv.getElementUtils().getDocComment(method);
         if (docComment == null || !docComment.contains("@param")) {

diff --git a/...e6/src/main/java/com/linecorp/armeria/common/brave/RequestContextCurrentTraceContext.java b/...e6/src/main/java/com/linecorp/armeria/common/brave/RequestContextCurrentTraceContext.java
@@ -174,7 +174,7 @@ public Scope newScope(@Nullable TraceContext currentSpan) {
 
     @UnstableApi
     @Override
-    public Scope decorateScope(TraceContext context, Scope scope) {
+    public Scope decorateScope(@Nullable TraceContext context, Scope scope) {
         // If a `Scope` is decorated, `ScopeDecorator`s populate some contexts as such as MDC, which are stored
         // to a thread-local. The activated contexts will be removed when `decoratedScope.close()` is called.
         // If `Scope.NOOP` is specified, CurrentTraceContext.decorateScope() performs nothing.

diff --git a/brave/brave6/src/main/java/com/linecorp/armeria/internal/common/brave/TraceContextUtil.java b/brave/brave6/src/main/java/com/linecorp/armeria/internal/common/brave/TraceContextUtil.java
@@ -39,7 +39,7 @@ public static TraceContext traceContext(RequestContext ctx) {
         return ctx.attr(TRACE_CONTEXT_KEY);
     }
 
-    public static void setTraceContext(RequestContext ctx, TraceContext traceContext) {
+    public static void setTraceContext(RequestContext ctx, @Nullable TraceContext traceContext) {
         ctx.setAttr(TRACE_CONTEXT_KEY, traceContext);
     }
 

diff --git a/build.gradle b/build.gradle
@@ -186,15 +186,12 @@ configure(projectsWithFlags('java')) {
         tasks.withType(JavaCompile) {
             options.errorprone.excludedPaths = '.*/gen-src/.*'
             options.errorprone.nullaway {
-                if (name.toLowerCase().contains("test")) {
-                    // Disable NullAway for tests for now.
+                if (name.contains("Test") || name.contains("Jmh")) {
+                    // Disable NullAway for tests and benchmarks for now.
                     disable()
                 } else if (name.matches(/compileJava[0-9]+.*/)) {
                     // Disable MR-JAR classes which seem to confuse NullAway and break the build.
                     disable()
-                } else if (project != project(':core')) {
-                    // TODO(trustin): Enable NullAway for all projects once we fix all violations.
-                    warn()
                 } else {
                     error()
                     assertsEnabled = true

diff --git a/consul/src/main/java/com/linecorp/armeria/internal/consul/HealthClient.java b/consul/src/main/java/com/linecorp/armeria/internal/consul/HealthClient.java
@@ -110,6 +110,16 @@ CompletableFuture<List<Endpoint>> healthyEndpoints(String serviceName, @Nullable
 
     @Nullable
     private static Endpoint toEndpoint(HealthService healthService) {
+        if (healthService.service == null) {
+            return null;
+        }
+        if (healthService.node == null) {
+            return null;
+        }
+
+        assert healthService.service != null;
+        assert healthService.node != null;
+
         if (!Strings.isNullOrEmpty(healthService.service.address)) {
             return Endpoint.of(healthService.service.address, healthService.service.port);
         } else if (!Strings.isNullOrEmpty(healthService.node.address)) {
@@ -122,59 +132,74 @@ private static Endpoint toEndpoint(HealthService healthService) {
     @JsonIgnoreProperties(ignoreUnknown = true)
     @JsonInclude(Include.NON_NULL)
     private static final class HealthService {
+        @Nullable
         @JsonProperty("Node")
         Node node;
 
+        @Nullable
         @JsonProperty("Service")
         Service service;
     }
 
     @JsonIgnoreProperties(ignoreUnknown = true)
     @JsonInclude(Include.NON_NULL)
     private static final class Node {
+        @Nullable
         @JsonProperty("ID")
         String id;
 
+        @Nullable
         @JsonProperty("Node")
         String node;
 
+        @Nullable
         @JsonProperty("Address")
         String address;
 
+        @Nullable
         @JsonProperty("Datacenter")
         String datacenter;
 
+        @Nullable
         @JsonProperty("TaggedAddresses")
         Object taggedAddresses;
 
+        @Nullable
         @JsonProperty("Meta")
         Map<String, Object> meta;
     }
 
     @JsonIgnoreProperties(ignoreUnknown = true)
     @JsonInclude(Include.NON_NULL)
     public static final class Service {
+        @Nullable
         @JsonProperty("ID")
         String id;
 
+        @Nullable
         @JsonProperty("Service")
         String service;
 
+        @Nullable
         @JsonProperty("Tags")
         String[] tags;
 
+        @Nullable
         @JsonProperty("Address")
         String address;
 
+        @Nullable
         @JsonProperty("TaggedAddresses")
         Object taggedAddresses;
 
+        @Nullable
         @JsonProperty("Meta")
         Map<String, Object> meta;
 
         @JsonProperty("Port")
         int port;
 
+        @Nullable
         @JsonProperty("Weights")
         Map<String, Object> weights;
     }

diff --git a/dropwizard2/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaConfigurationUtil.java b/dropwizard2/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaConfigurationUtil.java
@@ -127,7 +127,10 @@ static void configureServer(ServerBuilder serverBuilder, ArmeriaSettings setting
         }
 
         if (settings.getPorts().isEmpty()) {
-            serverBuilder.port(new ServerPort(DEFAULT_PORT.getPort(), DEFAULT_PORT.getProtocols()));
+            final int port = DEFAULT_PORT.getPort();
+            final List<SessionProtocol> protocols = DEFAULT_PORT.getProtocols();
+            assert protocols != null;
+            serverBuilder.port(new ServerPort(port, protocols));
         } else {
             configurePorts(serverBuilder, settings.getPorts());
         }
@@ -445,8 +448,9 @@ private static void configureAccessLog(ServerBuilder serverBuilder, AccessLog ac
         } else if ("combined".equals(accessLog.getType())) {
             serverBuilder.accessLogWriter(AccessLogWriter.combined(), true);
         } else if ("custom".equals(accessLog.getType())) {
-            serverBuilder
-                    .accessLogWriter(AccessLogWriter.custom(accessLog.getFormat()), true);
+            final String format = accessLog.getFormat();
+            assert format != null;
+            serverBuilder.accessLogWriter(AccessLogWriter.custom(format), true);
         }
     }
 

diff --git a/dropwizard2/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaServerFactory.java b/dropwizard2/src/main/java/com/linecorp/armeria/dropwizard/ArmeriaServerFactory.java
@@ -64,6 +64,7 @@ class ArmeriaServerFactory extends AbstractServerFactory {
     public static final String TYPE = "armeria";
     private static final Logger logger = LoggerFactory.getLogger(ArmeriaServerFactory.class);
 
+    @Nullable
     @JsonUnwrapped
     private @Valid ArmeriaSettings armeriaSettings;
 
@@ -80,6 +81,7 @@ class ArmeriaServerFactory extends AbstractServerFactory {
     @JsonProperty
     private boolean jerseyEnabled = true;
 
+    @Nullable
     @JsonIgnore
     public ServerBuilder getServerBuilder() {
         return serverBuilder;
@@ -184,6 +186,7 @@ private ScheduledThreadPoolExecutor newBlockingTaskExecutor() {
         return blockingTaskExecutor;
     }
 
+    @Nullable
     ArmeriaSettings getArmeriaSettings() {
         return armeriaSettings;
     }

diff --git a/dropwizard2/src/main/java/com/linecorp/armeria/dropwizard/ManagedArmeriaServer.java b/dropwizard2/src/main/java/com/linecorp/armeria/dropwizard/ManagedArmeriaServer.java
@@ -65,6 +65,7 @@ class ManagedArmeriaServer<T extends Configuration> implements Managed {
     public void start() throws Exception {
         logger.trace("Getting Armeria Server Builder");
         final ServerBuilder sb = ((ArmeriaServerFactory) serverFactory).getServerBuilder();
+        assert sb != null;
         logger.trace("Calling Server Configurator");
         serverConfigurator.configure(sb);
         server = sb.build();

diff --git a/eureka/src/main/java/com/linecorp/armeria/client/eureka/EurekaEndpointGroup.java b/eureka/src/main/java/com/linecorp/armeria/client/eureka/EurekaEndpointGroup.java
@@ -21,6 +21,7 @@
 import java.io.IOException;
 import java.net.URI;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import java.util.StringJoiner;
 import java.util.concurrent.CompletableFuture;
@@ -284,7 +285,7 @@ private static Function<byte[], List<Endpoint>> responseConverter(
                 } else if (appName != null) {
                     filter = instanceInfo -> appName.equals(instanceInfo.getAppName());
                 } else {
-                    filter = instanceInfo -> instanceId.equals(instanceInfo.getInstanceId());
+                    filter = instanceInfo -> Objects.equals(instanceId, instanceInfo.getInstanceId());
                 }
             }
             final StringJoiner joiner = new StringJoiner(",");

diff --git a/eureka/src/main/java/com/linecorp/armeria/internal/common/eureka/InstanceInfo.java b/eureka/src/main/java/com/linecorp/armeria/internal/common/eureka/InstanceInfo.java
@@ -43,6 +43,7 @@ public final class InstanceInfo {
 
     private static final Logger logger = LoggerFactory.getLogger(InstanceInfo.class);
 
+    @Nullable
     private final String instanceId;
 
     @Nullable

diff --git a/eureka/src/main/java/com/linecorp/armeria/server/eureka/EurekaUpdatingListener.java b/eureka/src/main/java/com/linecorp/armeria/server/eureka/EurekaUpdatingListener.java
@@ -133,6 +133,7 @@ public static EurekaUpdatingListenerBuilder builder(
 
     private final EurekaWebClient client;
     private final InstanceInfo initialInstanceInfo;
+    @Nullable
     private InstanceInfo instanceInfo;
     @Nullable
     private volatile ScheduledFuture<?> heartBeatFuture;
@@ -335,8 +336,9 @@ public void serverStopping(Server server) throws Exception {
         if (heartBeatFuture != null) {
             heartBeatFuture.cancel(false);
         }
+        final InstanceInfo instanceInfo = this.instanceInfo;
         final String appName = this.appName;
-        if (appName != null) {
+        if (instanceInfo != null && appName != null) {
             final String instanceId = instanceInfo.getInstanceId();
             assert instanceId != null;
             client.cancel(appName, instanceId).aggregate().handle((res, cause) -> {

diff --git a/...col/src/main/java/com/linecorp/armeria/common/graphql/protocol/DefaultGraphqlRequest.java b/...col/src/main/java/com/linecorp/armeria/common/graphql/protocol/DefaultGraphqlRequest.java
@@ -44,6 +44,7 @@ public String query() {
         return query;
     }
 
+    @Nullable
     @Override
     public String operationName() {
         return operationName;

diff --git a/graphql/src/main/java/com/linecorp/armeria/server/graphql/DefaultGraphqlErrorHandler.java b/graphql/src/main/java/com/linecorp/armeria/server/graphql/DefaultGraphqlErrorHandler.java
@@ -22,6 +22,8 @@
 
 import javax.annotation.Nonnull;
 
+import com.google.common.collect.ImmutableMap;
+
 import com.linecorp.armeria.common.HttpResponse;
 import com.linecorp.armeria.common.HttpStatus;
 import com.linecorp.armeria.common.MediaType;
@@ -42,7 +44,7 @@ enum DefaultGraphqlErrorHandler implements GraphqlErrorHandler {
     public HttpResponse handle(
             ServiceRequestContext ctx,
             ExecutionInput input,
-            ExecutionResult result,
+            @Nullable ExecutionResult result,
             @Nullable Throwable cause) {
 
         final MediaType produceType = GraphqlUtil.produceType(ctx.request().headers());
@@ -60,11 +62,11 @@ public HttpResponse handle(
             return HttpResponse.ofJson(HttpStatus.INTERNAL_SERVER_ERROR, produceType, specification);
         }
 
-        if (result.getErrors().stream().anyMatch(ValidationError.class::isInstance)) {
+        if (result != null && result.getErrors().stream().anyMatch(ValidationError.class::isInstance)) {
             return HttpResponse.ofJson(HttpStatus.BAD_REQUEST, produceType, result.toSpecification());
         }
 
-        return HttpResponse.ofJson(produceType, result.toSpecification());
+        return HttpResponse.ofJson(produceType, result != null ? result.toSpecification() : ImmutableMap.of());
     }
 
     private static Map<String, Object> toSpecification(Throwable cause) {

diff --git a/graphql/src/main/java/com/linecorp/armeria/server/graphql/DefaultGraphqlService.java b/graphql/src/main/java/com/linecorp/armeria/server/graphql/DefaultGraphqlService.java
@@ -139,7 +139,9 @@ private HttpResponse execute(
                     }));
         } catch (Throwable cause) {
             cause = Exceptions.peel(cause);
-            return errorHandler.handle(ctx, input, null, cause);
+            final HttpResponse res = errorHandler.handle(ctx, input, null, cause);
+            assert res != null : "DefaultGraphqlService.handle() returned null?";
+            return res;
         }
     }
 

diff --git a/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWSSubProtocol.java b/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWSSubProtocol.java
@@ -374,6 +374,7 @@ private static void writeError(WebSocketWriter out, String operationId, Throwabl
                 "id", operationId,
                 "payload", ImmutableList.of(
                         new GraphQLError() {
+                            @Nullable
                             @Override
                             public String getMessage() {
                                 return t.getMessage();
@@ -430,4 +431,3 @@ public Throwable fillInStackTrace() {
         }
     }
 }
-
diff --git a/grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/DeframedMessage.java b/grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/DeframedMessage.java
@@ -124,6 +124,7 @@ public void close() {
         if (buf != null) {
             buf.release();
         } else {
+            assert stream != null;
             try {
                 stream.close();
             } catch (IOException e) {

diff --git a/...c/main/java/com/linecorp/armeria/server/grpc/protocol/AbstractUnsafeUnaryGrpcService.java b/...c/main/java/com/linecorp/armeria/server/grpc/protocol/AbstractUnsafeUnaryGrpcService.java
@@ -142,6 +142,7 @@ protected final HttpResponse doPost(ServiceRequestContext ctx, HttpRequest req)
                             final HttpData content = framer.writePayload(responseMessage);
                             final ResponseHeaders responseHeaders = RESPONSE_HEADERS_MAP.get(
                                     serializationFormat);
+                            assert responseHeaders != null;
                             if (UnaryGrpcSerializationFormats.isGrpcWeb(serializationFormat)) {
                                 // Send trailer as a part of the body for gRPC-web.
                                 final HttpData serializedTrailers = framer.writePayload(

diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaChannel.java b/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaChannel.java
@@ -239,6 +239,9 @@ private <I, O> DefaultClientRequestContext newContext(HttpMethod method, HttpReq
         assert reqTarget != null : path;
         RequestTargetCache.putForClient(path, reqTarget);
 
+        final RequestOptions requestOptions = REQUEST_OPTIONS_MAP.get(methodDescriptor.getType());
+        assert requestOptions != null;
+
         return new DefaultClientRequestContext(
                 meterRegistry,
                 sessionProtocol,
@@ -248,7 +251,7 @@ private <I, O> DefaultClientRequestContext newContext(HttpMethod method, HttpReq
                 options(),
                 req,
                 null,
-                REQUEST_OPTIONS_MAP.get(methodDescriptor.getType()),
+                requestOptions,
                 System.nanoTime(),
                 SystemInfo.currentTimeMicros());
     }

diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java b/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java
@@ -47,6 +47,8 @@
 import com.linecorp.armeria.client.grpc.GrpcClientStubFactory;
 import com.linecorp.armeria.client.grpc.protocol.UnaryGrpcClient;
 import com.linecorp.armeria.client.retry.RetryingClient;
+import com.linecorp.armeria.common.HttpResponse;
+import com.linecorp.armeria.common.HttpStatus;
 import com.linecorp.armeria.common.Scheme;
 import com.linecorp.armeria.common.SerializationFormat;
 import com.linecorp.armeria.common.SessionProtocol;
@@ -200,7 +202,7 @@ private static ClientBuilderParams addTrailersExtractor(
                 originalDecoration.decorators();
 
         boolean foundRetryingClient = false;
-        final HttpClient noopClient = (ctx, req) -> null;
+        final HttpClient noopClient = (ctx, req) -> HttpResponse.of(HttpStatus.OK);
         for (Function<? super HttpClient, ? extends HttpClient> decorator : decorators) {
             final HttpClient decorated = decorator.apply(noopClient);
             if (decorated instanceof RetryingClient) {

diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/JavaGrpcClientStubFactory.java b/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/JavaGrpcClientStubFactory.java
@@ -22,6 +22,7 @@
 import java.lang.reflect.Method;
 
 import com.linecorp.armeria.client.grpc.GrpcClientStubFactory;
+import com.linecorp.armeria.common.annotation.Nullable;
 import com.linecorp.armeria.common.util.Exceptions;
 
 import io.grpc.Channel;
@@ -32,6 +33,7 @@
  */
 public final class JavaGrpcClientStubFactory implements GrpcClientStubFactory {
 
+    @Nullable
     @Override
     public ServiceDescriptor findServiceDescriptor(Class<?> clientType) {
         final String clientTypeName = clientType.getName();
@@ -59,6 +61,7 @@ public ServiceDescriptor findServiceDescriptor(Class<?> clientType) {
     @Override
     public Object newClientStub(Class<?> clientType, Channel channel) {
         final Method stubFactoryMethod = GrpcClientFactoryUtil.findStubFactoryMethod(clientType);
+        assert stubFactoryMethod != null : "No stub factory method found for " + clientType;
         try {
             return stubFactoryMethod.invoke(null, channel);
         } catch (IllegalAccessException | InvocationTargetException e) {