http://ofbiz.116.s1.nabble.com/Re-svn-commit-r1476296-in-ofbiz-trunk-framework-entity-src-org-ofbiz-entity-Delegator-java-GenericDea-tp4641196.html
- * boolean that specifies whether to clear related cache entries
+ * 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: <
> 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.
> }
>
> /*
>
>