You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@beam.apache.org by lc...@apache.org on 2017/06/27 16:23:17 UTC
[1/2] beam git commit: [BEAM-2514] Improve error message on missing
required value
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 <ow...@gmail.com>
Authored: Mon Jun 26 20:55:13 2017 +0800
Committer: Luke Cwik <lc...@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")));
+ }
}
[2/2] beam git commit: [BEAM-2514] Improve error message on missing
required value
Posted by lc...@apache.org.
[BEAM-2514] Improve error message on missing required value
This closes #3438
Project: http://git-wip-us.apache.org/repos/asf/beam/repo
Commit: http://git-wip-us.apache.org/repos/asf/beam/commit/99ef92bc
Tree: http://git-wip-us.apache.org/repos/asf/beam/tree/99ef92bc
Diff: http://git-wip-us.apache.org/repos/asf/beam/diff/99ef92bc
Branch: refs/heads/master
Commit: 99ef92bc97692bdba3ea8e10cc09d81f3d64b69b
Parents: 9df865a 01b3feb
Author: Luke Cwik <lc...@google.com>
Authored: Tue Jun 27 09:23:03 2017 -0700
Committer: Luke Cwik <lc...@google.com>
Committed: Tue Jun 27 09:23:03 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(-)
----------------------------------------------------------------------