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 <michael.br...@ecomify.de>
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 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/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 {
