Migrate remaining checkNotNull to checkArgumentNotNull/checkStateNotNull in options package #18719 (#37654)

* Start migrating argument null checks to checkArgumentNotNull (Issue #18719)

* Migrate remaining checkNotNull to checkArgumentNotNull/checkStateNotNull in options package

Replace Guava checkNotNull (throws NullPointerException) with Beam's
checkArgumentNotNull (throws IllegalArgumentException) for argument
validation and checkStateNotNull (throws IllegalStateException) for
internal state validation.

Files modified:
- SdkHarnessOptions.java: checkStateNotNull for return value check
- PipelineOptionsFactory.java: checkArgumentNotNull for argument check
- ValueProvider.java: checkArgumentNotNull + checkStateNotNull
- ValueProviders.java: checkStateNotNull for parsed result check
- PipelineOptionsValidator.java: checkArgumentNotNull for argument checks
- ProxyInvocationHandler.java: checkArgumentNotNull for argument check
- MetricNameFilter.java: cleanup commented-out import

Fixes #18719

Author: chandrakiranbolla1

sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/MetricNameFilter.java

@@ -17,7 +17,7 @@
  */
 package org.apache.beam.sdk.metrics;
 
-import static org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkNotNull;
+import static org.apache.beam.sdk.util.Preconditions.checkArgumentNotNull;
 
 import com.google.auto.value.AutoValue;
 import org.checkerframework.checker.nullness.qual.Nullable;
@@ -41,13 +41,13 @@ public static MetricNameFilter inNamespace(Class<?> namespace) {
   }
 
   public static MetricNameFilter named(String namespace, String name) {
-    checkNotNull(name, "Must specify a name");
+    checkArgumentNotNull(name, "Must specify a name");
     return new AutoValue_MetricNameFilter(namespace, name);
   }
 
   public static MetricNameFilter named(Class<?> namespace, String name) {
-    checkNotNull(namespace, "Must specify a inNamespace");
-    checkNotNull(name, "Must specify a name");
+    checkArgumentNotNull(namespace, "Must specify a namespace");
+    checkArgumentNotNull(name, "Must specify a name");
     return new AutoValue_MetricNameFilter(namespace.getName(), name);
   }
 }

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java

@@ -18,8 +18,8 @@
 package org.apache.beam.sdk.options;
 
 import static java.util.Locale.ROOT;
+import static org.apache.beam.sdk.util.Preconditions.checkArgumentNotNull;
 import static org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkArgument;
-import static org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkNotNull;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.core.JsonParseException;
@@ -133,8 +133,8 @@
  *       registered with this factory.
  * </ul>
  *
- * <p>See the <a
- * href="http://www.oracle.com/technetwork/java/javase/documentation/spec-136004.html">JavaBeans
+ * <p>See the <a href=
+ * "http://www.oracle.com/technetwork/java/javase/documentation/spec-136004.html">JavaBeans
  * specification</a> for more details as to what constitutes a property.
  */
 @SuppressWarnings({
@@ -277,7 +277,7 @@ private Builder(String[] args, boolean validation, boolean strictParsing, boolea
      * PipelineOptionsFactory#printHelp(PrintStream, Class)}.
      */
     public Builder fromArgs(String... args) {
-      checkNotNull(args, "Arguments should not be null.");
+      checkArgumentNotNull(args, "Arguments should not be null.");
       return new Builder(args, validation, strictParsing, true);
     }
 
@@ -338,7 +338,8 @@ public <T extends PipelineOptions> T as(Class<T> klass) {
         appNameOptions.setAppName(defaultAppName);
       }
 
-      // Ensure the options id has been populated either by the user using the command line
+      // Ensure the options id has been populated either by the user using the command
+      // line
       // or by the default value factory.
       t.getOptionsId();
 
@@ -433,7 +434,8 @@ private static String findCallersClassName() {
         break;
       }
     }
-    // Then find the first instance after that is not the PipelineOptionsFactory/Builder class.
+    // Then find the first instance after that is not the
+    // PipelineOptionsFactory/Builder class.
     while (elements.hasNext()) {
       StackTraceElement next = elements.next();
       if (!PIPELINE_OPTIONS_FACTORY_CLASSES.contains(next.getClassName())) {
@@ -587,7 +589,7 @@ public static Set<Class<? extends PipelineOptions>> getRegisteredOptions() {
    * window.
    */
   public static void printHelp(PrintStream out) {
-    checkNotNull(out);
+    checkArgumentNotNull(out);
     out.println("The set of registered options are:");
     Set<Class<? extends PipelineOptions>> sortedOptions =
         new TreeSet<>(ClassNameComparator.INSTANCE);
@@ -622,8 +624,8 @@ public static void printHelp(PrintStream out) {
    * This method will attempt to format its output to be compatible with a terminal window.
    */
   public static void printHelp(PrintStream out, Class<? extends PipelineOptions> iface) {
-    checkNotNull(out);
-    checkNotNull(iface);
+    checkArgumentNotNull(out);
+    checkArgumentNotNull(iface);
     CACHE.get().validateWellFormed(iface);
 
     Set<PipelineOptionSpec> properties = PipelineOptionsReflector.getOptionSpecs(iface, true);
@@ -697,7 +699,7 @@ public static void printHelp(PrintStream out, Class<? extends PipelineOptions> i
    */
   public static List<PipelineOptionDescriptor> describe(
       Set<Class<? extends PipelineOptions>> ifaces) {
-    checkNotNull(ifaces);
+    checkArgumentNotNull(ifaces);
     List<PipelineOptionDescriptor> result = new ArrayList<>();
     Set<Method> seenMethods = Sets.newHashSet();
 
@@ -869,7 +871,8 @@ private static List<PropertyDescriptor> getPropertyDescriptors(Set<Method> metho
     List<TypeMismatch> mismatches = new ArrayList<>();
     Set<String> usedDescriptors = Sets.newHashSet();
     /*
-     * Add all the getter/setter pairs to the list of descriptors removing the getter once
+     * Add all the getter/setter pairs to the list of descriptors removing the
+     * getter once
      * it has been paired up.
      */
     for (Method method : methods) {
@@ -995,7 +998,8 @@ private static List<PropertyDescriptor> validateClass(
             + "PipelineOptions proxy class to implement all of the interfaces.",
         iface.getName());
 
-    // Verify that there are no methods with the same name with two different return types.
+    // Verify that there are no methods with the same name with two different return
+    // types.
     validateReturnType(iface);
 
     SortedSet<Method> allInterfaceMethods =
@@ -1098,7 +1102,8 @@ private static void validateMethodAnnotations(
     validateGettersHaveConsistentAnnotation(
         methodNameToAllMethodMap, descriptors, AnnotationPredicates.JSON_SERIALIZE);
 
-    // Verify that if a method has either @JsonSerialize or @JsonDeserialize then it has both.
+    // Verify that if a method has either @JsonSerialize or @JsonDeserialize then it
+    // has both.
     validateMethodsHaveBothJsonSerializeAndDeserialize(descriptors);
 
     // Verify that no setter has @JsonIgnore.
@@ -1280,7 +1285,8 @@ private static void validateMethodsAreEitherBeanMethodOrKnownMethod(
       knownMethodsNames.add(method.getName());
     }
 
-    // Verify that no additional methods are on an interface that aren't a bean property.
+    // Verify that no additional methods are on an interface that aren't a bean
+    // property.
     // Because methods can have multiple declarations, we do a name-based comparison
     // here to prevent false positives.
     SortedSet<Method> unknownMethods = new TreeSet<>(MethodComparator.INSTANCE);
@@ -1823,7 +1829,8 @@ private static Object tryParseObject(String value, Method method) throws IOExcep
         try {
           tree = MAPPER.readTree("\"" + value + "\"");
         } catch (JsonParseException inner) {
-          // rethrow the original exception rather the one thrown from the fallback attempt
+          // rethrow the original exception rather the one thrown from the fallback
+          // attempt
           throw e;
         }
       } else {
@@ -1891,7 +1898,8 @@ private static <T extends PipelineOptions> Map<String, Object> parseObjects(
           }
         }
         Method method = propertyNamesToGetters.get(entry.getKey());
-        // Only allow empty argument values for String, String Array, and Collection<String>.
+        // Only allow empty argument values for String, String Array, and
+        // Collection<String>.
         Class<?> returnType = method.getReturnType();
         JavaType type = MAPPER.getTypeFactory().constructType(method.getGenericReturnType());
 
@@ -2089,7 +2097,7 @@ private void initializeRegistry(final ClassLoader loader) {
     }
 
     private synchronized void register(Class<? extends PipelineOptions> iface) {
-      checkNotNull(iface);
+      checkArgumentNotNull(iface);
       checkArgument(iface.isInterface(), "Only interface types are supported.");
 
       if (registeredOptions.contains(iface)) {
@@ -2144,15 +2152,17 @@ synchronized <T extends PipelineOptions> Registration<T> validateWellFormed(
         Class<T> iface, Set<Class<? extends PipelineOptions>> validatedPipelineOptionsInterfaces) {
       checkArgument(iface.isInterface(), "Only interface types are supported.");
 
-      // Validate that every inherited interface must extend PipelineOptions except for
+      // Validate that every inherited interface must extend PipelineOptions except
+      // for
       // PipelineOptions itself.
       validateInheritedInterfacesExtendPipelineOptions(iface);
 
       @SuppressWarnings("unchecked")
       Set<Class<? extends PipelineOptions>> combinedPipelineOptionsInterfaces =
           Stream.concat(validatedPipelineOptionsInterfaces.stream(), Stream.of(iface))
               .collect(Collectors.toSet());
-      // Validate that the view of all currently passed in options classes is well formed.
+      // Validate that the view of all currently passed in options classes is well
+      // formed.
       if (!combinedCache.containsKey(combinedPipelineOptionsInterfaces)) {
         final Class<?>[] interfaces = combinedPipelineOptionsInterfaces.toArray(EMPTY_CLASS_ARRAY);
         @SuppressWarnings("unchecked")

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsValidator.java

@@ -17,8 +17,8 @@
  */
 package org.apache.beam.sdk.options;
 
+import static org.apache.beam.sdk.util.Preconditions.checkArgumentNotNull;
 import static org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkArgument;
-import static org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkNotNull;
 
 import java.lang.reflect.Method;
 import java.lang.reflect.Proxy;
@@ -67,8 +67,8 @@ public static <T extends PipelineOptions> T validateCli(Class<T> klass, Pipeline
 
   private static <T extends PipelineOptions> T validate(
       Class<T> klass, PipelineOptions options, boolean isCli) {
-    checkNotNull(klass);
-    checkNotNull(options);
+    checkArgumentNotNull(klass);
+    checkArgumentNotNull(options);
     checkArgument(Proxy.isProxyClass(options.getClass()));
     checkArgument(Proxy.getInvocationHandler(options) instanceof ProxyInvocationHandler);
 

sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java

@@ -17,8 +17,8 @@
  */
 package org.apache.beam.sdk.options;
 
+import static org.apache.beam.sdk.util.Preconditions.checkArgumentNotNull;
 import static org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkArgument;
-import static org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkNotNull;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.core.JsonGenerator;
@@ -82,9 +82,9 @@
  * introspection of the proxy class to store and retrieve values based off of the property name.
  *
  * <p>Unset properties use the {@code @Default} metadata on the getter to return values. If there is
- * no {@code @Default} annotation on the getter, then a <a
- * href="https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html">default</a> as
- * per the Java Language Specification for the expected return type is returned.
+ * no {@code @Default} annotation on the getter, then a <a href=
+ * "https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html">default</a> as per the
+ * Java Language Specification for the expected return type is returned.
  *
  * <p>In addition to the getter/setter pairs, this proxy invocation handler supports {@link
  * Object#equals(Object)}, {@link Object#hashCode()}, {@link Object#toString()} and {@link
@@ -122,7 +122,8 @@ private ComputedProperties(
     <T extends PipelineOptions> ComputedProperties updated(
         Class<T> iface, T instance, List<PropertyDescriptor> propertyDescriptors) {
 
-      // these all use mutable maps and then copyOf, rather than a builder because builders enforce
+      // these all use mutable maps and then copyOf, rather than a builder because
+      // builders enforce
       // all keys are unique, and its possible they are not here.
       Map<String, String> allNewGetters = Maps.newHashMap(gettersToPropertyNames);
       Map<String, String> allNewSetters = Maps.newHashMap(settersToPropertyNames);
@@ -147,7 +148,8 @@ <T extends PipelineOptions> ComputedProperties updated(
   @SuppressFBWarnings("SE_BAD_FIELD")
   private volatile ComputedProperties computedProperties;
 
-  // ProxyInvocationHandler implements Serializable only for the sake of throwing an informative
+  // ProxyInvocationHandler implements Serializable only for the sake of throwing
+  // an informative
   // exception in writeObject()
   /**
    * Enumerating {@code options} must always be done on a copy made before accessing or deriving
@@ -217,7 +219,8 @@ public Object invoke(Object proxy, Method method, Object[] args) {
     ComputedProperties properties = computedProperties;
     if (properties.gettersToPropertyNames.containsKey(methodName)) {
       String propertyName = properties.gettersToPropertyNames.get(methodName);
-      // we can't use computeIfAbsent here because evaluating the default may cause more properties
+      // we can't use computeIfAbsent here because evaluating the default may cause
+      // more properties
       // to be evaluated, and computeIfAbsent is not re-entrant.
       if (!options.containsKey(propertyName)) {
         // Lazy bind the default to the method.
@@ -286,7 +289,7 @@ static BoundValue fromDefault(@Nullable Object value) {
    * @return An object that implements the interface {@code <T>}.
    */
   <T extends PipelineOptions> T as(Class<T> iface) {
-    checkNotNull(iface);
+    checkArgumentNotNull(iface);
     checkArgument(iface.isInterface(), "Not an interface: %s", iface);
 
     T existingOption = computedProperties.interfaceToProxyCache.getInstance(iface);
@@ -376,8 +379,10 @@ class PipelineOptionsDisplayData implements HasDisplayData {
      */
     @Override
     public void populateDisplayData(DisplayData.Builder builder) {
-      // We must first make a copy of the current options because a concurrent modification
-      // may add a new option after we have derived optionSpecs but before we have enumerated
+      // We must first make a copy of the current options because a concurrent
+      // modification
+      // may add a new option after we have derived optionSpecs but before we have
+      // enumerated
       // all the pipeline options.
       Map<String, BoundValue> copiedOptions = new HashMap<>(options);
       Set<PipelineOptionSpec> optionSpecs =
@@ -396,8 +401,10 @@ public void populateDisplayData(DisplayData.Builder builder) {
 
         for (PipelineOptionSpec optionSpec : specs) {
           if (!optionSpec.shouldSerialize()) {
-            // Options that are excluded for serialization (i.e. those with @JsonIgnore) are also
-            // excluded from display data. These options are generally not useful for display.
+            // Options that are excluded for serialization (i.e. those with @JsonIgnore) are
+            // also
+            // excluded from display data. These options are generally not useful for
+            // display.
             continue;
           }
 
@@ -481,7 +488,8 @@ private static String displayDataString(@Nullable Object value) {
         return Arrays.deepToString((Object[]) value);
       }
 
-      // At this point, we have some type of primitive array. Arrays.deepToString(..) requires an
+      // At this point, we have some type of primitive array. Arrays.deepToString(..)
+      // requires an
       // Object array, but will unwrap nested primitive arrays.
       String wrapped = Arrays.deepToString(new Object[] {value});
       return wrapped.substring(1, wrapped.length() - 1);
@@ -514,12 +522,17 @@ private Multimap<String, PipelineOptionSpec> buildOptionNameToSpecMap(
     // Filter out overridden options
     for (Map.Entry<String, Collection<PipelineOptionSpec>> entry : optionsMap.asMap().entrySet()) {
 
-      /* Compare all interfaces for an option pairwise (iface1, iface2) to look for type
-      hierarchies. If one is the base-class of the other, remove it from the output and continue
-      iterating.
-
-      This is an N^2 operation per-option, but the number of interfaces defining an option
-      should always be small (usually 1). */
+      /*
+       * Compare all interfaces for an option pairwise (iface1, iface2) to look for
+       * type
+       * hierarchies. If one is the base-class of the other, remove it from the output
+       * and continue
+       * iterating.
+       *
+       * This is an N^2 operation per-option, but the number of interfaces defining an
+       * option
+       * should always be small (usually 1).
+       */
       List<PipelineOptionSpec> specs = Lists.newArrayList(entry.getValue());
       if (specs.size() < 2) {
         // Only one known implementing interface, no need to check for inheritance
@@ -600,9 +613,9 @@ private static Object getValueFromJson(JsonNode node, Method method) {
 
   /**
    * Returns a default value for the method based upon {@code @Default} metadata on the getter to
-   * return values. If there is no {@code @Default} annotation on the getter, then a <a
-   * href="https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html">default</a> as
-   * per the Java Language Specification for the expected return type is returned.
+   * return values. If there is no {@code @Default} annotation on the getter, then a <a href=
+   * "https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html">default</a> as per
+   * the Java Language Specification for the expected return type is returned.
    *
    * @param proxy The proxy object for which we are attempting to get the default.
    * @param method The getter method that was invoked.
@@ -651,7 +664,8 @@ private Object getDefault(PipelineOptions proxy, Method method) {
     }
 
     /*
-     * We need to make sure that we return something appropriate for the return type. Thus we return
+     * We need to make sure that we return something appropriate for the return
+     * type. Thus we return
      * a default value as defined by the JLS.
      */
     return Defaults.defaultValue(method.getReturnType());
@@ -750,7 +764,8 @@ public void serialize(PipelineOptions value, JsonGenerator jgen, SerializerProvi
         throws IOException {
       ProxyInvocationHandler handler = (ProxyInvocationHandler) Proxy.getInvocationHandler(value);
       PipelineOptionsFactory.Cache cache = PipelineOptionsFactory.CACHE.get();
-      // We first copy and then filter out any properties that have been modified since
+      // We first copy and then filter out any properties that have been modified
+      // since
       // the last serialization of this PipelineOptions and then verify that
       // they are all serializable.
       Map<String, BoundValue> filteredOptions = Maps.newHashMap(handler.options);