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

[Jacques Le Roux]

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);
> +            }
>          }
>      }
>
>

[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);
>> +            }
>>          }
>>      }
>>
>>
>

[Jacques Le Roux]

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);
> >> +            }
> >>          }
> >>      }
> >>
> >>
> >