TSK-1642: now resolving complex objects during change detection properly

common/taskana-common/src/main/java/pro/taskana/common/internal/util/ObjectAttributeChangeDetector.java

@@ -1,12 +1,13 @@
 package pro.taskana.common.internal.util;
 
+import static pro.taskana.common.internal.util.CheckedFunction.wrap;
+
 import java.lang.reflect.Field;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import org.json.JSONObject;
 
@@ -23,82 +24,76 @@ public class ObjectAttributeChangeDetector {
    * @param newObject the new object for the comparison
    * @param <T> The generic type parameter
    * @return the details of all changed fields as JSON string
-   * @throws SystemException when any parameter is null
+   * @throws SystemException when any parameter is null or the class of oldObject and newObject do
+   *     not match
    */
   public static <T> String determineChangesInAttributes(T oldObject, T newObject) {
-    List<Field> fields = new ArrayList<>();
+    if (oldObject == null || newObject == null) {
-
-    if (Objects.isNull(oldObject) || Objects.isNull(newObject)) {
       throw new SystemException(
           "Null was provided as a parameter. Please provide two objects of the same type");
     }
 
-    Class<?> currentClass = oldObject.getClass();
+    Class<?> objectClass = oldObject.getClass();
-
+    if (List.class.isAssignableFrom(objectClass)) {
-    if (List.class.isAssignableFrom(currentClass)) {
-
       return compareLists(oldObject, newObject);
-
-    } else {
-
-      retrieveFields(fields, currentClass);
     }
 
-    Predicate<Triplet<Field, Object, Object>> areFieldsNotEqual =
+    // this has to be checked after we deal with List data types, because
-        fieldAndValuePairTriplet ->
+    // we want to allow different implementations of the List interface to work as well.
-            !Objects.equals(
+    if (!oldObject.getClass().equals(newObject.getClass())) {
-                fieldAndValuePairTriplet.getMiddle(), fieldAndValuePairTriplet.getRight());
+      throw new SystemException(
-    Predicate<Triplet<Field, Object, Object>> isFieldNotCustomAttributes =
+          String.format(
-        fieldAndValuePairTriplet ->
+              "The classes differ between the oldObject(%s) and newObject(%s). "
-            !fieldAndValuePairTriplet.getLeft().getName().equals("customAttributes");
+                  + "In order to detect changes properly they should not differ.",
+              oldObject.getClass().getName(), newObject.getClass().getName()));
+    }
 
     List<JSONObject> changedAttributes =
-        fields.stream()
+        retrieveAllFields(objectClass).stream()
             .peek(field -> field.setAccessible(true))
-            .map(
+            .filter(field -> !"customAttributes".equals(field.getName()))
-                CheckedFunction.wrap(
+            .map(wrap(field -> Triplet.of(field, field.get(oldObject), field.get(newObject))))
-                    field -> Triplet.of(field, field.get(oldObject), field.get(newObject))))
+            .filter(t -> !Objects.equals(t.getMiddle(), t.getRight()))
-            .filter(areFieldsNotEqual.and(isFieldNotCustomAttributes))
+            .map(t -> generateChangedAttribute(t.getLeft(), t.getMiddle(), t.getRight()))
-            .map(
-                fieldAndValuePairTriplet -> {
-                  JSONObject changedAttribute = new JSONObject();
-                  changedAttribute.put("fieldName", fieldAndValuePairTriplet.getLeft().getName());
-                  changedAttribute.put(
-                      "oldValue",
-                      Optional.ofNullable(fieldAndValuePairTriplet.getMiddle()).orElse(""));
-                  changedAttribute.put(
-                      "newValue",
-                      Optional.ofNullable(fieldAndValuePairTriplet.getRight()).orElse(""));
-                  return changedAttribute;
-                })
             .collect(Collectors.toList());
 
     JSONObject changes = new JSONObject();
     changes.put("changes", changedAttributes);
-
     return changes.toString();
   }
 
-  private static void retrieveFields(List<Field> fields, Class<?> currentClass) {
+  private static JSONObject generateChangedAttribute(
+      Field field, Object oldValue, Object newValue) {
+    JSONObject changedAttribute = new JSONObject();
+    changedAttribute.put("fieldName", field.getName());
+    changedAttribute.put(
+        "oldValue", Optional.ofNullable(oldValue).map(JSONObject::wrap).orElse(""));
+    changedAttribute.put(
+        "newValue", Optional.ofNullable(newValue).map(JSONObject::wrap).orElse(""));
+    return changedAttribute;
+  }
+
+  private static List<Field> retrieveAllFields(Class<?> currentClass) {
+    List<Field> fields = new ArrayList<>();
     while (currentClass.getSuperclass() != null) {
       fields.addAll(Arrays.asList(currentClass.getDeclaredFields()));
       currentClass = currentClass.getSuperclass();
     }
+    return fields;
   }
 
   private static <T> String compareLists(T oldObject, T newObject) {
-    if (!oldObject.equals(newObject)) {
+    if (oldObject.equals(newObject)) {
-      JSONObject changedAttribute = new JSONObject();
+      return "";
+    }
 
+    JSONObject changedAttribute = new JSONObject();
     changedAttribute.put("oldValue", oldObject);
     changedAttribute.put("newValue", newObject);
 
     JSONObject changes = new JSONObject();
-
     changes.put("changes", changedAttribute);
 
     return changes.toString();
   }
-    return "";
-  }
 }

history/taskana-simplehistory-provider/src/test/java/acceptance/events/task/CreateHistoryEventOnTaskTransferAccTest.java

@@ -148,20 +148,24 @@ class CreateHistoryEventOnTaskTransferAccTest extends AbstractAccTest {
     TaskHistoryEvent event = historyService.getTaskHistoryEvent(eventId);
     assertThat(event.getDetails()).isNotNull();
     JSONArray changes = new JSONObject(event.getDetails()).getJSONArray("changes");
-
     assertThat(changes.length()).isPositive();
-    for (int i = 0; i < changes.length(); i++) {
+    boolean foundField = false;
+    for (int i = 0; i < changes.length() && !foundField; i++) {
       JSONObject change = changes.getJSONObject(i);
       if (change.get("fieldName").equals("workbasketSummary")) {
-        String oldWorkbasketStr = (String) change.get("oldValue");
+        foundField = true;
-        String newWorkbasketStr = (String) change.get("newValue");
+        String oldWorkbasketStr = change.get("oldValue").toString();
+        String newWorkbasketStr = change.get("newValue").toString();
         WorkbasketService workbasketService = taskanaEngine.getWorkbasketService();
         Workbasket oldWorkbasket = workbasketService.getWorkbasket(expectedOldValue);
-        assertThat(oldWorkbasket.asSummary()).hasToString(oldWorkbasketStr);
+        assertThat(oldWorkbasketStr)
+            .isEqualTo(JSONObject.wrap(oldWorkbasket.asSummary()).toString());
         Workbasket newWorkbasket = workbasketService.getWorkbasket(expectedNewValue);
-        assertThat(newWorkbasket.asSummary()).hasToString(newWorkbasketStr);
+        assertThat(newWorkbasketStr)
+            .isEqualTo(JSONObject.wrap(newWorkbasket.asSummary()).toString());
       }
     }
+    assertThat(foundField).describedAs("changes do not contain field 'workbasketSummary'").isTrue();
 
     assertThat(event.getId()).startsWith("THI:");
     assertThat(event.getOldValue()).isEqualTo(expectedOldValue);