Hi Jacopo,
Thanks for your review. I understand your concerns but IMO if an exception has a good reason to be swallowed then this reason should be documented by
a comment in the catch. Else how can we quickly differentiate these cases from plain forgotten swallowed exceptions? Apart of course by deeply
analysing the concerned code which can be difficult in some cases (I agree not much here).
So I don't want to blindly revert but rather want to handle this case positively. So I think we need to have a look at each case and try to
collaborate for a better future for OFBiz
------------------------------------------------------------------------------------------------------------------------------------------------------
Let's take ProductionRunServices for a start: https://s.apache.org/bjbA
The 1st hunk concerns updateProductionRunTask()
- } catch (GenericEntityException gee) {
-
- } catch (GenericServiceException gee) {
-
+ } catch (GenericEntityException | GenericServiceException e) {
+ String errMsg = "Problem calling the
updateProductionRunTaskStatus service";
+ Debug.logError(e, errMsg, module);
+ return ServiceUtil.returnError(errMsg);
}
You put it in at r648703 here is the detail https://s.apache.org/uOWj
Then you improved it at r1043899 by adding
return ServiceUtil.returnError(ServiceUtil.getErrorMessage(resultService));
But you still let the swallowed exceptions (which BTW looks like a quick C/P, both named gee). If I review the concerned try I can't clearly
understand why. To me errors in services are handled but not other cases.
------------------------------------------------------------------------------------------------------------------------------------------------------
The 2nd hunk concerns approveRequirement
Here you swallowed the exception (I guess you wrote that before the Apache era)
and also use a ServiceUtil.returnError
return ServiceUtil.returnError(UtilProperties.getMessage(resource,
"ManufacturingRequirementNotExists", locale));
But we can have 2 cases here
1) no requirement w/o exception, OK
2) a simple entity exception for an unknown reason, swallowed
Why not handling the 2nd case?
The 3rd hunk concerns createProductionRunFromRequirement and is the same case
than the 2nd
For ShoppingCartEvents and ShoppingCartHelper, it's the same kind of cases than
the 2nd and 3rd hunks above. That should not be hard to document.
If we (both and All) agree on collaborating to document on purpose swallowed exceptions, even when you are not directly concerned, then I agree to
revert my changes, deal?
Jacques
Le 23/03/2017 à 09:04, Jacopo Cappellato a écrit :
Forwarding an email that I sent yesterday and seems to be lost in the net.
Jacopo
On Tue, Mar 21, 2017 at 10:29 AM, Jacopo Cappellato <
[email protected]> wrote:
Jacques,
I have some concerns about this and similar changes you are committing in
the attempt to improve exception handling: converting a swallowed exception
into a service/event error is actually introducing a pretty relevant
functional change. Maybe the original code was intended to ignore the
exception and move one with additional business logic: however a bulk
change without a deep study/testing of every specific change you are
introducing is not the right way to go in my opinion. I would recommend to
revert these commits and perform an analysis and testing before introducing
these changes.
Jacopo
On Tue, Mar 21, 2017 at 9:40 AM, <[email protected]> wrote:
Author: jleroux
Date: Tue Mar 21 08:40:07 2017
New Revision: 1787906
URL: http://svn.apache.org/viewvc?rev=1787906&view=rev
Log:
No functional changes, fixes a bunch of swallowed exceptions
Modified:
ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
main/java/org/apache/ofbiz/manufacturing/jobshopmgt/Produ
ctionRunServices.java
ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartEvents.java
ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartHelper.java
Modified: ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
main/java/org/apache/ofbiz/manufacturing/jobshopmgt/Produ
ctionRunServices.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
lications/manufacturing/src/main/java/org/apache/ofbiz/
manufacturing/jobshopmgt/ProductionRunServices.java?rev=
1787906&r1=1787905&r2=1787906&view=diff
============================================================
==================
--- ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
(original)
+++ ofbiz/ofbiz-framework/trunk/applications/manufacturing/src/
main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ProductionRunServices.java
Tue Mar 21 08:40:07 2017
@@ -2214,10 +2214,10 @@ public class ProductionRunServices {
}
}
}
- } catch (GenericEntityException gee) {
-
- } catch (GenericServiceException gee) {
-
+ } catch (GenericEntityException |
GenericServiceException e) {
+ String errMsg = "Problem calling the
updateProductionRunTaskStatus service";
+ Debug.logError(e, errMsg, module);
+ return ServiceUtil.returnError(errMsg);
}
}
}
@@ -2264,7 +2264,10 @@ public class ProductionRunServices {
GenericValue requirement = null;
try {
requirement = EntityQuery.use(delegator).fro
m("Requirement").where("requirementId", requirementId).queryOne();
- } catch (GenericEntityException gee) {
+ } catch (GenericEntityException e) {
+ String errMsg = "Problem calling the approveRequirement
service";
+ Debug.logError(e, errMsg, module);
+ return ServiceUtil.returnError(errMsg);
}
if (requirement == null) {
@@ -2295,7 +2298,10 @@ public class ProductionRunServices {
GenericValue requirement = null;
try {
requirement = EntityQuery.use(delegator).fro
m("Requirement").where("requirementId", requirementId).queryOne();
- } catch (GenericEntityException gee) {
+ } catch (GenericEntityException e) {
+ String errMsg = "Problem calling the
createProductionRunFromRequirement service";
+ Debug.logError(e, errMsg, module);
+ return ServiceUtil.returnError(errMsg);
}
if (requirement == null) {
return ServiceUtil.returnError(UtilProperties.getMessage(resource,
"ManufacturingRequirementNotExists", locale));
Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartEvents.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
lications/order/src/main/java/org/apache/ofbiz/order/shoppin
gcart/ShoppingCartEvents.java?rev=1787906&r1=1787905&r2=1787906&view=diff
============================================================
==================
--- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartEvents.java (original)
+++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartEvents.java Tue Mar 21
08:40:07 2017
@@ -1624,7 +1624,8 @@ public class ShoppingCartEvents {
.filterByDate()
.queryList();
} catch (GenericEntityException gee) {
- //
+ request.setAttribute("_ERROR_MESSAGE_",
gee.getMessage());
+ return "error";
}
if (UtilValidate.isNotEmpty(storeReps)) {
hasPermission = true;
@@ -1674,7 +1675,8 @@ public class ShoppingCartEvents {
try {
thisUserLogin = EntityQuery.use(delegator).fro
m("UserLogin").where("userLoginId", userLoginId).queryOne();
} catch (GenericEntityException gee) {
- //
+ request.setAttribute("_ERROR_MESSAGE_",
gee.getMessage());
+ return "error";
}
if (thisUserLogin != null) {
partyId = thisUserLogin.getString("partyId");
@@ -1687,7 +1689,8 @@ public class ShoppingCartEvents {
try {
thisParty =
EntityQuery.use(delegator).from("Party").where("partyId",
partyId).queryOne();
} catch (GenericEntityException gee) {
- //
+ request.setAttribute("_ERROR_MESSAGE_",
gee.getMessage());
+ return "error";
}
if (thisParty == null) {
request.setAttribute("_ERROR_MESSAGE_",
UtilProperties.getMessage(resource_error,"OrderCouldNotLocateTheSelectedParty",
locale));
Modified: ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartHelper.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/app
lications/order/src/main/java/org/apache/ofbiz/order/shoppin
gcart/ShoppingCartHelper.java?rev=1787906&r1=1787905&r2=1787906&view=diff
============================================================
==================
--- ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartHelper.java (original)
+++ ofbiz/ofbiz-framework/trunk/applications/order/src/main/java
/org/apache/ofbiz/order/shoppingcart/ShoppingCartHelper.java Tue Mar 21
08:40:07 2017
@@ -516,6 +516,7 @@ public class ShoppingCartHelper {
try {
requirement = EntityQuery.use(delegator).fro
m("Requirement").where("requirementId", requirementId).queryOne();
} catch (GenericEntityException gee) {
+ Debug.logError(gee, module);
}
if (requirement == null) {
return ServiceUtil.returnFailure(Util
Properties.getMessage(resource,
