You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Jacques Le Roux <ja...@les7arts.com> on 2017/03/31 19:17:41 UTC
Re: svn commit: r1789710 -
/ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/Get
ContentLookupList.groovy
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 jleroux@apache.org:
>> 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
>>
>>
>
>
Re: svn commit: r1789710 -
/ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/Get
ContentLookupList.groovy
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 jleroux@apache.org:
>>> 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
>>>
>>>
>>
>>
>
>