This closes #4021: [BEAM-3081] Findbugs: NonNull by default

NonNull by default in metrics Ignore findbugs in AutoValue generated classes NonNull by default for sdk/testing NonNull by default in sdk/state NonNull by default in sdk/runners NonNull by default in sdk/annotations NonNull by default in sdk/coders Make Java core SDK root dir NonNull by default Add dep on Apache-licensed findbugs-annotations implementation
apache · Oct 20, 2017 · 66fcda9 · 66fcda9
2 parents a29e0ad + a1311d4
commit 66fcda9
Show file tree

Hide file tree

Showing 28 changed files with 142 additions and 53 deletions.
diff --git a/pom.xml b/pom.xml
@@ -137,6 +137,7 @@
     <hamcrest.version>1.3</hamcrest.version>
     <jackson.version>2.8.9</jackson.version>
     <findbugs.version>3.0.1</findbugs.version>
+    <findbugs.annotations.version>1.3.9-1</findbugs.annotations.version>
     <joda.version>2.4</joda.version>
     <junit.version>4.12</junit.version>
     <mockito.version>1.9.5</mockito.version>
@@ -1036,6 +1037,12 @@
         <version>${findbugs.version}</version>
       </dependency>
 
+      <dependency>
+        <groupId>com.github.stephenc.findbugs</groupId>
+        <artifactId>findbugs-annotations</artifactId>
+        <version>${findbugs.annotations.version}</version>
+      </dependency>
+
       <dependency>
         <groupId>com.google.cloud.bigdataoss</groupId>
         <artifactId>gcsio</artifactId>

diff --git a/runners/core-java/src/main/java/org/apache/beam/runners/core/SideInputHandler.java b/runners/core-java/src/main/java/org/apache/beam/runners/core/SideInputHandler.java
@@ -174,7 +174,7 @@ public <T> T get(PCollectionView<T> sideInput, BoundedWindow window) {
     ValueState<Iterable<WindowedValue<?>>> state =
         stateInternals.state(StateNamespaces.window(windowCoder, window), stateTag);
 
-    Iterable<WindowedValue<?>> elements = state.read();
+    @Nullable Iterable<WindowedValue<?>> elements = state.read();
 
     if (elements == null) {
       elements = Collections.emptyList();

diff --git a/runners/core-java/src/main/java/org/apache/beam/runners/core/WatermarkHold.java b/runners/core-java/src/main/java/org/apache/beam/runners/core/WatermarkHold.java
@@ -483,9 +483,9 @@ public ReadableState<OldAndNewHolds> readLater() {
       @Override
       public OldAndNewHolds read() {
         // Read both the element and extra holds.
-        Instant elementHold = elementHoldState.read();
-        Instant extraHold = extraHoldState.read();
-        Instant oldHold;
+        @Nullable Instant elementHold = elementHoldState.read();
+        @Nullable Instant extraHold = extraHoldState.read();
+        @Nullable Instant oldHold;
         // Find the minimum, accounting for null.
         if (elementHold == null) {
           oldHold = extraHold;

diff --git a/...e-java/src/main/java/org/apache/beam/runners/core/triggers/TriggerStateMachineRunner.java b/...e-java/src/main/java/org/apache/beam/runners/core/triggers/TriggerStateMachineRunner.java
@@ -24,6 +24,7 @@
 import java.util.BitSet;
 import java.util.Collection;
 import java.util.Map;
+import javax.annotation.Nullable;
 import org.apache.beam.runners.core.MergingStateAccessor;
 import org.apache.beam.runners.core.StateAccessor;
 import org.apache.beam.runners.core.StateTag;
@@ -79,7 +80,7 @@ private FinishedTriggersBitSet readFinishedBits(ValueState<BitSet> state) {
       return FinishedTriggersBitSet.emptyWithCapacity(rootTrigger.getFirstIndexAfterSubtree());
     }
 
-    BitSet bitSet = state.read();
+    @Nullable BitSet bitSet = state.read();
     return bitSet == null
         ? FinishedTriggersBitSet.emptyWithCapacity(rootTrigger.getFirstIndexAfterSubtree())
             : FinishedTriggersBitSet.fromBitSet(bitSet);

diff --git a/sdks/java/build-tools/src/main/resources/beam/findbugs-filter.xml b/sdks/java/build-tools/src/main/resources/beam/findbugs-filter.xml
@@ -26,13 +26,18 @@
   <!-- The uncallable method error fails on @ProcessElement style methods -->
   <Bug pattern="UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS"/>
 
+  <!-- Suppress checking of AutoValue internals -->
+  <Match>
+    <Class name="~.*AutoValue_.*"/>
+  </Match>
+
   <!--
           Suppressed findbugs issues. All new issues should include a comment why they're
           suppressed.
 
           Suppressions should go in this file rather than inline using @SuppressFBWarnings to avoid
           unapproved artifact license.
-        -->
+	-->
   <Match>
     <Class name="org.apache.beam.fn.harness.control.BeamFnControlClient$InboundObserver"/>
     <Method name="onCompleted"/>

diff --git a/sdks/java/core/pom.xml b/sdks/java/core/pom.xml
@@ -226,6 +226,11 @@
       <artifactId>jsr305</artifactId>
     </dependency>
 
+    <dependency>
+      <groupId>com.github.stephenc.findbugs</groupId>
+      <artifactId>findbugs-annotations</artifactId>
+    </dependency>
+
     <dependency>
       <groupId>com.fasterxml.jackson.core</groupId>
       <artifactId>jackson-core</artifactId>

diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/Pipeline.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/Pipeline.java
@@ -29,6 +29,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import javax.annotation.Nullable;
 import org.apache.beam.sdk.annotations.Internal;
 import org.apache.beam.sdk.coders.CoderRegistry;
 import org.apache.beam.sdk.io.Read;
@@ -393,9 +394,13 @@ enum CompositeBehavior {
      */
     class Defaults implements PipelineVisitor {
 
-      private Pipeline pipeline;
+      @Nullable private Pipeline pipeline;
 
       protected Pipeline getPipeline() {
+        if (pipeline == null) {
+          throw new IllegalStateException(
+              "Illegal access to pipeline after visitor traversal was completed");
+        }
         return pipeline;
       }
 
@@ -484,7 +489,10 @@ OutputT applyTransform(String name, InputT input,
 
   private final TransformHierarchy transforms;
   private Set<String> usedFullNames = new HashSet<>();
-  private CoderRegistry coderRegistry;
+
+  /** Lazily initialized; access via {@link #getCoderRegistry()}. */
+  @Nullable private CoderRegistry coderRegistry;
+
   private final List<String> unstableNames = new ArrayList<>();
   private final PipelineOptions defaultOptions;
 

diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/annotations/package-info.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/annotations/package-info.java
@@ -18,4 +18,8 @@
 /**
  * Defines annotations used across the SDK.
  */
+@DefaultAnnotation(NonNull.class)
 package org.apache.beam.sdk.annotations;
+
+import edu.umd.cs.findbugs.annotations.DefaultAnnotation;
+import edu.umd.cs.findbugs.annotations.NonNull;
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/DefaultCoder.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/DefaultCoder.java
@@ -26,6 +26,7 @@
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.util.List;
+import javax.annotation.CheckForNull;
 import org.apache.beam.sdk.values.PCollection;
 import org.apache.beam.sdk.values.TypeDescriptor;
 import org.slf4j.Logger;
@@ -49,6 +50,7 @@
 @Target(ElementType.TYPE)
 @SuppressWarnings("rawtypes")
 public @interface DefaultCoder {
+  @CheckForNull
   Class<? extends Coder> value();
 
   /**
@@ -86,22 +88,23 @@ public <T> Coder<T> coderFor(TypeDescriptor<T> typeDescriptor,
                   clazz.getName()));
         }
 
-        if (defaultAnnotation.value() == null) {
+        Class<? extends Coder> defaultAnnotationValue = defaultAnnotation.value();
+        if (defaultAnnotationValue == null) {
           throw new CannotProvideCoderException(
-              String.format("Class %s has a @DefaultCoder annotation with a null value.",
-                  clazz.getName()));
+              String.format(
+                  "Class %s has a @DefaultCoder annotation with a null value.", clazz.getName()));
         }
 
         LOG.debug("DefaultCoder annotation found for {} with value {}",
-            clazz, defaultAnnotation.value());
+            clazz, defaultAnnotationValue);
 
         Method coderProviderMethod;
         try {
-          coderProviderMethod = defaultAnnotation.value().getMethod("getCoderProvider");
+          coderProviderMethod = defaultAnnotationValue.getMethod("getCoderProvider");
         } catch (NoSuchMethodException e) {
           throw new CannotProvideCoderException(String.format(
               "Unable to find 'public static CoderProvider getCoderProvider()' on %s",
-              defaultAnnotation.value()),
+              defaultAnnotationValue),
               e);
         }
 
@@ -115,7 +118,7 @@ public <T> Coder<T> coderFor(TypeDescriptor<T> typeDescriptor,
             | ExceptionInInitializerError e) {
           throw new CannotProvideCoderException(String.format(
               "Unable to invoke 'public static CoderProvider getCoderProvider()' on %s",
-              defaultAnnotation.value()),
+              defaultAnnotationValue),
               e);
         }
         return coderProvider.coderFor(typeDescriptor, componentCoders);

diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/LengthPrefixCoder.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/LengthPrefixCoder.java
@@ -26,7 +26,6 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.List;
-import javax.annotation.Nullable;
 import org.apache.beam.sdk.util.VarInt;
 
 /**
@@ -126,7 +125,7 @@ protected long getEncodedElementByteSize(T value) throws Exception {
    * {@inheritDoc}
    */
   @Override
-  public boolean isRegisterByteSizeObserverCheap(@Nullable T value) {
+  public boolean isRegisterByteSizeObserverCheap(T value) {
     return valueCoder.isRegisterByteSizeObserverCheap(value);
   }
 }
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/SerializableCoder.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/SerializableCoder.java
@@ -25,6 +25,7 @@
 import java.io.OutputStream;
 import java.io.Serializable;
 import java.util.List;
+import javax.annotation.Nullable;
 import org.apache.beam.sdk.values.TypeDescriptor;
 
 /**
@@ -105,7 +106,9 @@ public <T> Coder<T> coderFor(TypeDescriptor<T> typeDescriptor,
   }
 
   private final Class<T> type;
-  private transient TypeDescriptor<T> typeDescriptor;
+
+  /** Access via {@link #getEncodedTypeDescriptor()}. */
+  @Nullable private transient TypeDescriptor<T> typeDescriptor;
 
   protected SerializableCoder(Class<T> type, TypeDescriptor<T> typeDescriptor) {
     this.type = type;

diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/StructuredCoder.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/StructuredCoder.java
@@ -17,7 +17,6 @@
  */
 package org.apache.beam.sdk.coders;
 
-import java.util.Collections;
 import java.util.List;
 
 /**
@@ -46,12 +45,7 @@ protected StructuredCoder() {}
    * <p>The default components will be equal to the value returned by {@link #getCoderArguments()}.
    */
   public List<? extends Coder<?>> getComponents() {
-    List<? extends Coder<?>> coderArguments = getCoderArguments();
-    if (coderArguments == null) {
-      return Collections.emptyList();
-    } else {
-      return coderArguments;
-    }
+    return getCoderArguments();
   }
 
   /**

diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/VoidCoder.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/VoidCoder.java
@@ -19,6 +19,7 @@
 
 import java.io.InputStream;
 import java.io.OutputStream;
+import javax.annotation.Nullable;
 import org.apache.beam.sdk.values.TypeDescriptor;
 
 /**
@@ -44,6 +45,7 @@ public void encode(Void value, OutputStream outStream) {
   }
 
   @Override
+  @Nullable
   public Void decode(InputStream inStream) {
     // Nothing to read!
     return null;

diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/package-info.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/package-info.java
@@ -42,4 +42,8 @@
  * types.
  *
  */
+@DefaultAnnotation(NonNull.class)
 package org.apache.beam.sdk.coders;
+
+import edu.umd.cs.findbugs.annotations.DefaultAnnotation;
+import edu.umd.cs.findbugs.annotations.NonNull;
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/package-info.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/package-info.java
@@ -25,4 +25,8 @@
  * <p>Runners should look at {@link org.apache.beam.sdk.metrics.MetricsContainer} for details on
  * how to support metrics.
  */
+@DefaultAnnotation(NonNull.class)
 package org.apache.beam.sdk.metrics;
+
+import edu.umd.cs.findbugs.annotations.DefaultAnnotation;
+import edu.umd.cs.findbugs.annotations.NonNull;
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/package-info.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/package-info.java
@@ -31,4 +31,8 @@
  * where and how it should run after pipeline construction is complete.
  *
  */
+@DefaultAnnotation(NonNull.class)
 package org.apache.beam.sdk;
+
+import edu.umd.cs.findbugs.annotations.DefaultAnnotation;
+import edu.umd.cs.findbugs.annotations.NonNull;
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/TransformHierarchy.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/TransformHierarchy.java
@@ -309,11 +309,13 @@ private Map<TupleTag<?>, PCollection<?>> fullyExpand(POutput output) {
    * for initialization and ordered visitation.
    */
   public class Node {
-    private final Node enclosingNode;
+    // null for the root node, otherwise the enclosing node
+    @Nullable private final Node enclosingNode;
+
     // The PTransform for this node, which may be a composite PTransform.
     // The root of a TransformHierarchy is represented as a Node
     // with a null transform field.
-    private final PTransform<?, ?> transform;
+    @Nullable private final PTransform<?, ?> transform;
 
     private final String fullName;
 
@@ -324,21 +326,22 @@ public class Node {
     private final Map<TupleTag<?>, PValue> inputs;
 
     // TODO: track which outputs need to be exported to parent.
-    // Output of the transform, in expanded form.
-    private Map<TupleTag<?>, PValue> outputs;
+    // Output of the transform, in expanded form. Null if not yet set.
+    @Nullable private Map<TupleTag<?>, PValue> outputs;
 
     @VisibleForTesting
     boolean finishedSpecifying = false;
 
     /**
      * Creates the root-level node. The root level node has a null enclosing node, a null transform,
-     * an empty map of inputs, and a name equal to the empty string.
+     * an empty map of inputs, an empty map of outputs, and a name equal to the empty string.
      */
     private Node() {
       this.enclosingNode = null;
       this.transform = null;
       this.fullName = "";
       this.inputs = Collections.emptyMap();
+      this.outputs = Collections.emptyMap();
     }
 
     /**
@@ -469,7 +472,7 @@ public String getFullName() {
 
     /** Returns the transform input, in fully expanded form. */
     public Map<TupleTag<?>, PValue> getInputs() {
-      return inputs == null ? Collections.<TupleTag<?>, PValue>emptyMap() : inputs;
+      return inputs;
     }
 
     /**

diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/package-info.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/runners/package-info.java
@@ -20,4 +20,8 @@
 
  * <p>Internals for use by runners.
  */
+@DefaultAnnotation(NonNull.class)
 package org.apache.beam.sdk.runners;
+
+import edu.umd.cs.findbugs.annotations.DefaultAnnotation;
+import edu.umd.cs.findbugs.annotations.NonNull;
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/state/BagState.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/state/BagState.java
@@ -17,6 +17,7 @@
  */
 package org.apache.beam.sdk.state;
 
+import javax.annotation.Nonnull;
 import org.apache.beam.sdk.annotations.Experimental;
 import org.apache.beam.sdk.annotations.Experimental.Kind;
 
@@ -31,6 +32,11 @@
  */
 @Experimental(Kind.STATE)
 public interface BagState<T> extends GroupingState<T, Iterable<T>> {
+
+  @Override
+  @Nonnull
+  Iterable<T> read();
+
   @Override
   BagState<T> readLater();
 }
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/state/CombiningState.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/state/CombiningState.java
@@ -17,6 +17,7 @@
  */
 package org.apache.beam.sdk.state;
 
+import javax.annotation.Nonnull;
 import org.apache.beam.sdk.annotations.Experimental;
 import org.apache.beam.sdk.annotations.Experimental.Kind;
 import org.apache.beam.sdk.transforms.Combine.CombineFn;
@@ -35,6 +36,10 @@
 @Experimental(Kind.STATE)
 public interface CombiningState<InputT, AccumT, OutputT> extends GroupingState<InputT, OutputT> {
 
+  @Override
+  @Nonnull
+  OutputT read();
+
   /**
    * Read the merged accumulator for this state cell. It is implied that reading the state involves
    * reading the accumulator, so {@link #readLater} is sufficient to prefetch for this.