ofbiz-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Scott Gray <scott.g...@hotwaxmedia.com>
Subject Re: Bugs in OrderView.groovy
Date Mon, 01 Jun 2009 04:20:47 GMT
Just because the data model allows an order without a product store  
doesn't mean that the code does.  There are a million ways that you  
can cause errors in the system with incorrectly loaded data.

Regards
Scott

On 1/06/2009, at 8:56 AM, Cimballi wrote:

> Here is a data file which you can import and which will generate the
> null pointer exception when trying to view the order :
>
> <?xml version="1.0" encoding="UTF-8"?>
>
> <entity-engine-xml>
>
>  <OrderType description="Special Sales" hasTable="N"
>    orderTypeId="SPECIAL_SALES_ORDER" parentTypeId="SALES_ORDER" />
>
>  <OrderHeader orderId="OH0001" orderTypeId="SPECIAL_SALES_ORDER"
>    orderDate="2009-01-01 12:00:00.0" entryDate="2009-01-01 12:00:00.0"
>    statusId="ORDER_CREATED" />
>
>  <OrderRole orderId="OH0001" partyId="DemoCustomer"
>    roleTypeId="PLACING_CUSTOMER" />
>
> </entity-engine-xml>
>
> And the stack trace (the beginning) :
>
> 2009-05-31 15:54:22,417 (http-0.0.0.0-8443-1) [
> ControlServlet.java:204:ERROR]
> ---- exception report  
> ----------------------------------------------------------
> Error in request handler:
> Exception: org.ofbiz.widget.screen.ScreenRenderException
> Message: Error rendering screen
> [component://order/widget/ordermgr/ 
> OrderViewScreens.xml#OrderHeaderView]:
> java.lang.NullPointerException (null)
> ---- cause  
> ---------------------------------------------------------------------
> Exception: java.lang.NullPointerException
> Message: null
> ---- stack trace  
> ---------------------------------------------------------------
> java.lang.NullPointerException
> org 
> .codehaus 
> .groovy.runtime.InvokerHelper.getProperty(InvokerHelper.java:178)
> org 
> .codehaus 
> .groovy 
> .runtime 
> .ScriptBytecodeAdapter.getProperty(ScriptBytecodeAdapter.java:477)
> OrderView.run(OrderView.groovy:380)
> org.ofbiz.base.util.GroovyUtil.runScriptAtLocation(GroovyUtil.java: 
> 117)
> ...
>
> Cimballi
>
>
> On Sun, May 31, 2009 at 5:28 AM, Divesh Dutta
> <divesh.dutta@hotwaxmedia.com> wrote:
>> +1
>>
>> Thanks
>> --
>> Divesh
>>
>>
>> Ray wrote:
>>>
>>> Lots of talk about different types of contributors etc and it  
>>> should be
>>> noted there are also lots of types of bugs.
>>>
>>> Your other posts highlight specific: do this, do that, it crashes/ 
>>> doesn't
>>> provide the expected result. That's helpful and will tend to get a  
>>> response
>>> fairly quickly as they may not require as much time to verify and  
>>> fix.
>>>
>>> This post is very different and could be titled "Potential bugs in
>>> OrderView.groovy". I don't think anybody involved in OFBiz can  
>>> answer your
>>> post with out doing a full code review of the file.
>>>
>>> There are numerous possible reasons for including the first check  
>>> and not
>>> the second, it depends on lots of things and the file is split in  
>>> to several
>>> 'if' sections that may have a lot of impact on whether a  
>>> productStore is
>>> expected to be found. And in a file that is over 400 lines long it  
>>> could
>>> take some effort to assess and justify the one thing you've  
>>> highlighted
>>> before dealing with the "I didn't note all of them" others.
>>>
>>> I think with the:
>>>> If yes, why the first test ?
>>>> If no, there is missing a test in the second case.
>>> you might be over simplifying the problem as there are always other
>>> dependencies.
>>>
>>> So although it's a reasonable post to suggest there are problems  
>>> in the
>>> file the difficulty is you need someone else to volunteer a  
>>> reasonable
>>> amount of their own time to investigate, justify and fix a  
>>> potential issue.
>>> It's basically a retrospective code review that will take a lot of  
>>> effort,
>>> and carries it's own risks of introducing new problems.
>>>
>>> Generally code quality gets looked at when someone is working on a  
>>> file.
>>>
>>> Don't take it personally on this post but I suspect you won't get  
>>> someone
>>> jumping in and adding/removing a speculative 'if' wrapper as you are
>>> indirectly asking for quite a lot.
>>>
>>> On the other hand if you read the code and can produce a test case  
>>> that
>>> triggers a null pointer exception on line 380....
>>>
>>> Ray
>>>
>>>
>>> Cimballi wrote:
>>>>
>>>> Well, I don't know how to explain you, it seems evident to me...
>>>> In the first case you check if "productStore" is null or not. In  
>>>> the
>>>> second case, you don't check.
>>>> So, what is the correct behaviour ? Should an order be linked to a
>>>> productStore or not ?
>>>> If yes, why the first test ?
>>>> If no, there is missing a test in the second case.
>>>>
>>>>> From my point of view I would say no because an order with  
>>>>> products of
>>>>
>>>> type "service" don't need productStore.
>>>>
>>>> To Scott : you should consider there are different kind of
>>>> contributors on open source projects, I'm the kind of contributor  
>>>> who
>>>> send emails when I find something I think is a bug, I'm still not  
>>>> in
>>>> the category of "patch providers" ! :-)
>>>>
>>>> Cimballi
>>>>
>>>>
>>>> On Fri, May 29, 2009 at 7:16 PM, Scott Gray <scott.gray@hotwaxmedia.com

>>>> >
>>>> wrote:
>>>>>
>>>>> Hi Cimballi
>>>>>
>>>>> Inconsistencies aren't necessarily bugs, but you are most  
>>>>> welcome to
>>>>> create
>>>>> a patch and jira issue for the corrections you think should be  
>>>>> made.
>>>>>
>>>>> Thanks
>>>>> Scott
>>>>>
>>>>> On 30/05/2009, at 10:39 AM, Cimballi wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> There are several inconsistancies in the file
>>>>>>
>>>>>>
>>>>>> "applications/order/webapp/ordermgr/WEB-INF/actions/order/ 
>>>>>> OrderView.groovy".
>>>>>>
>>>>>> I didn't note all of them, but here is an example :
>>>>>>
>>>>>> Line 260, productStore can be null :
>>>>>>  productStore = orderHeader.getRelatedOne("ProductStore");
>>>>>>  if (productStore) {
>>>>>>      facility = productStore.getRelatedOne("Facility");
>>>>>>
>>>>>> Line 380, here productStore cannot be null :
>>>>>>  productStoreId =
>>>>>> orderHeader.getRelatedOne("ProductStore").productStoreId;
>>>>>>
>>>>>> Cimballi
>>>>>
>>>>
>>
>>


Mime
View raw message