SQOOP-812: Serialization of Configuration objects to and from json is not working properly

(Jarek Jarcec Cecho via Cheolsoo Park)

common/src/main/java/org/apache/sqoop/model/FormUtils.java

@@ -32,9 +32,7 @@
 
 /**
  * Util class for transforming data from correctly annotated configuration
- * objects to form structures and vice-versa.
+ * objects to different structures and vice-versa.
- *
- * TODO(jarcec): JSON methods needs rewrittion!
  */
 public class FormUtils {
 
@@ -271,6 +269,13 @@ public static void applyValidation(List<MForm> forms, Validation validation) {
     }
   }
 
+  /**
+   * Convert configuration object to JSON. Only filled properties are serialized,
+   * properties with null value are skipped.
+   *
+   * @param configuration Correctly annotated configuration object
+   * @return String of JSON representation
+   */
   @SuppressWarnings("unchecked")
   public static String toJson(Object configuration) {
     Class klass = configuration.getClass();
@@ -284,100 +289,161 @@ public static String toJson(Object configuration) {
         "Missing annotation Configuration on class " + klass.getName());
     }
 
-    JSONObject jsonObject = new JSONObject();
+    JSONObject jsonOutput = new JSONObject();
 
     // Iterate over all declared fields
-    for (Field field : klass.getDeclaredFields()) {
+    for (Field formField : klass.getDeclaredFields()) {
-      field.setAccessible(true);
+      formField.setAccessible(true);
-      String fieldName = field.getName();
+      String formName = formField.getName();
 
-      // Each field that should be part of user input should have Input
+      // We're processing only form validations
-      // annotation.
+      Form formAnnotation = formField.getAnnotation(Form.class);
-      Input inputAnnotation = field.getAnnotation(Input.class);
+      if(formAnnotation == null) {
+        continue;
+      }
 
-      Object value;
+      Object formValue;
       try {
-        value = field.get(configuration);
+        formValue = formField.get(configuration);
       } catch (IllegalAccessException e) {
         throw new SqoopException(ModelError.MODEL_005,
-          "Issue with field " + field.getName(), e);
+          "Issue with field " + formName, e);
       }
 
-      // Do not serialize all values
+      JSONObject jsonForm = new JSONObject();
-      if(inputAnnotation != null && value != null) {
-        Class type = field.getType();
 
-        // We need to support NULL, so we do not support primitive types
+      // Now process each input on the form
-        if(type.isPrimitive()) {
+      for(Field inputField : formField.getType().getDeclaredFields()) {
-          throw new SqoopException(ModelError.MODEL_007,
+        inputField.setAccessible(true);
-            "Detected primitive type " + type + " for field " + fieldName);
+        String inputName = inputField.getName();
+
+        Object value;
+        try {
+          value = inputField.get(formValue);
+        } catch (IllegalAccessException e) {
+          throw new SqoopException(ModelError.MODEL_005,
+            "Issue with field " + formName + "." + inputName, e);
         }
 
-        if(type == String.class) {
+        Input inputAnnotation = inputField.getAnnotation(Input.class);
-          jsonObject.put(fieldName, value);
+
-        } else if (type.isAssignableFrom(Map.class)) {
+        // Do not serialize all values
-          JSONObject map = new JSONObject();
+        if(inputAnnotation != null && value != null) {
-          for(Object key : ((Map)value).keySet()) {
+          Class type = inputField.getType();
-            map.put(key, map.get(key));
+
+          // We need to support NULL, so we do not support primitive types
+          if(type.isPrimitive()) {
+            throw new SqoopException(ModelError.MODEL_007,
+              "Detected primitive type " + type + " for field " + formName + "." + inputName);
+          }
+
+          if(type == String.class) {
+            jsonForm.put(inputName, value);
+          } else if (type.isAssignableFrom(Map.class)) {
+            JSONObject map = new JSONObject();
+            for(Object key : ((Map)value).keySet()) {
+              map.put(key, ((Map)value).get(key));
+            }
+            jsonForm.put(inputName, map);
+          } else if(type == Integer.class) {
+            jsonForm.put(inputName, value);
+          } else if(type.isEnum()) {
+            jsonForm.put(inputName, value.toString());
+          } else {
+            throw new SqoopException(ModelError.MODEL_004,
+              "Unsupported type " + type.getName() + " for input " + formName + "." + inputName);
           }
-          jsonObject.put(fieldName, map);
-        } else if(type == Integer.class) {
-          jsonObject.put(fieldName, value);
-        } else if(type.isEnum()) {
-          jsonObject.put(fieldName, value);
-        } else {
-          throw new SqoopException(ModelError.MODEL_004,
-            "Unsupported type " + type.getName() + " for input " + fieldName);
         }
       }
+
+      jsonOutput.put(formName, jsonForm);
     }
 
-    return jsonObject.toJSONString();
+    return jsonOutput.toJSONString();
   }
 
-  // TODO(jarcec): This method currently do not iterate over all fields and
+  /**
-  // therefore some fields might have original values when original object will
+   * Parse given input JSON string and move it's values to given configuration
-  // be reused. This is unfortunately not acceptable.
+   * object.
+   *
+   * @param json JSON representation of the configuration object
+   * @param configuration Configuration object to be filled
+   */
   public static void fillValues(String json, Object configuration) {
     Class klass = configuration.getClass();
 
-    JSONObject jsonObject = (JSONObject) JSONValue.parse(json);
+    JSONObject jsonForms = (JSONObject) JSONValue.parse(json);
 
-    for(Object k : jsonObject.keySet()) {
+    for(Field formField : klass.getDeclaredFields()) {
-      String key = (String)k;
+      formField.setAccessible(true);
+      String formName = formField.getName();
 
-      Field field;
+      // We're processing only form validations
-      try {
+      Form formAnnotation = formField.getAnnotation(Form.class);
-        field = klass.getDeclaredField(key);
+      if(formAnnotation == null) {
-      } catch (NoSuchFieldException e) {
+        continue;
-        throw new SqoopException(ModelError.MODEL_006,
-          "Missing field " + key, e);
       }
 
-      // We need to access this field even if it would be declared as private
-      field.setAccessible(true);
-      Class type = field.getType();
-
       try {
-        if(type == String.class) {
+        formField.set(configuration, formField.getType().newInstance());
-          field.set(configuration, jsonObject.get(key));
+      } catch (Exception e) {
-        } else if (type.isAssignableFrom(Map.class)) {
+        throw new SqoopException(ModelError.MODEL_005,
-          Map<String, String> map = new HashMap<String, String>();
+          "Issue with field " + formName, e);
-          for(Object kk : jsonObject.keySet()) {
+      }
-            map.put((String)kk, (String)jsonObject.get(kk));
+
-          }
+      JSONObject jsonInputs = (JSONObject) jsonForms.get(formField.getName());
-          field.set(key, map);
+      if(jsonInputs == null) {
-        } else if(type == Integer.class) {
+        continue;
-          field.set(configuration, jsonObject.get(key));
+      }
-        } else if(type == Integer.class) {
+
-          field.set(configuration, Enum.valueOf((Class<? extends Enum>)field.getType(), (String) jsonObject.get(key)));
+      Object formValue;
-        } else {
+      try {
-          throw new SqoopException(ModelError.MODEL_004,
+        formValue = formField.get(configuration);
-            "Unsupported type " + type.getName() + " for input " + key);
-        }
       } catch (IllegalAccessException e) {
         throw new SqoopException(ModelError.MODEL_005,
-          "Issue with field " + field.getName(), e);
+          "Issue with field " + formName, e);
+      }
+
+      for(Field inputField : formField.getType().getDeclaredFields()) {
+        inputField.setAccessible(true);
+        String inputName = inputField.getName();
+
+        Input inputAnnotation = inputField.getAnnotation(Input.class);
+
+        if(inputAnnotation == null || jsonInputs.get(inputName) == null) {
+          try {
+            inputField.set(formValue, null);
+          } catch (IllegalAccessException e) {
+            throw new SqoopException(ModelError.MODEL_005,
+              "Issue with field " + formName + "." + inputName, e);
+          }
+          continue;
+        }
+
+        Class type = inputField.getType();
+
+        try {
+          if(type == String.class) {
+            inputField.set(formValue, jsonInputs.get(inputName));
+          } else if (type.isAssignableFrom(Map.class)) {
+            Map<String, String> map = new HashMap<String, String>();
+            JSONObject jsonObject = (JSONObject) jsonInputs.get(inputName);
+            for(Object key : jsonObject.keySet()) {
+              map.put((String)key, (String)jsonObject.get(key));
+            }
+            inputField.set(formValue, map);
+          } else if(type == Integer.class) {
+            inputField.set(formValue, ((Long)jsonInputs.get(inputName)).intValue());
+          } else if(type.isEnum()) {
+            inputField.set(formValue, Enum.valueOf((Class<? extends Enum>) inputField.getType(), (String) jsonInputs.get(inputName)));
+          } else {
+            throw new SqoopException(ModelError.MODEL_004,
+              "Unsupported type " + type.getName() + " for input " + formName + "." + inputName);
+          }
+        } catch (IllegalAccessException e) {
+          throw new SqoopException(ModelError.MODEL_005,
+            "Issue with field " + formName + "." + inputName, e);
+        }
       }
     }
   }

common/src/test/java/org/apache/sqoop/model/TestFormUtils.java

@@ -114,6 +114,39 @@ public void testApplyValidation() {
       forms.get(0).getInputs().get(1).getValidationMessage());
   }
 
+  public void testJson() {
+    Config config = new Config();
+    config.aForm.a1 = "A";
+    config.bForm.b2 = "B";
+    config.cForm.intValue = 4;
+    config.cForm.map.put("C", "D");
+    config.cForm.enumeration = Enumeration.X;
+
+    String json = FormUtils.toJson(config);
+
+    Config targetConfig = new Config();
+
+    // Old values from should be always removed
+    targetConfig.aForm.a2 = "X";
+    targetConfig.bForm.b1 = "Y";
+    // Nulls in forms shouldn't be an issue either
+    targetConfig.cForm = null;
+
+    FormUtils.fillValues(json, targetConfig);
+
+    assertEquals("A", targetConfig.aForm.a1);
+    assertNull(targetConfig.aForm.a2);
+
+    assertNull(targetConfig.bForm.b1);
+    assertEquals("B", targetConfig.bForm.b2);
+
+    assertEquals((Integer)4, targetConfig.cForm.intValue);
+    assertEquals(1, targetConfig.cForm.map.size());
+    assertTrue(targetConfig.cForm.map.containsKey("C"));
+    assertEquals("D", targetConfig.cForm.map.get("C"));
+    assertEquals(Enumeration.X, targetConfig.cForm.enumeration);
+  }
+
   protected Validation getValidation() {
     Map<Validation.FormInput, Validation.Message> messages
       = new HashMap<Validation.FormInput, Validation.Message>();
@@ -153,6 +186,7 @@ protected List<MForm> getForms() {
     inputs = new LinkedList<MInput<?>>();
     inputs.add(new MIntegerInput("cForm.intValue"));
     inputs.add(new MMapInput("cForm.map"));
+    inputs.add(new MEnumInput("cForm.enumeration", new String[]{"X", "Y"}));
     ret.add(new MForm("cForm", inputs));
 
     return ret;
@@ -193,6 +227,11 @@ public static class BForm {
   public static class CForm {
     @Input Integer intValue;
     @Input Map<String, String> map;
+    @Input Enumeration enumeration;
+
+    public CForm() {
+      map = new HashMap<String, String>();
+    }
   }
 
   @FormClass
@@ -202,4 +241,9 @@ public static class DForm {
 
   public static class ConfigWithout {
   }
+
+  enum Enumeration {
+    X,
+    Y,
+  }
 }