Errors in screen while running findByAnd

[Adam Heath-2]

On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
> file are attached; that bug tracker is really awful.

This is all becoming very troubling.

== example methods
String a(String name, List list) {}
String a(String name, Map map) {}
String a(Object[] args) {}

String b(String name, List list) {}
String b(String name, Map map) {}
String b(Object... args) {}

== example templates
${object.a(string, null)}
${object.b(string, null)}

==

In 2.3.18, freemarker can't find any methods at all.  In 2.3.19, it can
find the b vararg method.  In either situation, that is wrong.

The correct situation for both is to say there are 2 matching methods,
the List and Map variants.  Without my patch, when a null is used, any
non-primitive method will never be considered as a possible candidate.

I'm still discussing with freemarker upstream, but I get the feeling
that they won't provide a fix for the 2.3 branch, and a true, correct
fix for 2.4 will be long in coming.

This means we need to come up with a workaround for ofbiz.  I'm up for
suggestions.  We really can't undo the change, that just doesn't seem
right to me.
.

[Adam Heath-2]

On 05/17/2012 11:50 PM, Adam Heath wrote:

> On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
>> file are attached; that bug tracker is really awful.
>
> This is all becoming very troubling.
>
> == example methods
> String a(String name, List list) {}
> String a(String name, Map map) {}
> String a(Object[] args) {}
>
> String b(String name, List list) {}
> String b(String name, Map map) {}
> String b(Object... args) {}
>
> == example templates
> ${object.a(string, null)}
> ${object.b(string, null)}
>
> ==
>
> In 2.3.18, freemarker can't find any methods at all. In 2.3.19, it can
> find the b vararg method. In either situation, that is wrong.
>
> The correct situation for both is to say there are 2 matching methods,
> the List and Map variants. Without my patch, when a null is used, any
> non-primitive method will never be considered as a possible candidate.
>
> I'm still discussing with freemarker upstream, but I get the feeling
> that they won't provide a fix for the 2.3 branch, and a true, correct
> fix for 2.4 will be long in coming.
>
> This means we need to come up with a workaround for ofbiz. I'm up for
> suggestions. We really can't undo the change, that just doesn't seem
> right to me.

In detail, using a null in a method call inside freemarker will be
*extremely* problematic.  Even if I rename my new findByAnd to smoething
else, passing a null to it will keep freemarker from finding it.

Imho, we should *not* be calling into the delegator inside freemarker
templates.  Templates should not be tied to the backend *at all*.  They
should only deal with 100% completely generic collection objects.
Because they are currently tied to the delegator, this makes it
*impossible* to have a separate web-frontend, that isn't tied to ofbiz
at all.

[Jacopo Cappellato-4]

On May 18, 2012, at 7:02 AM, Adam Heath wrote:

> Imho, we should *not* be calling into the delegator inside freemarker templates.  Templates should not be tied to the backend *at all*.  They should only deal with 100% completely generic collection objects. Because they are currently tied to the delegator, this makes it *impossible* to have a separate web-frontend, that isn't tied to ofbiz at all.

I agree on the general rule; unfortunately there are exceptions where enforcing this rule is too problematic; however I agree that the practice of calling the delegator API from FM is used much more than it should.
Apart from moving data preparation code to data preparation scripts where possible, we could also define a FM transform to implement these calls and this will protect the access to the delegator because in the transform we would perform all the necessary null checks etc...
The risk is to end up with a big sets of transforms that re-implement the delegator API, but we could start by implementing the calls that are currently clearly broken (findByAnd) and then create new ones (or refactor existing code) *before* you will refactor other delegator methods.
In this way we will have a temp fix (waiting for the official fix from FM) and also a better implementation of templates (that execute calls to delegator only thru a transform)

Kind regards,

Jacopo

[Adam Heath-2]

On 05/18/2012 12:52 AM, Jacopo Cappellato wrote:
>
> On May 18, 2012, at 7:02 AM, Adam Heath wrote:
>
>> Imho, we should *not* be calling into the delegator inside freemarker templates.  Templates should not be tied to the backend *at all*.  They should only deal with 100% completely generic collection objects. Because they are currently tied to the delegator, this makes it *impossible* to have a separate web-frontend, that isn't tied to ofbiz at all.
>
> I agree on the general rule; unfortunately there are exceptions where enforcing this rule is too problematic; however I agree that the practice of calling the delegator API from FM is used much more than it should.
> Apart from moving data preparation code to data preparation scripts where possible, we could also define a FM transform to implement these calls and this will protect the access to the delegator because in the transform we would perform all the necessary null checks etc...
> The risk is to end up with a big sets of transforms that re-implement the delegator API, but we could start by implementing the calls that are currently clearly broken (findByAnd) and then create new ones (or refactor existing code) *before* you will refactor other delegator methods.
> In this way we will have a temp fix (waiting for the official fix from FM) and also a better implementation of templates (that execute calls to delegator only thru a transform)

This problem with null parameter values in freemarker only occurs when
there are multiple methods with the same name.  If there is only a
single method with a name, then no overridden resolution logic happens.
  That's how the freemarker code functions.

So, if my new findByAnd variant gets a new name, then all can be well.
This is how find() and findList() got implemented, and why we never
noticed this freemarker problem before.

So, any suggestions for a new name?

[Jacopo Cappellato-4]

On May 18, 2012, at 7:56 AM, Adam Heath wrote:

> On 05/18/2012 12:52 AM, Jacopo Cappellato wrote:
>>
>> On May 18, 2012, at 7:02 AM, Adam Heath wrote:
>>
>>> Imho, we should *not* be calling into the delegator inside freemarker templates.  Templates should not be tied to the backend *at all*.  They should only deal with 100% completely generic collection objects. Because they are currently tied to the delegator, this makes it *impossible* to have a separate web-frontend, that isn't tied to ofbiz at all.
>>
>> I agree on the general rule; unfortunately there are exceptions where enforcing this rule is too problematic; however I agree that the practice of calling the delegator API from FM is used much more than it should.
>> Apart from moving data preparation code to data preparation scripts where possible, we could also define a FM transform to implement these calls and this will protect the access to the delegator because in the transform we would perform all the necessary null checks etc...
>> The risk is to end up with a big sets of transforms that re-implement the delegator API, but we could start by implementing the calls that are currently clearly broken (findByAnd) and then create new ones (or refactor existing code) *before* you will refactor other delegator methods.
>> In this way we will have a temp fix (waiting for the official fix from FM) and also a better implementation of templates (that execute calls to delegator only thru a transform)
>
> This problem with null parameter values in freemarker only occurs when there are multiple methods with the same name.  If there is only a single method with a name, then no overridden resolution logic happens.  That's how the freemarker code functions.
>
> So, if my new findByAnd variant gets a new name, then all can be well. This is how find() and findList() got implemented, and why we never noticed this freemarker problem before.
>
> So, any suggestions for a new name?

Would it make sense to wrap the delegator before adding it to the context of Freemarker? In this way we could "fix" the call for freemarker only and keep the official API as clean as possible.

Jacopo

[Adrian Crum-3]

On 5/18/2012 5:50 AM, Adam Heath wrote:

> On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
>> file are attached; that bug tracker is really awful.
>
> This is all becoming very troubling.
>
> == example methods
> String a(String name, List list) {}
> String a(String name, Map map) {}
> String a(Object[] args) {}
>
> String b(String name, List list) {}
> String b(String name, Map map) {}
> String b(Object... args) {}
>
> == example templates
> ${object.a(string, null)}
> ${object.b(string, null)}
>
> ==
>
> In 2.3.18, freemarker can't find any methods at all.  In 2.3.19, it
> can find the b vararg method.  In either situation, that is wrong.
>
> The correct situation for both is to say there are 2 matching methods,
> the List and Map variants.  Without my patch, when a null is used, any
> non-primitive method will never be considered as a possible candidate.
>
> I'm still discussing with freemarker upstream, but I get the feeling
> that they won't provide a fix for the 2.3 branch, and a true, correct
> fix for 2.4 will be long in coming.
>
> This means we need to come up with a workaround for ofbiz.  I'm up for
> suggestions.  We really can't undo the change, that just doesn't seem
> right to me.
> .

In my experience with FreeMarker, there are a lot of things that are
replaceable - like TemplateLoader and TemplateExceptionHandler. Does
FreeMarker have a "Object Method Resolver" that we can replace? Would a
custom BeansWrapper help?

-Adrian

[Deepak Dixit-2]

Hello Adam,

We can't pass *null* from ftl if we have to pass null then we can use *NULL* like:

<#assign productStroe = delegator.findList("ProductStore",NULL,NULL,NULL,NULL, false) />

Passing *null* is prohibited in NodeListModel class (http://www.docjar.org/docs/api/freemarker/ext/xml/NodeListModel.html).

To create an empty model, pass it an empty collection. If we pass an empty list in new findByAnd method then it should work fine:

<#assign shipmentReceipts = delegator.findByAnd("ShipmentReceipt", {"orderId": orderHeader.getString("orderId"), "orderItemSeqId": orderItem.orderItemSeqId}, [], false)/>

I have tested it and its working fine.


Thanks & Regards
--
Deepak Dixit

On May 18, 2012, at 1:15 PM, Adrian Crum wrote:

> On 5/18/2012 5:50 AM, Adam Heath wrote:
>> On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
>>> file are attached; that bug tracker is really awful.
>>
>> This is all becoming very troubling.
>>
>> == example methods
>> String a(String name, List list) {}
>> String a(String name, Map map) {}
>> String a(Object[] args) {}
>>
>> String b(String name, List list) {}
>> String b(String name, Map map) {}
>> String b(Object... args) {}
>>
>> == example templates
>> ${object.a(string, null)}
>> ${object.b(string, null)}
>>
>> ==
>>
>> In 2.3.18, freemarker can't find any methods at all.  In 2.3.19, it can find the b vararg method.  In either situation, that is wrong.
>>
>> The correct situation for both is to say there are 2 matching methods, the List and Map variants.  Without my patch, when a null is used, any non-primitive method will never be considered as a possible candidate.
>>
>> I'm still discussing with freemarker upstream, but I get the feeling that they won't provide a fix for the 2.3 branch, and a true, correct fix for 2.4 will be long in coming.
>>
>> This means we need to come up with a workaround for ofbiz.  I'm up for suggestions.  We really can't undo the change, that just doesn't seem right to me.
>> .
>
> In my experience with FreeMarker, there are a lot of things that are replaceable - like TemplateLoader and TemplateExceptionHandler. Does FreeMarker have a "Object Method Resolver" that we can replace? Would a custom BeansWrapper help?
>
> -Adrian
>

[Jacopo Cappellato-4]

that API is actually rather old and the class has been deprecated and then reimplemented.

Jacopo

On May 18, 2012, at 10:13 AM, Deepak Dixit wrote:

> Hello Adam,
>
> We can't pass *null* from ftl if we have to pass null then we can use *NULL* like:
>
> <#assign productStroe = delegator.findList("ProductStore",NULL,NULL,NULL,NULL, false) />
>
> Passing *null* is prohibited in NodeListModel class (http://www.docjar.org/docs/api/freemarker/ext/xml/NodeListModel.html).
>
> To create an empty model, pass it an empty collection. If we pass an empty list in new findByAnd method then it should work fine:
>
> <#assign shipmentReceipts = delegator.findByAnd("ShipmentReceipt", {"orderId": orderHeader.getString("orderId"), "orderItemSeqId": orderItem.orderItemSeqId}, [], false)/>
>
> I have tested it and its working fine.
>
>
> Thanks & Regards
> --
> Deepak Dixit
>
> On May 18, 2012, at 1:15 PM, Adrian Crum wrote:
>
>> On 5/18/2012 5:50 AM, Adam Heath wrote:
>>> On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
>>>> file are attached; that bug tracker is really awful.
>>>
>>> This is all becoming very troubling.
>>>
>>> == example methods
>>> String a(String name, List list) {}
>>> String a(String name, Map map) {}
>>> String a(Object[] args) {}
>>>
>>> String b(String name, List list) {}
>>> String b(String name, Map map) {}
>>> String b(Object... args) {}
>>>
>>> == example templates
>>> ${object.a(string, null)}
>>> ${object.b(string, null)}
>>>
>>> ==
>>>
>>> In 2.3.18, freemarker can't find any methods at all.  In 2.3.19, it can find the b vararg method.  In either situation, that is wrong.
>>>
>>> The correct situation for both is to say there are 2 matching methods, the List and Map variants.  Without my patch, when a null is used, any non-primitive method will never be considered as a possible candidate.
>>>
>>> I'm still discussing with freemarker upstream, but I get the feeling that they won't provide a fix for the 2.3 branch, and a true, correct fix for 2.4 will be long in coming.
>>>
>>> This means we need to come up with a workaround for ofbiz.  I'm up for suggestions.  We really can't undo the change, that just doesn't seem right to me.
>>> .
>>
>> In my experience with FreeMarker, there are a lot of things that are replaceable - like TemplateLoader and TemplateExceptionHandler. Does FreeMarker have a "Object Method Resolver" that we can replace? Would a custom BeansWrapper help?
>>
>> -Adrian
>>
>

[Adam Heath-2]

On 05/18/2012 03:13 AM, Deepak Dixit wrote:

> Hello Adam,
>
> We can't pass *null* from ftl if we have to pass null then we can use *NULL* like:
>
> <#assign productStroe = delegator.findList("ProductStore",NULL,NULL,NULL,NULL, false) />
>
> Passing *null* is prohibited in NodeListModel class (http://www.docjar.org/docs/api/freemarker/ext/xml/NodeListModel.html).
>
> To create an empty model, pass it an empty collection. If we pass an empty list in new findByAnd method then it should work fine:
>
> <#assign shipmentReceipts = delegator.findByAnd("ShipmentReceipt", {"orderId": orderHeader.getString("orderId"), "orderItemSeqId": orderItem.orderItemSeqId}, [], false)/>

I thought of this, and rejected it.  It will cause more memory churn
than is really nescessary.  All the dummy empty objects.  The same
goes for Static['Collections'].EMPTY_MAP, as that has too much runtime
method invocation overhead.

Altho, maybe if ofbiz auto-inserted EMPTY_MAP and EMPTY_LIST, but even
that would require a context variable lookup, for what really should
be a *real* constant value.

And, I do not want to *have* to modify *every* *single* *ftl* line
again that calls the new findByAnd.

[Adam Heath-2]

On 05/18/2012 02:45 AM, Adrian Crum wrote:

> On 5/18/2012 5:50 AM, Adam Heath wrote:
>> On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
>>> file are attached; that bug tracker is really awful.
>>
>> This is all becoming very troubling.
>>
>> == example methods
>> String a(String name, List list) {}
>> String a(String name, Map map) {}
>> String a(Object[] args) {}
>>
>> String b(String name, List list) {}
>> String b(String name, Map map) {}
>> String b(Object... args) {}
>>
>> == example templates
>> ${object.a(string, null)}
>> ${object.b(string, null)}
>>
>> ==
>>
>> In 2.3.18, freemarker can't find any methods at all.  In 2.3.19, it
>> can find the b vararg method.  In either situation, that is wrong.
>>
>> The correct situation for both is to say there are 2 matching
>> methods, the List and Map variants.  Without my patch, when a null
>> is used, any non-primitive method will never be considered as a
>> possible candidate.
>>
>> I'm still discussing with freemarker upstream, but I get the feeling
>> that they won't provide a fix for the 2.3 branch, and a true,
>> correct fix for 2.4 will be long in coming.
>>
>> This means we need to come up with a workaround for ofbiz.  I'm up
>> for suggestions.  We really can't undo the change, that just doesn't
>> seem right to me.
>> .
>
> In my experience with FreeMarker, there are a lot of things that are
> replaceable - like TemplateLoader and TemplateExceptionHandler. Does
> FreeMarker have a "Object Method Resolver" that we can replace? Would
> a custom BeansWrapper help?

I'm currently in discussion with upstream(1), they may give me commit
rights after I sign a CLA.

Their suggestion right now is a new property on BeansWrapper.

1:
https://sourceforge.net/tracker/?func=detail&aid=3527625&group_id=794&atid=100794#

[Adam Heath-2]

On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
> file are attached; that bug tracker is really awful.

I've got another one to attach, a more proper fix, that makes the
change configurable.

ps: Have I said how much I hate that tracker?

[Jacopo Cappellato-4]

doing it right now

Jacopo

On May 18, 2012, at 6:01 PM, Adam Heath wrote:

> On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
>> file are attached; that bug tracker is really awful.
>
> I've got another one to attach, a more proper fix, that makes the
> change configurable.
>
> ps: Have I said how much I hate that tracker?
> <freemarker-nullArgs-fix.patch>

[Jacopo Cappellato-4]

Sorry,

it took some time because I was in a call.
The patch is attached now.

Your secretary,

Jacopo

On May 18, 2012, at 6:01 PM, Adam Heath wrote:

> On 05/17/2012 12:23 PM, Jacopo Cappellato wrote:
>> file are attached; that bug tracker is really awful.
>
> I've got another one to attach, a more proper fix, that makes the
> change configurable.
>
> ps: Have I said how much I hate that tracker?
> <freemarker-nullArgs-fix.patch>