[GitHub] [beam] lukecwik commented on a change in pull request #15338: [BEAM-12767] Improve PipelineOption parsing UX

[GitBox <gi...@apache.org>]

lukecwik commented on a change in pull request #15338:
URL: https://github.com/apache/beam/pull/15338#discussion_r690614446



##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
##########
@@ -1572,6 +1588,63 @@ public int compare(Method o1, Method o2) {
     return builder.build();
   }
 
+  /**
+   * Attempt to parse an input string into an instance of `type` using an {@link ObjectMapper}.
+   *
+   * <p>If the getter method is annotated with {@link
+   * com.fasterxml.jackson.databind.annotation.JsonDeserialize} the specified deserializer will be
+   * used, otherwise the default ObjectMapper deserialization strategy.
+   *
+   * <p>Parsing is attempted twice, once with the raw string value. If that attempt fails, another
+   * attempt is made by wrapping the value in quotes so that it is interpreted as a JSON string.
+   */
+  private static Object tryParseObject(String value, JavaType type, Method method)
+      throws IOException {
+    AnnotationCollector ac = AnnotationCollector.emptyCollector();
+    for (Annotation ann : method.getAnnotations()) {
+      ac = ac.addOrOverride(ann);
+    }
+
+    AnnotatedMethod annotatedMethod =
+        new AnnotatedMethod(
+            new TypeResolutionContext.Empty(MAPPER.getTypeFactory()),
+            method,
+            ac.asAnnotationMap(),
+            null);
+
+    BeanPropertyDefinition propDef =
+        SimpleBeanPropertyDefinition.construct(MAPPER.getDeserializationConfig(), annotatedMethod);
+
+    BeanProperty prop =
+        new MethodProperty(
+            propDef,
+            type,
+            MAPPER.getDeserializationConfig().findTypeDeserializer(type),
+            ac.asAnnotations(),
+            annotatedMethod);
+
+    JsonNode tree;
+    try {
+      tree = MAPPER.readTree(value);
+    } catch (JsonParseException e) {
+      // try again, quoting the input string if it wasn't already
+      if (!(value.startsWith("\"") && value.endsWith("\""))) {
+        try {
+          tree = MAPPER.readTree("\"" + value + "\"");
+        } catch (JsonParseException inner) {
+          // rethrow the original exception rather the one thrown from the fallback attempt
+          throw e;
+        }
+      } else {
+        throw e;
+      }
+    }
+
+    JsonParser parser = new TreeTraversingParser(tree, MAPPER);
+    parser.nextToken();
+    return DESERIALIZATION_CONTEXT.readPropertyValue(parser, prop, type);

Review comment:
       This sounds like a useful improvement but we'll want `@JsonSerialize` as well since we need the value to round trip to JSON and back (unless the getter is marked with `@JsonIgnore` in that case we don't need the `@JsonSerialize`). 
   
   But be aware that you'll want to ensure that all instances used via the "as" transform use the same annotation since multiple PipelineOptions can define the same property effectively and we would want to limit how inconsistent conversions can appear so this will limit the feature a little.
   For example with `@JsonIgnore` https://github.com/apache/beam/blob/2ab002486345f5d97f11ccfbeb86541796c080e0/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java#L941 
   https://github.com/apache/beam/blob/2ab002486345f5d97f11ccfbeb86541796c080e0/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java#L1054
   

##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
##########
@@ -1572,6 +1588,63 @@ public int compare(Method o1, Method o2) {
     return builder.build();
   }
 
+  /**
+   * Attempt to parse an input string into an instance of `type` using an {@link ObjectMapper}.
+   *
+   * <p>If the getter method is annotated with {@link
+   * com.fasterxml.jackson.databind.annotation.JsonDeserialize} the specified deserializer will be
+   * used, otherwise the default ObjectMapper deserialization strategy.
+   *
+   * <p>Parsing is attempted twice, once with the raw string value. If that attempt fails, another
+   * attempt is made by wrapping the value in quotes so that it is interpreted as a JSON string.
+   */
+  private static Object tryParseObject(String value, JavaType type, Method method)
+      throws IOException {
+    AnnotationCollector ac = AnnotationCollector.emptyCollector();
+    for (Annotation ann : method.getAnnotations()) {
+      ac = ac.addOrOverride(ann);
+    }
+
+    AnnotatedMethod annotatedMethod =
+        new AnnotatedMethod(
+            new TypeResolutionContext.Empty(MAPPER.getTypeFactory()),
+            method,
+            ac.asAnnotationMap(),
+            null);
+
+    BeanPropertyDefinition propDef =
+        SimpleBeanPropertyDefinition.construct(MAPPER.getDeserializationConfig(), annotatedMethod);
+
+    BeanProperty prop =
+        new MethodProperty(
+            propDef,
+            type,
+            MAPPER.getDeserializationConfig().findTypeDeserializer(type),
+            ac.asAnnotations(),
+            annotatedMethod);
+
+    JsonNode tree;
+    try {
+      tree = MAPPER.readTree(value);
+    } catch (JsonParseException e) {
+      // try again, quoting the input string if it wasn't already
+      if (!(value.startsWith("\"") && value.endsWith("\""))) {
+        try {
+          tree = MAPPER.readTree("\"" + value + "\"");

Review comment:
       I saw the example in the test but am lost as two why converting `--myInstant=2021-07-17` actually fails. Would you be able to add an example of the failure to the tests?
   Also, is this an issue with the type deserialize not being able to handle JSON string -> object conversion?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org