beam-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From lc...@apache.org
Subject [1/2] beam git commit: [BEAM-2514] Improve error message on missing required value
Date Tue, 27 Jun 2017 16:23:17 GMT
Repository: beam
Updated Branches:
  refs/heads/master 9df865add -> 99ef92bc9


[BEAM-2514] Improve error message on missing required value


Project: http://git-wip-us.apache.org/repos/asf/beam/repo
Commit: http://git-wip-us.apache.org/repos/asf/beam/commit/01b3febc
Tree: http://git-wip-us.apache.org/repos/asf/beam/tree/01b3febc
Diff: http://git-wip-us.apache.org/repos/asf/beam/diff/01b3febc

Branch: refs/heads/master
Commit: 01b3febc62debf8b4fd1582225d59768d231601a
Parents: 9df865a
Author: manuzhang <owenzhang1990@gmail.com>
Authored: Mon Jun 26 20:55:13 2017 +0800
Committer: Luke Cwik <lcwik@google.com>
Committed: Tue Jun 27 09:22:30 2017 -0700

----------------------------------------------------------------------
 .../sdk/options/PipelineOptionsFactory.java     | 18 +++++---
 .../sdk/options/PipelineOptionsValidator.java   | 34 +++++++++++++--
 .../sdk/options/ProxyInvocationHandler.java     |  4 ++
 .../options/PipelineOptionsValidatorTest.java   | 44 ++++++++++++++++++++
 .../sdk/options/ProxyInvocationHandlerTest.java |  7 ++++
 5 files changed, 97 insertions(+), 10 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/beam/blob/01b3febc/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
----------------------------------------------------------------------
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
index c0990cb..d7e6cc8 100644
--- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
+++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
@@ -184,18 +184,20 @@ public class PipelineOptionsFactory {
     private final String[] args;
     private final boolean validation;
     private final boolean strictParsing;
+    private final boolean isCli;
 
     // Do not allow direct instantiation
     private Builder() {
-      this(null, false, true);
+      this(null, false, true, false);
     }
 
     private Builder(String[] args, boolean validation,
-        boolean strictParsing) {
+        boolean strictParsing, boolean isCli) {
       this.defaultAppName = findCallersClassName();
       this.args = args;
       this.validation = validation;
       this.strictParsing = strictParsing;
+      this.isCli = isCli;
     }
 
     /**
@@ -237,7 +239,7 @@ public class PipelineOptionsFactory {
      */
     public Builder fromArgs(String... args) {
       checkNotNull(args, "Arguments should not be null.");
-      return new Builder(args, validation, strictParsing);
+      return new Builder(args, validation, strictParsing, true);
     }
 
     /**
@@ -247,7 +249,7 @@ public class PipelineOptionsFactory {
      * validation.
      */
     public Builder withValidation() {
-      return new Builder(args, true, strictParsing);
+      return new Builder(args, true, strictParsing, isCli);
     }
 
     /**
@@ -255,7 +257,7 @@ public class PipelineOptionsFactory {
      * arguments.
      */
     public Builder withoutStrictParsing() {
-      return new Builder(args, validation, false);
+      return new Builder(args, validation, false, isCli);
     }
 
     /**
@@ -300,7 +302,11 @@ public class PipelineOptionsFactory {
       }
 
       if (validation) {
-        PipelineOptionsValidator.validate(klass, t);
+        if (isCli) {
+          PipelineOptionsValidator.validateCli(klass, t);
+        } else {
+          PipelineOptionsValidator.validate(klass, t);
+        }
       }
       return t;
     }

http://git-wip-us.apache.org/repos/asf/beam/blob/01b3febc/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsValidator.java
----------------------------------------------------------------------
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsValidator.java
b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsValidator.java
index bd54ec3..fcffd74 100644
--- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsValidator.java
+++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsValidator.java
@@ -43,9 +43,29 @@ public class PipelineOptionsValidator {
    *
    * @param klass The interface to fetch validation criteria from.
    * @param options The {@link PipelineOptions} to validate.
-   * @return The type
+   * @return Validated options.
    */
   public static <T extends PipelineOptions> T validate(Class<T> klass, PipelineOptions
options) {
+    return validate(klass, options, false);
+  }
+
+  /**
+   * Validates that the passed {@link PipelineOptions} from command line interface (CLI)
+   * conforms to all the validation criteria from the passed in interface.
+   *
+   * <p>Note that the interface requested must conform to the validation criteria specified
on
+   * {@link PipelineOptions#as(Class)}.
+   *
+   * @param klass The interface to fetch validation criteria from.
+   * @param options The {@link PipelineOptions} to validate.
+   * @return Validated options.
+   */
+  public static <T extends PipelineOptions> T validateCli(Class<T> klass, PipelineOptions
options) {
+    return validate(klass, options, true);
+  }
+
+  private static <T extends PipelineOptions> T validate(Class<T> klass, PipelineOptions
options,
+      boolean isCli) {
     checkNotNull(klass);
     checkNotNull(options);
     checkArgument(Proxy.isProxyClass(options.getClass()));
@@ -67,9 +87,15 @@ public class PipelineOptionsValidator {
             requiredGroups.put(requiredGroup, method);
           }
         } else {
-          checkArgument(handler.invoke(asClassOptions, method, null) != null,
-              "Missing required value for [%s, \"%s\"]. ",
-              method, getDescription(method));
+          if (isCli) {
+            checkArgument(handler.invoke(asClassOptions, method, null) != null,
+                "Missing required value for [--%s, \"%s\"]. ",
+                handler.getOptionName(method), getDescription(method));
+          } else {
+            checkArgument(handler.invoke(asClassOptions, method, null) != null,
+                "Missing required value for [%s, \"%s\"]. ",
+                method, getDescription(method));
+          }
         }
       }
     }

http://git-wip-us.apache.org/repos/asf/beam/blob/01b3febc/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
----------------------------------------------------------------------
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
index 3842388..926a7b9 100644
--- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
+++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java
@@ -166,6 +166,10 @@ class ProxyInvocationHandler implements InvocationHandler, Serializable
{
         + Arrays.toString(args) + "].");
   }
 
+  public String getOptionName(Method method) {
+    return gettersToPropertyNames.get(method.getName());
+  }
+
   private void writeObject(java.io.ObjectOutputStream stream)
       throws IOException {
     throw new NotSerializableException(

http://git-wip-us.apache.org/repos/asf/beam/blob/01b3febc/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsValidatorTest.java
----------------------------------------------------------------------
diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsValidatorTest.java
b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsValidatorTest.java
index 120d5ed..f8cd00f 100644
--- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsValidatorTest.java
+++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsValidatorTest.java
@@ -60,6 +60,18 @@ public class PipelineOptionsValidatorTest {
   }
 
   @Test
+  public void testWhenRequiredOptionIsSetAndClearedCli() {
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage("Missing required value for "
+        + "[--object, \"Fake Description\"].");
+
+    Required required = PipelineOptionsFactory.fromArgs(new String[]{"--object=blah"})
+        .as(Required.class);
+    required.setObject(null);
+    PipelineOptionsValidator.validateCli(Required.class, required);
+  }
+
+  @Test
   public void testWhenRequiredOptionIsNeverSet() {
     expectedException.expect(IllegalArgumentException.class);
     expectedException.expectMessage("Missing required value for "
@@ -70,6 +82,17 @@ public class PipelineOptionsValidatorTest {
     PipelineOptionsValidator.validate(Required.class, required);
   }
 
+
+  @Test
+  public void testWhenRequiredOptionIsNeverSetCli() {
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage("Missing required value for "
+        + "[--object, \"Fake Description\"].");
+
+    Required required = PipelineOptionsFactory.fromArgs(new String[]{}).as(Required.class);
+    PipelineOptionsValidator.validateCli(Required.class, required);
+  }
+
   @Test
   public void testWhenRequiredOptionIsNeverSetOnSuperInterface() {
     expectedException.expect(IllegalArgumentException.class);
@@ -81,6 +104,16 @@ public class PipelineOptionsValidatorTest {
     PipelineOptionsValidator.validate(Required.class, options);
   }
 
+  @Test
+  public void testWhenRequiredOptionIsNeverSetOnSuperInterfaceCli() {
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage("Missing required value for "
+        + "[--object, \"Fake Description\"].");
+
+    PipelineOptions options = PipelineOptionsFactory.fromArgs(new String[]{}).create();
+    PipelineOptionsValidator.validateCli(Required.class, options);
+  }
+
   /** A test interface that overrides the parent's method. */
   public interface SubClassValidation extends Required {
     @Override
@@ -100,6 +133,17 @@ public class PipelineOptionsValidatorTest {
     PipelineOptionsValidator.validate(Required.class, required);
   }
 
+  @Test
+  public void testValidationOnOverriddenMethodsCli() throws Exception {
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage("Missing required value for "
+        + "[--object, \"Fake Description\"].");
+
+    SubClassValidation required = PipelineOptionsFactory.fromArgs(new String[]{})
+        .as(SubClassValidation.class);
+    PipelineOptionsValidator.validateCli(Required.class, required);
+  }
+
   /** A test interface with a required group. */
   public interface GroupRequired extends PipelineOptions {
     @Validation.Required(groups = {"ham"})

http://git-wip-us.apache.org/repos/asf/beam/blob/01b3febc/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
----------------------------------------------------------------------
diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
index d90cb42..fb0a0d7 100644
--- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
+++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
@@ -1031,4 +1031,11 @@ public class ProxyInvocationHandlerTest {
     expectedException.expectCause(Matchers.<Throwable>instanceOf(NotSerializableException.class));
     SerializableUtils.clone(new CapturesOptions());
   }
+
+  @Test
+  public void testGetOptionNameFromMethod() throws NoSuchMethodException {
+    ProxyInvocationHandler handler = new ProxyInvocationHandler(Maps.<String, Object>newHashMap());
+    handler.as(BaseOptions.class);
+    assertEquals("foo", handler.getOptionName(BaseOptions.class.getMethod("getFoo")));
+  }
 }


Mime
View raw message