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 <jacques.le.r...@les7arts.com> wrote: > > Le 10/09/2017 à 19:56, Taher Alkhateeb a écrit : >> >> Inline >> >> On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux >> <jacques.le.r...@les7arts.com> 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 >>>> <michael.br...@ecomify.de> >>>> 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 jler...@apache.org: >>>>>>>>>>> >>>>>>>>>>> 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 { >>>>>>>>>>> >>>>>>>>>>> >
