Re: svn commit: r1789710 - /ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy

[Michael Brohl-3]

Hi Jacques,

I think this is a functional change because you not only print the
exception to the error log but you also put it in the errorMsgList. In
this way, the error is printed to the user interface. This might not be
intended because the VIEW_SIZE controls the number of entries in the
before it is paginated and is normally set to a default if missing or
wrong. No need to bring this to the UI. [1]

Additionally, it is not translated because the error message is hard
coded which is not acceptable IMO. [2]

I would change the implementation according to the viewIndex block at
the beginning (catching the exception and setting a default value, which
should be read from the configuration file as it is done in other places).

Looking further, there are other weeknesses, e.g. the errorMsgList is
filled after it is already put in the request attribute
_ERROR_MESSAGE_LIST_.

This whole file must be refactored.

Please revert or change your changes at least for [1] or [2].

Thanks,

Michael


Am 31.03.17 um 18:56 schrieb [hidden email]:

> Author: jleroux
> Date: Fri Mar 31 16:56:04 2017
> New Revision: 1789710
>
> URL: http://svn.apache.org/viewvc?rev=1789710&view=rev
> Log:
> Fixed: Fix Default or Empty Catch block in Java and Groovy files
> (OFBIZ-8341)
>
> While working on OFBIZ-7759, I stumbled upon this by chance.
> It's unlikely that this NumberFormatException is ever caught, but not a reason
> to swallow it.
>
> I checked there are no other swallowed exceptions in Groovy files.
>
> Modified:
>      ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy?rev=1789710&r1=1789709&r2=1789710&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 16:56:04 2017
> @@ -69,7 +69,8 @@ context.curFindString = curFindString
>   try {
>       viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).intValue()
>   } catch (NumberFormatException nfe) {
> -
> +    Debug.logError(nfe, "Caught an exception : " + nfe.toString(), "GetContentLookupList.groovy")
> +    errMsgList.add("Entered value is non-numeric for numeric field: " + field.getName())
>   }
>  
>   context.viewSize = viewSize
>
>

[Jacques Le Roux]

Hi Michael,
Le 31/03/2017 à 20:00, Michael Brohl a écrit :
> Hi Jacques,
>
> I think this is a functional change because you not only print the exception to the error log but you also put it in the errorMsgList. In this way,
> the error is printed to the user interface. This might not be intended because the VIEW_SIZE controls the number of entries in the before it is
> paginated and is normally set to a default if missing or wrong. No need to bring this to the UI. [1]

You are right, no need to put that in the UI. This also resolves [2] and the need to put in the request attribute _ERROR_MESSAGE_LIST_.
Actually it's very unlikely that we ever get this issue, but it's possible in theory, so I agree the log is enough.
So no need to revert, I'll just remove this 2nd line in the catch.

Jacques

>
> Additionally, it is not translated because the error message is hard coded which is not acceptable IMO. [2]
>
> I would change the implementation according to the viewIndex block at the beginning (catching the exception and setting a default value, which
> should be read from the configuration file as it is done in other places).
>
> Looking further, there are other weeknesses, e.g. the errorMsgList is filled after it is already put in the request attribute _ERROR_MESSAGE_LIST_.
>
> This whole file must be refactored.
>
> Please revert or change your changes at least for [1] or [2].
>
> Thanks,
>
> Michael
>
>
> Am 31.03.17 um 18:56 schrieb [hidden email]:
>> Author: jleroux
>> Date: Fri Mar 31 16:56:04 2017
>> New Revision: 1789710
>>
>> URL: http://svn.apache.org/viewvc?rev=1789710&view=rev
>> Log:
>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>> (OFBIZ-8341)
>>
>> While working on OFBIZ-7759, I stumbled upon this by chance.
>> It's unlikely that this NumberFormatException is ever caught, but not a reason
>> to swallow it.
>>
>> I checked there are no other swallowed exceptions in Groovy files.
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy?rev=1789710&r1=1789709&r2=1789710&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 16:56:04 2017
>> @@ -69,7 +69,8 @@ context.curFindString = curFindString
>>   try {
>>       viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).intValue()
>>   } catch (NumberFormatException nfe) {
>> -
>> +    Debug.logError(nfe, "Caught an exception : " + nfe.toString(), "GetContentLookupList.groovy")
>> +    errMsgList.add("Entered value is non-numeric for numeric field: " + field.getName())
>>   }
>>     context.viewSize = viewSize
>>
>>
>
>

[Jacques Le Roux]

BTW I forgot to thank you for your review.

Jacques


Le 31/03/2017 à 21:17, Jacques Le Roux a écrit :

> Hi Michael,
> Le 31/03/2017 à 20:00, Michael Brohl a écrit :
>> Hi Jacques,
>>
>> I think this is a functional change because you not only print the exception to the error log but you also put it in the errorMsgList. In this way,
>> the error is printed to the user interface. This might not be intended because the VIEW_SIZE controls the number of entries in the before it is
>> paginated and is normally set to a default if missing or wrong. No need to bring this to the UI. [1]
>
> You are right, no need to put that in the UI. This also resolves [2] and the need to put in the request attribute _ERROR_MESSAGE_LIST_.
> Actually it's very unlikely that we ever get this issue, but it's possible in theory, so I agree the log is enough.
> So no need to revert, I'll just remove this 2nd line in the catch.
>
> Jacques
>>
>> Additionally, it is not translated because the error message is hard coded which is not acceptable IMO. [2]
>>
>> I would change the implementation according to the viewIndex block at the beginning (catching the exception and setting a default value, which
>> should be read from the configuration file as it is done in other places).
>>
>> Looking further, there are other weeknesses, e.g. the errorMsgList is filled after it is already put in the request attribute _ERROR_MESSAGE_LIST_.
>>
>> This whole file must be refactored.
>>
>> Please revert or change your changes at least for [1] or [2].
>>
>> Thanks,
>>
>> Michael
>>
>>
>> Am 31.03.17 um 18:56 schrieb [hidden email]:
>>> Author: jleroux
>>> Date: Fri Mar 31 16:56:04 2017
>>> New Revision: 1789710
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1789710&view=rev
>>> Log:
>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>> (OFBIZ-8341)
>>>
>>> While working on OFBIZ-7759, I stumbled upon this by chance.
>>> It's unlikely that this NumberFormatException is ever caught, but not a reason
>>> to swallow it.
>>>
>>> I checked there are no other swallowed exceptions in Groovy files.
>>>
>>> Modified:
>>> ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy?rev=1789710&r1=1789709&r2=1789710&view=diff
>>> ==============================================================================
>>> --- ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy (original)
>>> +++ ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 16:56:04 2017
>>> @@ -69,7 +69,8 @@ context.curFindString = curFindString
>>>   try {
>>>       viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).intValue()
>>>   } catch (NumberFormatException nfe) {
>>> -
>>> +    Debug.logError(nfe, "Caught an exception : " + nfe.toString(), "GetContentLookupList.groovy")
>>> +    errMsgList.add("Entered value is non-numeric for numeric field: " + field.getName())
>>>   }
>>>     context.viewSize = viewSize
>>>
>>>
>>
>>
>
>

[Michael Brohl-3]

BTW, this groovy implementation is a good argument for RTC. This code
should have never been committed.

Regards,

Michael

Am 31.03.17 um 20:00 schrieb Michael Brohl:

> Hi Jacques,
>
> I think this is a functional change because you not only print the
> exception to the error log but you also put it in the errorMsgList. In
> this way, the error is printed to the user interface. This might not
> be intended because the VIEW_SIZE controls the number of entries in
> the before it is paginated and is normally set to a default if missing
> or wrong. No need to bring this to the UI. [1]
>
> Additionally, it is not translated because the error message is hard
> coded which is not acceptable IMO. [2]
>
> I would change the implementation according to the viewIndex block at
> the beginning (catching the exception and setting a default value,
> which should be read from the configuration file as it is done in
> other places).
>
> Looking further, there are other weeknesses, e.g. the errorMsgList is
> filled after it is already put in the request attribute
> _ERROR_MESSAGE_LIST_.
>
> This whole file must be refactored.
>
> Please revert or change your changes at least for [1] or [2].
>
> Thanks,
>
> Michael
>
>
> Am 31.03.17 um 18:56 schrieb [hidden email]:
>> Author: jleroux
>> Date: Fri Mar 31 16:56:04 2017
>> New Revision: 1789710
>>
>> URL: http://svn.apache.org/viewvc?rev=1789710&view=rev
>> Log:
>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>> (OFBIZ-8341)
>>
>> While working on OFBIZ-7759, I stumbled upon this by chance.
>> It's unlikely that this NumberFormatException is ever caught, but not
>> a reason
>> to swallow it.
>>
>> I checked there are no other swallowed exceptions in Groovy files.
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy?rev=1789710&r1=1789709&r2=1789710&view=diff
>> ==============================================================================
>>
>> ---
>> ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>> (original)
>> +++
>> ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>> Fri Mar 31 16:56:04 2017
>> @@ -69,7 +69,8 @@ context.curFindString = curFindString
>>   try {
>>       viewSize =
>> Integer.valueOf((String)parameters.get("VIEW_SIZE")).intValue()
>>   } catch (NumberFormatException nfe) {
>> -
>> +    Debug.logError(nfe, "Caught an exception : " + nfe.toString(),
>> "GetContentLookupList.groovy")
>> +    errMsgList.add("Entered value is non-numeric for numeric field:
>> " + field.getName())
>>   }
>>     context.viewSize = viewSize
>>
>>
>
>

[Jacques Le Roux]

Things got diluted then https://lists.apache.org/list.html?dev@...:gte=5y:815651

It's hard to review long patches, for instance have a look at https://issues.apache.org/jira/browse/OFBIZ-9254
In this case it might look simple, but it's kinda hypnotic. Looking always at the same pattern, at some point you can easily miss a typo or such.

It does not mean that we should not...

Jacques


Le 31/03/2017 à 22:25, Michael Brohl a écrit :

> BTW, this groovy implementation is a good argument for RTC. This code should have never been committed.
>
> Regards,
>
> Michael
>
> Am 31.03.17 um 20:00 schrieb Michael Brohl:
>> Hi Jacques,
>>
>> I think this is a functional change because you not only print the exception to the error log but you also put it in the errorMsgList. In this way,
>> the error is printed to the user interface. This might not be intended because the VIEW_SIZE controls the number of entries in the before it is
>> paginated and is normally set to a default if missing or wrong. No need to bring this to the UI. [1]
>>
>> Additionally, it is not translated because the error message is hard coded which is not acceptable IMO. [2]
>>
>> I would change the implementation according to the viewIndex block at the beginning (catching the exception and setting a default value, which
>> should be read from the configuration file as it is done in other places).
>>
>> Looking further, there are other weeknesses, e.g. the errorMsgList is filled after it is already put in the request attribute _ERROR_MESSAGE_LIST_.
>>
>> This whole file must be refactored.
>>
>> Please revert or change your changes at least for [1] or [2].
>>
>> Thanks,
>>
>> Michael
>>
>>
>> Am 31.03.17 um 18:56 schrieb [hidden email]:
>>> Author: jleroux
>>> Date: Fri Mar 31 16:56:04 2017
>>> New Revision: 1789710
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1789710&view=rev
>>> Log:
>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>>> (OFBIZ-8341)
>>>
>>> While working on OFBIZ-7759, I stumbled upon this by chance.
>>> It's unlikely that this NumberFormatException is ever caught, but not a reason
>>> to swallow it.
>>>
>>> I checked there are no other swallowed exceptions in Groovy files.
>>>
>>> Modified:
>>> ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy?rev=1789710&r1=1789709&r2=1789710&view=diff
>>> ==============================================================================
>>> --- ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy (original)
>>> +++ ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 16:56:04 2017
>>> @@ -69,7 +69,8 @@ context.curFindString = curFindString
>>>   try {
>>>       viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).intValue()
>>>   } catch (NumberFormatException nfe) {
>>> -
>>> +    Debug.logError(nfe, "Caught an exception : " + nfe.toString(), "GetContentLookupList.groovy")
>>> +    errMsgList.add("Entered value is non-numeric for numeric field: " + field.getName())
>>>   }
>>>     context.viewSize = viewSize
>>>
>>>
>>
>>
>
>