OLINGO-848: getting a property of an entity flags the entity as changed

Signed-off-by: Christian Amend <christian.amend@sap.com>

ext/client-proxy/src/main/java/org/apache/olingo/ext/proxy/commons/AbstractCollectionInvocationHandler.java

@@ -64,8 +64,6 @@ public abstract class AbstractCollectionInvocationHandler<T extends Serializable
   private final Map<Class<? extends AbstractTerm>, Object> annotationsByTerm =
           new HashMap<Class<? extends AbstractTerm>, Object>();
 
-  private boolean changed = false;
-
   public AbstractCollectionInvocationHandler(
           final AbstractService<?> service,
           final Collection<T> items,
@@ -174,7 +172,6 @@ public abstract class AbstractCollectionInvocationHandler<T extends Serializable
         service.getContext().entityContext().attachNew(handler);
       }
     }
-    changed = true;
     return items.add(element);
   }
 
@@ -186,7 +183,6 @@ public abstract class AbstractCollectionInvocationHandler<T extends Serializable
         return false;
       }
 
-      changed = true;
       return referenceItems.add(id.toASCIIString());
     }
 
@@ -243,7 +239,6 @@ public abstract class AbstractCollectionInvocationHandler<T extends Serializable
 
   @Override
   public boolean addAll(final Collection<? extends T> collection) {
-    changed = true;
     return items.addAll(collection);
   }
 
@@ -324,8 +319,4 @@ public abstract class AbstractCollectionInvocationHandler<T extends Serializable
     this.uri = this.baseURI == null ? null : getClient().newURIBuilder(baseURI.toASCIIString());
     this.nextPageURI = null;
   }
-
-  public boolean isChanged() {
-    return changed;
-  }
 }

ext/client-proxy/src/main/java/org/apache/olingo/ext/proxy/commons/AbstractStructuredInvocationHandler.java

@@ -93,10 +93,6 @@ public abstract class AbstractStructuredInvocationHandler extends AbstractInvoca
 
   protected final Map<NavigationProperty, Object> linkCache = new HashMap<NavigationProperty, Object>();
 
-  protected int propertiesTag = 0;
-
-  protected int linksTag = 0;
-
   protected final Map<String, EdmStreamValue> streamedPropertyChanges = new HashMap<String, EdmStreamValue>();
 
   protected final Map<String, EdmStreamValue> streamedPropertyCache = new HashMap<String, EdmStreamValue>();
@@ -386,7 +382,6 @@ public abstract class AbstractStructuredInvocationHandler extends AbstractInvoca
         }
 
         if (res != null) {
-          addPropertyChanges(name, res);
           propertyCache.put(name, res);
         }
 
@@ -526,7 +521,79 @@ public abstract class AbstractStructuredInvocationHandler extends AbstractInvoca
   }
 
   public Map<String, Object> getPropertyChanges() {
-    return propertyChanges;
+    Map<String, Object> changedProperties = new HashMap<String, Object>();
+    changedProperties.putAll(propertyChanges);
+
+    for (Map.Entry<String, Object> propertyCacheEntry : propertyCache.entrySet()) {
+      if (hasCachedPropertyChanged(propertyCacheEntry.getValue())) {
+        changedProperties.put(propertyCacheEntry.getKey(), propertyCacheEntry.getValue());
+      }
+    }
+
+    return changedProperties;
+  }
+
+  protected boolean hasCachedPropertyChanged(final Object cachedValue) {
+    AbstractStructuredInvocationHandler structuredInvocationHandler = getStructuredInvocationHandler(cachedValue);
+    if (structuredInvocationHandler != null) {
+      return structuredInvocationHandler.isChanged();
+    }
+
+    return false;
+  }
+
+  public boolean isChanged() {
+    return !linkChanges.isEmpty()
+        || hasPropertyChanges();
+  }
+
+  protected boolean hasPropertyChanges() {
+    return !propertyChanges.isEmpty() || hasDeepPropertyChanges();
+  }
+
+  protected boolean hasDeepPropertyChanges() {
+    for (Object propertyValue : propertyCache.values()) {
+      if (hasCachedPropertyChanged(propertyValue)) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+  
+  public void applyChanges() {
+    streamedPropertyCache.putAll(streamedPropertyChanges);
+    streamedPropertyChanges.clear();
+    propertyCache.putAll(propertyChanges);
+    propertyChanges.clear();
+    linkCache.putAll(linkChanges);
+    linkChanges.clear();
+    
+    applyChangesOnChildren();
+  }
+
+  protected void applyChangesOnChildren() {
+    for (Object propertyValue : propertyCache.values()) {
+      applyChanges(propertyValue);
+    }
+  }
+
+  protected void applyChanges(final Object cachedValue) {
+    AbstractStructuredInvocationHandler structuredInvocationHandler = getStructuredInvocationHandler(cachedValue);
+    if (structuredInvocationHandler != null) {
+      structuredInvocationHandler.applyChanges();
+    }
+  }
+  
+  protected AbstractStructuredInvocationHandler getStructuredInvocationHandler(final Object value) {
+    if (value != null && Proxy.isProxyClass(value.getClass())) {
+      InvocationHandler invocationHandler = Proxy.getInvocationHandler(value);
+      if (invocationHandler instanceof AbstractStructuredInvocationHandler) {
+        return (AbstractStructuredInvocationHandler) invocationHandler;
+      }
+    }
+
+    return null;
   }
 
   public Collection<String> readAdditionalPropertyNames() {
@@ -567,14 +634,11 @@ public abstract class AbstractStructuredInvocationHandler extends AbstractInvoca
   }
 
   protected void addPropertyChanges(final String name, final Object value) {
-    final int checkpoint = propertyChanges.hashCode();
+    propertyCache.remove(name);
-    updatePropertiesTag(checkpoint);
     propertyChanges.put(name, value);
   }
 
   protected void addLinkChanges(final NavigationProperty navProp, final Object value) {
-    final int checkpoint = linkChanges.hashCode();
-    updateLinksTag(checkpoint);
     linkChanges.put(navProp, value);
 
     if (linkCache.containsKey(navProp)) {
@@ -582,18 +646,6 @@ public abstract class AbstractStructuredInvocationHandler extends AbstractInvoca
     }
   }
 
-  protected void updatePropertiesTag(final int checkpoint) {
-    if (propertiesTag == 0 || checkpoint == propertiesTag) {
-      propertiesTag = propertyChanges.hashCode();
-    }
-  }
-
-  protected void updateLinksTag(final int checkpoint) {
-    if (linksTag == 0 || checkpoint == linksTag) {
-      linksTag = linkChanges.hashCode();
-    }
-  }
-
   public Map<String, EdmStreamValue> getStreamedPropertyChanges() {
     return streamedPropertyChanges;
   }
@@ -642,8 +694,6 @@ public abstract class AbstractStructuredInvocationHandler extends AbstractInvoca
 
   protected abstract void load();
 
-  public abstract boolean isChanged();
-
   protected abstract <T extends ClientProperty> List<T> getInternalProperties();
 
   protected abstract ClientProperty getInternalProperty(final String name);

ext/client-proxy/src/main/java/org/apache/olingo/ext/proxy/commons/ComplexInvocationHandler.java

@@ -27,10 +27,10 @@ import org.apache.commons.lang3.tuple.ImmutablePair;
 import org.apache.commons.lang3.tuple.Pair;
 import org.apache.olingo.client.api.communication.request.retrieve.ODataPropertyRequest;
 import org.apache.olingo.client.api.communication.response.ODataRetrieveResponse;
-import org.apache.olingo.client.api.uri.URIBuilder;
 import org.apache.olingo.client.api.domain.ClientComplexValue;
 import org.apache.olingo.client.api.domain.ClientLinked;
 import org.apache.olingo.client.api.domain.ClientProperty;
+import org.apache.olingo.client.api.uri.URIBuilder;
 import org.apache.olingo.commons.api.edm.FullQualifiedName;
 import org.apache.olingo.ext.proxy.AbstractService;
 import org.apache.olingo.ext.proxy.api.annotations.ComplexType;
@@ -144,11 +144,6 @@ public class ComplexInvocationHandler extends AbstractStructuredInvocationHandle
     return retrieveNavigationProperty(property, getter);
   }
 
-  @Override
-  public boolean isChanged() {
-    return getEntityHandler() == null ? false : getEntityHandler().isChanged();
-  }
-
   @Override
   protected void load() {
     try {

ext/client-proxy/src/main/java/org/apache/olingo/ext/proxy/commons/EntityInvocationHandler.java

@@ -229,13 +229,12 @@ public class EntityInvocationHandler extends AbstractStructuredInvocationHandler
     this.streamedPropertyChanges.clear();
     this.streamedPropertyCache.clear();
     this.propertyChanges.clear();
+    this.propertyCache.clear();
     this.linkChanges.clear();
     this.linkCache.clear();
-    this.propertiesTag = 0;
-    this.linksTag = 0;
     this.annotations.clear();
   }
-
+  
   public EntityUUID getUUID() {
     return uuid;
   }
@@ -301,11 +300,10 @@ public class EntityInvocationHandler extends AbstractStructuredInvocationHandler
     return isChanged(true);
   }
 
-  public boolean isChanged(final boolean deep) {
+  public boolean isChanged(final boolean considerStreamProperties) {
-    return this.linkChanges.hashCode() != this.linksTag
+    return super.isChanged()
-        || this.propertyChanges.hashCode() != this.propertiesTag
+        || (considerStreamProperties && (stream != null
-        || (deep && (this.stream != null
+            || !streamedPropertyChanges.isEmpty()));
-        || !this.streamedPropertyChanges.isEmpty()));
   }
 
   public void uploadStream(final EdmStreamValue stream) {

ext/client-proxy/src/main/java/org/apache/olingo/ext/proxy/commons/NonTransactionalPersistenceManagerImpl.java

@@ -74,15 +74,27 @@ public class NonTransactionalPersistenceManagerImpl extends AbstractPersistenceM
         }
 
         if (entry.getValue() != null
-                && response instanceof ODataEntityCreateResponse && response.getStatusCode() == 201) {
+            && response instanceof ODataEntityCreateResponse && (response.getStatusCode() == 201 || response
-          entry.getValue().setEntity(((ODataEntityCreateResponse<?>) response).getBody());
+                .getStatusCode() == 204)) {
-          responses.put(index, entry.getValue().getEntityURI());
+          if (response.getStatusCode() == 201) {
-          LOG.debug("Upgrade created object '{}'", entry.getValue());
+            entry.getValue().setEntity(((ODataEntityCreateResponse<?>) response).getBody());
+            responses.put(index, entry.getValue().getEntityURI());
+            LOG.debug("Upgrade created object '{}'", entry.getValue());
+          } else {
+            entry.getValue().applyChanges();
+            responses.put(index, null);
+          }
         } else if (entry.getValue() != null
-                && response instanceof ODataEntityUpdateResponse && response.getStatusCode() == 200) {
+            && response instanceof ODataEntityUpdateResponse && (response.getStatusCode() == 200 || response
-          entry.getValue().setEntity(((ODataEntityUpdateResponse<?>) response).getBody());
+                .getStatusCode() == 204)) {
-          responses.put(index, entry.getValue().getEntityURI());
+          if (response.getStatusCode() == 200) {
-          LOG.debug("Upgrade updated object '{}'", entry.getValue());
+            entry.getValue().setEntity(((ODataEntityUpdateResponse<?>) response).getBody());
+            responses.put(index, entry.getValue().getEntityURI());
+            LOG.debug("Upgrade updated object '{}'", entry.getValue());
+          } else {
+            entry.getValue().applyChanges();
+            responses.put(index, null);
+          }
         } else {
           responses.put(index, null);
         }

ext/client-proxy/src/main/java/org/apache/olingo/ext/proxy/commons/TransactionalPersistenceManagerImpl.java

@@ -116,12 +116,22 @@ public class TransactionalPersistenceManagerImpl extends AbstractPersistenceMana
         final EntityInvocationHandler handler = items.get(changesetItemId);
 
         if (handler != null) {
-          if (res instanceof ODataEntityCreateResponse && res.getStatusCode() == 201) {
+          if (res instanceof ODataEntityCreateResponse && (res.getStatusCode() == 201 || res
-            handler.setEntity(((ODataEntityCreateResponse<?>) res).getBody());
+              .getStatusCode() == 204)) {
-            LOG.debug("Upgrade created object '{}'", handler);
+            if (res.getStatusCode() == 201) {
-          } else if (res instanceof ODataEntityUpdateResponse && res.getStatusCode() == 200) {
+              handler.setEntity(((ODataEntityCreateResponse<?>) res).getBody());
-            handler.setEntity(((ODataEntityUpdateResponse<?>) res).getBody());
+              LOG.debug("Upgrade created object '{}'", handler);
-            LOG.debug("Upgrade updated object '{}'", handler);
+            } else {
+              handler.applyChanges();
+            }
+          } else if (res instanceof ODataEntityUpdateResponse && (res.getStatusCode() == 200 || res
+              .getStatusCode() == 204)) {
+            if (res.getStatusCode() == 201) {
+              handler.setEntity(((ODataEntityUpdateResponse<?>) res).getBody());
+              LOG.debug("Upgrade updated object '{}'", handler);
+            } else {
+              handler.applyChanges();
+            }
           }
         }
       }

fit/src/test/java/org/apache/olingo/fit/proxy/ChangeDetectionTestITCase.java

@@ -0,0 +1,188 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.olingo.fit.proxy;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.lang.reflect.Proxy;
+import java.sql.Timestamp;
+import java.util.Calendar;
+
+import org.apache.commons.lang3.RandomUtils;
+import org.apache.olingo.ext.proxy.api.ComplexType;
+import org.apache.olingo.ext.proxy.api.EntityCollection;
+import org.apache.olingo.ext.proxy.api.EntityType;
+import org.apache.olingo.ext.proxy.commons.ComplexInvocationHandler;
+import org.apache.olingo.ext.proxy.commons.EntityCollectionInvocationHandler;
+import org.apache.olingo.ext.proxy.commons.EntityInvocationHandler;
+// CHECKSTYLE:OFF (Maven checkstyle)
+import org.apache.olingo.fit.proxy.staticservice.microsoft.test.odata.services.odatawcfservice.InMemoryEntities;
+import org.apache.olingo.fit.proxy.staticservice.microsoft.test.odata.services.odatawcfservice.types.Account;
+import org.apache.olingo.fit.proxy.staticservice.microsoft.test.odata.services.odatawcfservice.types.Address;
+import org.apache.olingo.fit.proxy.staticservice.microsoft.test.odata.services.odatawcfservice.types.Customer;
+import org.apache.olingo.fit.proxy.staticservice.microsoft.test.odata.services.odatawcfservice.types.Order;
+import org.apache.olingo.fit.proxy.staticservice.microsoft.test.odata.services.odatawcfservice.types.OrderCollection;
+import org.apache.olingo.fit.proxy.staticservice.microsoft.test.odata.services.odatawcfservice.types.PaymentInstrument;
+import org.apache.olingo.fit.proxy.staticservice.microsoft.test.odata.services.odatawcfservice.types.PaymentInstrumentCollection;
+// CHECKSTYLE:ON (Maven checkstyle)
+import org.junit.Test;
+
+public class ChangeDetectionTestITCase extends AbstractTestITCase {
+
+  @Test
+  public void entityUnchangedOnGetProperty() {
+    final Customer customer = getContainer().getCustomers().getByKey(1).load();
+    assertFalse(isChanged(customer));
+
+    customer.getLastName();
+
+    assertFalse(isChanged(customer));
+  }
+
+  @Test
+  public void entityChangedOnSetProperty() {
+    final Customer customer = getContainer().getCustomers().getByKey(1).load();
+    assertFalse(isChanged(customer));
+
+    customer.setLastName("Test");
+
+    assertTrue(isChanged(customer));
+
+    getContainer().flush();
+    assertFalse(isChanged(customer));
+  }
+
+  @Test
+  public void entityUnchangedOnGetComplexProperty() {
+    final Customer customer = getContainer().getCustomers().getByKey(1).load();
+    assertFalse(isChanged(customer));
+
+    final Address homeAddress = customer.getHomeAddress();
+    assertFalse(isChanged(customer));
+
+    homeAddress.getCity();
+    assertFalse(isChanged(homeAddress));
+    assertFalse(isChanged(customer));
+  }
+
+  @Test
+  public void entityChangedOnSetComplexProperty() {
+    final Customer customer = getContainer().getCustomers().getByKey(2).load();
+    assertFalse(isChanged(customer));
+
+    final Address newAdress = getContainer().newComplexInstance(Address.class);
+    customer.setHomeAddress(newAdress);
+
+    assertTrue(isChanged(customer));
+
+    getContainer().flush();
+    assertFalse(isChanged(customer));
+  }
+
+  @Test
+  public void entityChangedOnSetPropertyOfComplexProperty() {
+    final Customer customer = getContainer().getCustomers().getByKey(1).load();
+    assertFalse(isChanged(customer));
+
+    final Address homeAddress = customer.getHomeAddress();
+    homeAddress.setCity("Test");
+
+    assertTrue(isChanged(customer));
+
+    getContainer().flush();
+    assertFalse(isChanged(customer));
+  }
+
+  @Test
+  public void entityUnchangedOnGetNavigationProperty() {
+    final Customer customer = getContainer().getCustomers().getByKey(1).load();
+    assertFalse(isChanged(customer));
+
+    customer.getOrders();
+
+    assertFalse(isChanged(customer));
+  }
+
+  @Test
+  public void entityChangedOnAddNavigationProperty() {
+    final Account account = getContainer().getAccounts().getByKey(101).load();
+    assertFalse(isChanged(account));
+
+    final PaymentInstrumentCollection instruments = account.getMyPaymentInstruments().execute();
+    assertFalse(isChanged(account));
+
+    final PaymentInstrument instrument = getContainer().newEntityInstance(PaymentInstrument.class);
+    final int id = RandomUtils.nextInt(101999, 105000);
+    instrument.setPaymentInstrumentID(id);
+    instrument.setFriendlyName("New one");
+    instrument.setCreatedDate(new Timestamp(Calendar.getInstance().getTimeInMillis()));
+    instruments.add(instrument);
+
+    assertTrue(isChanged(instrument));
+    assertFalse(isChanged(account));
+
+    getContainer().flush();
+    assertFalse(isChanged(instrument));
+  }
+
+  @Test
+  public void entityCollectionUnchangedOnGet() {
+    final Customer customer = getContainer().getCustomers().getByKey(1).load();
+    assertFalse(isChanged(customer));
+
+    final OrderCollection orders = customer.getOrders().execute();
+    assertFalse(isChanged(customer));
+
+    for (Order order : orders) {
+      assertFalse(isChanged(order));
+      order.getOrderDate();
+      assertFalse(isChanged(order));
+    }
+
+    assertFalse(isChanged(customer));
+  }
+
+  protected InMemoryEntities getContainer() {
+    return container;
+  }
+
+  protected boolean isChanged(final EntityType<?> entity) {
+    EntityInvocationHandler invocationHandler = getInvocationHandler(entity);
+    return invocationHandler.isChanged();
+  }
+
+  protected boolean isChanged(final ComplexType<?> complex) {
+    ComplexInvocationHandler invocationHandler = getInvocationHandler(complex);
+    return invocationHandler.isChanged();
+  }
+
+  protected EntityInvocationHandler getInvocationHandler(final EntityType<?> entity) {
+    return (EntityInvocationHandler) Proxy.getInvocationHandler(entity);
+  }
+
+  protected ComplexInvocationHandler getInvocationHandler(final ComplexType<?> complex) {
+    return (ComplexInvocationHandler) Proxy.getInvocationHandler(complex);
+  }
+
+  protected EntityCollectionInvocationHandler<?> getInvocationHandler(
+      final EntityCollection<?, ?, ?> complex) {
+    return (EntityCollectionInvocationHandler<?>) Proxy.getInvocationHandler(complex);
+  }
+}