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 {
