http://ofbiz.116.s1.nabble.com/Re-svn-commit-r1476296-in-ofbiz-trunk-framework-entity-src-org-ofbiz-entity-Delegator-java-GenericDea-tp4641196p4641198.html
> Adrian,
>
> In javadoc of numbers of methods of Delegator.java you added this sentence
> - * boolean that specifies whether to clear related cache entries
> - * for this primaryKey to be created
> + * boolean that specifies whether or not to automatically clear
> + * cache entries related to this operation. This should always be
> + * <code>true</code> - otherwise you will lose data integrity.Now I wonder if we should no more and remove all those . What's the point of letting users not doing a cache clear there?ThanksJacquesFrom: <
[hidden email]>
>> Author: adrianc
>> Date: Fri Apr 26 17:04:49 2013
>> New Revision: 1476296
>>
>> URL:
http://svn.apache.org/r1476296>> Log:
>> Some bug fixes for entity engine caches. GenericDelegator was inconsistent in clearing caches - some methods cleared the cache before the entity operation (wrong) while others cleared the cache after the entity operation (right). Also, I updated the Delegator JavaDocs to warn against skipping the cache clearing step - which shouldn't be done. Finally, the AbstractEntityConditionCache.storeHook method doesn't work (for a number of reasons) so I bypassed it.
>>
>> Modified:
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/AbstractEntityConditionCache.java
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/Cache.java
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/eca/EntityEcaHandler.java
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/test/EntityTestSuite.java
>>
>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java
>> URL:
http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java?rev=1476296&r1=1476295&r2=1476296&view=diff>> ==============================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java Fri Apr 26 17:04:49 2013
>> @@ -159,8 +159,9 @@ public interface Delegator {
>> * @param primaryKey
>> * The GenericPK to create a value in the datasource from
>> * @param doCacheClear
>> - * boolean that specifies whether to clear related cache entries
>> - * for this primaryKey to be created
>> + * boolean that specifies whether or not to automatically clear
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return GenericValue instance containing the new instance
>> */
>> public GenericValue create(GenericPK primaryKey, boolean doCacheClear) throws GenericEntityException;
>> @@ -183,7 +184,8 @@ public interface Delegator {
>> * The GenericValue to create a value in the datasource from
>> * @param doCacheClear
>> * boolean that specifies whether or not to automatically clear
>> - * cache entries related to this operation
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return GenericValue instance containing the new instance
>> */
>> public GenericValue create(GenericValue value, boolean doCacheClear) throws GenericEntityException;
>> @@ -222,7 +224,8 @@ public interface Delegator {
>> * instance
>> * @param doCacheClear
>> * boolean that specifies whether or not to automatically clear
>> - * cache entries related to this operation
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return GenericValue instance containing the new or updated instance
>> */
>> public GenericValue createOrStore(GenericValue value, boolean doCacheClear) throws GenericEntityException;
>> @@ -950,7 +953,8 @@ public interface Delegator {
>> * GenericValue instance containing the entity to refresh
>> * @param doCacheClear
>> * boolean that specifies whether or not to automatically clear
>> - * cache entries related to this operation
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> */
>> public void refresh(GenericValue value, boolean doCacheClear) throws GenericEntityException;
>>
>> @@ -999,7 +1003,8 @@ public interface Delegator {
>> * or by and fields to remove
>> * @param doCacheClear
>> * boolean that specifies whether or not to automatically clear
>> - * cache entries related to this operation
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> */
>> public int removeAll(List<? extends GenericEntity> dummyPKs, boolean doCacheClear) throws GenericEntityException;
>> @@ -1013,8 +1018,9 @@ public interface Delegator {
>> * @param entityName
>> * The Name of the Entity as defined in the entity XML file
>> * @param doCacheClear
>> - * boolean that specifies whether to clear cache entries for this
>> - * value to be removed
>> + * boolean that specifies whether or not to automatically clear
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @param fields
>> * The fields of the named entity to query by with their
>> * corresponding values
>> @@ -1045,8 +1051,9 @@ public interface Delegator {
>> * The fields of the named entity to query by with their
>> * corresponding values
>> * @param doCacheClear
>> - * boolean that specifies whether to clear cache entries for this
>> - * value to be removed
>> + * boolean that specifies whether or not to automatically clear
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> */
>> public int removeByAnd(String entityName, Map<String, ? extends Object> fields, boolean doCacheClear) throws GenericEntityException;
>> @@ -1083,8 +1090,9 @@ public interface Delegator {
>> * @param condition
>> * The condition used to restrict the removing
>> * @param doCacheClear
>> - * boolean that specifies whether to clear cache entries for this
>> - * value to be removed
>> + * boolean that specifies whether or not to automatically clear
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> */
>> public int removeByCondition(String entityName, EntityCondition condition, boolean doCacheClear) throws GenericEntityException;
>> @@ -1104,8 +1112,9 @@ public interface Delegator {
>> * @param primaryKey
>> * The primary key of the entity to remove.
>> * @param doCacheClear
>> - * boolean that specifies whether to clear cache entries for this
>> - * primaryKey to be removed
>> + * boolean that specifies whether or not to automatically clear
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> */
>> public int removeByPrimaryKey(GenericPK primaryKey, boolean doCacheClear) throws GenericEntityException;
>> @@ -1135,8 +1144,9 @@ public interface Delegator {
>> * @param value
>> * GenericValue instance containing the entity
>> * @param doCacheClear
>> - * boolean that specifies whether to clear cache entries for this
>> - * value to be removed
>> + * boolean that specifies whether or not to automatically clear
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> */
>> public int removeRelated(String relationName, GenericValue value, boolean doCacheClear) throws GenericEntityException;
>> @@ -1156,8 +1166,9 @@ public interface Delegator {
>> * @param value
>> * The GenericValue object of the entity to remove.
>> * @param doCacheClear
>> - * boolean that specifies whether to clear cache entries for this
>> - * value to be removed
>> + * boolean that specifies whether or not to automatically clear
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> */
>> public int removeValue(GenericValue value, boolean doCacheClear) throws GenericEntityException;
>> @@ -1199,7 +1210,8 @@ public interface Delegator {
>> * GenericValue instance containing the entity
>> * @param doCacheClear
>> * boolean that specifies whether or not to automatically clear
>> - * cache entries related to this operation
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> */
>> public int store(GenericValue value, boolean doCacheClear) throws GenericEntityException;
>> @@ -1236,7 +1248,8 @@ public interface Delegator {
>> * store
>> * @param doCacheClear
>> * boolean that specifies whether or not to automatically clear
>> - * cache entries related to this operation
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> */
>> public int storeAll(List<GenericValue> values, boolean doCacheClear) throws GenericEntityException;
>> @@ -1256,7 +1269,8 @@ public interface Delegator {
>> * store
>> * @param doCacheClear
>> * boolean that specifies whether or not to automatically clear
>> - * cache entries related to this operation
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @param createDummyFks
>> * boolean that specifies whether or not to automatically create
>> * "dummy" place holder FKs
>> @@ -1288,8 +1302,9 @@ public interface Delegator {
>> * @param condition
>> * The condition that restricts the list of stored values
>> * @param doCacheClear
>> - * boolean that specifies whether to clear cache entries for
>> - * these values
>> + * boolean that specifies whether or not to automatically clear
>> + * cache entries related to this operation. This should always be
>> + * <code>true</code> - otherwise you will lose data integrity.
>> * @return int representing number of rows effected by this operation
>> * @throws GenericEntityException
>> */
>>
>> 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=1476296&r1=1476295&r2=1476296&view=diff>> ==============================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Fri Apr 26 17:04:49 2013
>> @@ -995,12 +995,6 @@ public class GenericDelegator implements
>>
>> GenericHelper helper = getEntityHelper(primaryKey.getEntityName());
>>
>> - if (doCacheClear) {
>> - // always clear cache before the operation
>> - ecaRunner.evalRules(EntityEcaHandler.EV_CACHE_CLEAR, EntityEcaHandler.OP_REMOVE, primaryKey, false);
>> - this.clearCacheLine(primaryKey);
>> - }
>> -
>> ecaRunner.evalRules(EntityEcaHandler.EV_RUN, EntityEcaHandler.OP_REMOVE, primaryKey, false);
>>
>> // if audit log on for any fields, save old value before removing so it's still there
>> @@ -1013,6 +1007,11 @@ public class GenericDelegator implements
>> removedEntity = this.findOne(primaryKey.getEntityName(), primaryKey, false);
>> }
>> int num = helper.removeByPrimaryKey(primaryKey);
>> + if (doCacheClear) {
>> + ecaRunner.evalRules(EntityEcaHandler.EV_CACHE_CLEAR, EntityEcaHandler.OP_REMOVE, primaryKey, false);
>> + this.clearCacheLine(primaryKey);
>> + }
>> +
>> this.saveEntitySyncRemoveInfo(primaryKey);
>>
>> if (testMode) {
>> @@ -1064,11 +1063,6 @@ public class GenericDelegator implements
>>
>> GenericHelper helper = getEntityHelper(value.getEntityName());
>>
>> - if (doCacheClear) {
>> - ecaRunner.evalRules(EntityEcaHandler.EV_CACHE_CLEAR, EntityEcaHandler.OP_REMOVE, value, false);
>> - this.clearCacheLine(value);
>> - }
>> -
>> ecaRunner.evalRules(EntityEcaHandler.EV_RUN, EntityEcaHandler.OP_REMOVE, value, false);
>>
>> // if audit log on for any fields, save old value before actual remove
>> @@ -1084,6 +1078,11 @@ 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 (doCacheClear) {
>> + ecaRunner.evalRules(EntityEcaHandler.EV_CACHE_CLEAR, EntityEcaHandler.OP_REMOVE, value, false);
>> + this.clearCacheLine(value);
>> + }
>> +
>>
>> if (testMode) {
>> if (removedValue != null) {
>> @@ -1329,12 +1328,6 @@ public class GenericDelegator implements
>> ecaRunner.evalRules(EntityEcaHandler.EV_VALIDATE, EntityEcaHandler.OP_STORE, value, false);
>> GenericHelper helper = getEntityHelper(value.getEntityName());
>>
>> - if (doCacheClear) {
>> - // always clear cache before the operation
>> - ecaRunner.evalRules(EntityEcaHandler.EV_CACHE_CLEAR, EntityEcaHandler.OP_STORE, value, false);
>> - this.clearCacheLine(value);
>> - }
>> -
>> ecaRunner.evalRules(EntityEcaHandler.EV_RUN, EntityEcaHandler.OP_STORE, value, false);
>> this.encryptFields(value);
>>
>> @@ -1350,6 +1343,10 @@ public class GenericDelegator implements
>> }
>>
>> int retVal = helper.store(value);
>> + if (doCacheClear) {
>> + ecaRunner.evalRules(EntityEcaHandler.EV_CACHE_CLEAR, EntityEcaHandler.OP_STORE, value, false);
>> + this.clearCacheLine(value);
>> + }
>>
>> if (testMode) {
>> storeForTestRollback(new TestOperation(OperationType.UPDATE, updatedEntity));
>> @@ -2192,11 +2189,6 @@ public class GenericDelegator implements
>> * @see org.ofbiz.entity.Delegator#clearCacheLine(org.ofbiz.entity.GenericValue, boolean)
>> */
>> public void clearCacheLine(GenericValue value, boolean distribute) {
>> - // TODO: make this a bit more intelligent by passing in the operation being done (create, update, remove) so we can not do unnecessary cache clears...
>> - // for instance:
>> - // on create don't clear by primary cache (and won't clear original values because there won't be any)
>> - // on remove don't clear by and for new values, but do for original values
>> -
>> // Debug.logInfo("running clearCacheLine for value: " + value + ", distribute: " + distribute, module);
>> if (value == null) {
>> return;
>>
>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/AbstractEntityConditionCache.java
>> URL:
http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/AbstractEntityConditionCache.java?rev=1476296&r1=1476295&r2=1476296&view=diff>> ==============================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/AbstractEntityConditionCache.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/AbstractEntityConditionCache.java Fri Apr 26 17:04:49 2013
>> @@ -59,6 +59,21 @@ public abstract class AbstractEntityCond
>> return conditionCache.put(key, value);
>> }
>>
>> + /**
>> + * Removes all condition caches that include the specified entity.
>> + */
>> + public void remove(GenericEntity entity) {
>> + UtilCache.clearCache(getCacheName(entity.getEntityName()));
>> + ModelEntity model = entity.getModelEntity();
>> + if (model != null) {
>> + Iterator<String> it = model.getViewConvertorsIterator();
>> + while (it.hasNext()) {
>> + String targetEntityName = it.next();
>> + UtilCache.clearCache(getCacheName(targetEntityName));
>> + }
>> + }
>> + }
>> +
>> public void remove(String entityName, EntityCondition condition) {
>> UtilCache<EntityCondition, ConcurrentMap<K, V>> cache = getCache(entityName);
>> if (cache == null) return;
>>
>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/Cache.java
>> URL:
http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/Cache.java?rev=1476296&r1=1476295&r2=1476296&view=diff>> ==============================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/Cache.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/cache/Cache.java Fri Apr 26 17:04:49 2013
>> @@ -112,16 +112,22 @@ public class Cache {
>> public GenericValue remove(GenericEntity entity) {
>> if (Debug.verboseOn()) Debug.logVerbose("Cache remove GenericEntity: " + entity, module);
>> GenericValue oldEntity = entityCache.remove(entity.getPrimaryKey());
>> - entityListCache.storeHook(entity, null);
>> - entityObjectCache.storeHook(entity, null);
>> + // Workaround because AbstractEntityConditionCache.storeHook doesn't work.
>> + entityListCache.remove(entity);
>> + entityObjectCache.remove(entity);
>> + // entityListCache.storeHook(entity, null);
>> + // entityObjectCache.storeHook(entity, null);
>> return oldEntity;
>> }
>>
>> public GenericValue remove(GenericPK pk) {
>> if (Debug.verboseOn()) Debug.logVerbose("Cache remove GenericPK: " + pk, module);
>> GenericValue oldEntity = entityCache.remove(pk);
>> - entityListCache.storeHook(pk, null);
>> - entityObjectCache.storeHook(pk, null);
>> + // Workaround because AbstractEntityConditionCache.storeHook doesn't work.
>> + entityListCache.remove(pk);
>> + entityObjectCache.remove(pk);
>> + // entityListCache.storeHook(pk, null);
>> + // entityObjectCache.storeHook(pk, null);
>> return oldEntity;
>> }
>> }
>>
>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/eca/EntityEcaHandler.java
>> URL:
http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/eca/EntityEcaHandler.java?rev=1476296&r1=1476295&r2=1476296&view=diff>> ==============================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/eca/EntityEcaHandler.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/eca/EntityEcaHandler.java Fri Apr 26 17:04:49 2013
>> @@ -34,6 +34,9 @@ public interface EntityEcaHandler<T> {
>> public static final String EV_VALIDATE = "validate";
>> public static final String EV_RUN = "run";
>> public static final String EV_RETURN = "return";
>> + /**
>> + * Invoked after the entity operation, but before the cache is cleared.
>> + */
>> public static final String EV_CACHE_CLEAR = "cache-clear";
>> public static final String EV_CACHE_CHECK = "cache-check";
>> public static final String EV_CACHE_PUT = "cache-put";
>>
>> 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=1476296&r1=1476295&r2=1476296&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 Fri Apr 26 17:04:49 2013
>> @@ -107,7 +107,7 @@ public class EntityTestSuite extends Ent
>> observer.arg = null;
>> GenericValue clonedValue = (GenericValue) testValue.clone();
>> clonedValue.put("description", "New Testing Type #1");
>> - assertTrue("Observable has changed", testValue.hasChanged());
>> + assertTrue("Cloned Observable has changed", clonedValue.hasChanged());
>> assertEquals("Observer called with cloned GenericValue field name", "description", observer.arg);
>> // now store it
>> testValue.store();
>> @@ -142,12 +142,11 @@ public class EntityTestSuite extends Ent
>> */
>> public void testEntityCache() throws Exception {
>> // 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"));
>> + GenericValue testValue = delegator.findOne("TestingType", true, "testingTypeId", "TEST-3");
>> + assertEquals("Retrieved from cache value has the correct description", "Testing Type #3", testValue.getString("description"));
>> // Test immutable
>> try {
>> - testValue.put("description", "New Testing Type #2");
>> - testValue.store();
>> + testValue.put("description", "New Testing Type #3");
>> fail("Modified an immutable GenericValue");
>> } catch (IllegalStateException e) {
>> }
>> @@ -156,6 +155,17 @@ public class EntityTestSuite extends Ent
>> fail("Modified an immutable GenericValue");
>> } catch (UnsupportedOperationException e) {
>> }
>> + // Test entity value update operation updates the cache
>> + testValue = (GenericValue) testValue.clone();
>> + testValue.put("description", "New Testing Type #3");
>> + testValue.store();
>> + testValue = delegator.findOne("TestingType", true, "testingTypeId", "TEST-3");
>> + assertEquals("Retrieved from cache value has the correct description", "New Testing Type #3", testValue.getString("description"));
>> + // Test entity value remove operation updates the cache
>> + testValue = (GenericValue) testValue.clone();
>> + testValue.remove();
>> + testValue = delegator.findOne("TestingType", true, "testingTypeId", "TEST-3");
>> + assertEquals("Retrieved from cache value is null", null, testValue);
>> // Test entity condition cache
>> EntityCondition testCondition = EntityCondition.makeCondition("description", EntityOperator.EQUALS, "Testing Type #2");
>> List<GenericValue> testList = delegator.findList("TestingType", testCondition, null, null, null, true);
>> @@ -165,7 +175,6 @@ public class EntityTestSuite extends Ent
>> // Test immutable
>> try {
>> testValue.put("description", "New Testing Type #2");
>> - testValue.store();
>> fail("Modified an immutable GenericValue");
>> } catch (IllegalStateException e) {
>> }
>> @@ -174,13 +183,24 @@ public class EntityTestSuite extends Ent
>> fail("Modified an immutable GenericValue");
>> } catch (UnsupportedOperationException e) {
>> }
>> - /* Commenting this out for now because the tests fail due to flaws in the EntityListCache implementation.
>> + // Test entity value create operation updates the cache
>> testValue = (GenericValue) testValue.clone();
>> + testValue.put("testingTypeId", "TEST-9");
>> + testValue.create();
>> + testList = delegator.findList("TestingType", testCondition, null, null, null, true);
>> + assertEquals("Delegator findList returned two values", 2, testList.size());
>> + // Test entity value update operation updates the cache
>> testValue.put("description", "New Testing Type #2");
>> testValue.store();
>> testList = delegator.findList("TestingType", testCondition, null, null, null, true);
>> + assertEquals("Delegator findList returned one value", 1, testList.size());
>> + // Test entity value remove operation updates the cache
>> + testValue = testList.get(0);
>> + testValue = (GenericValue) testValue.clone();
>> + testValue.remove();
>> + testList = delegator.findList("TestingType", testCondition, null, null, null, true);
>> assertEquals("Delegator findList returned empty list", 0, testList.size());
>> - */
>> + // TODO: Test view entities.
>> }
>>
>> /*
>>
>>