Re: svn commit: r1787906 - in /ofbiz/ofbiz-framework/trunk/applications: manufacturing/src/main/java/org/apache/ofbiz/manufacturing/jobshopmgt/ order/src/main/java/org/apache/ofbiz/order/shoppingcart/

[Jacopo Cappellato-5]

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 <
[hidden email]> 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, <[hidden email]> 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,
>>
>>
>>
>

[Jacques Le Roux]

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 <
> [hidden email]> 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, <[hidden email]> 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,
>>>
>>>
>>>

[Jacopo Cappellato-5]

On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
[hidden email]> wrote:

> [...]
> 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?


We are not negotiating: I have simply asked you to revert the changes in
which you have changed the functional behavior of the system without
testing OR test the new behavior and confirm it is working fine.

In general I like the effort of improving this old code containing
swallowed exceptions by providing more comments, documentation etc... or
completely refactoring it; but this has to be done with proper testing.

I hope this clarifies my request.

Jacopo

[Scott Gray-3]

I agree with Jacopo, these are very functional changes that shouldn't be
made without understanding the workflow.  It's good to clean up the code
but it shouldn't be done blindly.

On 25 March 2017 at 02:13, Jacopo Cappellato <
[hidden email]> wrote:

> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
> [hidden email]> wrote:
>
> > [...]
> > 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?
>
>
> We are not negotiating: I have simply asked you to revert the changes in
> which you have changed the functional behavior of the system without
> testing OR test the new behavior and confirm it is working fine.
>
> In general I like the effort of improving this old code containing
> swallowed exceptions by providing more comments, documentation etc... or
> completely refactoring it; but this has to be done with proper testing.
>
> I hope this clarifies my request.
>
> Jacopo
>

[Jacques Le Roux]

I also agreed, I just expected some help to comment why the exceptions were swallowed :/

Jacques


Le 24/03/2017 à 21:09, Scott Gray a écrit :

> I agree with Jacopo, these are very functional changes that shouldn't be
> made without understanding the workflow.  It's good to clean up the code
> but it shouldn't be done blindly.
>
> On 25 March 2017 at 02:13, Jacopo Cappellato <
> [hidden email]> wrote:
>
>> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>>> [...]
>>> 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?
>>
>> We are not negotiating: I have simply asked you to revert the changes in
>> which you have changed the functional behavior of the system without
>> testing OR test the new behavior and confirm it is working fine.
>>
>> In general I like the effort of improving this old code containing
>> swallowed exceptions by providing more comments, documentation etc... or
>> completely refactoring it; but this has to be done with proper testing.
>>
>> I hope this clarifies my request.
>>
>> Jacopo
>>

[Jacques Le Roux]

Le 24/03/2017 à 14:13, Jacopo Cappellato a écrit :

> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> [...]
>> 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?
>
> We are not negotiating: I have simply asked you to revert the changes in
> which you have changed the functional behavior of the system without
> testing OR test the new behavior and confirm it is working fine.

I thought you could have given some information on the documentation to put into the swallowed exceptions :/
> In general I like the effort of improving this old code containing
> swallowed exceptions by providing more comments, documentation etc... or
> completely refactoring it; but this has to be done with proper testing.
>
> I hope this clarifies my request.
>
> Jacopo
>
OK so I'll try to answer to your request.
No need to say that all the tests pass. We know we don't directly cover all services but at least we got that when running all tests (gradlew
testIntegration):
2017-03-24 14:33:03,864 |main |ServiceDispatcher             |T| Sync service [test-dispatcher-Oz3Z0DRojV/updateProductionRunTask] finished in [830]
milliseconds
2017-03-24 14:33:04,173 |main |ServiceDispatcher             |T| Sync service [test-dispatcher-Oz3Z0DRojV/updateProductionRunTask] finished in [162]
milliseconds
It's part of testProductionRunQuickIssueAndProduce through a call to productionRunDeclareAndProduce itself calling updateProductionRunTask

I think we can agree that the successful behaviour can't have been derailed by my changes. It's only when a  GenericEntityException or
GenericServiceException is raised that it might be have been changed, right? So at least in a normal situation (no exceptions) it's OK.

Now what happens when an exception is raised?
In case of a GenericEntityException 2 expressions are concerned. In case of issues it's better to catch them rather than ignore them. Nothing was done
before, it was simply ignored. How could let a GenericEntityException swallowed here be better? It's better now IMO, shit happens, better not let it
hits the fan.

In case of a GenericServiceException the sync call to issueProductionRunTaskComponent is concerned. It's a simple-method.
Here are the 2 cases in log when putting a typo in issueProductionRunTaskComponent

With exception handled:
2017-03-24 20:29:38,267 |main |ServiceDispatcher             |T| [[Sync service failed...- total:0.0,since last(Begin):0.0]] -
'test-dispatcher-m10o2bxhIR / issueProductionRunTaskComponent'
2017-03-24 20:29:38,267 |main |TransactionUtil               |W| Calling transaction setRollbackOnly; this stack trace shows where this is happening:
java.lang.Exception: Service [issueProductionRunTaskComponent] threw an unexpected exception/error
         at org.apache.ofbiz.entity.transaction.TransactionUtil.setRollbackOnly(TransactionUtil.java:361) [ofbiz.jar:?]
         at org.apache.ofbiz.entity.transaction.TransactionUtil.rollback(TransactionUtil.java:302) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:512) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:227) [ofbiz.jar:?]
         at org.apache.ofbiz.service.GenericDispatcherFactory$GenericDispatcher.runSync(GenericDispatcherFactory.java:88) [ofbiz.jar:?]
         at org.apache.ofbiz.manufacturing.jobshopmgt.ProductionRunServices.updateProductionRunTask(ProductionRunServices.java:2210) [ofbiz.jar:?]
         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_121]
         at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) ~[?:1.8.0_121]
         at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) ~[?:1.8.0_121]
         at java.lang.reflect.Method.invoke(Unknown Source) ~[?:1.8.0_121]
         at org.apache.ofbiz.service.engine.StandardJavaEngine.serviceInvoker(StandardJavaEngine.java:100) [ofbiz.jar:?]
         at org.apache.ofbiz.service.engine.StandardJavaEngine.runSync(StandardJavaEngine.java:57) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:395) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:227) [ofbiz.jar:?]
         at org.apache.ofbiz.service.GenericDispatcherFactory$GenericDispatcher.runSync(GenericDispatcherFactory.java:88) [ofbiz.jar:?]
         at org.apache.ofbiz.manufacturing.jobshopmgt.ProductionRunServices.productionRunDeclareAndProduce(ProductionRunServices.java:1882) [ofbiz.jar:?]
         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_121]
         at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) ~[?:1.8.0_121]
         at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) ~[?:1.8.0_121]
         at java.lang.reflect.Method.invoke(Unknown Source) ~[?:1.8.0_121]
         at org.apache.ofbiz.service.engine.StandardJavaEngine.serviceInvoker(StandardJavaEngine.java:100) [ofbiz.jar:?]
         at org.apache.ofbiz.service.engine.StandardJavaEngine.runSync(StandardJavaEngine.java:57) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:395) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:227) [ofbiz.jar:?]
         at org.apache.ofbiz.service.GenericDispatcherFactory$GenericDispatcher.runSync(GenericDispatcherFactory.java:103) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.method.callops.CallService.exec(CallService.java:217) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.runSubOps(SimpleMethod.java:310) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.exec(SimpleMethod.java:457) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.runSimpleMethod(SimpleMethod.java:274) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.runSimpleService(SimpleMethod.java:287) [ofbiz.jar:?]
         at org.apache.ofbiz.testtools.SimpleMethodTest.run(SimpleMethodTest.java:85) [ofbiz.jar:?]
         at junit.framework.TestSuite.runTest(TestSuite.java:243) [junit-dep-4.10.jar:?]
         at junit.framework.TestSuite.run(TestSuite.java:238) [junit-dep-4.10.jar:?]
         at org.apache.ofbiz.testtools.TestRunContainer.start(TestRunContainer.java:152) [ofbiz.jar:?]
         at org.apache.ofbiz.base.container.ContainerLoader.startLoadedContainers(ContainerLoader.java:148) [ofbiz.jar:?]
         at org.apache.ofbiz.base.container.ContainerLoader.load(ContainerLoader.java:73) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.StartupControlPanel.loadStartupLoaders(StartupControlPanel.java:202) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.StartupControlPanel.start(StartupControlPanel.java:69) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.Start.main(Start.java:84) [ofbiz.jar:?]
2017-03-24 20:29:38,273 |main |ProductionRunServices         |E| Problem calling the updateProductionRunTaskStatus service
org.apache.ofbiz.service.GenericServiceException: Error running simple method [issueProductionRunTaskComponent] in XML file
[component://manufacturing/minilang/jobshopmgt/ProductionRunServices.xml]: (Could n
ot read SimpleMethod XML document [file:/C:/projectsASF/ofbiz-framework/applications/manufacturing/minilang/jobshopmgt/ProductionRunServices.xml]:
(The value of attribute "value-field" associated with an ele
ment type "entity-one" must not contain the '<' character.))

with exception swallowed
2017-03-24 20:50:03,596 |main |ServiceDispatcher             |T| [[Sync service failed...- total:0.0,since last(Begin):0.0]] -
'test-dispatcher-T7iFbK65R2 / issueProductionRunTaskComponent'
2017-03-24 20:50:03,596 |main |TransactionUtil               |W| Calling transaction setRollbackOnly; this stack trace shows where this is happening:
java.lang.Exception: Service [issueProductionRunTaskComponent] threw an unexpected exception/error
         at org.apache.ofbiz.entity.transaction.TransactionUtil.setRollbackOnly(TransactionUtil.java:361) [ofbiz.jar:?]
         at org.apache.ofbiz.entity.transaction.TransactionUtil.rollback(TransactionUtil.java:302) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:512) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:227) [ofbiz.jar:?]
         at org.apache.ofbiz.service.GenericDispatcherFactory$GenericDispatcher.runSync(GenericDispatcherFactory.java:88) [ofbiz.jar:?]
         at org.apache.ofbiz.manufacturing.jobshopmgt.ProductionRunServices.updateProductionRunTask(ProductionRunServices.java:2210) [ofbiz.jar:?]
         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_121]
         at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) ~[?:1.8.0_121]
         at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) ~[?:1.8.0_121]
         at java.lang.reflect.Method.invoke(Unknown Source) ~[?:1.8.0_121]
         at org.apache.ofbiz.service.engine.StandardJavaEngine.serviceInvoker(StandardJavaEngine.java:100) [ofbiz.jar:?]
         at org.apache.ofbiz.service.engine.StandardJavaEngine.runSync(StandardJavaEngine.java:57) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:395) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:227) [ofbiz.jar:?]
         at org.apache.ofbiz.service.GenericDispatcherFactory$GenericDispatcher.runSync(GenericDispatcherFactory.java:88) [ofbiz.jar:?]
         at org.apache.ofbiz.manufacturing.jobshopmgt.ProductionRunServices.productionRunDeclareAndProduce(ProductionRunServices.java:1882) [ofbiz.jar:?]
         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_121]
         at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) ~[?:1.8.0_121]
         at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) ~[?:1.8.0_121]
         at java.lang.reflect.Method.invoke(Unknown Source) ~[?:1.8.0_121]
         at org.apache.ofbiz.service.engine.StandardJavaEngine.serviceInvoker(StandardJavaEngine.java:100) [ofbiz.jar:?]
         at org.apache.ofbiz.service.engine.StandardJavaEngine.runSync(StandardJavaEngine.java:57) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:395) [ofbiz.jar:?]
         at org.apache.ofbiz.service.ServiceDispatcher.runSync(ServiceDispatcher.java:227) [ofbiz.jar:?]
         at org.apache.ofbiz.service.GenericDispatcherFactory$GenericDispatcher.runSync(GenericDispatcherFactory.java:103) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.method.callops.CallService.exec(CallService.java:217) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.runSubOps(SimpleMethod.java:310) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.exec(SimpleMethod.java:457) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.runSimpleMethod(SimpleMethod.java:274) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.runSimpleService(SimpleMethod.java:287) [ofbiz.jar:?]
         at org.apache.ofbiz.testtools.SimpleMethodTest.run(SimpleMethodTest.java:85) [ofbiz.jar:?]
         at junit.framework.TestSuite.runTest(TestSuite.java:243) [junit-dep-4.10.jar:?]
         at junit.framework.TestSuite.run(TestSuite.java:238) [junit-dep-4.10.jar:?]
         at org.apache.ofbiz.testtools.TestRunContainer.start(TestRunContainer.java:152) [ofbiz.jar:?]
         at org.apache.ofbiz.base.container.ContainerLoader.startLoadedContainers(ContainerLoader.java:148) [ofbiz.jar:?]
         at org.apache.ofbiz.base.container.ContainerLoader.load(ContainerLoader.java:73) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.StartupControlPanel.loadStartupLoaders(StartupControlPanel.java:202) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.StartupControlPanel.start(StartupControlPanel.java:69) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.Start.main(Start.java:84) [ofbiz.jar:?]
2017-03-24 20:50:03,600 |main |ServiceEcaRule                |I| For Service ECA [workEffortGenericPermission] on [return] got false for condition:
[hasPermission][equals][false][true][Boolean
]
2017-03-24 20:50:03,600 |main |ServiceDispatcher             |T| Sync service [test-dispatcher-T7iFbK65R2/workEffortGenericPermission] finished in [1]
milliseconds
2017-03-24 20:50:03,600 |main |TransactionUtil               |W| Active transaction marked for rollback in place, so no transaction begun; this stack
trace shows when the exception began:
java.lang.Exception: Tx Stack Placeholder
         at org.apache.ofbiz.entity.transaction.TransactionUtil.setTransactionBeginStack(TransactionUtil.java:720) ~[ofbiz.jar:?]
         at org.apache.ofbiz.entity.transaction.TransactionUtil.begin(TransactionUtil.java:156) ~[ofbiz.jar:?]
         at org.apache.ofbiz.entity.transaction.TransactionUtil.begin(TransactionUtil.java:111) ~[ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.exec(SimpleMethod.java:441) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.runSimpleMethod(SimpleMethod.java:274) [ofbiz.jar:?]
         at org.apache.ofbiz.minilang.SimpleMethod.runSimpleService(SimpleMethod.java:287) [ofbiz.jar:?]
         at org.apache.ofbiz.testtools.SimpleMethodTest.run(SimpleMethodTest.java:85) [ofbiz.jar:?]
         at junit.framework.TestSuite.runTest(TestSuite.java:243) [junit-dep-4.10.jar:?]
         at junit.framework.TestSuite.run(TestSuite.java:238) [junit-dep-4.10.jar:?]
         at org.apache.ofbiz.testtools.TestRunContainer.start(TestRunContainer.java:152) [ofbiz.jar:?]
         at org.apache.ofbiz.base.container.ContainerLoader.startLoadedContainers(ContainerLoader.java:148) [ofbiz.jar:?]
         at org.apache.ofbiz.base.container.ContainerLoader.load(ContainerLoader.java:73) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.StartupControlPanel.loadStartupLoaders(StartupControlPanel.java:202) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.StartupControlPanel.start(StartupControlPanel.java:69) [ofbiz.jar:?]
         at org.apache.ofbiz.base.start.Start.main(Start.java:84) [ofbiz.jar:?]
2017-03-24 20:50:03,632 |main |UtilProperties                |I| ResourceBundle MiniLangErrorUiLabels (en) created in 0.031s with 4 properties
2017-03-24 20:50:03,632 |main |ServiceDispatcher             |E| Error in Service [updateWorkEffort]: Error trying to begin transaction, could not
process method: The current transaction is ma
rked for rollback, not beginning a new transaction and aborting current operation; the rollbackOnly was caused by: Service
[issueProductionRunTaskComponent] threw an unexpected exception/errororg.apache.ofbiz
.service.GenericServiceException: Error running simple method [issueProductionRunTaskComponent] in XML file
[component://manufacturing/minilang/jobshopmgt/ProductionRunServices.xml]: (Could not read SimpleMe
thod XML document [file:/C:/projectsASF/ofbiz-framework/applications/manufacturing/minilang/jobshopmgt/ProductionRunServices.xml]: (The value of
attribute "value-field" associated with an element type "entit
y-one" must not contain the '<' character.)) (Error running simple method [issueProductionRunTaskComponent] in XML file
[component://manufacturing/minilang/jobshopmgt/ProductionRunServices.xml]: (Could not r
ead SimpleMethod XML document [file:/C:/projectsASF/ofbiz-framework/applications/manufacturing/minilang/jobshopmgt/ProductionRunServices.xml]: (The
value of attribute "value-field" associated with an element
  type "entity-one" must not contain the '<' character.)))

I'd argue that the 1st stack is easier to understand. I could continue on other cases but they are simpler and it's not my intention to defend my
commit when we have so much other tasks pending. Oh boys, I hoped more collaboration...

Now feel free to revert my commit if you still think it's a bad thing, but sincerely I'm not convinced! If you do so I'll then at least add comments
to explain the situation...

Jacques

[Michael Brohl-3]

+1

The lack of code documentation is not a free ticket to just change the
code behaviour without proper analysis.

The right process should be

1. discuss

2. provide a patch

3. let others review/comment

4. decide

5. commit

It is really dangerous to easily change code like this.

Jacques, please be not so hasty with committing stuff. We have had a lot
of similar cases with reverts, committing half done solutions and such
lately. And please be aware that others might not have so much time to
follow every commit in detail, analyze and comment promptly.

It really worries me because we lose quality and it's not easy to detect
errors and changed functionality in such a complex project. And don't
rely too much on the tests as we don't have such a high test coverage.

Thanks for some more patience,

Michael


Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:

> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> [...]
>> 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?
>
> We are not negotiating: I have simply asked you to revert the changes in
> which you have changed the functional behavior of the system without
> testing OR test the new behavior and confirm it is working fine.
>
> In general I like the effort of improving this old code containing
> swallowed exceptions by providing more comments, documentation etc... or
> completely refactoring it; but this has to be done with proper testing.
>
> I hope this clarifies my request.
>
> Jacopo
>

[Pierre Smits]

So you're suggesting to apply RtC in stead op CtR regarding improvement
issues, Michael?

I suggest you start that discussion in a new thread and work towards
consensus and documenting.

Best regards,

Pierre

On Saturday, March 25, 2017, Michael Brohl <[hidden email]> wrote:

> +1
>
> The lack of code documentation is not a free ticket to just change the
> code behaviour without proper analysis.
>
> The right process should be
>
> 1. discuss
>
> 2. provide a patch
>
> 3. let others review/comment
>
> 4. decide
>
> 5. commit
>
> It is really dangerous to easily change code like this.
>
> Jacques, please be not so hasty with committing stuff. We have had a lot
> of similar cases with reverts, committing half done solutions and such
> lately. And please be aware that others might not have so much time to
> follow every commit in detail, analyze and comment promptly.
>
> It really worries me because we lose quality and it's not easy to detect
> errors and changed functionality in such a complex project. And don't rely
> too much on the tests as we don't have such a high test coverage.
>
> Thanks for some more patience,
>
> Michael
>
>
> Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:
>
>> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>> [...]
>>> 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?
>>>
>>
>> We are not negotiating: I have simply asked you to revert the changes in
>> which you have changed the functional behavior of the system without
>> testing OR test the new behavior and confirm it is working fine.
>>
>> In general I like the effort of improving this old code containing
>> swallowed exceptions by providing more comments, documentation etc... or
>> completely refactoring it; but this has to be done with proper testing.
>>
>> I hope this clarifies my request.
>>
>> Jacopo
>>
>>
>
>

--
Pierre Smits

ORRTIZ.COM <http://www.orrtiz.com>
OFBiz based solutions & services

OFBiz Extensions Marketplace
http://oem.ofbizci.net/oci-2/

[taher]

I think we should start a discussion about these types of commits. I have
concerns and problems with the way commits are getting done at the moment.
Meanwhile, I also agree with Jacopo on the need for a thorough review
instead of these _bulk_ commits without looking carefully at all possible
affected locations in the code base. I mean even with extreme care we
sometimes catch deeply hidden bugs.

On Sat, Mar 25, 2017 at 3:21 PM, Michael Brohl <[hidden email]>
wrote:

> +1
>
> The lack of code documentation is not a free ticket to just change the
> code behaviour without proper analysis.
>
> The right process should be
>
> 1. discuss
>
> 2. provide a patch
>
> 3. let others review/comment
>
> 4. decide
>
> 5. commit
>
> It is really dangerous to easily change code like this.
>
> Jacques, please be not so hasty with committing stuff. We have had a lot
> of similar cases with reverts, committing half done solutions and such
> lately. And please be aware that others might not have so much time to
> follow every commit in detail, analyze and comment promptly.
>
> It really worries me because we lose quality and it's not easy to detect
> errors and changed functionality in such a complex project. And don't rely
> too much on the tests as we don't have such a high test coverage.
>
> Thanks for some more patience,
>
> Michael
>
>
> Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:
>
> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>> [...]
>>> 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?
>>>
>>
>> We are not negotiating: I have simply asked you to revert the changes in
>> which you have changed the functional behavior of the system without
>> testing OR test the new behavior and confirm it is working fine.
>>
>> In general I like the effort of improving this old code containing
>> swallowed exceptions by providing more comments, documentation etc... or
>> completely refactoring it; but this has to be done with proper testing.
>>
>> I hope this clarifies my request.
>>
>> Jacopo
>>
>>
>
>

[Jacopo Cappellato-5]

On Sat, Mar 25, 2017 at 1:21 PM, Michael Brohl <[hidden email]>
wrote:

> +1
>
> The lack of code documentation is not a free ticket to just change the
> code behaviour without proper analysis.
>
> The right process should be
>
> 1. discuss
>
> 2. provide a patch
>
> 3. let others review/comment
>
> 4. decide
>
> 5. commit
>
> It is really dangerous to easily change code like this.
>
> Jacques, please be not so hasty with committing stuff. We have had a lot
> of similar cases with reverts, committing half done solutions and such
> lately. And please be aware that others might not have so much time to
> follow every commit in detail, analyze and comment promptly.
>
> It really worries me because we lose quality and it's not easy to detect
> errors and changed functionality in such a complex project. And don't rely
> too much on the tests as we don't have such a high test coverage.
>
> Thanks for some more patience,
>
> Michael
>
>

I am fine with continuing with the Commit-Then-Review approach (and
leverage Review-ThenCommit approach only when more appropriate) provided
that the committer is willing to accept (by reverting or performing further
work as requested by the reviewers) negative reviews. What we should really
avoid is committers dragging their feet, pushing back reviews, trying to
argue and negotiate to keep their code as committed.

Jacopo

[Jacopo Cappellato-5]

On Sat, Mar 25, 2017 at 8:23 AM, Jacques Le Roux <
[hidden email]> wrote:

> [...]
> Now feel free to revert my commit if you still think it's a bad thing, but
> sincerely I'm not convinced! If you do so I'll then at least add comments
> to explain the situation...


The analysis and tests that you did in this email to reply to my request
are actually what I was asking for that is (quoting myself):

"[...] to revert the changes in which you have changed the functional
behavior of the system without testing OR test the new behavior and confirm
it is working fine."

So this is the right direction: please similarly complete your analysis and
testing on any other code modified/affected by your commits or revert the
functional changes that you can't analyze or test.

Thanks

Jacopo

[Michael Brohl-3]

Hi Jacopo,

I'm not for a general change to Review-Then-Commit but pleading for a
more sensible way to handle these things as a responsible committer.

As a guideline: if in doubt, always provide a patch and describe what
the change should do or fix and let others review.

Of course not for trivial changes or fixes. But when it comes to new
modules, significantly changing the behaviour of the business logic etc.
I think it's better to apply Review-Then-Commit.

And of course, if others want a commit to be reverted, we should do it
first and then discuss.

Regards,

Michael


Am 27.03.17 um 10:07 schrieb Jacopo Cappellato:

> On Sat, Mar 25, 2017 at 1:21 PM, Michael Brohl <[hidden email]>
> wrote:
>
>> +1
>>
>> The lack of code documentation is not a free ticket to just change the
>> code behaviour without proper analysis.
>>
>> The right process should be
>>
>> 1. discuss
>>
>> 2. provide a patch
>>
>> 3. let others review/comment
>>
>> 4. decide
>>
>> 5. commit
>>
>> It is really dangerous to easily change code like this.
>>
>> Jacques, please be not so hasty with committing stuff. We have had a lot
>> of similar cases with reverts, committing half done solutions and such
>> lately. And please be aware that others might not have so much time to
>> follow every commit in detail, analyze and comment promptly.
>>
>> It really worries me because we lose quality and it's not easy to detect
>> errors and changed functionality in such a complex project. And don't rely
>> too much on the tests as we don't have such a high test coverage.
>>
>> Thanks for some more patience,
>>
>> Michael
>>
>>
> I am fine with continuing with the Commit-Then-Review approach (and
> leverage Review-ThenCommit approach only when more appropriate) provided
> that the committer is willing to accept (by reverting or performing further
> work as requested by the reviewers) negative reviews. What we should really
> avoid is committers dragging their feet, pushing back reviews, trying to
> argue and negotiate to keep their code as committed.
>
> Jacopo
>

[Jacopo Cappellato-5]

On Mon, Mar 27, 2017 at 10:24 AM, Michael Brohl <[hidden email]>
wrote:

> Hi Jacopo,
>
> I'm not for a general change to Review-Then-Commit but pleading for a more
> sensible way to handle these things as a responsible committer.
>
> As a guideline: if in doubt, always provide a patch and describe what the
> change should do or fix and let others review.
>
> Of course not for trivial changes or fixes. But when it comes to new
> modules, significantly changing the behaviour of the business logic etc. I
> think it's better to apply Review-Then-Commit.
>
> And of course, if others want a commit to be reverted, we should do it
> first and then discuss.
>
> Regards,
>
> Michael
>

I totally agree with all you wrote Michael.

Jacopo

[Jacques Le Roux]

Le 25/03/2017 à 13:21, Michael Brohl a écrit :
> +1
>
> The lack of code documentation is not a free ticket to just change the code behaviour without proper analysis.

It's not because the swallowed exceptions where not documented that I decided to catch them. I rather proposed later to at least document them; if we
had fear (ie no proofs) that it was going to break the flow.
What make you think that I did not a proper analysis? Actually it was a fast analysis based on 2 principles:
1) Exceptions should be catched because they fail fast. It's then easier to analyse a stack error.
https://www.owasp.org/index.php/Exception_handling_techniques#Swallowing_Exceptions. You may prefer https://www.google.com/search?q=swallowing+exceptions
And yes, I did a lot of OFBiz log analysis, where stack errors are very important. And no, most of the errors I had to analyse were not mine!
2) Returning an error from a service does not guarantee that all cases are covered, notably exceptions

I must admit I did not then dig as much as I did to convince Jacopo I was not derailing the flow. But that's what I had in mind. It was not a shoot in
the dark as you seem to think.

>
> The right process should be
>
> 1. discuss
>
> 2. provide a patch
>
> 3. let others review/comment
>
> 4. decide
>
> 5. commit

I don't agree with this process for present changes. It's much, too much bureaucracy in this case, sorry to say. I though agree that there are cases
where it's necessary, and I sometimes do so.
>
> It is really dangerous to easily change code like this.
Why? Please explain and prove your allegations...
>
> Jacques, please be not so hasty with committing stuff.
I don't commit stuff hastily. I commit a lot, but not hastily. I make errors, who does not?
> We have had a lot of similar cases with reverts,
"A lot of similar cases with reverts"? Really? Which ones?
> committing half done solutions and such lately.
"half done solutions and such", have you examples?
> And please be aware that others might not have so much time to follow every commit in detail, analyze and comment promptly.
I don't ask anybody to follow the pace I have currently the chance to have. But if I follow your comment, then we would use RTC, for now it's CTR. And
with RTC, OFBiz evolution, which is not currently brilliant,  would begin to stale.
>
> It really worries me because we lose quality and it's not easy to detect errors
It never easy to detect errors. It needs a lot of work. Do you suggest that I put errors in code by negligence? Have a look at what I do, and you will
be convinced on the contrary. I track errors as much as I can and I help others to fix them when they are not mine.
> and changed functionality in such a complex project.
I repeat, again, I did not change any functionalities, hence the "No functional change". Prove the contrary...
> And don't rely too much on the tests as we don't have such a high test coverage.
I don't rely only on tests, but yes I also rely on them, who does not?
>
> Thanks for some more patience,
That I can understand, but not all the FUD above, and not going to RTC because of fear. If you have something to say, please comment the code and
detail the problems you see, then we can discuss...

Jacques

>
> Michael
>
>
> Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:
>> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>>> [...]
>>> 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?
>>
>> We are not negotiating: I have simply asked you to revert the changes in
>> which you have changed the functional behavior of the system without
>> testing OR test the new behavior and confirm it is working fine.
>>
>> In general I like the effort of improving this old code containing
>> swallowed exceptions by providing more comments, documentation etc... or
>> completely refactoring it; but this has to be done with proper testing.
>>
>> I hope this clarifies my request.
>>
>> Jacopo
>>
>
>

[Michael Brohl-3]

Hi Jacques,

Am 28.03.17 um 05:47 schrieb Jacques Le Roux:

> Le 25/03/2017 à 13:21, Michael Brohl a écrit :
>> +1
>>
>> The lack of code documentation is not a free ticket to just change
>> the code behaviour without proper analysis.
>
> It's not because the swallowed exceptions where not documented that I
> decided to catch them. I rather proposed later to at least document
> them; if we had fear (ie no proofs) that it was going to break the flow.
> What make you think that I did not a proper analysis? Actually it was
> a fast analysis based on 2 principles:
> 1) Exceptions should be catched because they fail fast. It's then
> easier to analyse a stack error.
> https://www.owasp.org/index.php/Exception_handling_techniques#Swallowing_Exceptions.
> You may prefer https://www.google.com/search?q=swallowing+exceptions
> And yes, I did a lot of OFBiz log analysis, where stack errors are
> very important. And no, most of the errors I had to analyse were not
> mine!
> 2) Returning an error from a service does not guarantee that all cases
> are covered, notably exceptions
>

These are just general rules and patterns and do not prove that a proper
analysis for this special case was made.

> I must admit I did not then dig as much as I did to convince Jacopo I
> was not derailing the flow. But that's what I had in mind. It was not
> a shoot in the dark as you seem to think.
>

I did not say that it was a shot in the dark but that it needs proper
analysis. You admit for yourself that you did it after Jacopo asked you
to revert the commit and not before you committed. That's what worries me.

>>
>> The right process should be
>>
>> 1. discuss
>>
>> 2. provide a patch
>>
>> 3. let others review/comment
>>
>> 4. decide
>>
>> 5. commit
> I don't agree with this process for present changes. It's much, too
> much bureaucracy in this case, sorry to say. I though agree that there
> are cases where it's necessary, and I sometimes do so.

That's not bureaucracy but collaboration and a principle to assure
quality and prevent errors. As I said before, I won't apply this process
to any single fix or trivial enhancement.
In this case, what would be so difficult to put the patch for your
proposal in Jira and asked for some opinions from other contributors?

In https://issues.apache.org/jira/browse/OFBIZ-9123 you agreed that this
woul be the better approach by stating " I concur with Michael's
opinion, notably the well stated 4 points process."

>>
>> It is really dangerous to easily change code like this.
> Why? Please explain and prove your allegations...

It's obvious that changing program logic without proper analysis is
dangerous, especially in a complex system like OFBiz, isn't it?
>>
>> Jacques, please be not so hasty with committing stuff.
> I don't commit stuff hastily. I commit a lot, but not hastily. I make
> errors, who does not?

You might call it like you want, my impression is that slowing down a
bit and collaborating more by providing a patch instead of directly
commiting your work might reduce the number of errors and save us a lot
of time discussing and reverting afterwards.

>> We have had a lot of similar cases with reverts,
> "A lot of similar cases with reverts"? Really? Which ones?

Just search for "revert/reverted/reverts" in the commits mailing list
and you'll find them.

>> committing half done solutions and such lately.
> "half done solutions and such", have you examples?

We had several disccussions about them, the encryption issue and the
addition of the PriCat component come to mind. There were some others
but I won't spent time to dig them up again.

>> And please be aware that others might not have so much time to follow
>> every commit in detail, analyze and comment promptly.
> I don't ask anybody to follow the pace I have currently the chance to
> have. But if I follow your comment, then we would use RTC, for now
> it's CTR. And with RTC, OFBiz evolution, which is not currently
> brilliant,  would begin to stale.

In my opinion, the evolution should be towards quality, ease of
maintenance and robustness, not about commiting lots of stuff in a short
amount of time. A well balanced use of RTC and CTR will help all of us.

>>
>> It really worries me because we lose quality and it's not easy to
>> detect errors
> It never easy to detect errors. It needs a lot of work. Do you suggest
> that I put errors in code by negligence? Have a look at what I do, and
> you will be convinced on the contrary. I track errors as much as I can
> and I help others to fix them when they are not mine.

I did not say any of this nor do I suggest it.

>> and changed functionality in such a complex project.
> I repeat, again, I did not change any functionalities, hence the "No
> functional change". Prove the contrary...
>> And don't rely too much on the tests as we don't have such a high
>> test coverage.
> I don't rely only on tests, but yes I also rely on them, who does not?
>>
>> Thanks for some more patience,
> That I can understand, but not all the FUD above, and not going to RTC
> because of fear. If you have something to say, please comment the code
> and detail the problems you see, then we can discuss...

This statement tells me that you are not willing to collaborate.
Instead, you call arguments for stability and quality assurance FUD and
refuse to the request to revert your commit.

Well, that's sad but everyone has his/her own way to contribute...

Michael

>
> Jacques
>>
>> Michael
>>
>>
>> Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:
>>> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
>>> [hidden email]> wrote:
>>>
>>>> [...]
>>>> 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?
>>>
>>> We are not negotiating: I have simply asked you to revert the
>>> changes in
>>> which you have changed the functional behavior of the system without
>>> testing OR test the new behavior and confirm it is working fine.
>>>
>>> In general I like the effort of improving this old code containing
>>> swallowed exceptions by providing more comments, documentation
>>> etc... or
>>> completely refactoring it; but this has to be done with proper testing.
>>>
>>> I hope this clarifies my request.
>>>
>>> Jacopo
>>>
>>
>>
>
>

[Jacques Le Roux]

Le 27/03/2017 à 10:01, Jacopo Cappellato a écrit :

> On Sat, Mar 25, 2017 at 8:23 AM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> [...]
>> Now feel free to revert my commit if you still think it's a bad thing, but
>> sincerely I'm not convinced! If you do so I'll then at least add comments
>> to explain the situation...
>
> The analysis and tests that you did in this email to reply to my request
> are actually what I was asking for that is (quoting myself):
>
> "[...] to revert the changes in which you have changed the functional
> behavior of the system without testing OR test the new behavior and confirm
> it is working fine."
>
> So this is the right direction: please similarly complete your analysis and
> testing on any other code modified/affected by your commits or revert the
> functional changes that you can't analyze or test.
>
> Thanks
>
> Jacopo
>

To keep things clear I'll answer you an everybody in the related Jira "Fix Default or Empty Catch block in Java files" OFBIZ-8341
I will explain each commit in the Jira for everybody interested.
It will more clear there where this is at least some limited ways of formatting (code, etc.)

Jacques

[Jacques Le Roux]

It will more clear there where this is at least some limited ways of formatting (code, etc.)
>
> Jacques
>
>
Typo
It will more clear there where there is at least some limited ways of formatting (code, etc.)

Jacques

[Jacques Le Roux]

Le 29/03/2017 à 15:27, Jacques Le Roux a écrit :

> It will more clear there where this is at least some limited ways of formatting (code, etc.)
>>
>> Jacques
>>
>>
> Typo
> It will more clear there where there is at least some limited ways of formatting (code, etc.)
>
> Jacques
>

:D still a word missing
It will BE more clear there, where there is at least some limited ways of formatting (code, etc.)

Jacques

[Jacques Le Roux]

Hi Michael,

It will be my last answer, and I'll try to be positive, just using facts, else we will never end.


Le 29/03/2017 à 11:23, Michael Brohl a écrit :

> Hi Jacques,
>
> Am 28.03.17 um 05:47 schrieb Jacques Le Roux:
>> Le 25/03/2017 à 13:21, Michael Brohl a écrit :
>>> +1
>>>
>>> The lack of code documentation is not a free ticket to just change the code behaviour without proper analysis.
>>
>> It's not because the swallowed exceptions where not documented that I decided to catch them. I rather proposed later to at least document them; if
>> we had fear (ie no proofs) that it was going to break the flow.
>> What make you think that I did not a proper analysis? Actually it was a fast analysis based on 2 principles:
>> 1) Exceptions should be catched because they fail fast. It's then easier to analyse a stack error.
>> https://www.owasp.org/index.php/Exception_handling_techniques#Swallowing_Exceptions. You may prefer
>> https://www.google.com/search?q=swallowing+exceptions
>> And yes, I did a lot of OFBiz log analysis, where stack errors are very important. And no, most of the errors I had to analyse were not mine!
>> 2) Returning an error from a service does not guarantee that all cases are covered, notably exceptions
>>
>
> These are just general rules and patterns and do not prove that a proper analysis for this special case was made.

I did a proper analysis based on these patterns and rules. Jacopo asked me to prove I was right, it's not the same. I got into much details then
applying the ideas I had initially. Like if I was in a trial, where Jacopo, Scott, Taher and you were accusing me.
Now how could I prove that I previously did a proper analysis? It was in my brain, hard to say.
Let me ask you another question. In which cases is it good to swallow an exception? I believe there are very rare cases, if any. And having a
swallowed exception is a smell for refactoring.
>> I must admit I did not then dig as much as I did to convince Jacopo I was not derailing the flow. But that's what I had in mind. It was not a shoot
>> in the dark as you seem to think.
>>
>
> I did not say that it was a shot in the dark but that it needs proper analysis. You admit for yourself that you did it after Jacopo asked you to
> revert the commit and not before you committed. That's what worries me.
Actually it's very different when you are thinking and when you are writing/doing.

>
>>>
>>> The right process should be
>>>
>>> 1. discuss
>>>
>>> 2. provide a patch
>>>
>>> 3. let others review/comment
>>>
>>> 4. decide
>>>
>>> 5. commit
>> I don't agree with this process for present changes. It's much, too much bureaucracy in this case, sorry to say. I though agree that there are
>> cases where it's necessary, and I sometimes do so.
>
> That's not bureaucracy but collaboration and a principle to assure quality and prevent errors. As I said before, I won't apply this process to any
> single fix or trivial enhancement.
> In this case, what would be so difficult to put the patch for your proposal in Jira and asked for some opinions from other contributors?

That's something I do sometimes. But wait contributors do that, and here is the situation: 259 issues with patches available waiting
https://issues.apache.org/jira/issues/?filter=12340473
among 51 bugs https://issues.apache.org/jira/issues/?filter=12333848
So before submitting a patch I always balance if it's a good idea or if I can take the responsibility of directly committing. I also always consider
the 10" action rule. If I can do it in 10" then I do it right away. Somrtimes I end shaving the yak and would slap myself :/
It still happens that I prefer to submit a patch and there are cases in Jira. https://issues.apache.org/jira/issues/?filter=12340482 Not all
attachments are patches, but even if it's only a half+, it's still around 100 cases
>
> In https://issues.apache.org/jira/browse/OFBIZ-9123 you agreed that this woul be the better approach by stating " I concur with Michael's opinion,
> notably the well stated 4 points process."
>
But, if you read all the comment https://s.apache.org/4HFD I then did a review and agree it was OK. Here is another sentence is the same comment
<<I did not a thorough review, but globally this is very simple and seems well done, so for me it's only lacking documentation. At this stage I'd not
ask to revert.>>
For me this did not need to be reverted and been postponed. Later, after Pierre's comment on FTL templates, I asked Jinghai twice to follow the
structure we "recently" (then) adopted. I guess he missed the 1st and did quickly on the 2nd.
>>>
>>> It is really dangerous to easily change code like this.
>> Why? Please explain and prove your allegations...
>
> It's obvious that changing program logic without proper analysis is dangerous, especially in a complex system like OFBiz, isn't it?
I agree OFBiz is complex but this was not complex, people feared it was complex, it was not. "You shall not swallow exceptions" could be the mantra of
the week.
>>> Jacques, please be not so hasty with committing stuff.
>> I don't commit stuff hastily. I commit a lot, but not hastily. I make errors, who does not?
>
> You might call it like you want, my impression is that slowing down a bit and collaborating more by providing a patch instead of directly commiting
> your work might reduce the number of errors and save us a lot of time discussing and reverting afterwards.
I know you prefer RTC over CTR. With 259 issues, among 51 bugs with patches available waiting I don't think RTC would improve this statement, I
believe the contrary.
It's good that committers review commits. And it would be as good if they were reviewing waiting patches before they become stale...
>
>>> We have had a lot of similar cases with reverts,
>> "A lot of similar cases with reverts"? Really? Which ones?
>
> Just search for "revert/reverted/reverts" in the commits mailing list and you'll find them.
Roughly
I did 357 commits in the last 3 months (see the stats panel) at https://lists.apache.org/list.html?commits@...:lte=3M:jleroux
Among them 15 reverts https://lists.apache.org/list.html?commits@...:lte=3M:jleroux%20reverted
4%, this does not look as terrible stats to me. We have a saying in French which translate to "Only those who do nothing make no mistakes." I don't
want committers, of course including myself, to feel afraid of committing.
But maybe you should ask in a new thread about his point. But please it must simple RTC vs CTR, because having too much rules will not help, but will
blur things.

>
>>> committing half done solutions and such lately.
>> "half done solutions and such", have you examples?
>
> We had several disccussions about them, the encryption issue and the addition of the PriCat component come to mind. There were some others but I
> won't spent time to dig them up again.
Please could you explain what you have in mind with "the encryption issue". For Pricat I already answered above.
>
>>> And please be aware that others might not have so much time to follow every commit in detail, analyze and comment promptly.
>> I don't ask anybody to follow the pace I have currently the chance to have. But if I follow your comment, then we would use RTC, for now it's CTR.
>> And with RTC, OFBiz evolution, which is not currently brilliant,  would begin to stale.
>
> In my opinion, the evolution should be towards quality, ease of maintenance and robustness, not about commiting lots of stuff in a short amount of
> time. A well balanced use of RTC and CTR will help all of us.
Good words, who will be against "quality, ease of maintenance and robustness". I said it already I'm not systematically against RTC, but I'm against
systematic RTC.

>
>>>
>>> It really worries me because we lose quality and it's not easy to detect errors
>> It never easy to detect errors. It needs a lot of work. Do you suggest that I put errors in code by negligence? Have a look at what I do, and you
>> will be convinced on the contrary. I track errors as much as I can and I help others to fix them when they are not mine.
>
> I did not say any of this nor do I suggest it.
>>> and changed functionality in such a complex project.
>> I repeat, again, I did not change any functionalities, hence the "No functional change". Prove the contrary...
>>> And don't rely too much on the tests as we don't have such a high test coverage.
>> I don't rely only on tests, but yes I also rely on them, who does not?
>>>
>>> Thanks for some more patience,
>> That I can understand, but not all the FUD above, and not going to RTC because of fear. If you have something to say, please comment the code and
>> detail the problems you see, then we can discuss...
>
> This statement tells me that you are not willing to collaborate. Instead, you call arguments for stability and quality assurance FUD and refuse to
> the request to revert your commit.

I don't "call argument for stability and quality assurance" FUD. I wanted to say that I'm still waiting your review and details about the issue you
found in my commit.

> Well, that's sad but everyone has his/her own way to contribute...
I'm not sad, I'm happy :) Happy committer is happy

As a last word: I know we both want to do good for OFBiz; so I think we should stop this thread here or with your last answer.

Jacques

>
> Michael
>
>>
>> Jacques
>>>
>>> Michael
>>>
>>>
>>> Am 24.03.17 um 14:13 schrieb Jacopo Cappellato:
>>>> On Fri, Mar 24, 2017 at 10:56 AM, Jacques Le Roux <
>>>> [hidden email]> wrote:
>>>>
>>>>> [...]
>>>>> 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?
>>>>
>>>> We are not negotiating: I have simply asked you to revert the changes in
>>>> which you have changed the functional behavior of the system without
>>>> testing OR test the new behavior and confirm it is working fine.
>>>>
>>>> In general I like the effort of improving this old code containing
>>>> swallowed exceptions by providing more comments, documentation etc... or
>>>> completely refactoring it; but this has to be done with proper testing.
>>>>
>>>> I hope this clarifies my request.
>>>>
>>>> Jacopo
>>>>
>>>
>>>
>>
>>
>
>

[Jacques Le Roux]

Le 30/03/2017 à 14:16, Jacques Le Roux a écrit :
> It still happens that I prefer to submit a patch and there are cases in Jira. https://issues.apache.org/jira/issues/?filter=12340482 Not all
> attachments are patches, but even if it's only a half+, it's still around 100 cases
Sorry, this is a personal filter I can only view, use rather https://issues.apache.org/jira/issues/?filter=12340487

Jacques