Login  Register

Re: svn commit: r1527626 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options Options
Embed post
Permalink
Reply | Threaded
Open this post in threaded view
| More
Print post
Permalink

Re: svn commit: r1527626 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java

Adrian Crum-3
1903 posts
I just noticed another problem in the GenericDelegator.findOne method.
The try-catch-finally logic is flawed. The
TransactionUtil.commit(beganTransaction) statement belongs in the try
block, and the catch clause should catch ALL exceptions.

The way things are structured now, if an exception is thrown in the try
block, the transaction will be committed. It should only commit the
transaction if the code executes successfully all the way to the end of
the try block.

Any thoughts?

Adrian Crum
Sandglass Software
www.sandglass-software.com

On 9/30/2013 9:13 AM, [hidden email] wrote:

> Author: adrianc
> Date: Mon Sep 30 16:13:03 2013
> New Revision: 1527626
>
> URL: http://svn.apache.org/r1527626
> Log:
> Fixed a subtle flaw in the GenericDelegator.findOne method. When a database query returns no result, GenericValue.NULL_VALUE is put in the pk cache - so future findOne calls will know the entity value doesn't exist. But the findOne method never checked for GenericValue.NULL_VALUE in cache gets, so the database was queried again for an entity value we already know doesn't exist.
>
> https://issues.apache.org/jira/browse/OFBIZ-5332
>
> Modified:
>      ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.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=1527626&r1=1527625&r2=1527626&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original)
> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Mon Sep 30 16:13:03 2013
> @@ -1580,8 +1580,10 @@ public class GenericDelegator implements
>           EntityEcaRuleRunner<?> ecaRunner = this.getEcaRuleRunner(entityName);
>           if (useCache) {
>               ecaRunner.evalRules(EntityEcaHandler.EV_CACHE_CHECK, EntityEcaHandler.OP_FIND, primaryKey, false);
> -
> -            GenericValue value = this.getFromPrimaryKeyCache(primaryKey);
> +            GenericValue value = cache.get(primaryKey);
> +            if (value == GenericValue.NULL_VALUE) {
> +                return null;
> +            }
>               if (value != null) {
>                   return value;
>               }
>
>
Reply | Threaded
Open this post in threaded view
| More
Print post
Permalink

Re: svn commit: r1527626 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java

Adrian Crum-3
1903 posts
I ran a test and confirmed this is a problem. The transaction is
committed after the exception is thrown. I will commit a fix soon.

Adrian Crum
Sandglass Software
www.sandglass-software.com

On 9/30/2013 9:36 AM, Adrian Crum wrote:

> I just noticed another problem in the GenericDelegator.findOne method.
> The try-catch-finally logic is flawed. The
> TransactionUtil.commit(beganTransaction) statement belongs in the try
> block, and the catch clause should catch ALL exceptions.
>
> The way things are structured now, if an exception is thrown in the try
> block, the transaction will be committed. It should only commit the
> transaction if the code executes successfully all the way to the end of
> the try block.
>
> Any thoughts?
>
> Adrian Crum
> Sandglass Software
> www.sandglass-software.com
>
> On 9/30/2013 9:13 AM, [hidden email] wrote:
>> Author: adrianc
>> Date: Mon Sep 30 16:13:03 2013
>> New Revision: 1527626
>>
>> URL: http://svn.apache.org/r1527626
>> Log:
>> Fixed a subtle flaw in the GenericDelegator.findOne method. When a
>> database query returns no result, GenericValue.NULL_VALUE is put in
>> the pk cache - so future findOne calls will know the entity value
>> doesn't exist. But the findOne method never checked for
>> GenericValue.NULL_VALUE in cache gets, so the database was queried
>> again for an entity value we already know doesn't exist.
>>
>> https://issues.apache.org/jira/browse/OFBIZ-5332
>>
>> Modified:
>>
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.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=1527626&r1=1527625&r2=1527626&view=diff
>>
>> ==============================================================================
>>
>> ---
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original)
>>
>> +++
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Mon
>> Sep 30 16:13:03 2013
>> @@ -1580,8 +1580,10 @@ public class GenericDelegator implements
>>           EntityEcaRuleRunner<?> ecaRunner =
>> this.getEcaRuleRunner(entityName);
>>           if (useCache) {
>>               ecaRunner.evalRules(EntityEcaHandler.EV_CACHE_CHECK,
>> EntityEcaHandler.OP_FIND, primaryKey, false);
>> -
>> -            GenericValue value =
>> this.getFromPrimaryKeyCache(primaryKey);
>> +            GenericValue value = cache.get(primaryKey);
>> +            if (value == GenericValue.NULL_VALUE) {
>> +                return null;
>> +            }
>>               if (value != null) {
>>                   return value;
>>               }
>>
>>