Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java

[Michael Brohl-3]

Hi Jacques,

I think directly returning the result inside the catch block changes the
logic of the code (the adjustments are not added).

Please have another look.

Thanks,

Michael


Am 04.09.17 um 17:12 schrieb [hidden email]:

> Author: jleroux
> Date: Mon Sep  4 15:12:23 2017
> New Revision: 1807240
>
> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
> Log:
> Fixed: Fix Default or Empty Catch block in Java and Groovy files
> (OFBIZ-8341)
>
> In many Java and Groovy files we have auto generated catch blocks or empty catch
> blocks.
> To avoid such exception swallowing this should be improved to at least log the
> error and also return error in case of service.
>
> jleroux: I can't see what we could do more here, unlikely anyway
>
> Modified:
>      ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java Mon Sep  4 15:12:23 2017
> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>                   List<GenericValue> workOrderItemFulfillments = null;
>                   try {
>                       workOrderItemFulfillments = orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment", UtilMisc.toMap("orderId", orderItem.getString("orderId"), "orderItemSeqId", orderItem.getString("orderItemSeqId")), null, true);
> -                } catch (GenericEntityException e) {}
> +                } catch (GenericEntityException e) {
> +                    Debug.logError(e, module);
> +                    return result;
> +                }
>                   if (workOrderItemFulfillments != null) {
>                       Iterator<GenericValue> iter = workOrderItemFulfillments.iterator();
> -                    if (iter.hasNext())    {
> +                    if (iter.hasNext()) {
>                           GenericValue WorkOrderItemFulfillment = iter.next();
>                           GenericValue workEffort = null;
>                           try {
>
>

[Scott Gray-3]

Definitely changes the behavior, but the behavior wasn't good before (and
still isn't good now).  If we can't return an accurate order item sub-total
then we should be throwing an exception IMO.

This is often why we can't just bulk fix the issues reported by static
analysis tools, because the fix requires a thought-out solution that's
totally dependent on the context of the code in question.  Hiding these
issues by removing an obvious problem and replacing it with a quick fix
that doesn't solve the core problem only makes these bugs harder to find in
the future.  Better to put a FIXME tag in if there's no time to properly
analyse the issue.

Also worth noting the reality in this case, if your system is throwing
EntityExceptions from a simple db read then whatever you're trying
accomplish is already going to error out somewhere along the chain before
or after this method is called.

Regards
Scott

On 5 September 2017 at 07:28, Michael Brohl <[hidden email]>
wrote:

> Hi Jacques,
>
> I think directly returning the result inside the catch block changes the
> logic of the code (the adjustments are not added).
>
> Please have another look.
>
> Thanks,
>
> Michael
>
>
> Am 04.09.17 um 17:12 schrieb [hidden email]:
>
> Author: jleroux
>> Date: Mon Sep  4 15:12:23 2017
>> New Revision: 1807240
>>
>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>> Log:
>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>> (OFBIZ-8341)
>>
>> In many Java and Groovy files we have auto generated catch blocks or
>> empty catch
>> blocks.
>> To avoid such exception swallowing this should be improved to at least
>> log the
>> error and also return error in case of service.
>>
>> jleroux: I can't see what we could do more here, unlikely anyway
>>
>> Modified:
>>      ofbiz/ofbiz-framework/trunk/applications/order/src/main/jav
>> a/org/apache/ofbiz/order/order/OrderReadHelper.java
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>> /org/apache/ofbiz/order/order/OrderReadHelper.java
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
>> lications/order/src/main/java/org/apache/ofbiz/order/order/
>> OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>> /org/apache/ofbiz/order/order/OrderReadHelper.java (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>> /org/apache/ofbiz/order/order/OrderReadHelper.java Mon Sep  4 15:12:23
>> 2017
>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>                   List<GenericValue> workOrderItemFulfillments = null;
>>                   try {
>>                       workOrderItemFulfillments =
>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>> "orderItemSeqId", orderItem.getString("orderItemSeqId")), null, true);
>> -                } catch (GenericEntityException e) {}
>> +                } catch (GenericEntityException e) {
>> +                    Debug.logError(e, module);
>> +                    return result;
>> +                }
>>                   if (workOrderItemFulfillments != null) {
>>                       Iterator<GenericValue> iter =
>> workOrderItemFulfillments.iterator();
>> -                    if (iter.hasNext())    {
>> +                    if (iter.hasNext()) {
>>                           GenericValue WorkOrderItemFulfillment =
>> iter.next();
>>                           GenericValue workEffort = null;
>>                           try {
>>
>>
>>
>
>

[Jacques Le Roux]

Yes thanks Michael,

I agree with Scott about rather throwing an exception

Jacques


Le 04/09/2017 à 21:28, Michael Brohl a écrit :

> Hi Jacques,
>
> I think directly returning the result inside the catch block changes the logic of the code (the adjustments are not added).
>
> Please have another look.
>
> Thanks,
>
> Michael
>
>
> Am 04.09.17 um 17:12 schrieb [hidden email]:
>> Author: jleroux
>> Date: Mon Sep  4 15:12:23 2017
>> New Revision: 1807240
>>
>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>> Log:
>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>> (OFBIZ-8341)
>>
>> In many Java and Groovy files we have auto generated catch blocks or empty catch
>> blocks.
>> To avoid such exception swallowing this should be improved to at least log the
>> error and also return error in case of service.
>>
>> jleroux: I can't see what we could do more here, unlikely anyway
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java Mon Sep  4 15:12:23 2017
>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>                   List<GenericValue> workOrderItemFulfillments = null;
>>                   try {
>>                       workOrderItemFulfillments = orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment", UtilMisc.toMap("orderId",
>> orderItem.getString("orderId"), "orderItemSeqId", orderItem.getString("orderItemSeqId")), null, true);
>> -                } catch (GenericEntityException e) {}
>> +                } catch (GenericEntityException e) {
>> +                    Debug.logError(e, module);
>> +                    return result;
>> +                }
>>                   if (workOrderItemFulfillments != null) {
>>                       Iterator<GenericValue> iter = workOrderItemFulfillments.iterator();
>> -                    if (iter.hasNext())    {
>> +                    if (iter.hasNext()) {
>>                           GenericValue WorkOrderItemFulfillment = iter.next();
>>                           GenericValue workEffort = null;
>>                           try {
>>
>>
>
>

[Jacques Le Roux]

Thanks Scott, inline...

Le 05/09/2017 à 06:21, Scott Gray a écrit :
> Definitely changes the behavior, but the behavior wasn't good before (and
> still isn't good now).  If we can't return an accurate order item sub-total
> then we should be throwing an exception IMO.
>
> This is often why we can't just bulk fix the issues reported by static
> analysis tools, because the fix requires a thought-out solution that's
> totally dependent on the context of the code in question.
I did it by hand. I do all the remaining ones I spotted by hand
As explained in the Jira (8341) I look for them using  "catch (*) {}". There are still 80 of them
And yes I was a bit unsure of what to do. I tried to minimise changing the behaviour, wrong idea.
I agree that it should be better to throw an exception (ahem... a lot of exceptions)

> Hiding these
> issues by removing an obvious problem and replacing it with a quick fix
> that doesn't solve the core problem only makes these bugs harder to find in
> the future.
Actually it's slightly better than before because there is something in log.
The same is used locally in getShippableTotal(), I agree it's not good

> Better to put a FIXME tag in if there's no time to properly
> analyse the issue.
It was not lack of time, but lack of thought :/
As you suggested, it would be better to throw exceptions.
But the changes are important, so before committing I suggest a patch in OFBIZ-8341.
Please all person concerned review. If we agree, that will be the way I'll use for other cases like that.

> Also worth noting the reality in this case, if your system is throwing
> EntityExceptions from a simple db read then whatever you're trying
> accomplish is already going to error out somewhere along the chain before
> or after this method is called.
That was my thought. And that's also why I wrote it's was unlikely. I meant that it was unlikely to be present only there.
Still the best solution is to throw an exception in such cases, "you shall not swallow an exception" is the mantra.

I also wrote in this Jira
"It would be good for instance for a type of exception and a type of file (service, event, helper/handler/test/etc.) to use and adopt a same type of
exception handling.". I meant everywhere...
If we agree, It will be one more type in the collection... I'll try to write all the found types in then Jira and then, once all collected, in best
practices in wiki.
BTW there is a large usage of GeneralException in OrderServices.java. And the usage is not consistent. This is the kind of things we should thought
about if we want to refactor a monster like that (6411 lines).

Jacques

>
> Regards
> Scott
>
> On 5 September 2017 at 07:28, Michael Brohl <[hidden email]>
> wrote:
>
>> Hi Jacques,
>>
>> I think directly returning the result inside the catch block changes the
>> logic of the code (the adjustments are not added).
>>
>> Please have another look.
>>
>> Thanks,
>>
>> Michael
>>
>>
>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>
>> Author: jleroux
>>> Date: Mon Sep  4 15:12:23 2017
>>> New Revision: 1807240
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>> Log:
>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>> (OFBIZ-8341)
>>>
>>> In many Java and Groovy files we have auto generated catch blocks or
>>> empty catch
>>> blocks.
>>> To avoid such exception swallowing this should be improved to at least
>>> log the
>>> error and also return error in case of service.
>>>
>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>
>>> Modified:
>>>       ofbiz/ofbiz-framework/trunk/applications/order/src/main/jav
>>> a/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>>> /org/apache/ofbiz/order/order/OrderReadHelper.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
>>> lications/order/src/main/java/org/apache/ofbiz/order/order/
>>> OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>> ============================================================
>>> ==================
>>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>>> /org/apache/ofbiz/order/order/OrderReadHelper.java (original)
>>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
>>> /org/apache/ofbiz/order/order/OrderReadHelper.java Mon Sep  4 15:12:23
>>> 2017
>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>                    List<GenericValue> workOrderItemFulfillments = null;
>>>                    try {
>>>                        workOrderItemFulfillments =
>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>>> "orderItemSeqId", orderItem.getString("orderItemSeqId")), null, true);
>>> -                } catch (GenericEntityException e) {}
>>> +                } catch (GenericEntityException e) {
>>> +                    Debug.logError(e, module);
>>> +                    return result;
>>> +                }
>>>                    if (workOrderItemFulfillments != null) {
>>>                        Iterator<GenericValue> iter =
>>> workOrderItemFulfillments.iterator();
>>> -                    if (iter.hasNext())    {
>>> +                    if (iter.hasNext()) {
>>>                            GenericValue WorkOrderItemFulfillment =
>>> iter.next();
>>>                            GenericValue workEffort = null;
>>>                            try {
>>>
>>>
>>>
>>

[Jacques Le Roux]

If nobody disagree my last comments at  OFBIZ-8341 I will commit the "OFBIZ-8341- OrderReadHelper.patch" this weekend

Jacques


Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :

> Yes thanks Michael,
>
> I agree with Scott about rather throwing an exception
>
> Jacques
>
>
> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>> Hi Jacques,
>>
>> I think directly returning the result inside the catch block changes the logic of the code (the adjustments are not added).
>>
>> Please have another look.
>>
>> Thanks,
>>
>> Michael
>>
>>
>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>> Author: jleroux
>>> Date: Mon Sep  4 15:12:23 2017
>>> New Revision: 1807240
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>> Log:
>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>> (OFBIZ-8341)
>>>
>>> In many Java and Groovy files we have auto generated catch blocks or empty catch
>>> blocks.
>>> To avoid such exception swallowing this should be improved to at least log the
>>> error and also return error in case of service.
>>>
>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>
>>> Modified:
>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>> ==============================================================================
>>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java (original)
>>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java Mon Sep  4 15:12:23 2017
>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>                   List<GenericValue> workOrderItemFulfillments = null;
>>>                   try {
>>>                       workOrderItemFulfillments = orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment", UtilMisc.toMap("orderId",
>>> orderItem.getString("orderId"), "orderItemSeqId", orderItem.getString("orderItemSeqId")), null, true);
>>> -                } catch (GenericEntityException e) {}
>>> +                } catch (GenericEntityException e) {
>>> +                    Debug.logError(e, module);
>>> +                    return result;
>>> +                }
>>>                   if (workOrderItemFulfillments != null) {
>>>                       Iterator<GenericValue> iter = workOrderItemFulfillments.iterator();
>>> -                    if (iter.hasNext())    {
>>> +                    if (iter.hasNext()) {
>>>                           GenericValue WorkOrderItemFulfillment = iter.next();
>>>                           GenericValue workEffort = null;
>>>                           try {
>>>
>>>
>>
>>
>
>

[Michael Brohl-3]

I disagree, not on the patch itself but on the time frame you give us to
review and think about it.

I see no reason to put pressure on this issue.

Regards,

Michael


Am 08.09.17 um 10:02 schrieb Jacques Le Roux:

> If nobody disagree my last comments at  OFBIZ-8341 I will commit the
> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>
> Jacques
>
>
> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>> Yes thanks Michael,
>>
>> I agree with Scott about rather throwing an exception
>>
>> Jacques
>>
>>
>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>> Hi Jacques,
>>>
>>> I think directly returning the result inside the catch block changes
>>> the logic of the code (the adjustments are not added).
>>>
>>> Please have another look.
>>>
>>> Thanks,
>>>
>>> Michael
>>>
>>>
>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>> Author: jleroux
>>>> Date: Mon Sep  4 15:12:23 2017
>>>> New Revision: 1807240
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>> Log:
>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>> (OFBIZ-8341)
>>>>
>>>> In many Java and Groovy files we have auto generated catch blocks
>>>> or empty catch
>>>> blocks.
>>>> To avoid such exception swallowing this should be improved to at
>>>> least log the
>>>> error and also return error in case of service.
>>>>
>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>
>>>> Modified:
>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>
>>>>
>>>> Modified:
>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>> ==============================================================================
>>>>
>>>> ---
>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>> (original)
>>>> +++
>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>> Mon Sep  4 15:12:23 2017
>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>                   List<GenericValue> workOrderItemFulfillments = null;
>>>>                   try {
>>>>                       workOrderItemFulfillments =
>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>>>> "orderItemSeqId", orderItem.getString("orderItemSeqId")), null, true);
>>>> -                } catch (GenericEntityException e) {}
>>>> +                } catch (GenericEntityException e) {
>>>> +                    Debug.logError(e, module);
>>>> +                    return result;
>>>> +                }
>>>>                   if (workOrderItemFulfillments != null) {
>>>>                       Iterator<GenericValue> iter =
>>>> workOrderItemFulfillments.iterator();
>>>> -                    if (iter.hasNext())    {
>>>> +                    if (iter.hasNext()) {
>>>>                           GenericValue WorkOrderItemFulfillment =
>>>> iter.next();
>>>>                           GenericValue workEffort = null;
>>>>                           try {
>>>>
>>>>
>>>
>>>
>>
>>
>

[Jacques Le Roux]

No worries Michael,

I can wait a week more

Jacques


Le 08/09/2017 à 10:42, Michael Brohl a écrit :

> I disagree, not on the patch itself but on the time frame you give us to review and think about it.
>
> I see no reason to put pressure on this issue.
>
> Regards,
>
> Michael
>
>
> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>> If nobody disagree my last comments at OFBIZ-8341 I will commit the "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>
>> Jacques
>>
>>
>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>> Yes thanks Michael,
>>>
>>> I agree with Scott about rather throwing an exception
>>>
>>> Jacques
>>>
>>>
>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>> Hi Jacques,
>>>>
>>>> I think directly returning the result inside the catch block changes the logic of the code (the adjustments are not added).
>>>>
>>>> Please have another look.
>>>>
>>>> Thanks,
>>>>
>>>> Michael
>>>>
>>>>
>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>> Author: jleroux
>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>> New Revision: 1807240
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>> Log:
>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>>> (OFBIZ-8341)
>>>>>
>>>>> In many Java and Groovy files we have auto generated catch blocks or empty catch
>>>>> blocks.
>>>>> To avoid such exception swallowing this should be improved to at least log the
>>>>> error and also return error in case of service.
>>>>>
>>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>>
>>>>> Modified:
>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>
>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>> ==============================================================================
>>>>> --- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java (original)
>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java Mon Sep  4 15:12:23 2017
>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>                   List<GenericValue> workOrderItemFulfillments = null;
>>>>>                   try {
>>>>>                       workOrderItemFulfillments = orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment", UtilMisc.toMap("orderId",
>>>>> orderItem.getString("orderId"), "orderItemSeqId", orderItem.getString("orderItemSeqId")), null, true);
>>>>> -                } catch (GenericEntityException e) {}
>>>>> +                } catch (GenericEntityException e) {
>>>>> +                    Debug.logError(e, module);
>>>>> +                    return result;
>>>>> +                }
>>>>>                   if (workOrderItemFulfillments != null) {
>>>>>                       Iterator<GenericValue> iter = workOrderItemFulfillments.iterator();
>>>>> -                    if (iter.hasNext())    {
>>>>> +                    if (iter.hasNext()) {
>>>>>                           GenericValue WorkOrderItemFulfillment = iter.next();
>>>>>                           GenericValue workEffort = null;
>>>>>                           try {
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>

[Michael Brohl-3]

Thanks, Jacques.

Regards,

Michael

Am 08.09.17 um 10:54 schrieb Jacques Le Roux:

> No worries Michael,
>
> I can wait a week more
>
> Jacques
>
>
> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
>> I disagree, not on the patch itself but on the time frame you give us
>> to review and think about it.
>>
>> I see no reason to put pressure on this issue.
>>
>> Regards,
>>
>> Michael
>>
>>
>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>>> If nobody disagree my last comments at OFBIZ-8341 I will commit the
>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>>
>>> Jacques
>>>
>>>
>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>>> Yes thanks Michael,
>>>>
>>>> I agree with Scott about rather throwing an exception
>>>>
>>>> Jacques
>>>>
>>>>
>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>>> Hi Jacques,
>>>>>
>>>>> I think directly returning the result inside the catch block
>>>>> changes the logic of the code (the adjustments are not added).
>>>>>
>>>>> Please have another look.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Michael
>>>>>
>>>>>
>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>>> Author: jleroux
>>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>>> New Revision: 1807240
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>>> Log:
>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>>>> (OFBIZ-8341)
>>>>>>
>>>>>> In many Java and Groovy files we have auto generated catch blocks
>>>>>> or empty catch
>>>>>> blocks.
>>>>>> To avoid such exception swallowing this should be improved to at
>>>>>> least log the
>>>>>> error and also return error in case of service.
>>>>>>
>>>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>>>
>>>>>> Modified:
>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>
>>>>>>
>>>>>> Modified:
>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>> URL:
>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>>> ==============================================================================
>>>>>>
>>>>>> ---
>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>> (original)
>>>>>> +++
>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>> Mon Sep  4 15:12:23 2017
>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>>                   List<GenericValue> workOrderItemFulfillments =
>>>>>> null;
>>>>>>                   try {
>>>>>>                       workOrderItemFulfillments =
>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>>>>>> "orderItemSeqId", orderItem.getString("orderItemSeqId")), null,
>>>>>> true);
>>>>>> -                } catch (GenericEntityException e) {}
>>>>>> +                } catch (GenericEntityException e) {
>>>>>> +                    Debug.logError(e, module);
>>>>>> +                    return result;
>>>>>> +                }
>>>>>>                   if (workOrderItemFulfillments != null) {
>>>>>>                       Iterator<GenericValue> iter =
>>>>>> workOrderItemFulfillments.iterator();
>>>>>> -                    if (iter.hasNext())    {
>>>>>> +                    if (iter.hasNext()) {
>>>>>>                           GenericValue WorkOrderItemFulfillment =
>>>>>> iter.next();
>>>>>>                           GenericValue workEffort = null;
>>>>>>                           try {
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>

[taher]

What I understand is that the patch is essentially changing the
signature of most methods to throw an exception. On a first glance
this seems to be putting the code in a worst state because now you're
adding complexity for the caller to figure out how to handle these
exceptions.

What is the purpose of this change? What is the gain?

On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl <[hidden email]> wrote:

> Thanks, Jacques.
>
> Regards,
>
> Michael
>
> Am 08.09.17 um 10:54 schrieb Jacques Le Roux:
>
>> No worries Michael,
>>
>> I can wait a week more
>>
>> Jacques
>>
>>
>> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
>>>
>>> I disagree, not on the patch itself but on the time frame you give us to
>>> review and think about it.
>>>
>>> I see no reason to put pressure on this issue.
>>>
>>> Regards,
>>>
>>> Michael
>>>
>>>
>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>>>>
>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit the
>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>>>
>>>> Jacques
>>>>
>>>>
>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>>>>
>>>>> Yes thanks Michael,
>>>>>
>>>>> I agree with Scott about rather throwing an exception
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>>>>
>>>>>> Hi Jacques,
>>>>>>
>>>>>> I think directly returning the result inside the catch block changes
>>>>>> the logic of the code (the adjustments are not added).
>>>>>>
>>>>>> Please have another look.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Michael
>>>>>>
>>>>>>
>>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>>>>
>>>>>>> Author: jleroux
>>>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>>>> New Revision: 1807240
>>>>>>>
>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>>>> Log:
>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>>>>> (OFBIZ-8341)
>>>>>>>
>>>>>>> In many Java and Groovy files we have auto generated catch blocks or
>>>>>>> empty catch
>>>>>>> blocks.
>>>>>>> To avoid such exception swallowing this should be improved to at
>>>>>>> least log the
>>>>>>> error and also return error in case of service.
>>>>>>>
>>>>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>>>>
>>>>>>> Modified:
>>>>>>>
>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>
>>>>>>> Modified:
>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>> URL:
>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>>>>
>>>>>>> ==============================================================================
>>>>>>> ---
>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>> (original)
>>>>>>> +++
>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>> Mon Sep  4 15:12:23 2017
>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>>>                   List<GenericValue> workOrderItemFulfillments =
>>>>>>> null;
>>>>>>>                   try {
>>>>>>>                       workOrderItemFulfillments =
>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"), "orderItemSeqId",
>>>>>>> orderItem.getString("orderItemSeqId")), null, true);
>>>>>>> -                } catch (GenericEntityException e) {}
>>>>>>> +                } catch (GenericEntityException e) {
>>>>>>> +                    Debug.logError(e, module);
>>>>>>> +                    return result;
>>>>>>> +                }
>>>>>>>                   if (workOrderItemFulfillments != null) {
>>>>>>>                       Iterator<GenericValue> iter =
>>>>>>> workOrderItemFulfillments.iterator();
>>>>>>> -                    if (iter.hasNext())    {
>>>>>>> +                    if (iter.hasNext()) {
>>>>>>>                           GenericValue WorkOrderItemFulfillment =
>>>>>>> iter.next();
>>>>>>>                           GenericValue workEffort = null;
>>>>>>>                           try {
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[Jacques Le Roux]

I easily see 3 alternatives:

 1. Swallow the exception, like we currently do. This should be forbidden in all cases, I'd veto that!-
 2. Log an error and return a wrong result, like it's currently done by getShippableTotal() in the same class. That's what I proposed, but instead of
    returning ZERO, I returned the already calculated result.
 3. Throw an error, based on Scott's suggestion in dev ML, like the patch does.

Sincerely I have not strong opinions about 2 and 3. But technically I prefer 3 because the exception must then be handled by the caller (very unlikely
to be isolated, we talk about a GenericEntityException here). That's how an API should be written, the exception should pop to the initial topmost caller.

If you have another alternative, I'm ready to discuss it.

Maybe you will suggest to refactor the whole thing (class, classes, etc.) but sincerely then an elaborated plan would be needed beforehand.
And in any case it's then an order magnitude more complex that the 2 solutions above. I don't say it's impossible, just that I'll maybe not see it :)

Jacques


Le 10/09/2017 à 18:22, Taher Alkhateeb a écrit :

> What I understand is that the patch is essentially changing the
> signature of most methods to throw an exception. On a first glance
> this seems to be putting the code in a worst state because now you're
> adding complexity for the caller to figure out how to handle these
> exceptions.
>
> What is the purpose of this change? What is the gain?
>
> On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl <[hidden email]> wrote:
>> Thanks, Jacques.
>>
>> Regards,
>>
>> Michael
>>
>> Am 08.09.17 um 10:54 schrieb Jacques Le Roux:
>>
>>> No worries Michael,
>>>
>>> I can wait a week more
>>>
>>> Jacques
>>>
>>>
>>> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
>>>> I disagree, not on the patch itself but on the time frame you give us to
>>>> review and think about it.
>>>>
>>>> I see no reason to put pressure on this issue.
>>>>
>>>> Regards,
>>>>
>>>> Michael
>>>>
>>>>
>>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit the
>>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>>>>> Yes thanks Michael,
>>>>>>
>>>>>> I agree with Scott about rather throwing an exception
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>>>>> Hi Jacques,
>>>>>>>
>>>>>>> I think directly returning the result inside the catch block changes
>>>>>>> the logic of the code (the adjustments are not added).
>>>>>>>
>>>>>>> Please have another look.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Michael
>>>>>>>
>>>>>>>
>>>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>>>>> Author: jleroux
>>>>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>>>>> New Revision: 1807240
>>>>>>>>
>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>>>>> Log:
>>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>>>>>> (OFBIZ-8341)
>>>>>>>>
>>>>>>>> In many Java and Groovy files we have auto generated catch blocks or
>>>>>>>> empty catch
>>>>>>>> blocks.
>>>>>>>> To avoid such exception swallowing this should be improved to at
>>>>>>>> least log the
>>>>>>>> error and also return error in case of service.
>>>>>>>>
>>>>>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>
>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>> URL:
>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> ---
>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>> (original)
>>>>>>>> +++
>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>> Mon Sep  4 15:12:23 2017
>>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>>>>                    List<GenericValue> workOrderItemFulfillments =
>>>>>>>> null;
>>>>>>>>                    try {
>>>>>>>>                        workOrderItemFulfillments =
>>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"), "orderItemSeqId",
>>>>>>>> orderItem.getString("orderItemSeqId")), null, true);
>>>>>>>> -                } catch (GenericEntityException e) {}
>>>>>>>> +                } catch (GenericEntityException e) {
>>>>>>>> +                    Debug.logError(e, module);
>>>>>>>> +                    return result;
>>>>>>>> +                }
>>>>>>>>                    if (workOrderItemFulfillments != null) {
>>>>>>>>                        Iterator<GenericValue> iter =
>>>>>>>> workOrderItemFulfillments.iterator();
>>>>>>>> -                    if (iter.hasNext())    {
>>>>>>>> +                    if (iter.hasNext()) {
>>>>>>>>                            GenericValue WorkOrderItemFulfillment =
>>>>>>>> iter.next();
>>>>>>>>                            GenericValue workEffort = null;
>>>>>>>>                            try {
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>

[taher]

Inline

On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux
<[hidden email]> wrote:

> I easily see 3 alternatives:
>
> 1. Swallow the exception, like we currently do. This should be forbidden in
> all cases, I'd veto that!-
> 2. Log an error and return a wrong result, like it's currently done by
> getShippableTotal() in the same class. That's what I proposed, but instead
> of
>    returning ZERO, I returned the already calculated result.
> 3. Throw an error, based on Scott's suggestion in dev ML, like the patch
> does.

As I've mentioned numerous time, i don't recommend mass fixes because
you might be fixing an error with another error. Bulk changes are
never a good idea because it means you don't know exactly what you're
doing or how the code is affected. Such changes need to happen slowly
and carefully. Just because because we have swallowed exceptions does
not mean we mass fix them. You might trigger other unknown side
effects without carefully studying every thing.
My recommendation is to not touch anything, maybe a log message would
be enough, but not to change anything else without a careful deep look
at the code.
>
> Sincerely I have not strong opinions about 2 and 3. But technically I prefer
> 3 because the exception must then be handled by the caller (very unlikely to
> be isolated, we talk about a GenericEntityException here). That's how an API
> should be written, the exception should pop to the initial topmost caller.
Not necessarily, the exception propagation strategy is dependent on
the design, architecture and intent. Exception handling can get real
nasty real quick if not studied carefully, it will pollute your design
and make things painful for everyone. Sometimes it makes sense to
escalate an exception, and sometimes it makes sense to handle it
immediately, it just depends on the situation.
>
> If you have another alternative, I'm ready to discuss it.
Already suggested above
>
> Maybe you will suggest to refactor the whole thing (class, classes, etc.)
> but sincerely then an elaborated plan would be needed beforehand.
> And in any case it's then an order magnitude more complex that the 2
> solutions above. I don't say it's impossible, just that I'll maybe not see
> it :)
Ahhh, interesting observation, but even I don't know what I would
suggest next :) I write things when I think them.

>
> Jacques
>
>
>
> Le 10/09/2017 à 18:22, Taher Alkhateeb a écrit :
>>
>> What I understand is that the patch is essentially changing the
>> signature of most methods to throw an exception. On a first glance
>> this seems to be putting the code in a worst state because now you're
>> adding complexity for the caller to figure out how to handle these
>> exceptions.
>>
>> What is the purpose of this change? What is the gain?
>>
>> On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl <[hidden email]>
>> wrote:
>>>
>>> Thanks, Jacques.
>>>
>>> Regards,
>>>
>>> Michael
>>>
>>> Am 08.09.17 um 10:54 schrieb Jacques Le Roux:
>>>
>>>> No worries Michael,
>>>>
>>>> I can wait a week more
>>>>
>>>> Jacques
>>>>
>>>>
>>>> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
>>>>>
>>>>> I disagree, not on the patch itself but on the time frame you give us
>>>>> to
>>>>> review and think about it.
>>>>>
>>>>> I see no reason to put pressure on this issue.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Michael
>>>>>
>>>>>
>>>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>>>>>>
>>>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit the
>>>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>>>>>>
>>>>>>> Yes thanks Michael,
>>>>>>>
>>>>>>> I agree with Scott about rather throwing an exception
>>>>>>>
>>>>>>> Jacques
>>>>>>>
>>>>>>>
>>>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>>>>>>
>>>>>>>> Hi Jacques,
>>>>>>>>
>>>>>>>> I think directly returning the result inside the catch block changes
>>>>>>>> the logic of the code (the adjustments are not added).
>>>>>>>>
>>>>>>>> Please have another look.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Michael
>>>>>>>>
>>>>>>>>
>>>>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>>>>>>
>>>>>>>>> Author: jleroux
>>>>>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>>>>>> New Revision: 1807240
>>>>>>>>>
>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>>>>>> Log:
>>>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>>>>>>> (OFBIZ-8341)
>>>>>>>>>
>>>>>>>>> In many Java and Groovy files we have auto generated catch blocks
>>>>>>>>> or
>>>>>>>>> empty catch
>>>>>>>>> blocks.
>>>>>>>>> To avoid such exception swallowing this should be improved to at
>>>>>>>>> least log the
>>>>>>>>> error and also return error in case of service.
>>>>>>>>>
>>>>>>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>>>>>>
>>>>>>>>> Modified:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>
>>>>>>>>> Modified:
>>>>>>>>>
>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>> URL:
>>>>>>>>>
>>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ==============================================================================
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>> (original)
>>>>>>>>> +++
>>>>>>>>>
>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>> Mon Sep  4 15:12:23 2017
>>>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>>>>>                    List<GenericValue> workOrderItemFulfillments =
>>>>>>>>> null;
>>>>>>>>>                    try {
>>>>>>>>>                        workOrderItemFulfillments =
>>>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>>>>>>>>> "orderItemSeqId",
>>>>>>>>> orderItem.getString("orderItemSeqId")), null, true);
>>>>>>>>> -                } catch (GenericEntityException e) {}
>>>>>>>>> +                } catch (GenericEntityException e) {
>>>>>>>>> +                    Debug.logError(e, module);
>>>>>>>>> +                    return result;
>>>>>>>>> +                }
>>>>>>>>>                    if (workOrderItemFulfillments != null) {
>>>>>>>>>                        Iterator<GenericValue> iter =
>>>>>>>>> workOrderItemFulfillments.iterator();
>>>>>>>>> -                    if (iter.hasNext())    {
>>>>>>>>> +                    if (iter.hasNext()) {
>>>>>>>>>                            GenericValue WorkOrderItemFulfillment =
>>>>>>>>> iter.next();
>>>>>>>>>                            GenericValue workEffort = null;
>>>>>>>>>                            try {
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>

[Jacques Le Roux]

Le 10/09/2017 à 19:56, Taher Alkhateeb a écrit :

> Inline
>
> On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux
> <[hidden email]> wrote:
>> I easily see 3 alternatives:
>>
>> 1. Swallow the exception, like we currently do. This should be forbidden in
>> all cases, I'd veto that!-
>> 2. Log an error and return a wrong result, like it's currently done by
>> getShippableTotal() in the same class. That's what I proposed, but instead
>> of
>>     returning ZERO, I returned the already calculated result.
>> 3. Throw an error, based on Scott's suggestion in dev ML, like the patch
>> does.
> As I've mentioned numerous time, i don't recommend mass fixes because
> you might be fixing an error with another error. Bulk changes are
> never a good idea because it means you don't know exactly what you're
> doing or how the code is affected. Such changes need to happen slowly
> and carefully. Just because because we have swallowed exceptions does
> not mean we mass fix them.

Thanks for the lesson, but wait! This is not a bulk fix, it's only ONE fix.
And if you look closely at OFBIZ-8341 you will see that I don't handle change there as bulk changes. I review and change each case one by one.
It's not the 1st time you get back to this point? You know what, I got it ;)

Now If you try, like me, to fix it using Scott's proposition of throwing the exception, you will see that you need to throw it from every places I did.
I don't see a way to avoid cascading exceptions in such cases, do you?

> You might trigger other unknown side
> effects without carefully studying every thing.
> My recommendation is to not touch anything, maybe a log message would
> be enough, but not to change anything else without a careful deep look
> at the code.
I proposed logging a message, but reverted on Scott's request. You both need to agree now :)

>> Sincerely I have not strong opinions about 2 and 3. But technically I prefer
>> 3 because the exception must then be handled by the caller (very unlikely to
>> be isolated, we talk about a GenericEntityException here). That's how an API
>> should be written, the exception should pop to the initial topmost caller.
> Not necessarily, the exception propagation strategy is dependent on
> the design, architecture and intent. Exception handling can get real
> nasty real quick if not studied carefully, it will pollute your design
> and make things painful for everyone. Sometimes it makes sense to
> escalate an exception, and sometimes it makes sense to handle it
> immediately, it just depends on the situation.

Yes I agree, and what do you prefer there?

>> If you have another alternative, I'm ready to discuss it.
> Already suggested above
Already done at r1807240 and reverted after Scott's suggestion, see 1st message in this thread.

>
>> Maybe you will suggest to refactor the whole thing (class, classes, etc.)
>> but sincerely then an elaborated plan would be needed beforehand.
>> And in any case it's then an order magnitude more complex that the 2
>> solutions above. I don't say it's impossible, just that I'll maybe not see
>> it :)
> Ahhh, interesting observation, but even I don't know what I would
> suggest next :) I write things when I think them.
I do so, I think we all work like that. But trying to anticipate is also a good thing. Hence my comment at http://markmail.org/message/gme2yztimtf43oso
Let me quote myself again to make it more clear for everybody:

<<"I'll sometimes create subtasks or new Jira issues to differentiate cases that
need to be discussed. It would be good for instance for a type of exception and a type of file
(service, event, helper/handler/test/etc.) to use and adopt a same type of exception handling."

Having patterns would help everybody, when creating, reviewing, refactoring,
etc. >>

I think it's time, and a good opportunity here, to discuss about that. I think a specific thread would be required.
But, like you kindly said, ("I write things when I think them.") it's hard to anticipate, especially when handling previously swallowed exceptions, it
seems.

Jacques

>> Jacques
>>
>>
>>
>> Le 10/09/2017 à 18:22, Taher Alkhateeb a écrit :
>>> What I understand is that the patch is essentially changing the
>>> signature of most methods to throw an exception. On a first glance
>>> this seems to be putting the code in a worst state because now you're
>>> adding complexity for the caller to figure out how to handle these
>>> exceptions.
>>>
>>> What is the purpose of this change? What is the gain?
>>>
>>> On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl <[hidden email]>
>>> wrote:
>>>> Thanks, Jacques.
>>>>
>>>> Regards,
>>>>
>>>> Michael
>>>>
>>>> Am 08.09.17 um 10:54 schrieb Jacques Le Roux:
>>>>
>>>>> No worries Michael,
>>>>>
>>>>> I can wait a week more
>>>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
>>>>>> I disagree, not on the patch itself but on the time frame you give us
>>>>>> to
>>>>>> review and think about it.
>>>>>>
>>>>>> I see no reason to put pressure on this issue.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Michael
>>>>>>
>>>>>>
>>>>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>>>>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit the
>>>>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>>>>>>
>>>>>>> Jacques
>>>>>>>
>>>>>>>
>>>>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>>>>>>> Yes thanks Michael,
>>>>>>>>
>>>>>>>> I agree with Scott about rather throwing an exception
>>>>>>>>
>>>>>>>> Jacques
>>>>>>>>
>>>>>>>>
>>>>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>>>>>>> Hi Jacques,
>>>>>>>>>
>>>>>>>>> I think directly returning the result inside the catch block changes
>>>>>>>>> the logic of the code (the adjustments are not added).
>>>>>>>>>
>>>>>>>>> Please have another look.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Michael
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>>>>>>> Author: jleroux
>>>>>>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>>>>>>> New Revision: 1807240
>>>>>>>>>>
>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>>>>>>> Log:
>>>>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>>>>>>>> (OFBIZ-8341)
>>>>>>>>>>
>>>>>>>>>> In many Java and Groovy files we have auto generated catch blocks
>>>>>>>>>> or
>>>>>>>>>> empty catch
>>>>>>>>>> blocks.
>>>>>>>>>> To avoid such exception swallowing this should be improved to at
>>>>>>>>>> least log the
>>>>>>>>>> error and also return error in case of service.
>>>>>>>>>>
>>>>>>>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>>>>>>>
>>>>>>>>>> Modified:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>
>>>>>>>>>> Modified:
>>>>>>>>>>
>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>> URL:
>>>>>>>>>>
>>>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ==============================================================================
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>> (original)
>>>>>>>>>> +++
>>>>>>>>>>
>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>> Mon Sep  4 15:12:23 2017
>>>>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>>>>>>                     List<GenericValue> workOrderItemFulfillments =
>>>>>>>>>> null;
>>>>>>>>>>                     try {
>>>>>>>>>>                         workOrderItemFulfillments =
>>>>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>>>>>>>>>> "orderItemSeqId",
>>>>>>>>>> orderItem.getString("orderItemSeqId")), null, true);
>>>>>>>>>> -                } catch (GenericEntityException e) {}
>>>>>>>>>> +                } catch (GenericEntityException e) {
>>>>>>>>>> +                    Debug.logError(e, module);
>>>>>>>>>> +                    return result;
>>>>>>>>>> +                }
>>>>>>>>>>                     if (workOrderItemFulfillments != null) {
>>>>>>>>>>                         Iterator<GenericValue> iter =
>>>>>>>>>> workOrderItemFulfillments.iterator();
>>>>>>>>>> -                    if (iter.hasNext())    {
>>>>>>>>>> +                    if (iter.hasNext()) {
>>>>>>>>>>                             GenericValue WorkOrderItemFulfillment =
>>>>>>>>>> iter.next();
>>>>>>>>>>                             GenericValue workEffort = null;
>>>>>>>>>>                             try {
>>>>>>>>>>
>>>>>>>>>>

[taher]

Quoting Scott: "This is often why we can't just bulk fix the issues
reported by static analysis tools, because the fix requires a
thought-out solution that's totally dependent on the context of the
code in question" So I guess him and I are in agreement at least in
this point. Furthermore, to my understanding, the objection from
Michael and Scott in this commit is not the logging, but the returning
of the result which alters the behavior.

So the correct way to handle this issue IMHO is to examine exactly
what calls each one of the methods and then make sure the alteration
satisfies the requirements of the callers. Again quoting Scott:
"Better to put a FIXME tag in if there's no time to properly analyse
the issue." and I second that.

On Mon, Sep 11, 2017 at 10:05 AM, Jacques Le Roux
<[hidden email]> wrote:

>
> Le 10/09/2017 à 19:56, Taher Alkhateeb a écrit :
>>
>> Inline
>>
>> On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux
>> <[hidden email]> wrote:
>>>
>>> I easily see 3 alternatives:
>>>
>>> 1. Swallow the exception, like we currently do. This should be forbidden
>>> in
>>> all cases, I'd veto that!-
>>> 2. Log an error and return a wrong result, like it's currently done by
>>> getShippableTotal() in the same class. That's what I proposed, but
>>> instead
>>> of
>>>     returning ZERO, I returned the already calculated result.
>>> 3. Throw an error, based on Scott's suggestion in dev ML, like the patch
>>> does.
>>
>> As I've mentioned numerous time, i don't recommend mass fixes because
>> you might be fixing an error with another error. Bulk changes are
>> never a good idea because it means you don't know exactly what you're
>> doing or how the code is affected. Such changes need to happen slowly
>> and carefully. Just because because we have swallowed exceptions does
>> not mean we mass fix them.
>
> Thanks for the lesson, but wait! This is not a bulk fix, it's only ONE fix.
> And if you look closely at OFBIZ-8341 you will see that I don't handle
> change there as bulk changes. I review and change each case one by one.
> It's not the 1st time you get back to this point? You know what, I got it ;)
>
> Now If you try, like me, to fix it using Scott's proposition of throwing the
> exception, you will see that you need to throw it from every places I did.
> I don't see a way to avoid cascading exceptions in such cases, do you?
>
>> You might trigger other unknown side
>> effects without carefully studying every thing.
>> My recommendation is to not touch anything, maybe a log message would
>> be enough, but not to change anything else without a careful deep look
>> at the code.
>
> I proposed logging a message, but reverted on Scott's request. You both need
> to agree now :)
>
>>> Sincerely I have not strong opinions about 2 and 3. But technically I
>>> prefer
>>> 3 because the exception must then be handled by the caller (very unlikely
>>> to
>>> be isolated, we talk about a GenericEntityException here). That's how an
>>> API
>>> should be written, the exception should pop to the initial topmost
>>> caller.
>>
>> Not necessarily, the exception propagation strategy is dependent on
>> the design, architecture and intent. Exception handling can get real
>> nasty real quick if not studied carefully, it will pollute your design
>> and make things painful for everyone. Sometimes it makes sense to
>> escalate an exception, and sometimes it makes sense to handle it
>> immediately, it just depends on the situation.
>
> Yes I agree, and what do you prefer there?
>
>>> If you have another alternative, I'm ready to discuss it.
>>
>> Already suggested above
>
> Already done at r1807240 and reverted after Scott's suggestion, see 1st
> message in this thread.
>
>>
>>> Maybe you will suggest to refactor the whole thing (class, classes, etc.)
>>> but sincerely then an elaborated plan would be needed beforehand.
>>> And in any case it's then an order magnitude more complex that the 2
>>> solutions above. I don't say it's impossible, just that I'll maybe not
>>> see
>>> it :)
>>
>> Ahhh, interesting observation, but even I don't know what I would
>> suggest next :) I write things when I think them.
>
> I do so, I think we all work like that. But trying to anticipate is also a
> good thing. Hence my comment at http://markmail.org/message/gme2yztimtf43oso
> Let me quote myself again to make it more clear for everybody:
>
> <<"I'll sometimes create subtasks or new Jira issues to differentiate cases
> that
> need to be discussed. It would be good for instance for a type of exception
> and a type of file
> (service, event, helper/handler/test/etc.) to use and adopt a same type of
> exception handling."
>
> Having patterns would help everybody, when creating, reviewing, refactoring,
> etc. >>
>
> I think it's time, and a good opportunity here, to discuss about that. I
> think a specific thread would be required.
> But, like you kindly said, ("I write things when I think them.") it's hard
> to anticipate, especially when handling previously swallowed exceptions, it
> seems.
>
> Jacques
>
>
>
>>> Jacques
>>>
>>>
>>>
>>> Le 10/09/2017 à 18:22, Taher Alkhateeb a écrit :
>>>>
>>>> What I understand is that the patch is essentially changing the
>>>> signature of most methods to throw an exception. On a first glance
>>>> this seems to be putting the code in a worst state because now you're
>>>> adding complexity for the caller to figure out how to handle these
>>>> exceptions.
>>>>
>>>> What is the purpose of this change? What is the gain?
>>>>
>>>> On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl
>>>> <[hidden email]>
>>>> wrote:
>>>>>
>>>>> Thanks, Jacques.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Michael
>>>>>
>>>>> Am 08.09.17 um 10:54 schrieb Jacques Le Roux:
>>>>>
>>>>>> No worries Michael,
>>>>>>
>>>>>> I can wait a week more
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>>
>>>>>> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
>>>>>>>
>>>>>>> I disagree, not on the patch itself but on the time frame you give us
>>>>>>> to
>>>>>>> review and think about it.
>>>>>>>
>>>>>>> I see no reason to put pressure on this issue.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Michael
>>>>>>>
>>>>>>>
>>>>>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>>>>>>>>
>>>>>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit the
>>>>>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>>>>>>>
>>>>>>>> Jacques
>>>>>>>>
>>>>>>>>
>>>>>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>>>>>>>>
>>>>>>>>> Yes thanks Michael,
>>>>>>>>>
>>>>>>>>> I agree with Scott about rather throwing an exception
>>>>>>>>>
>>>>>>>>> Jacques
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>>>>>>>>
>>>>>>>>>> Hi Jacques,
>>>>>>>>>>
>>>>>>>>>> I think directly returning the result inside the catch block
>>>>>>>>>> changes
>>>>>>>>>> the logic of the code (the adjustments are not added).
>>>>>>>>>>
>>>>>>>>>> Please have another look.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Michael
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>>>>>>>>
>>>>>>>>>>> Author: jleroux
>>>>>>>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>>>>>>>> New Revision: 1807240
>>>>>>>>>>>
>>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>>>>>>>> Log:
>>>>>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>>>>>>>>> (OFBIZ-8341)
>>>>>>>>>>>
>>>>>>>>>>> In many Java and Groovy files we have auto generated catch blocks
>>>>>>>>>>> or
>>>>>>>>>>> empty catch
>>>>>>>>>>> blocks.
>>>>>>>>>>> To avoid such exception swallowing this should be improved to at
>>>>>>>>>>> least log the
>>>>>>>>>>> error and also return error in case of service.
>>>>>>>>>>>
>>>>>>>>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>>>>>>>>
>>>>>>>>>>> Modified:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>
>>>>>>>>>>> Modified:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>> URL:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> ==============================================================================
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>> (original)
>>>>>>>>>>> +++
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>> Mon Sep  4 15:12:23 2017
>>>>>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>>>>>>>                     List<GenericValue> workOrderItemFulfillments
>>>>>>>>>>> =
>>>>>>>>>>> null;
>>>>>>>>>>>                     try {
>>>>>>>>>>>                         workOrderItemFulfillments =
>>>>>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>>>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>>>>>>>>>>> "orderItemSeqId",
>>>>>>>>>>> orderItem.getString("orderItemSeqId")), null, true);
>>>>>>>>>>> -                } catch (GenericEntityException e) {}
>>>>>>>>>>> +                } catch (GenericEntityException e) {
>>>>>>>>>>> +                    Debug.logError(e, module);
>>>>>>>>>>> +                    return result;
>>>>>>>>>>> +                }
>>>>>>>>>>>                     if (workOrderItemFulfillments != null) {
>>>>>>>>>>>                         Iterator<GenericValue> iter =
>>>>>>>>>>> workOrderItemFulfillments.iterator();
>>>>>>>>>>> -                    if (iter.hasNext())    {
>>>>>>>>>>> +                    if (iter.hasNext()) {
>>>>>>>>>>>                             GenericValue WorkOrderItemFulfillment
>>>>>>>>>>> =
>>>>>>>>>>> iter.next();
>>>>>>>>>>>                             GenericValue workEffort = null;
>>>>>>>>>>>                             try {
>>>>>>>>>>>
>>>>>>>>>>>
>

[Scott Gray-3]

Since this is such a complex problem perhaps we better come up with a best
practice since we have so many Helper/Worker/Reader classes in OFBiz and a
bit of API consistency goes a long way.

If a GenericEntityException is thrown within a Helper/Worker/Reader class
method, how should we handle it?

Options:
- Swallow it and return an incorrect value (current situation)
- Log the exception then swallow it and return an incorrect value (Jacques'
reverted fix)
- Add a throws declaration and let the caller decide if and how to handle it
- Log the exception and return null
- Re-throw as an unchecked exception such as an IllegalArgumentException
(this is already done in the constructors of that class)

IMO we currently have the worst of the options in place.  If a method
encounters an issue that prevents it from fulfilling it's function then the
caller should be made aware of that fact, either by throwing an exception
or at a minimum by returning null.  Only the caller knows what it wanted
the return value for, I don't see how handling an error is too much
complexity for it.  Maybe for the caller the value was just a nice-to-have
or maybe it was critical to get the correct value, either way I'm certain
the caller doesn't want an incorrect value masquerading as correct.

We have lots of Worker/Reader/Helper classes that have to deal with this
situation and they aren't so unique or complex that we can't come up with a
best practice for how they should be handled.  I don't generally like bulk
fixes but I do like some consistency within our APIs.

If you're willing to put energy into this Jacques, it would be good to get
the current "lay of the land" in terms of how all of our other helper
classes manage this problem.

Regards
Scott

On 11 September 2017 at 20:28, Taher Alkhateeb <[hidden email]>
wrote:

> Quoting Scott: "This is often why we can't just bulk fix the issues
> reported by static analysis tools, because the fix requires a
> thought-out solution that's totally dependent on the context of the
> code in question" So I guess him and I are in agreement at least in
> this point. Furthermore, to my understanding, the objection from
> Michael and Scott in this commit is not the logging, but the returning
> of the result which alters the behavior.
>
> So the correct way to handle this issue IMHO is to examine exactly
> what calls each one of the methods and then make sure the alteration
> satisfies the requirements of the callers. Again quoting Scott:
> "Better to put a FIXME tag in if there's no time to properly analyse
> the issue." and I second that.
>
> On Mon, Sep 11, 2017 at 10:05 AM, Jacques Le Roux
> <[hidden email]> wrote:
> >
> > Le 10/09/2017 à 19:56, Taher Alkhateeb a écrit :
> >>
> >> Inline
> >>
> >> On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux
> >> <[hidden email]> wrote:
> >>>
> >>> I easily see 3 alternatives:
> >>>
> >>> 1. Swallow the exception, like we currently do. This should be
> forbidden
> >>> in
> >>> all cases, I'd veto that!-
> >>> 2. Log an error and return a wrong result, like it's currently done by
> >>> getShippableTotal() in the same class. That's what I proposed, but
> >>> instead
> >>> of
> >>>     returning ZERO, I returned the already calculated result.
> >>> 3. Throw an error, based on Scott's suggestion in dev ML, like the
> patch
> >>> does.
> >>
> >> As I've mentioned numerous time, i don't recommend mass fixes because
> >> you might be fixing an error with another error. Bulk changes are
> >> never a good idea because it means you don't know exactly what you're
> >> doing or how the code is affected. Such changes need to happen slowly
> >> and carefully. Just because because we have swallowed exceptions does
> >> not mean we mass fix them.
> >
> > Thanks for the lesson, but wait! This is not a bulk fix, it's only ONE
> fix.
> > And if you look closely at OFBIZ-8341 you will see that I don't handle
> > change there as bulk changes. I review and change each case one by one.
> > It's not the 1st time you get back to this point? You know what, I got
> it ;)
> >
> > Now If you try, like me, to fix it using Scott's proposition of throwing
> the
> > exception, you will see that you need to throw it from every places I
> did.
> > I don't see a way to avoid cascading exceptions in such cases, do you?
> >
> >> You might trigger other unknown side
> >> effects without carefully studying every thing.
> >> My recommendation is to not touch anything, maybe a log message would
> >> be enough, but not to change anything else without a careful deep look
> >> at the code.
> >
> > I proposed logging a message, but reverted on Scott's request. You both
> need
> > to agree now :)
> >
> >>> Sincerely I have not strong opinions about 2 and 3. But technically I
> >>> prefer
> >>> 3 because the exception must then be handled by the caller (very
> unlikely
> >>> to
> >>> be isolated, we talk about a GenericEntityException here). That's how
> an
> >>> API
> >>> should be written, the exception should pop to the initial topmost
> >>> caller.
> >>
> >> Not necessarily, the exception propagation strategy is dependent on
> >> the design, architecture and intent. Exception handling can get real
> >> nasty real quick if not studied carefully, it will pollute your design
> >> and make things painful for everyone. Sometimes it makes sense to
> >> escalate an exception, and sometimes it makes sense to handle it
> >> immediately, it just depends on the situation.
> >
> > Yes I agree, and what do you prefer there?
> >
> >>> If you have another alternative, I'm ready to discuss it.
> >>
> >> Already suggested above
> >
> > Already done at r1807240 and reverted after Scott's suggestion, see 1st
> > message in this thread.
> >
> >>
> >>> Maybe you will suggest to refactor the whole thing (class, classes,
> etc.)
> >>> but sincerely then an elaborated plan would be needed beforehand.
> >>> And in any case it's then an order magnitude more complex that the 2
> >>> solutions above. I don't say it's impossible, just that I'll maybe not
> >>> see
> >>> it :)
> >>
> >> Ahhh, interesting observation, but even I don't know what I would
> >> suggest next :) I write things when I think them.
> >
> > I do so, I think we all work like that. But trying to anticipate is also
> a
> > good thing. Hence my comment at http://markmail.org/message/
> gme2yztimtf43oso
> > Let me quote myself again to make it more clear for everybody:
> >
> > <<"I'll sometimes create subtasks or new Jira issues to differentiate
> cases
> > that
> > need to be discussed. It would be good for instance for a type of
> exception
> > and a type of file
> > (service, event, helper/handler/test/etc.) to use and adopt a same type
> of
> > exception handling."
> >
> > Having patterns would help everybody, when creating, reviewing,
> refactoring,
> > etc. >>
> >
> > I think it's time, and a good opportunity here, to discuss about that. I
> > think a specific thread would be required.
> > But, like you kindly said, ("I write things when I think them.") it's
> hard
> > to anticipate, especially when handling previously swallowed exceptions,
> it
> > seems.
> >
> > Jacques
> >
> >
> >
> >>> Jacques
> >>>
> >>>
> >>>
> >>> Le 10/09/2017 à 18:22, Taher Alkhateeb a écrit :
> >>>>
> >>>> What I understand is that the patch is essentially changing the
> >>>> signature of most methods to throw an exception. On a first glance
> >>>> this seems to be putting the code in a worst state because now you're
> >>>> adding complexity for the caller to figure out how to handle these
> >>>> exceptions.
> >>>>
> >>>> What is the purpose of this change? What is the gain?
> >>>>
> >>>> On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl
> >>>> <[hidden email]>
> >>>> wrote:
> >>>>>
> >>>>> Thanks, Jacques.
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>> Michael
> >>>>>
> >>>>> Am 08.09.17 um 10:54 schrieb Jacques Le Roux:
> >>>>>
> >>>>>> No worries Michael,
> >>>>>>
> >>>>>> I can wait a week more
> >>>>>>
> >>>>>> Jacques
> >>>>>>
> >>>>>>
> >>>>>> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
> >>>>>>>
> >>>>>>> I disagree, not on the patch itself but on the time frame you give
> us
> >>>>>>> to
> >>>>>>> review and think about it.
> >>>>>>>
> >>>>>>> I see no reason to put pressure on this issue.
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>>
> >>>>>>> Michael
> >>>>>>>
> >>>>>>>
> >>>>>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
> >>>>>>>>
> >>>>>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit
> the
> >>>>>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
> >>>>>>>>
> >>>>>>>> Jacques
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
> >>>>>>>>>
> >>>>>>>>> Yes thanks Michael,
> >>>>>>>>>
> >>>>>>>>> I agree with Scott about rather throwing an exception
> >>>>>>>>>
> >>>>>>>>> Jacques
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
> >>>>>>>>>>
> >>>>>>>>>> Hi Jacques,
> >>>>>>>>>>
> >>>>>>>>>> I think directly returning the result inside the catch block
> >>>>>>>>>> changes
> >>>>>>>>>> the logic of the code (the adjustments are not added).
> >>>>>>>>>>
> >>>>>>>>>> Please have another look.
> >>>>>>>>>>
> >>>>>>>>>> Thanks,
> >>>>>>>>>>
> >>>>>>>>>> Michael
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
> >>>>>>>>>>>
> >>>>>>>>>>> Author: jleroux
> >>>>>>>>>>> Date: Mon Sep  4 15:12:23 2017
> >>>>>>>>>>> New Revision: 1807240
> >>>>>>>>>>>
> >>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
> >>>>>>>>>>> Log:
> >>>>>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy
> files
> >>>>>>>>>>> (OFBIZ-8341)
> >>>>>>>>>>>
> >>>>>>>>>>> In many Java and Groovy files we have auto generated catch
> blocks
> >>>>>>>>>>> or
> >>>>>>>>>>> empty catch
> >>>>>>>>>>> blocks.
> >>>>>>>>>>> To avoid such exception swallowing this should be improved to
> at
> >>>>>>>>>>> least log the
> >>>>>>>>>>> error and also return error in case of service.
> >>>>>>>>>>>
> >>>>>>>>>>> jleroux: I can't see what we could do more here, unlikely
> anyway
> >>>>>>>>>>>
> >>>>>>>>>>> Modified:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> java/org/apache/ofbiz/order/order/OrderReadHelper.java
> >>>>>>>>>>>
> >>>>>>>>>>> Modified:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> java/org/apache/ofbiz/order/order/OrderReadHelper.java
> >>>>>>>>>>> URL:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> applications/order/src/main/java/org/apache/ofbiz/order/
> order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> ============================================================
> ==================
> >>>>>>>>>>> ---
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> java/org/apache/ofbiz/order/order/OrderReadHelper.java
> >>>>>>>>>>> (original)
> >>>>>>>>>>> +++
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
> java/org/apache/ofbiz/order/order/OrderReadHelper.java
> >>>>>>>>>>> Mon Sep  4 15:12:23 2017
> >>>>>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
> >>>>>>>>>>>                     List<GenericValue>
> workOrderItemFulfillments
> >>>>>>>>>>> =
> >>>>>>>>>>> null;
> >>>>>>>>>>>                     try {
> >>>>>>>>>>>                         workOrderItemFulfillments =
> >>>>>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
> >>>>>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
> >>>>>>>>>>> "orderItemSeqId",
> >>>>>>>>>>> orderItem.getString("orderItemSeqId")), null, true);
> >>>>>>>>>>> -                } catch (GenericEntityException e) {}
> >>>>>>>>>>> +                } catch (GenericEntityException e) {
> >>>>>>>>>>> +                    Debug.logError(e, module);
> >>>>>>>>>>> +                    return result;
> >>>>>>>>>>> +                }
> >>>>>>>>>>>                     if (workOrderItemFulfillments != null) {
> >>>>>>>>>>>                         Iterator<GenericValue> iter =
> >>>>>>>>>>> workOrderItemFulfillments.iterator();
> >>>>>>>>>>> -                    if (iter.hasNext())    {
> >>>>>>>>>>> +                    if (iter.hasNext()) {
> >>>>>>>>>>>                             GenericValue
> WorkOrderItemFulfillment
> >>>>>>>>>>> =
> >>>>>>>>>>> iter.next();
> >>>>>>>>>>>                             GenericValue workEffort = null;
> >>>>>>>>>>>                             try {
> >>>>>>>>>>>
> >>>>>>>>>>>
> >
>

[Jacques Le Roux]

Le 11/09/2017 à 10:28, Taher Alkhateeb a écrit :
> Quoting Scott: "This is often why we can't just bulk fix the issues
> reported by static analysis tools, because the fix requires a
> thought-out solution that's totally dependent on the context of the
> code in question" So I guess him and I are in agreement at least in
> this point.
I also agree with you and (again) I'm not using a static analysis tool.
As I said in the Jira (please check) I simply look for "catch (*) {}" and fix them one by one, we have still 80 at the moment...

> Furthermore, to my understanding, the objection from
> Michael and Scott in this commit is not the logging, but the returning
> of the result which alters the behavior.
I already spoke about that. While at it, what would be the better return to do?
Using a ZERO (as done in getShippableTotal()) is not better IMO.
So I decided to log and return the value at the point of the exception.
What would you suggest? I still believe throwing an exception is better because then the uppermost caller knows something is wrong.

> So the correct way to handle this issue IMHO is to examine exactly
> what calls each one of the methods and then make sure the alteration
> satisfies the requirements of the callers. Again quoting Scott:
> "Better to put a FIXME tag in if there's no time to properly analyse
> the issue." and I second that.
I'm working on OFBIZ-8341 for months. We are on this one, why delay it more? Just put in in the limbo ?

Jacques

>
> On Mon, Sep 11, 2017 at 10:05 AM, Jacques Le Roux
> <[hidden email]> wrote:
>> Le 10/09/2017 à 19:56, Taher Alkhateeb a écrit :
>>> Inline
>>>
>>> On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux
>>> <[hidden email]> wrote:
>>>> I easily see 3 alternatives:
>>>>
>>>> 1. Swallow the exception, like we currently do. This should be forbidden
>>>> in
>>>> all cases, I'd veto that!-
>>>> 2. Log an error and return a wrong result, like it's currently done by
>>>> getShippableTotal() in the same class. That's what I proposed, but
>>>> instead
>>>> of
>>>>      returning ZERO, I returned the already calculated result.
>>>> 3. Throw an error, based on Scott's suggestion in dev ML, like the patch
>>>> does.
>>> As I've mentioned numerous time, i don't recommend mass fixes because
>>> you might be fixing an error with another error. Bulk changes are
>>> never a good idea because it means you don't know exactly what you're
>>> doing or how the code is affected. Such changes need to happen slowly
>>> and carefully. Just because because we have swallowed exceptions does
>>> not mean we mass fix them.
>> Thanks for the lesson, but wait! This is not a bulk fix, it's only ONE fix.
>> And if you look closely at OFBIZ-8341 you will see that I don't handle
>> change there as bulk changes. I review and change each case one by one.
>> It's not the 1st time you get back to this point? You know what, I got it ;)
>>
>> Now If you try, like me, to fix it using Scott's proposition of throwing the
>> exception, you will see that you need to throw it from every places I did.
>> I don't see a way to avoid cascading exceptions in such cases, do you?
>>
>>> You might trigger other unknown side
>>> effects without carefully studying every thing.
>>> My recommendation is to not touch anything, maybe a log message would
>>> be enough, but not to change anything else without a careful deep look
>>> at the code.
>> I proposed logging a message, but reverted on Scott's request. You both need
>> to agree now :)
>>
>>>> Sincerely I have not strong opinions about 2 and 3. But technically I
>>>> prefer
>>>> 3 because the exception must then be handled by the caller (very unlikely
>>>> to
>>>> be isolated, we talk about a GenericEntityException here). That's how an
>>>> API
>>>> should be written, the exception should pop to the initial topmost
>>>> caller.
>>> Not necessarily, the exception propagation strategy is dependent on
>>> the design, architecture and intent. Exception handling can get real
>>> nasty real quick if not studied carefully, it will pollute your design
>>> and make things painful for everyone. Sometimes it makes sense to
>>> escalate an exception, and sometimes it makes sense to handle it
>>> immediately, it just depends on the situation.
>> Yes I agree, and what do you prefer there?
>>
>>>> If you have another alternative, I'm ready to discuss it.
>>> Already suggested above
>> Already done at r1807240 and reverted after Scott's suggestion, see 1st
>> message in this thread.
>>
>>>> Maybe you will suggest to refactor the whole thing (class, classes, etc.)
>>>> but sincerely then an elaborated plan would be needed beforehand.
>>>> And in any case it's then an order magnitude more complex that the 2
>>>> solutions above. I don't say it's impossible, just that I'll maybe not
>>>> see
>>>> it :)
>>> Ahhh, interesting observation, but even I don't know what I would
>>> suggest next :) I write things when I think them.
>> I do so, I think we all work like that. But trying to anticipate is also a
>> good thing. Hence my comment at http://markmail.org/message/gme2yztimtf43oso
>> Let me quote myself again to make it more clear for everybody:
>>
>> <<"I'll sometimes create subtasks or new Jira issues to differentiate cases
>> that
>> need to be discussed. It would be good for instance for a type of exception
>> and a type of file
>> (service, event, helper/handler/test/etc.) to use and adopt a same type of
>> exception handling."
>>
>> Having patterns would help everybody, when creating, reviewing, refactoring,
>> etc. >>
>>
>> I think it's time, and a good opportunity here, to discuss about that. I
>> think a specific thread would be required.
>> But, like you kindly said, ("I write things when I think them.") it's hard
>> to anticipate, especially when handling previously swallowed exceptions, it
>> seems.
>>
>> Jacques
>>
>>
>>
>>>> Jacques
>>>>
>>>>
>>>>
>>>> Le 10/09/2017 à 18:22, Taher Alkhateeb a écrit :
>>>>> What I understand is that the patch is essentially changing the
>>>>> signature of most methods to throw an exception. On a first glance
>>>>> this seems to be putting the code in a worst state because now you're
>>>>> adding complexity for the caller to figure out how to handle these
>>>>> exceptions.
>>>>>
>>>>> What is the purpose of this change? What is the gain?
>>>>>
>>>>> On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl
>>>>> <[hidden email]>
>>>>> wrote:
>>>>>> Thanks, Jacques.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Michael
>>>>>>
>>>>>> Am 08.09.17 um 10:54 schrieb Jacques Le Roux:
>>>>>>
>>>>>>> No worries Michael,
>>>>>>>
>>>>>>> I can wait a week more
>>>>>>>
>>>>>>> Jacques
>>>>>>>
>>>>>>>
>>>>>>> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
>>>>>>>> I disagree, not on the patch itself but on the time frame you give us
>>>>>>>> to
>>>>>>>> review and think about it.
>>>>>>>>
>>>>>>>> I see no reason to put pressure on this issue.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>>
>>>>>>>> Michael
>>>>>>>>
>>>>>>>>
>>>>>>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>>>>>>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit the
>>>>>>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>>>>>>>>
>>>>>>>>> Jacques
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>>>>>>>>> Yes thanks Michael,
>>>>>>>>>>
>>>>>>>>>> I agree with Scott about rather throwing an exception
>>>>>>>>>>
>>>>>>>>>> Jacques
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>>>>>>>>> Hi Jacques,
>>>>>>>>>>>
>>>>>>>>>>> I think directly returning the result inside the catch block
>>>>>>>>>>> changes
>>>>>>>>>>> the logic of the code (the adjustments are not added).
>>>>>>>>>>>
>>>>>>>>>>> Please have another look.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Michael
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>>>>>>>>> Author: jleroux
>>>>>>>>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>>>>>>>>> New Revision: 1807240
>>>>>>>>>>>>
>>>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>>>>>>>>> Log:
>>>>>>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>>>>>>>>>>> (OFBIZ-8341)
>>>>>>>>>>>>
>>>>>>>>>>>> In many Java and Groovy files we have auto generated catch blocks
>>>>>>>>>>>> or
>>>>>>>>>>>> empty catch
>>>>>>>>>>>> blocks.
>>>>>>>>>>>> To avoid such exception swallowing this should be improved to at
>>>>>>>>>>>> least log the
>>>>>>>>>>>> error and also return error in case of service.
>>>>>>>>>>>>
>>>>>>>>>>>> jleroux: I can't see what we could do more here, unlikely anyway
>>>>>>>>>>>>
>>>>>>>>>>>> Modified:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>>
>>>>>>>>>>>> Modified:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>> URL:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ==============================================================================
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>> (original)
>>>>>>>>>>>> +++
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>> Mon Sep  4 15:12:23 2017
>>>>>>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>>>>>>>>                      List<GenericValue> workOrderItemFulfillments
>>>>>>>>>>>> =
>>>>>>>>>>>> null;
>>>>>>>>>>>>                      try {
>>>>>>>>>>>>                          workOrderItemFulfillments =
>>>>>>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>>>>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>>>>>>>>>>>> "orderItemSeqId",
>>>>>>>>>>>> orderItem.getString("orderItemSeqId")), null, true);
>>>>>>>>>>>> -                } catch (GenericEntityException e) {}
>>>>>>>>>>>> +                } catch (GenericEntityException e) {
>>>>>>>>>>>> +                    Debug.logError(e, module);
>>>>>>>>>>>> +                    return result;
>>>>>>>>>>>> +                }
>>>>>>>>>>>>                      if (workOrderItemFulfillments != null) {
>>>>>>>>>>>>                          Iterator<GenericValue> iter =
>>>>>>>>>>>> workOrderItemFulfillments.iterator();
>>>>>>>>>>>> -                    if (iter.hasNext())    {
>>>>>>>>>>>> +                    if (iter.hasNext()) {
>>>>>>>>>>>>                              GenericValue WorkOrderItemFulfillment
>>>>>>>>>>>> =
>>>>>>>>>>>> iter.next();
>>>>>>>>>>>>                              GenericValue workEffort = null;
>>>>>>>>>>>>                              try {
>>>>>>>>>>>>
>>>>>>>>>>>>

[Jacques Le Roux]

Le 11/09/2017 à 10:34, Scott Gray a écrit :

> Since this is such a complex problem perhaps we better come up with a best
> practice since we have so many Helper/Worker/Reader classes in OFBiz and a
> bit of API consistency goes a long way.
>
> If a GenericEntityException is thrown within a Helper/Worker/Reader class
> method, how should we handle it?
>
> Options:
> - Swallow it and return an incorrect value (current situation)
> - Log the exception then swallow it and return an incorrect value (Jacques'
> reverted fix)
> - Add a throws declaration and let the caller decide if and how to handle it
> - Log the exception and return null
> - Re-throw as an unchecked exception such as an IllegalArgumentException
> (this is already done in the constructors of that class)
>
> IMO we currently have the worst of the options in place.  If a method
> encounters an issue that prevents it from fulfilling it's function then the
> caller should be made aware of that fact, either by throwing an exception
> or at a minimum by returning null.  Only the caller knows what it wanted
> the return value for, I don't see how handling an error is too much
> complexity for it.  Maybe for the caller the value was just a nice-to-have
> or maybe it was critical to get the correct value, either way I'm certain
> the caller doesn't want an incorrect value masquerading as correct.
>
> We have lots of Worker/Reader/Helper classes that have to deal with this
> situation and they aren't so unique or complex that we can't come up with a
> best practice for how they should be handled.  I don't generally like bulk
> fixes but I do like some consistency within our APIs.
>
> If you're willing to put energy into this Jacques, it would be good to get
> the current "lay of the land" in terms of how all of our other helper
> classes manage this problem.

Thanks Scott,

That's exactly the type of answer I called for. I'll get deeper in this

Jacques

>
> Regards
> Scott
>
> On 11 September 2017 at 20:28, Taher Alkhateeb <[hidden email]>
> wrote:
>
>> Quoting Scott: "This is often why we can't just bulk fix the issues
>> reported by static analysis tools, because the fix requires a
>> thought-out solution that's totally dependent on the context of the
>> code in question" So I guess him and I are in agreement at least in
>> this point. Furthermore, to my understanding, the objection from
>> Michael and Scott in this commit is not the logging, but the returning
>> of the result which alters the behavior.
>>
>> So the correct way to handle this issue IMHO is to examine exactly
>> what calls each one of the methods and then make sure the alteration
>> satisfies the requirements of the callers. Again quoting Scott:
>> "Better to put a FIXME tag in if there's no time to properly analyse
>> the issue." and I second that.
>>
>> On Mon, Sep 11, 2017 at 10:05 AM, Jacques Le Roux
>> <[hidden email]> wrote:
>>> Le 10/09/2017 à 19:56, Taher Alkhateeb a écrit :
>>>> Inline
>>>>
>>>> On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux
>>>> <[hidden email]> wrote:
>>>>> I easily see 3 alternatives:
>>>>>
>>>>> 1. Swallow the exception, like we currently do. This should be
>> forbidden
>>>>> in
>>>>> all cases, I'd veto that!-
>>>>> 2. Log an error and return a wrong result, like it's currently done by
>>>>> getShippableTotal() in the same class. That's what I proposed, but
>>>>> instead
>>>>> of
>>>>>      returning ZERO, I returned the already calculated result.
>>>>> 3. Throw an error, based on Scott's suggestion in dev ML, like the
>> patch
>>>>> does.
>>>> As I've mentioned numerous time, i don't recommend mass fixes because
>>>> you might be fixing an error with another error. Bulk changes are
>>>> never a good idea because it means you don't know exactly what you're
>>>> doing or how the code is affected. Such changes need to happen slowly
>>>> and carefully. Just because because we have swallowed exceptions does
>>>> not mean we mass fix them.
>>> Thanks for the lesson, but wait! This is not a bulk fix, it's only ONE
>> fix.
>>> And if you look closely at OFBIZ-8341 you will see that I don't handle
>>> change there as bulk changes. I review and change each case one by one.
>>> It's not the 1st time you get back to this point? You know what, I got
>> it ;)
>>> Now If you try, like me, to fix it using Scott's proposition of throwing
>> the
>>> exception, you will see that you need to throw it from every places I
>> did.
>>> I don't see a way to avoid cascading exceptions in such cases, do you?
>>>
>>>> You might trigger other unknown side
>>>> effects without carefully studying every thing.
>>>> My recommendation is to not touch anything, maybe a log message would
>>>> be enough, but not to change anything else without a careful deep look
>>>> at the code.
>>> I proposed logging a message, but reverted on Scott's request. You both
>> need
>>> to agree now :)
>>>
>>>>> Sincerely I have not strong opinions about 2 and 3. But technically I
>>>>> prefer
>>>>> 3 because the exception must then be handled by the caller (very
>> unlikely
>>>>> to
>>>>> be isolated, we talk about a GenericEntityException here). That's how
>> an
>>>>> API
>>>>> should be written, the exception should pop to the initial topmost
>>>>> caller.
>>>> Not necessarily, the exception propagation strategy is dependent on
>>>> the design, architecture and intent. Exception handling can get real
>>>> nasty real quick if not studied carefully, it will pollute your design
>>>> and make things painful for everyone. Sometimes it makes sense to
>>>> escalate an exception, and sometimes it makes sense to handle it
>>>> immediately, it just depends on the situation.
>>> Yes I agree, and what do you prefer there?
>>>
>>>>> If you have another alternative, I'm ready to discuss it.
>>>> Already suggested above
>>> Already done at r1807240 and reverted after Scott's suggestion, see 1st
>>> message in this thread.
>>>
>>>>> Maybe you will suggest to refactor the whole thing (class, classes,
>> etc.)
>>>>> but sincerely then an elaborated plan would be needed beforehand.
>>>>> And in any case it's then an order magnitude more complex that the 2
>>>>> solutions above. I don't say it's impossible, just that I'll maybe not
>>>>> see
>>>>> it :)
>>>> Ahhh, interesting observation, but even I don't know what I would
>>>> suggest next :) I write things when I think them.
>>> I do so, I think we all work like that. But trying to anticipate is also
>> a
>>> good thing. Hence my comment at http://markmail.org/message/
>> gme2yztimtf43oso
>>> Let me quote myself again to make it more clear for everybody:
>>>
>>> <<"I'll sometimes create subtasks or new Jira issues to differentiate
>> cases
>>> that
>>> need to be discussed. It would be good for instance for a type of
>> exception
>>> and a type of file
>>> (service, event, helper/handler/test/etc.) to use and adopt a same type
>> of
>>> exception handling."
>>>
>>> Having patterns would help everybody, when creating, reviewing,
>> refactoring,
>>> etc. >>
>>>
>>> I think it's time, and a good opportunity here, to discuss about that. I
>>> think a specific thread would be required.
>>> But, like you kindly said, ("I write things when I think them.") it's
>> hard
>>> to anticipate, especially when handling previously swallowed exceptions,
>> it
>>> seems.
>>>
>>> Jacques
>>>
>>>
>>>
>>>>> Jacques
>>>>>
>>>>>
>>>>>
>>>>> Le 10/09/2017 à 18:22, Taher Alkhateeb a écrit :
>>>>>> What I understand is that the patch is essentially changing the
>>>>>> signature of most methods to throw an exception. On a first glance
>>>>>> this seems to be putting the code in a worst state because now you're
>>>>>> adding complexity for the caller to figure out how to handle these
>>>>>> exceptions.
>>>>>>
>>>>>> What is the purpose of this change? What is the gain?
>>>>>>
>>>>>> On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl
>>>>>> <[hidden email]>
>>>>>> wrote:
>>>>>>> Thanks, Jacques.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Michael
>>>>>>>
>>>>>>> Am 08.09.17 um 10:54 schrieb Jacques Le Roux:
>>>>>>>
>>>>>>>> No worries Michael,
>>>>>>>>
>>>>>>>> I can wait a week more
>>>>>>>>
>>>>>>>> Jacques
>>>>>>>>
>>>>>>>>
>>>>>>>> Le 08/09/2017 à 10:42, Michael Brohl a écrit :
>>>>>>>>> I disagree, not on the patch itself but on the time frame you give
>> us
>>>>>>>>> to
>>>>>>>>> review and think about it.
>>>>>>>>>
>>>>>>>>> I see no reason to put pressure on this issue.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>>
>>>>>>>>> Michael
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux:
>>>>>>>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit
>> the
>>>>>>>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend
>>>>>>>>>>
>>>>>>>>>> Jacques
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit :
>>>>>>>>>>> Yes thanks Michael,
>>>>>>>>>>>
>>>>>>>>>>> I agree with Scott about rather throwing an exception
>>>>>>>>>>>
>>>>>>>>>>> Jacques
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit :
>>>>>>>>>>>> Hi Jacques,
>>>>>>>>>>>>
>>>>>>>>>>>> I think directly returning the result inside the catch block
>>>>>>>>>>>> changes
>>>>>>>>>>>> the logic of the code (the adjustments are not added).
>>>>>>>>>>>>
>>>>>>>>>>>> Please have another look.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Michael
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Am 04.09.17 um 17:12 schrieb [hidden email]:
>>>>>>>>>>>>> Author: jleroux
>>>>>>>>>>>>> Date: Mon Sep  4 15:12:23 2017
>>>>>>>>>>>>> New Revision: 1807240
>>>>>>>>>>>>>
>>>>>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev
>>>>>>>>>>>>> Log:
>>>>>>>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy
>> files
>>>>>>>>>>>>> (OFBIZ-8341)
>>>>>>>>>>>>>
>>>>>>>>>>>>> In many Java and Groovy files we have auto generated catch
>> blocks
>>>>>>>>>>>>> or
>>>>>>>>>>>>> empty catch
>>>>>>>>>>>>> blocks.
>>>>>>>>>>>>> To avoid such exception swallowing this should be improved to
>> at
>>>>>>>>>>>>> least log the
>>>>>>>>>>>>> error and also return error in case of service.
>>>>>>>>>>>>>
>>>>>>>>>>>>> jleroux: I can't see what we could do more here, unlikely
>> anyway
>>>>>>>>>>>>> Modified:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>> java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>>> Modified:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>> java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>>> URL:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>> applications/order/src/main/java/org/apache/ofbiz/order/
>> order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ============================================================
>> ==================
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>> java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>>> (original)
>>>>>>>>>>>>> +++
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/
>> java/org/apache/ofbiz/order/order/OrderReadHelper.java
>>>>>>>>>>>>> Mon Sep  4 15:12:23 2017
>>>>>>>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper {
>>>>>>>>>>>>>                      List<GenericValue>
>> workOrderItemFulfillments
>>>>>>>>>>>>> =
>>>>>>>>>>>>> null;
>>>>>>>>>>>>>                      try {
>>>>>>>>>>>>>                          workOrderItemFulfillments =
>>>>>>>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment",
>>>>>>>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"),
>>>>>>>>>>>>> "orderItemSeqId",
>>>>>>>>>>>>> orderItem.getString("orderItemSeqId")), null, true);
>>>>>>>>>>>>> -                } catch (GenericEntityException e) {}
>>>>>>>>>>>>> +                } catch (GenericEntityException e) {
>>>>>>>>>>>>> +                    Debug.logError(e, module);
>>>>>>>>>>>>> +                    return result;
>>>>>>>>>>>>> +                }
>>>>>>>>>>>>>                      if (workOrderItemFulfillments != null) {
>>>>>>>>>>>>>                          Iterator<GenericValue> iter =
>>>>>>>>>>>>> workOrderItemFulfillments.iterator();
>>>>>>>>>>>>> -                    if (iter.hasNext())    {
>>>>>>>>>>>>> +                    if (iter.hasNext()) {
>>>>>>>>>>>>>                              GenericValue
>> WorkOrderItemFulfillment
>>>>>>>>>>>>> =
>>>>>>>>>>>>> iter.next();
>>>>>>>>>>>>>                              GenericValue workEffort = null;
>>>>>>>>>>>>>                              try {
>>>>>>>>>>>>>
>>>>>>>>>>>>>