FIX: DB.json().toBean(target) can update existing lists (#85)

* FIX: DB.json().toBean(target) can update existing lists

* remove sys.print, typo

---------

Co-authored-by: Roland Praml <roland.praml@foconis.de>
Co-authored-by: Juri Skrobko <juri.skrobko@foconis.de>

ebean-core/src/main/java/io/ebeaninternal/api/json/SpiJsonReader.java

@@ -34,5 +34,5 @@ public interface SpiJsonReader {
 
   Object readValueUsingObjectMapper(Class<?> propertyType) throws IOException;
 
-  boolean intercept();
+  boolean update();
 }

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptorJsonHelp.java

@@ -133,7 +133,12 @@ final class BeanDescriptorJsonHelp<T> {
         String key = parser.getCurrentName();
         BeanProperty p = desc.beanProperty(key);
         if (p != null) {
-          p.jsonRead(readJson, bean);
+          if (p.isVersion() && readJson.update() ) {
+            // skip version prop during update
+            p.jsonRead(readJson);
+          } else {
+            p.jsonRead(readJson, bean);
+          }
         } else {
           // read an unmapped property
           if (unmappedProperties == null) {

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java

@@ -20,9 +20,9 @@ import io.ebeaninternal.api.SpiExpressionRequest;
 import io.ebeaninternal.api.SpiQuery;
 import io.ebeaninternal.api.json.SpiJsonReader;
 import io.ebeaninternal.api.json.SpiJsonWriter;
+import io.ebeaninternal.server.bind.DataBind;
 import io.ebeaninternal.server.core.EncryptAlias;
 import io.ebeaninternal.server.core.InternString;
-import io.ebeaninternal.server.bind.DataBind;
 import io.ebeaninternal.server.deploy.generatedproperty.GeneratedProperty;
 import io.ebeaninternal.server.deploy.generatedproperty.GeneratedWhenCreated;
 import io.ebeaninternal.server.deploy.generatedproperty.GeneratedWhenModified;
@@ -1408,15 +1408,20 @@ public class BeanProperty implements ElPropertyValue, Property, STreeProperty {
   }
 
   public void jsonRead(SpiJsonReader ctx, EntityBean bean) throws IOException {
+    Object objValue = jsonRead(ctx);
+    if (jsonDeserialize) {
+      if (ctx.update()) {
+        setValueIntercept(bean, objValue);
+      } else {
+        setValue(bean, objValue);
+      }
+    }
+  }
+
+  public Object jsonRead(SpiJsonReader ctx) throws IOException {
     JsonToken event = ctx.nextToken();
     if (JsonToken.VALUE_NULL == event) {
-      if (jsonDeserialize) {
-        if (ctx.intercept()) {
-          setValueIntercept(bean, null);
-        } else {
-          setValue(bean, null);
-        }
-      }
+      return null;
     } else {
       // expect to read non-null json value
       Object objValue;
@@ -1433,13 +1438,7 @@ public class BeanProperty implements ElPropertyValue, Property, STreeProperty {
           CoreLog.log.log(ERROR, msg, e);
         }
       }
-      if (jsonDeserialize) {
-        if (ctx.intercept()) {
-          setValueIntercept(bean, objValue);
-        } else {
-          setValue(bean, objValue);
-        }
-      }
+      return objValue;
     }
   }
 

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocMany.java

@@ -2,6 +2,8 @@ package io.ebeaninternal.server.deploy;
 
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 import io.ebean.SqlUpdate;
 import io.ebean.Transaction;
 import io.ebean.bean.BeanCollection;
@@ -27,6 +29,7 @@ import java.io.StringWriter;
 import java.util.*;
 
 import static java.lang.System.Logger.Level.ERROR;
+import static java.lang.System.Logger.Level.WARNING;
 
 /**
  * Property mapped to a List Set or Map.
@@ -937,7 +940,7 @@ public class BeanPropertyAssocMany<T> extends BeanPropertyAssoc<T> implements ST
     if (elementDescriptor != null) {
       elementDescriptor.jsonWriteMapEntry(ctx, entry);
     } else {
-      targetDescriptor.jsonWrite(ctx, (EntityBean)entry.getValue());
+      targetDescriptor.jsonWrite(ctx, (EntityBean) entry.getValue());
     }
   }
 
@@ -1003,7 +1006,7 @@ public class BeanPropertyAssocMany<T> extends BeanPropertyAssoc<T> implements ST
     if (JsonToken.VALUE_NULL == event) {
       return null;
     }
-    return jsonReadCollection(ctx, null);
+    return jsonReadCollection(ctx, null, null);
   }
 
   /**
@@ -1016,15 +1019,16 @@ public class BeanPropertyAssocMany<T> extends BeanPropertyAssoc<T> implements ST
   /**
    * Read the collection (JSON Array) containing entity beans.
    */
-  public Object jsonReadCollection(SpiJsonReader readJson, EntityBean parentBean) throws IOException {
+  public Object jsonReadCollection(SpiJsonReader readJson, EntityBean parentBean, Object targets) throws IOException {
     if (elementDescriptor != null && elementDescriptor.isJsonReadCollection()) {
       return elementDescriptor.jsonReadCollection(readJson, parentBean);
     }
     BeanCollection<?> collection = createEmpty(parentBean);
     BeanCollectionAdd add = beanCollectionAdd(collection);
+    Map<Object, T> existingBeans = extractBeans(targets);
     do {
-      // CHECKME: Update existing list entry here?
-      EntityBean detailBean = (EntityBean) targetDescriptor.jsonRead(readJson, name, null);
+
+      EntityBean detailBean = getDetailBean(readJson, existingBeans);
       if (detailBean == null) {
         // read the entire array
         break;
@@ -1039,6 +1043,63 @@ public class BeanPropertyAssocMany<T> extends BeanPropertyAssoc<T> implements ST
     return collection;
   }
 
+  /**
+   * Find bean in the target collection and reuse it for JSON update.
+   */
+  private EntityBean getDetailBean(SpiJsonReader readJson, Map<Object, T> targets) throws IOException {
+    BeanProperty idProperty = targetDescriptor.idProperty();
+    if (targets == null || idProperty == null) {
+      return (EntityBean) targetDescriptor.jsonRead(readJson, name, null);
+    } else {
+      JsonToken token = readJson.parser().nextToken();
+      if (JsonToken.VALUE_NULL == token || JsonToken.END_ARRAY == token) {
+        return null;
+      }
+      // extract the id. We have to buffer the JSON;
+      ObjectNode node = readJson.mapper().readTree(readJson.parser());
+      SpiJsonReader jsonReader = readJson.forJson(node.traverse());
+      JsonNode idNode = node.get(idProperty.name());
+      Object id = idNode == null ? null : idProperty.jsonRead(readJson.forJson(idNode.traverse()));
+      return (EntityBean) targetDescriptor.jsonRead(jsonReader, name, targets.get(id));
+    }
+  }
+
+  /**
+   * Extract beans, that are currently in the target collection. (targets can be a List/Set/Map)
+   */
+  private Map<Object, T> extractBeans(Object targets) {
+    Collection<T> beans;
+
+    if (targets == null) {
+      return null;
+    } else if (targets instanceof Map) {
+      if (((Map<?, T>) targets).isEmpty()) {
+        return null;
+      }
+      beans = ((Map<?, T>) targets).values();
+    } else if (targets instanceof Collection) {
+      if (((Collection<?>) targets).isEmpty()) {
+        return null;
+      }
+      beans = (Collection<T>) targets;
+    } else {
+      CoreLog.log.log(WARNING, "Found non collection value " + targets.getClass().getSimpleName());
+      return null;
+    }
+
+    BeanProperty idProp = targetDescriptor.idProperty();
+    Map<Object, T> ret = new HashMap<>();
+    for (T bean : beans) {
+      if (bean instanceof EntityBean) {
+        Object id = idProp.getValue((EntityBean) bean);
+        if (id != null) {
+          ret.put(id, bean);
+        }
+      }
+    }
+    return ret.isEmpty() ? null : ret;
+  }
+
   /**
    * Bind all the property values to the SqlUpdate.
    */

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocManyJsonHelp.java

@@ -50,10 +50,10 @@ class BeanPropertyAssocManyJsonHelp {
     if (JsonToken.START_ARRAY != event && JsonToken.START_OBJECT != event) {
       throw new JsonParseException(parser, "Unexpected token " + event + " - expecting start array or object");
     }
-    if (readJson.intercept()) {
-      many.setValueIntercept(parentBean, many.jsonReadCollection(readJson, parentBean));
+    if (readJson.update()) {
+      many.setValueIntercept(parentBean, many.jsonReadCollection(readJson, parentBean, many.value(parentBean)));
     } else {
-      many.setValue(parentBean, many.jsonReadCollection(readJson, parentBean));
+      many.setValue(parentBean, many.jsonReadCollection(readJson, parentBean, null));
     }
   }
 

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocOne.java

@@ -787,9 +787,10 @@ public class BeanPropertyAssocOne<T> extends BeanPropertyAssoc<T> implements STr
   @Override
   public void jsonRead(SpiJsonReader readJson, EntityBean bean) throws IOException {
     if (jsonDeserialize && targetDescriptor != null) {
-      T target = (T) value(bean);
+      // CHECKME: may we skip reading the object from the json stream?
+      T target = readJson.update() ? (T) value(bean) : null;
       T assocBean = targetDescriptor.jsonRead(readJson, name, target);
-      if (readJson.intercept()) {
+      if (readJson.update()) {
         setValueIntercept(bean, assocBean);
       } else {
         setValue(bean, assocBean);
@@ -797,6 +798,11 @@ public class BeanPropertyAssocOne<T> extends BeanPropertyAssoc<T> implements STr
     }
   }
 
+  @Override
+  public Object jsonRead(SpiJsonReader readJson) throws IOException {
+    return targetDescriptor.jsonRead(readJson, name, null);
+  }
+
   public boolean isReference(Object detailBean) {
     EntityBean eb = (EntityBean) detailBean;
     return targetDescriptor.isReference(eb._ebean_getIntercept());

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertySimpleCollection.java

@@ -19,7 +19,7 @@ public final class BeanPropertySimpleCollection<T> extends BeanPropertyAssocMany
   }
 
   @Override
-  public Object jsonReadCollection(SpiJsonReader readJson, EntityBean parentBean) throws IOException {
+  public Object jsonReadCollection(SpiJsonReader readJson, EntityBean parentBean, Object collectionValue) throws IOException {
     return elementDescriptor.jsonReadCollection(readJson, parentBean);
   }
 }

ebean-core/src/main/java/io/ebeaninternal/server/json/ReadJson.java

@@ -32,13 +32,13 @@ public final class ReadJson implements SpiJsonReader {
   private final Object objectMapper;
   private final PersistenceContext persistenceContext;
   private final LoadContext loadContext;
-  private final boolean intercept;
+  private final boolean update;
   private final boolean enableLazyLoading;
 
   /**
    * Construct with parser and readOptions.
    */
-  public ReadJson(BeanDescriptor<?> desc, JsonParser parser, JsonReadOptions readOptions, Object objectMapper, boolean intercept) {
+  public ReadJson(BeanDescriptor<?> desc, JsonParser parser, JsonReadOptions readOptions, Object objectMapper, boolean update) {
     this.rootDesc = desc;
     this.parser = parser;
     this.objectMapper = objectMapper;
@@ -48,7 +48,7 @@ public final class ReadJson implements SpiJsonReader {
     // only create visitorMap, pathStack if needed ...
     this.visitorMap = (readOptions == null) ? null : readOptions.getVisitorMap();
     this.pathStack = (visitorMap == null && loadContext == null) ? null : new PathStack();
-    this.intercept = intercept;
+    this.update = update;
   }
 
   /**
@@ -62,7 +62,7 @@ public final class ReadJson implements SpiJsonReader {
     this.objectMapper = source.objectMapper;
     this.persistenceContext = source.persistenceContext;
     this.loadContext = source.loadContext;
-    this.intercept = source.intercept;
+    this.update = source.update;
     this.enableLazyLoading = source.enableLazyLoading;
   }
 
@@ -213,10 +213,10 @@ public final class ReadJson implements SpiJsonReader {
   }
 
   /**
-   * Do we have to set values via intercept or not.
+   * Do we update an existing bean? This meeans we have to set values via intercept and handle collections.
    */
   @Override
-  public boolean intercept() {
-    return intercept;
+  public boolean update() {
+    return update;
   }
 }

ebean-test/src/test/java/org/tests/text/json/TestJsonBeanDescriptorParse.java

@@ -12,14 +12,19 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.tests.model.basic.Address;
+import org.tests.model.basic.Contact;
+import org.tests.model.basic.ContactNote;
 import org.tests.model.basic.Customer;
 
 import java.io.IOException;
 import java.io.StringReader;
+import java.util.Comparator;
 import java.util.Set;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class TestJsonBeanDescriptorParse extends BaseTestCase {
 
@@ -32,6 +37,17 @@ public class TestJsonBeanDescriptorParse extends BaseTestCase {
     address.setLine1("foo");
     DB.save(address);
     customer.setBillingAddress(address);
+
+    Contact alfred = new Contact("Alfred", "P");
+    alfred.setId(789); // set some deterministic id
+    ContactNote note = new ContactNote("Drinks", "Order 100l beer");
+    note.setId(890);
+    alfred.getNotes().add(note);
+    customer.getContacts().add(alfred);
+    Contact anton = new Contact("Anton", "P");
+    anton.setId(790);
+    anton.getNotes().add(new ContactNote("Equipment", "Organize barbecue"));
+    customer.getContacts().add(anton);
     DB.save(customer);
   }
 
@@ -59,6 +75,38 @@ public class TestJsonBeanDescriptorParse extends BaseTestCase {
     assertThat(beanState.dirtyValues()).isEmpty();
   }
 
+  @Test
+  public void testJsonManyUpdate() throws IOException {
+
+    Customer customer = DB.find(Customer.class, 234);
+    String json =
+      "{\"contacts\": [ "
+        + "  {\"id\": 789, \"lastName\": \"Praml\", \"notes\": ["
+        + "    {\"id\": 890,\"title\": \"Beer\",\"note\": \"Order 200l beer\",\"version\": 17},"
+        + "    {\"title\": \"Food\",\"note\": \"Order 20 steaks\"}"
+        + "  ]},"
+        + "  {\"id\": 790, \"firstName\": \"Anton\",  \"lastName\": null, \"notes\" : null } "
+        + "]}";
+    DB.json().toBean(customer, json);
+    DB.save(customer);
+
+    customer = DB.find(Customer.class, 234);
+    assertThat(customer.getContacts()).hasSize(2);
+    customer.getContacts().sort(Comparator.comparing(Contact::getId));
+
+    Contact contact = customer.getContacts().get(0);
+    assertThat(contact.getFirstName()).isEqualTo("Alfred");
+    assertThat(contact.getLastName()).isEqualTo("Praml");
+    assertThat(contact.getNotes()).hasSize(2).extracting(ContactNote::getNote)
+      .containsExactlyInAnyOrder("Order 200l beer", "Order 20 steaks");
+
+    contact = customer.getContacts().get(1);
+    assertThat(contact.getFirstName()).isEqualTo("Anton");
+    assertThat(contact.getLastName()).isEqualTo(null);
+    assertThat(contact.getNotes()).hasSize(1);
+
+  }
+
   @Test
   public void testJsonUpdate() throws IOException {
     Customer customer = DB.find(Customer.class, 234);