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: <adri...@apache.org> > 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. > } > > /* > >
