You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by ad...@apache.org on 2013/04/24 23:23:12 UTC

svn commit: r1471687 - in /ofbiz/trunk/framework/entity/src/org/ofbiz/entity: GenericDelegator.java GenericEntity.java GenericValue.java test/EntityTestSuite.java

Author: adrianc
Date: Wed Apr 24 21:23:12 2013
New Revision: 1471687

URL: http://svn.apache.org/r1471687
Log:
More work on GenericEntity and GenericValue immutability.

Modified:
    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java
    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericValue.java
    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/test/EntityTestSuite.java

Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java?rev=1471687&r1=1471686&r2=1471687&view=diff
==============================================================================
--- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original)
+++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Wed Apr 24 21:23:12 2013
@@ -1010,7 +1010,7 @@ public class GenericDelegator implements
 
             GenericValue removedEntity = null;
             if (testMode) {
-                removedEntity = this.findOne(primaryKey.entityName, primaryKey, false);
+                removedEntity = this.findOne(primaryKey.getEntityName(), primaryKey, false);
             }
             int num = helper.removeByPrimaryKey(primaryKey);
             this.saveEntitySyncRemoveInfo(primaryKey);
@@ -1082,6 +1082,8 @@ public class GenericDelegator implements
             }
 
             int num = helper.removeByPrimaryKey(value.getPrimaryKey());
+            // Need to call removedFromDatasource() here because the helper calls removedFromDatasource() on the PK instead of the GenericEntity.
+            value.removedFromDatasource();
 
             if (testMode) {
                 if (removedValue != null) {
@@ -1344,11 +1346,10 @@ public class GenericDelegator implements
             GenericValue updatedEntity = null;
 
             if (testMode) {
-                updatedEntity = this.findOne(value.entityName, value.getPrimaryKey(), false);
+                updatedEntity = this.findOne(value.getEntityName(), value.getPrimaryKey(), false);
             }
 
             int retVal = helper.store(value);
-            value.clearChanged();
 
             if (testMode) {
                 storeForTestRollback(new TestOperation(OperationType.UPDATE, updatedEntity));

Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1471687&r1=1471686&r2=1471687&view=diff
==============================================================================
--- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original)
+++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Wed Apr 24 21:23:12 2013
@@ -80,10 +80,10 @@ public class GenericEntity implements Ma
     private transient Observable observable = new Observable();
 
     /** Name of the GenericDelegator, used to re-get the GenericDelegator when deserialized */
-    protected String delegatorName = null;
+    private String delegatorName = null;
 
     /** Reference to an instance of GenericDelegator used to do some basic operations on this entity value. If null various methods in this class will fail. This is automatically set by the GenericDelegator for all GenericValue objects instantiated through it. You may set this manually for objects you instantiate manually, but it is optional. */
-    protected transient Delegator internalDelegator = null;
+    private transient Delegator internalDelegator = null;
 
     /** Contains the fields for this entity. Note that this should always be a
      *  HashMap to allow for two things: non-synchronized reads (synchronized
@@ -92,24 +92,22 @@ public class GenericEntity implements Ma
      *  between desiring to set a value to null and desiring to not modify the
      *  current value on an update.
      */
-    protected Map<String, Object> fields = new HashMap<String, Object>();
+    private Map<String, Object> fields = new HashMap<String, Object>();
 
     /** Contains the entityName of this entity, necessary for efficiency when creating EJBs */
-    protected String entityName = null;
+    private String entityName = null;
 
     /** Contains the ModelEntity instance that represents the definition of this entity, not to be serialized */
-    protected transient ModelEntity modelEntity = null;
+    private transient ModelEntity modelEntity = null;
 
-    /** Denotes whether or not this entity has been modified, or is known to be out of sync with the persistent record */
-    protected boolean modified = false;
-    protected boolean generateHashCode = true;
-    protected int cachedHashCode = 0;
+    private boolean generateHashCode = true;
+    private int cachedHashCode = 0;
 
     /** Used to specify whether or not this representation of the entity can be changed; generally cleared when this object comes from a cache */
-    protected boolean mutable = true;
+    private boolean mutable = true;
 
     /** This is an internal field used to specify that a value has come from a sync process and that the auto-stamps should not be over-written */
-    protected boolean isFromEntitySync = false;
+    private boolean isFromEntitySync = false;
 
     /** Creates new GenericEntity - Should never be used, prefer the other options. */
     protected GenericEntity() { }
@@ -242,7 +240,6 @@ public class GenericEntity implements Ma
         this.fields = new HashMap<String, Object>();
         this.entityName = null;
         this.modelEntity = null;
-        this.modified = false;
         this.generateHashCode = true;
         this.cachedHashCode = 0;
         this.mutable = true;
@@ -264,23 +261,37 @@ public class GenericEntity implements Ma
         this.setDelegator(newValue.getDelegator());
         this.generateHashCode = newValue.generateHashCode;
         this.cachedHashCode = newValue.cachedHashCode;
-        this.modified = false;
         this.observable = new Observable(newValue.observable);
     }
 
+    /**
+     * 
+     * @deprecated Use hasChanged()
+     */
     public boolean isModified() {
-        return this.modified;
+        return this.hasChanged();
     }
 
+    /**
+     * Flags this object as being synchronized with the data source.
+     * The entity engine will call this method immediately after
+     * populating this object with data from the data source.
+     */
     public void synchronizedWithDatasource() {
         assertIsMutable();
-        this.modified = false;
+        this.clearChanged();
     }
 
+    /**
+     * Flags this object as being removed from the data source.
+     * The entity engine will call this method immediately after
+     * removing this value from the data source. Once this method is
+     * called, the object is immutable.
+     */
     public void removedFromDatasource() {
         assertIsMutable();
-        // seems kind of minimal, but should do for now...
-        this.modified = true;
+        this.hasChanged();
+        this.setImmutable();
     }
 
     public boolean isMutable() {
@@ -463,7 +474,6 @@ public class GenericEntity implements Ma
             Object old = fields.put(name, value);
 
             generateHashCode = true;
-            modified = true;
             this.setChanged();
             this.notifyObservers(name);
             return old;
@@ -996,9 +1006,7 @@ public class GenericEntity implements Ma
      * @return java.util.Map
      */
     public Map<String, Object> getAllFields() {
-        Map<String, Object> newMap = new HashMap<String, Object>();
-        newMap.putAll(this.fields);
-        return newMap;
+        return new HashMap<String, Object>(this.fields);
     }
 
     /** Used by clients to specify exactly the fields they are interested in
@@ -1518,7 +1526,9 @@ public class GenericEntity implements Ma
     }
 
     public static class NullGenericEntity extends GenericEntity implements NULL {
-        protected NullGenericEntity() { }
+        protected NullGenericEntity() {
+            this.setImmutable();
+        }
 
         @Override
         public String getEntityName() {

Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericValue.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericValue.java?rev=1471687&r1=1471686&r2=1471687&view=diff
==============================================================================
--- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericValue.java (original)
+++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericValue.java Wed Apr 24 21:23:12 2013
@@ -24,6 +24,7 @@ import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Collections;
 
 import org.ofbiz.base.util.Debug;
 import org.ofbiz.base.util.UtilMisc;
@@ -53,11 +54,9 @@ public class GenericValue extends Generi
     // FIXME: This is a bad idea. Another process could change the related values after they are added to the Map.
     public transient Map<String, GenericValue> relatedOneCache = null;
 
-    /** This Map will contain the original field values from the database iff
-     * this GenericValue came from the database. If it was made manually it will
-     * no have this Map, ie it will be null to not take up memory.
+    /** A Map containing the original field values from the database.
      */
-    protected Map<String, Object> originalDbValues = null;
+    private Map<String, Object> originalDbValues = null;
 
     /** Creates new GenericValue */
     public static GenericValue create(ModelEntity modelEntity) {
@@ -108,7 +107,7 @@ public class GenericValue extends Generi
     @Override
     public void synchronizedWithDatasource() {
         super.synchronizedWithDatasource();
-        this.copyOriginalDbValues();
+        this.originalDbValues = Collections.unmodifiableMap(getAllFields());
     }
 
     public GenericValue create() throws GenericEntityException {
@@ -137,22 +136,12 @@ public class GenericValue extends Generi
 
     public Object getOriginalDbValue(String name) {
         if (getModelEntity().getField(name) == null) {
-            throw new IllegalArgumentException("[GenericEntity.get] \"" + name + "\" is not a field of " + entityName);
+            throw new IllegalArgumentException("[GenericEntity.get] \"" + name + "\" is not a field of " + getEntityName());
         }
         if (originalDbValues == null) return null;
         return originalDbValues.get(name);
     }
 
-    /** This should only be called by the Entity Engine once a GenericValue has
-     * been read from the database so that we have a copy of the original field
-     * values from the Db.
-     */
-    public void copyOriginalDbValues() {
-        // FIXME: There is no guarantee this.fields was not modified.
-        this.originalDbValues = new HashMap<String, Object>();
-        this.originalDbValues.putAll(this.fields);
-    }
-
     /** Get the named Related Entity for the GenericValue from the persistent store
      *@param relationName String containing the relation name which is the combination of relation.title and relation.rel-entity-name as specified in the entity XML definition file
      *@return List of GenericValue instances as specified in the relation definition

Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/test/EntityTestSuite.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/test/EntityTestSuite.java?rev=1471687&r1=1471686&r2=1471687&view=diff
==============================================================================
--- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/test/EntityTestSuite.java (original)
+++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/test/EntityTestSuite.java Wed Apr 24 21:23:12 2013
@@ -98,6 +98,7 @@ public class EntityTestSuite extends Ent
         GenericValue testValue = delegator.findOne("TestingType", false, "testingTypeId", "TEST-1");
         assertEquals("Retrieved value has the correct description", "Testing Type #1", testValue.getString("description"));
         // Test Observable aspect
+        assertFalse("Observable has not changed", testValue.hasChanged());
         TestObserver observer = new TestObserver();
         testValue.addObserver(observer);
         testValue.put("description", "New Testing Type #1");
@@ -111,12 +112,31 @@ public class EntityTestSuite extends Ent
         // now store it
         testValue.store();
         assertFalse("Observable has not changed", testValue.hasChanged());
-
         // now retrieve it again and make sure that the updated value is correct
         testValue = delegator.findOne("TestingType", false, "testingTypeId", "TEST-1");
         assertEquals("Retrieved value has the correct description", "New Testing Type #1", testValue.getString("description"));
     }
 
+    public void testRemoveValue() throws Exception {
+        // Retrieve a sample GenericValue, make sure it's correct
+        GenericValue testValue = delegator.findOne("TestingType", false, "testingTypeId", "TEST-4");
+        assertEquals("Retrieved value has the correct description", "Testing Type #4", testValue.getString("description"));
+        testValue.remove();
+        // Test immutable
+        try {
+            testValue.put("description", "New Testing Type #4");
+            fail("Modified an immutable GenericValue");
+        } catch (IllegalStateException e) {
+        }
+        try {
+            testValue.remove("description");
+            fail("Modified an immutable GenericValue");
+        } catch (UnsupportedOperationException e) {
+        }
+        testValue = delegator.findOne("TestingType", false, "testingTypeId", "TEST-4");
+        assertEquals("Finding removed value returns null", null, testValue);
+    }
+
     /*
      * Tests the entity cache
      */