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 {
