Author: adrianc Date: Mon Apr 22 20:46:36 2013 New Revision: 1470712 URL: http://svn.apache.org/r1470712 Log: Make sure an immutable GenericEntity really is immutable.
Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java ofbiz/trunk/framework/entity/src/org/ofbiz/entity/test/EntityTestSuite.java 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=1470712&r1=1470711&r2=1470712&view=diff ============================================================================== --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original) +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Mon Apr 22 20:46:36 2013 @@ -144,8 +144,15 @@ public class GenericEntity implements Ma return newEntity; } + protected void assertIsMutable() { + if (!this.mutable) { + throw new IllegalStateException("This object has been flagged as immutable (unchangeable), probably because it came from an Entity Engine cache. Cannot modify an immutable entity object."); + } + } + /** Creates new GenericEntity */ protected void init(ModelEntity modelEntity) { + assertIsMutable(); if (modelEntity == null) { throw new IllegalArgumentException("Cannot create a GenericEntity with a null modelEntity parameter"); } @@ -160,6 +167,7 @@ public class GenericEntity implements Ma /** Creates new GenericEntity from existing Map */ protected void init(Delegator delegator, ModelEntity modelEntity, Map<String, ? extends Object> fields) { + assertIsMutable(); if (modelEntity == null) { throw new IllegalArgumentException("Cannot create a GenericEntity with a null modelEntity parameter"); } @@ -177,6 +185,7 @@ public class GenericEntity implements Ma /** Creates new GenericEntity from existing Map */ protected void init(Delegator delegator, ModelEntity modelEntity, Object singlePkValue) { + assertIsMutable(); if (modelEntity == null) { throw new IllegalArgumentException("Cannot create a GenericEntity with a null modelEntity parameter"); } @@ -197,6 +206,7 @@ public class GenericEntity implements Ma /** Copy Constructor: Creates new GenericEntity from existing GenericEntity */ protected void init(GenericEntity value) { + assertIsMutable(); // check some things if (value.entityName == null) { throw new IllegalArgumentException("Cannot create a GenericEntity with a null entityName in the modelEntity parameter"); @@ -211,6 +221,7 @@ public class GenericEntity implements Ma } public void reset() { + assertIsMutable(); // from GenericEntity this.delegatorName = null; this.internalDelegator = null; @@ -225,6 +236,7 @@ public class GenericEntity implements Ma } public void refreshFromValue(GenericEntity newValue) throws GenericEntityException { + assertIsMutable(); if (newValue == null) { throw new GenericEntityException("Could not refresh value, new value not found for: " + this); } @@ -233,8 +245,7 @@ public class GenericEntity implements Ma if (!thisPK.equals(newPK)) { throw new GenericEntityException("Could not refresh value, new value did not have the same primary key; this PK=" + thisPK + ", new value PK=" + newPK); } - // FIXME: This is dangerous - two instances sharing a common field Map is a bad idea. - this.fields = newValue.fields; + this.fields = new HashMap<String, Object>(newValue.fields); this.setDelegator(newValue.getDelegator()); this.generateHashCode = newValue.generateHashCode; this.cachedHashCode = newValue.cachedHashCode; @@ -247,10 +258,12 @@ public class GenericEntity implements Ma } public void synchronizedWithDatasource() { + assertIsMutable(); this.modified = false; } public void removedFromDatasource() { + assertIsMutable(); // seems kind of minimal, but should do for now... this.modified = true; } @@ -260,7 +273,10 @@ public class GenericEntity implements Ma } public void setImmutable() { - this.mutable = false; + if (this.mutable) { + this.mutable = false; + this.fields = Collections.unmodifiableMap(this.fields); + } } /** @@ -274,6 +290,7 @@ public class GenericEntity implements Ma * @param isFromEntitySync The isFromEntitySync to set. */ public void setIsFromEntitySync(boolean isFromEntitySync) { + assertIsMutable(); this.isFromEntitySync = isFromEntitySync; } @@ -308,6 +325,7 @@ public class GenericEntity implements Ma /** Set the GenericDelegator instance that created this value object and that is responsible for it. */ public void setDelegator(Delegator internalDelegator) { + assertIsMutable(); if (internalDelegator == null) return; this.delegatorName = internalDelegator.getDelegatorName(); this.internalDelegator = internalDelegator; @@ -386,11 +404,7 @@ public class GenericEntity implements Ma * @param setIfNull Specifies whether or not to set the value if it is null */ public synchronized Object set(String name, Object value, boolean setIfNull) { - if (!this.mutable) { - // comment this out to disable the mutable check - throw new IllegalStateException("This object has been flagged as immutable (unchangeable), probably because it came from an Entity Engine cache. Cannot set a value in an immutable entity object."); - } - + assertIsMutable(); ModelField modelField = getModelEntity().getField(name); if (modelField == null) { throw new IllegalArgumentException("[GenericEntity.set] \"" + name + "\" is not a field of " + entityName + ", must be one of: " + getModelEntity().fieldNameString()); @@ -444,12 +458,16 @@ public class GenericEntity implements Ma } public void dangerousSetNoCheckButFast(ModelField modelField, Object value) { + assertIsMutable(); if (modelField == null) throw new IllegalArgumentException("Cannot set field with a null modelField"); generateHashCode = true; this.fields.put(modelField.getName(), value); + this.setChanged(); + this.notifyObservers(modelField.getName()); } public Object dangerousGetNoCheckButFast(ModelField modelField) { + assertIsMutable(); if (modelField == null) throw new IllegalArgumentException("Cannot get field with a null modelField"); return this.fields.get(modelField.getName()); } 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=1470712&r1=1470711&r2=1470712&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 Mon Apr 22 20:46:36 2013 @@ -122,12 +122,18 @@ public class EntityTestSuite extends Ent // Test primary key cache GenericValue testValue = delegator.findOne("TestingType", true, "testingTypeId", "TEST-2"); assertEquals("Retrieved from cache value has the correct description", "Testing Type #2", testValue.getString("description")); + // Test immutable try { testValue.put("description", "New Testing Type #2"); testValue.store(); fail("Modified an immutable GenericValue"); } catch (IllegalStateException e) { } + try { + testValue.remove("description"); + fail("Modified an immutable GenericValue"); + } catch (UnsupportedOperationException e) { + } // Test entity condition cache /* Commenting this out for now because the tests fail due to flaws in the EntityListCache implementation. EntityCondition testCondition = EntityCondition.makeCondition("description", EntityOperator.EQUALS, "Testing Type #2");