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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Jacques Le Roux
Administrator
Should we not put this in release ?

Jacques

> Author: jonesde
> Date: Wed Aug 29 01:58:10 2007
> New Revision: 570706
>
> URL: http://svn.apache.org/viewvc?rev=570706&view=rev
> Log:
> Cleaned up exception and error handling in getNextSeqId code, was redundant and the tx code in getNextSeqIdLong that getNextSeqId
always calls wasn't handling commits right
>
> 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=570706&r1=570705&r2=570706&view=diff

> ==============================================================================
> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original)
> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Wed Aug 29 01:58:10 2007
> @@ -37,6 +37,7 @@
>  import org.xml.sax.SAXException;
>
>  import org.ofbiz.base.util.Debug;
> +import org.ofbiz.base.util.GeneralRuntimeException;
>  import org.ofbiz.base.util.UtilFormatOut;
>  import org.ofbiz.base.util.UtilMisc;
>  import org.ofbiz.base.util.UtilValidate;
> @@ -2434,41 +2435,17 @@
>       *@return Long with the next seq id for the given sequence name
>       */
>      public String getNextSeqId(String seqName, long staggerMax) {
> -        boolean beganTransaction = false;
> -        try {
> -            if (alwaysUseTransaction) {
> -                beganTransaction = TransactionUtil.begin();
> -            }
> +        Long nextSeqLong = this.getNextSeqIdLong(seqName, staggerMax);
>
> -            Long nextSeqLong = this.getNextSeqIdLong(seqName, staggerMax);
> -
> -            if (nextSeqLong == null) {
> -                throw new IllegalArgumentException("Could not get next sequenced ID for sequence name: " + seqName);
> -            }
> +        if (nextSeqLong == null) {
> +            // NOTE: the getNextSeqIdLong method SHOULD throw a runtime exception when no sequence value is found, which means we
should never see it get here

> +            throw new IllegalArgumentException("Could not get next sequenced ID for sequence name: " + seqName);
> +        }
>
> -            if (UtilValidate.isNotEmpty(this.getDelegatorInfo().sequencedIdPrefix)) {
> -                return this.getDelegatorInfo().sequencedIdPrefix + nextSeqLong.toString();
> -            } else {
> -                return nextSeqLong.toString();
> -            }
> -        } catch (Exception e) {
> -            String errMsg = "Failure in getNextSeqId operation for seqName [" + seqName + "]: " + e.toString() + ". Rolling back
transaction";

> -            Debug.logError(e, errMsg, module);
> -            try {
> -                // only rollback the transaction if we started one...
> -                TransactionUtil.rollback(beganTransaction, errMsg, e);
> -            } catch (GenericEntityException e2) {
> -                Debug.logError(e2, "[GenericDelegator] Could not rollback transaction: " + e2.toString(), module);
> -            }
> -            Debug.logError(e, "[GenericDelegator] Error getting next sequence ID: " + e.toString(), module);
> -            return null;
> -        } finally {
> -            try {
> -                // only commit the transaction if we started one...
> -                TransactionUtil.commit(beganTransaction);
> -            } catch (GenericTransactionException e1) {
> -                Debug.logError(e1, "[GenericDelegator] Could not commit transaction: " + e1.toString(), module);
> -            }
> +        if (UtilValidate.isNotEmpty(this.getDelegatorInfo().sequencedIdPrefix)) {
> +            return this.getDelegatorInfo().sequencedIdPrefix + nextSeqLong.toString();
> +        } else {
> +            return nextSeqLong.toString();
>          }
>      }
>
> @@ -2506,21 +2483,24 @@
>
>              Long newSeqId = sequencer == null ? null : sequencer.getNextSeqId(seqName, staggerMax);
>
> -            // only commit the transaction if we started one...
> -            TransactionUtil.commit(beganTransaction);
> -
>              return newSeqId;
>          } catch (GenericEntityException e) {
>              String errMsg = "Failure in getNextSeqIdLong operation for seqName [" + seqName + "]: " + e.toString() + ". Rolling
back transaction.";

> -            Debug.logError(e, errMsg, module);
>              try {
>                  // only rollback the transaction if we started one...
>                  TransactionUtil.rollback(beganTransaction, errMsg, e);
>              } catch (GenericEntityException e2) {
>                  Debug.logError(e2, "[GenericDelegator] Could not rollback transaction: " + e2.toString(), module);
>              }
> -            Debug.logError(e, "[GenericDelegator] Error getting next sequence ID: " + e.toString(), module);
> -            return null;
> +            // rather than logging the problem and returning null, thus hiding the problem, throw an exception
> +            throw new GeneralRuntimeException(errMsg, e);
> +        } finally {
> +            try {
> +                // only commit the transaction if we started one...
> +                TransactionUtil.commit(beganTransaction);
> +            } catch (GenericTransactionException e1) {
> +                Debug.logError(e1, "[GenericDelegator] Could not commit transaction: " + e1.toString(), module);
> +            }
>          }
>      }
>
>

Reply | Threaded
Open this post in threaded view
|

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

David E Jones

This could go into the release branch. It is the sort of bug that probably wouldn't come up much because of the way this code is generally used... but it is pretty much a bug fix.

-David


Jacques Le Roux wrote:

> Should we not put this in release ?
>
> Jacques
>
>> Author: jonesde
>> Date: Wed Aug 29 01:58:10 2007
>> New Revision: 570706
>>
>> URL: http://svn.apache.org/viewvc?rev=570706&view=rev
>> Log:
>> Cleaned up exception and error handling in getNextSeqId code, was redundant and the tx code in getNextSeqIdLong that getNextSeqId
> always calls wasn't handling commits right
>> 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=570706&r1=570705&r2=570706&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Wed Aug 29 01:58:10 2007
>> @@ -37,6 +37,7 @@
>>  import org.xml.sax.SAXException;
>>
>>  import org.ofbiz.base.util.Debug;
>> +import org.ofbiz.base.util.GeneralRuntimeException;
>>  import org.ofbiz.base.util.UtilFormatOut;
>>  import org.ofbiz.base.util.UtilMisc;
>>  import org.ofbiz.base.util.UtilValidate;
>> @@ -2434,41 +2435,17 @@
>>       *@return Long with the next seq id for the given sequence name
>>       */
>>      public String getNextSeqId(String seqName, long staggerMax) {
>> -        boolean beganTransaction = false;
>> -        try {
>> -            if (alwaysUseTransaction) {
>> -                beganTransaction = TransactionUtil.begin();
>> -            }
>> +        Long nextSeqLong = this.getNextSeqIdLong(seqName, staggerMax);
>>
>> -            Long nextSeqLong = this.getNextSeqIdLong(seqName, staggerMax);
>> -
>> -            if (nextSeqLong == null) {
>> -                throw new IllegalArgumentException("Could not get next sequenced ID for sequence name: " + seqName);
>> -            }
>> +        if (nextSeqLong == null) {
>> +            // NOTE: the getNextSeqIdLong method SHOULD throw a runtime exception when no sequence value is found, which means we
> should never see it get here
>> +            throw new IllegalArgumentException("Could not get next sequenced ID for sequence name: " + seqName);
>> +        }
>>
>> -            if (UtilValidate.isNotEmpty(this.getDelegatorInfo().sequencedIdPrefix)) {
>> -                return this.getDelegatorInfo().sequencedIdPrefix + nextSeqLong.toString();
>> -            } else {
>> -                return nextSeqLong.toString();
>> -            }
>> -        } catch (Exception e) {
>> -            String errMsg = "Failure in getNextSeqId operation for seqName [" + seqName + "]: " + e.toString() + ". Rolling back
> transaction";
>> -            Debug.logError(e, errMsg, module);
>> -            try {
>> -                // only rollback the transaction if we started one...
>> -                TransactionUtil.rollback(beganTransaction, errMsg, e);
>> -            } catch (GenericEntityException e2) {
>> -                Debug.logError(e2, "[GenericDelegator] Could not rollback transaction: " + e2.toString(), module);
>> -            }
>> -            Debug.logError(e, "[GenericDelegator] Error getting next sequence ID: " + e.toString(), module);
>> -            return null;
>> -        } finally {
>> -            try {
>> -                // only commit the transaction if we started one...
>> -                TransactionUtil.commit(beganTransaction);
>> -            } catch (GenericTransactionException e1) {
>> -                Debug.logError(e1, "[GenericDelegator] Could not commit transaction: " + e1.toString(), module);
>> -            }
>> +        if (UtilValidate.isNotEmpty(this.getDelegatorInfo().sequencedIdPrefix)) {
>> +            return this.getDelegatorInfo().sequencedIdPrefix + nextSeqLong.toString();
>> +        } else {
>> +            return nextSeqLong.toString();
>>          }
>>      }
>>
>> @@ -2506,21 +2483,24 @@
>>
>>              Long newSeqId = sequencer == null ? null : sequencer.getNextSeqId(seqName, staggerMax);
>>
>> -            // only commit the transaction if we started one...
>> -            TransactionUtil.commit(beganTransaction);
>> -
>>              return newSeqId;
>>          } catch (GenericEntityException e) {
>>              String errMsg = "Failure in getNextSeqIdLong operation for seqName [" + seqName + "]: " + e.toString() + ". Rolling
> back transaction.";
>> -            Debug.logError(e, errMsg, module);
>>              try {
>>                  // only rollback the transaction if we started one...
>>                  TransactionUtil.rollback(beganTransaction, errMsg, e);
>>              } catch (GenericEntityException e2) {
>>                  Debug.logError(e2, "[GenericDelegator] Could not rollback transaction: " + e2.toString(), module);
>>              }
>> -            Debug.logError(e, "[GenericDelegator] Error getting next sequence ID: " + e.toString(), module);
>> -            return null;
>> +            // rather than logging the problem and returning null, thus hiding the problem, throw an exception
>> +            throw new GeneralRuntimeException(errMsg, e);
>> +        } finally {
>> +            try {
>> +                // only commit the transaction if we started one...
>> +                TransactionUtil.commit(beganTransaction);
>> +            } catch (GenericTransactionException e1) {
>> +                Debug.logError(e1, "[GenericDelegator] Could not commit transaction: " + e1.toString(), module);
>> +            }
>>          }
>>      }
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Jacques Le Roux
Administrator
In 571088, I used my new interactive merging batch :o)

Jacques

De : "David E Jones" <[hidden email]>
>
> This could go into the release branch. It is the sort of bug that probably wouldn't come up much because of the way this code is
generally used... but it is pretty much a bug fix.

>
> -David
>
>
> Jacques Le Roux wrote:
> > Should we not put this in release ?
> >
> > Jacques
> >
> >> Author: jonesde
> >> Date: Wed Aug 29 01:58:10 2007
> >> New Revision: 570706
> >>
> >> URL: http://svn.apache.org/viewvc?rev=570706&view=rev
> >> Log:
> >> Cleaned up exception and error handling in getNextSeqId code, was redundant and the tx code in getNextSeqIdLong that
getNextSeqId
> > always calls wasn't handling commits right
> >> 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=570706&r1=570705&r2=570706&view=diff

> >> ==============================================================================
> >> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original)
> >> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Wed Aug 29 01:58:10 2007
> >> @@ -37,6 +37,7 @@
> >>  import org.xml.sax.SAXException;
> >>
> >>  import org.ofbiz.base.util.Debug;
> >> +import org.ofbiz.base.util.GeneralRuntimeException;
> >>  import org.ofbiz.base.util.UtilFormatOut;
> >>  import org.ofbiz.base.util.UtilMisc;
> >>  import org.ofbiz.base.util.UtilValidate;
> >> @@ -2434,41 +2435,17 @@
> >>       *@return Long with the next seq id for the given sequence name
> >>       */
> >>      public String getNextSeqId(String seqName, long staggerMax) {
> >> -        boolean beganTransaction = false;
> >> -        try {
> >> -            if (alwaysUseTransaction) {
> >> -                beganTransaction = TransactionUtil.begin();
> >> -            }
> >> +        Long nextSeqLong = this.getNextSeqIdLong(seqName, staggerMax);
> >>
> >> -            Long nextSeqLong = this.getNextSeqIdLong(seqName, staggerMax);
> >> -
> >> -            if (nextSeqLong == null) {
> >> -                throw new IllegalArgumentException("Could not get next sequenced ID for sequence name: " + seqName);
> >> -            }
> >> +        if (nextSeqLong == null) {
> >> +            // NOTE: the getNextSeqIdLong method SHOULD throw a runtime exception when no sequence value is found, which means
we

> > should never see it get here
> >> +            throw new IllegalArgumentException("Could not get next sequenced ID for sequence name: " + seqName);
> >> +        }
> >>
> >> -            if (UtilValidate.isNotEmpty(this.getDelegatorInfo().sequencedIdPrefix)) {
> >> -                return this.getDelegatorInfo().sequencedIdPrefix + nextSeqLong.toString();
> >> -            } else {
> >> -                return nextSeqLong.toString();
> >> -            }
> >> -        } catch (Exception e) {
> >> -            String errMsg = "Failure in getNextSeqId operation for seqName [" + seqName + "]: " + e.toString() + ". Rolling
back

> > transaction";
> >> -            Debug.logError(e, errMsg, module);
> >> -            try {
> >> -                // only rollback the transaction if we started one...
> >> -                TransactionUtil.rollback(beganTransaction, errMsg, e);
> >> -            } catch (GenericEntityException e2) {
> >> -                Debug.logError(e2, "[GenericDelegator] Could not rollback transaction: " + e2.toString(), module);
> >> -            }
> >> -            Debug.logError(e, "[GenericDelegator] Error getting next sequence ID: " + e.toString(), module);
> >> -            return null;
> >> -        } finally {
> >> -            try {
> >> -                // only commit the transaction if we started one...
> >> -                TransactionUtil.commit(beganTransaction);
> >> -            } catch (GenericTransactionException e1) {
> >> -                Debug.logError(e1, "[GenericDelegator] Could not commit transaction: " + e1.toString(), module);
> >> -            }
> >> +        if (UtilValidate.isNotEmpty(this.getDelegatorInfo().sequencedIdPrefix)) {
> >> +            return this.getDelegatorInfo().sequencedIdPrefix + nextSeqLong.toString();
> >> +        } else {
> >> +            return nextSeqLong.toString();
> >>          }
> >>      }
> >>
> >> @@ -2506,21 +2483,24 @@
> >>
> >>              Long newSeqId = sequencer == null ? null : sequencer.getNextSeqId(seqName, staggerMax);
> >>
> >> -            // only commit the transaction if we started one...
> >> -            TransactionUtil.commit(beganTransaction);
> >> -
> >>              return newSeqId;
> >>          } catch (GenericEntityException e) {
> >>              String errMsg = "Failure in getNextSeqIdLong operation for seqName [" + seqName + "]: " + e.toString() + ".
Rolling

> > back transaction.";
> >> -            Debug.logError(e, errMsg, module);
> >>              try {
> >>                  // only rollback the transaction if we started one...
> >>                  TransactionUtil.rollback(beganTransaction, errMsg, e);
> >>              } catch (GenericEntityException e2) {
> >>                  Debug.logError(e2, "[GenericDelegator] Could not rollback transaction: " + e2.toString(), module);
> >>              }
> >> -            Debug.logError(e, "[GenericDelegator] Error getting next sequence ID: " + e.toString(), module);
> >> -            return null;
> >> +            // rather than logging the problem and returning null, thus hiding the problem, throw an exception
> >> +            throw new GeneralRuntimeException(errMsg, e);
> >> +        } finally {
> >> +            try {
> >> +                // only commit the transaction if we started one...
> >> +                TransactionUtil.commit(beganTransaction);
> >> +            } catch (GenericTransactionException e1) {
> >> +                Debug.logError(e1, "[GenericDelegator] Could not commit transaction: " + e1.toString(), module);
> >> +            }
> >>          }
> >>      }
> >>
> >>
> >