findByAnd update: <#assign /> broken

[Adam Heath-2]

Why do SimpleMethod.java and ModelScreen.java both do
context.put("null", GenericEntity.NULL_FIELD)?  That changes *all* null
values in freemarker into that static instance, even when the target
type of the variable is *not* going to be part of some entity-like map.

If I remove both of those calls(actually, remove one, change the other
to place it as "nullField"), then the broken ordermgr link given earlier
starts to finally work.  And, I think that even the nullField constant
should not be removed, as NullField should only be used internally by
the entity engine.

[Jacques Le Roux]

From: "Adam Heath" <[hidden email]>
> Why do SimpleMethod.java and ModelScreen.java both do
> context.put("null", GenericEntity.NULL_FIELD)?

For ModelScreen.java.renderScreenString(), and SimpleMethod.exec()
I just can say that it's there for age, at least since we came out of ASF incubation.

ModelScreen.java.renderScreenString(), the comment says
// make sure the "null" object is in there for entity ops

in SimpleMethod.exec(), I read
// always put the null field object in as "null"
methodContext.putEnv("null", GenericEntity.NULL_FIELD);
methodContext.putEnv("nullField", GenericEntity.NULL_FIELD);

I guess it's also related to entity ops.

I agree that the use of GenericEntity.NULL_FIELD in relation with null is not consistent in OFBiz and that's an issue. I already
crossed it working on
https://issues.apache.org/jira/browse/OFBIZ-4602 (totally unrelated though; there is a discrepancy between entitysync and services)

>That changes *all* null
> values in freemarker into that static instance, even when the target
> type of the variable is *not* going to be part of some entity-like map.
>
> If I remove both of those calls(actually, remove one, change the other
> to place it as "nullField"), then the broken ordermgr link given earlier
> starts to finally work.  And, I think that even the nullField constant
> should not be removed, as NullField should only be used internally by
> the entity engine.

From ScreenRenderer.populateBasicContext(), where I read
// make sure the "nullField" object is in there for entity ops; note this is nullField and not null because as null causes problems
in FreeMarker and such...
context.put("nullField", GenericEntity.NULL_FIELD);

I think you are heading in the right direction. But I wonder if you will not discover other issues (like I did with OFBIZ-4602)

HTH

Jacques

[Adrian Crum-3]

That is an ugly workaround that I would like to see go away. Removing it
could potentially break a lot of things.

-Adrian

On 5/21/2012 6:48 AM, Adam Heath wrote:

> Why do SimpleMethod.java and ModelScreen.java both do
> context.put("null", GenericEntity.NULL_FIELD)?  That changes *all*
> null values in freemarker into that static instance, even when the
> target type of the variable is *not* going to be part of some
> entity-like map.
>
> If I remove both of those calls(actually, remove one, change the other
> to place it as "nullField"), then the broken ordermgr link given
> earlier starts to finally work.  And, I think that even the nullField
> constant should not be removed, as NullField should only be used
> internally by the entity engine.

[Adam Heath-2]

On 05/21/2012 07:41 AM, Adrian Crum wrote:
> That is an ugly workaround that I would like to see go away. Removing it
> could potentially break a lot of things.

Do you mean that putting "null" into the context is an ugly work-around,
and that it should not be done, as I suggest?  And are you also saying
that removing it would cause breakage on pages that depend on "null"
resolving to NullField?

>
> -Adrian
>
> On 5/21/2012 6:48 AM, Adam Heath wrote:
>> Why do SimpleMethod.java and ModelScreen.java both do
>> context.put("null", GenericEntity.NULL_FIELD)? That changes *all* null
>> values in freemarker into that static instance, even when the target
>> type of the variable is *not* going to be part of some entity-like map.
>>
>> If I remove both of those calls(actually, remove one, change the other
>> to place it as "nullField"), then the broken ordermgr link given
>> earlier starts to finally work. And, I think that even the nullField
>> constant should not be removed, as NullField should only be used
>> internally by the entity engine.

[Jacques Le Roux]

From: "Adrian Crum" <[hidden email]>
> That is an ugly workaround that I would like to see go away.

Yes I'd also love to have things consistent between entitysync and services. https://issues.apache.org/jira/browse/OFBIZ-4602

I introduced "xsi:nil", "true" in XmlSerializer for SOAP  and use it for null values in Map

See makeElement in
 http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/serialize/XmlSerializer.java?r1=1243026&r2=1243025&pathrev=1243026Comments at http://svn.apache.org/viewvc?view=revision&revision=1243026I feel it's the way to go, but I also know that there are some pitfalls ahead...Jacques>Removing it could potentially break a lot of things.>> -Adrian>> On 5/21/2012 6:48 AM, Adam Heath wrote:>> Why do SimpleMethod.java and ModelScreen.java both do context.put("null", GenericEntity.NULL_FIELD)?  That changes *all* nullvalues in freemarker into that static instance, even when the target type of the variable is *not* going to be part of someentity-like map.>>>> If I remove both of those calls(actually, remove one, change the other to place it as "nullField"), then the broken ordermgr linkgiven earlier starts to finally work.  And, I think that even the nullField constant should not be removed, as NullField should onlybe used internally by the entity engine.

[Adam Heath-2]

On 05/21/2012 09:09 AM, Jacques Le Roux wrote:
> From: "Adrian Crum" <[hidden email]>
>> That is an ugly workaround that I would like to see go away.
>
> Yes I'd also love to have things consistent between entitysync and
> services. https://issues.apache.org/jira/browse/OFBIZ-4602

I just realized, that we should probably remove the "null" anyways, as
we aren't consistent.  The original problem I was fixing with freemarker
did *not* have this [null-field] issue.  This was because it was part of
a ${} expansion, which was *not* going thru the 2 classes I mentioned.
Which means we had an inconsistency.

The <#assign/> *does* go thru these classes, so that's why this breaks
in a different way.

ps: All of these changes will need to be backported to 12.04.  I'll be
doing that when the time is right.

[Adrian Crum-3]

I meant putting GenericEntity.NULL_FIELD in the map with the "null" key.
In other words, I am in favor of your changes. Sorry for the confusion.

-Adrian

On 5/21/2012 2:30 PM, Adam Heath wrote:

> On 05/21/2012 07:41 AM, Adrian Crum wrote:
>> That is an ugly workaround that I would like to see go away. Removing it
>> could potentially break a lot of things.
>
> Do you mean that putting "null" into the context is an ugly
> work-around, and that it should not be done, as I suggest?  And are
> you also saying that removing it would cause breakage on pages that
> depend on "null" resolving to NullField?
>
>>
>> -Adrian
>>
>> On 5/21/2012 6:48 AM, Adam Heath wrote:
>>> Why do SimpleMethod.java and ModelScreen.java both do
>>> context.put("null", GenericEntity.NULL_FIELD)? That changes *all* null
>>> values in freemarker into that static instance, even when the target
>>> type of the variable is *not* going to be part of some entity-like map.
>>>
>>> If I remove both of those calls(actually, remove one, change the other
>>> to place it as "nullField"), then the broken ordermgr link given
>>> earlier starts to finally work. And, I think that even the nullField
>>> constant should not be removed, as NullField should only be used
>>> internally by the entity engine.
>

[Adam Heath-2]

On 05/21/2012 09:36 AM, Adrian Crum wrote:
> I meant putting GenericEntity.NULL_FIELD in the map with the "null" key.
> In other words, I am in favor of your changes. Sorry for the confusion.

You still didn't answer how removing it could break things.  Unless you
meant that was a thinko.  And if it was, you didn't correct it.

I'm about to commit my changes, except for the "null" stuff.  If I get
your ok, I'd commit that too.

>
> -Adrian
>
> On 5/21/2012 2:30 PM, Adam Heath wrote:
>> On 05/21/2012 07:41 AM, Adrian Crum wrote:
>>> That is an ugly workaround that I would like to see go away. Removing it
>>> could potentially break a lot of things.
>>
>> Do you mean that putting "null" into the context is an ugly
>> work-around, and that it should not be done, as I suggest? And are you
>> also saying that removing it would cause breakage on pages that depend
>> on "null" resolving to NullField?
>>
>>>
>>> -Adrian
>>>
>>> On 5/21/2012 6:48 AM, Adam Heath wrote:
>>>> Why do SimpleMethod.java and ModelScreen.java both do
>>>> context.put("null", GenericEntity.NULL_FIELD)? That changes *all* null
>>>> values in freemarker into that static instance, even when the target
>>>> type of the variable is *not* going to be part of some entity-like map.
>>>>
>>>> If I remove both of those calls(actually, remove one, change the other
>>>> to place it as "nullField"), then the broken ordermgr link given
>>>> earlier starts to finally work. And, I think that even the nullField
>>>> constant should not be removed, as NullField should only be used
>>>> internally by the entity engine.
>>

[Adrian Crum-3]

As far as Mini-language is concerned, I think it only affects the
comparison operators. But those comparisons should specifically check
for null, not compare something to GenericEntity.NULL_FIELD. Let me look
at them again.

-Adrian

On 5/21/2012 3:38 PM, Adam Heath wrote:

> On 05/21/2012 09:36 AM, Adrian Crum wrote:
>> I meant putting GenericEntity.NULL_FIELD in the map with the "null" key.
>> In other words, I am in favor of your changes. Sorry for the confusion.
>
> You still didn't answer how removing it could break things.  Unless
> you meant that was a thinko.  And if it was, you didn't correct it.
>
> I'm about to commit my changes, except for the "null" stuff.  If I get
> your ok, I'd commit that too.
>
>>
>> -Adrian
>>
>> On 5/21/2012 2:30 PM, Adam Heath wrote:
>>> On 05/21/2012 07:41 AM, Adrian Crum wrote:
>>>> That is an ugly workaround that I would like to see go away.
>>>> Removing it
>>>> could potentially break a lot of things.
>>>
>>> Do you mean that putting "null" into the context is an ugly
>>> work-around, and that it should not be done, as I suggest? And are you
>>> also saying that removing it would cause breakage on pages that depend
>>> on "null" resolving to NullField?
>>>
>>>>
>>>> -Adrian
>>>>
>>>> On 5/21/2012 6:48 AM, Adam Heath wrote:
>>>>> Why do SimpleMethod.java and ModelScreen.java both do
>>>>> context.put("null", GenericEntity.NULL_FIELD)? That changes *all*
>>>>> null
>>>>> values in freemarker into that static instance, even when the target
>>>>> type of the variable is *not* going to be part of some entity-like
>>>>> map.
>>>>>
>>>>> If I remove both of those calls(actually, remove one, change the
>>>>> other
>>>>> to place it as "nullField"), then the broken ordermgr link given
>>>>> earlier starts to finally work. And, I think that even the nullField
>>>>> constant should not be removed, as NullField should only be used
>>>>> internally by the entity engine.
>>>
>

[Adrian Crum-3]

Okay, I think we're safe. I recently refactored all of that to make it
more reliable. If something breaks, the places to look are the
org.ofbiz.minilang.method.conditional.Compare class and
org.ofbiz.minilang.MiniLangUtil.convertType(...) method.

-Adrian

On 5/21/2012 3:42 PM, Adrian Crum wrote:

> As far as Mini-language is concerned, I think it only affects the
> comparison operators. But those comparisons should specifically check
> for null, not compare something to GenericEntity.NULL_FIELD. Let me
> look at them again.
>
> -Adrian
>
> On 5/21/2012 3:38 PM, Adam Heath wrote:
>> On 05/21/2012 09:36 AM, Adrian Crum wrote:
>>> I meant putting GenericEntity.NULL_FIELD in the map with the "null"
>>> key.
>>> In other words, I am in favor of your changes. Sorry for the confusion.
>>
>> You still didn't answer how removing it could break things.  Unless
>> you meant that was a thinko.  And if it was, you didn't correct it.
>>
>> I'm about to commit my changes, except for the "null" stuff.  If I
>> get your ok, I'd commit that too.
>>
>>>
>>> -Adrian
>>>
>>> On 5/21/2012 2:30 PM, Adam Heath wrote:
>>>> On 05/21/2012 07:41 AM, Adrian Crum wrote:
>>>>> That is an ugly workaround that I would like to see go away.
>>>>> Removing it
>>>>> could potentially break a lot of things.
>>>>
>>>> Do you mean that putting "null" into the context is an ugly
>>>> work-around, and that it should not be done, as I suggest? And are you
>>>> also saying that removing it would cause breakage on pages that depend
>>>> on "null" resolving to NullField?
>>>>
>>>>>
>>>>> -Adrian
>>>>>
>>>>> On 5/21/2012 6:48 AM, Adam Heath wrote:
>>>>>> Why do SimpleMethod.java and ModelScreen.java both do
>>>>>> context.put("null", GenericEntity.NULL_FIELD)? That changes *all*
>>>>>> null
>>>>>> values in freemarker into that static instance, even when the target
>>>>>> type of the variable is *not* going to be part of some
>>>>>> entity-like map.
>>>>>>
>>>>>> If I remove both of those calls(actually, remove one, change the
>>>>>> other
>>>>>> to place it as "nullField"), then the broken ordermgr link given
>>>>>> earlier starts to finally work. And, I think that even the nullField
>>>>>> constant should not be removed, as NullField should only be used
>>>>>> internally by the entity engine.
>>>>
>>

[Adam Heath-2]

On 05/21/2012 10:05 AM, Adrian Crum wrote:
> Okay, I think we're safe. I recently refactored all of that to make it
> more reliable. If something breaks, the places to look are the
> org.ofbiz.minilang.method.conditional.Compare class and
> org.ofbiz.minilang.MiniLangUtil.convertType(...) method.

The test cases work, and the ordermgr link given earlier works.  I
haven't done full-backend testing.

I think after I get this all working again, I'll add local-ofbiz tests
for the freemarker issues I've discovered.  At least in base, that
will be simple(api tests are easier to write).  The "null" problem in
minilang and widget are more difficult, as I'm not that used to
writing those.

> On 5/21/2012 3:42 PM, Adrian Crum wrote:
>> As far as Mini-language is concerned, I think it only affects the
>> comparison operators. But those comparisons should specifically
>> check for null, not compare something to GenericEntity.NULL_FIELD.
>> Let me look at them again.

[Adrian Crum-3]

On 5/21/2012 4:25 PM, Adam Heath wrote:

> On 05/21/2012 10:05 AM, Adrian Crum wrote:
>> Okay, I think we're safe. I recently refactored all of that to make it
>> more reliable. If something breaks, the places to look are the
>> org.ofbiz.minilang.method.conditional.Compare class and
>> org.ofbiz.minilang.MiniLangUtil.convertType(...) method.
> The test cases work, and the ordermgr link given earlier works.  I
> haven't done full-backend testing.
>
> I think after I get this all working again, I'll add local-ofbiz tests
> for the freemarker issues I've discovered.  At least in base, that
> will be simple(api tests are easier to write).  The "null" problem in
> minilang and widget are more difficult, as I'm not that used to
> writing those.

I will work on creating Mini-lang unit tests when I am finished with the
overhaul. Unfortunately, for the moment, we have to rely on the higher
level tests and user complaints to detect Mini-language problems.


>
>> On 5/21/2012 3:42 PM, Adrian Crum wrote:
>>> As far as Mini-language is concerned, I think it only affects the
>>> comparison operators. But those comparisons should specifically
>>> check for null, not compare something to GenericEntity.NULL_FIELD.
>>> Let me look at them again.

[Adam Heath-2]

On 05/21/2012 10:28 AM, Adrian Crum wrote:
> I will work on creating Mini-lang unit tests when I am finished with
> the overhaul. Unfortunately, for the moment, we have to rely on the
> higher level tests and user complaints to detect Mini-language problems.

Speaking of, I've noticed a lot of deprecation warnings during runtime
recently; either minilang or widget, I can't recall.  Those shouldn't
have been deprecated until all the call sites were fixed.  Then the
deprecation tag could be added.

[Adrian Crum-3]

On 5/21/2012 4:30 PM, Adam Heath wrote:
> On 05/21/2012 10:28 AM, Adrian Crum wrote:
>> I will work on creating Mini-lang unit tests when I am finished with
>> the overhaul. Unfortunately, for the moment, we have to rely on the
>> higher level tests and user complaints to detect Mini-language problems.
> Speaking of, I've noticed a lot of deprecation warnings during runtime
> recently; either minilang or widget, I can't recall.  Those shouldn't
> have been deprecated until all the call sites were fixed.  Then the
> deprecation tag could be added.

Okay, I can turn Mini-language validation off for now.

-Adrian

[Adam Heath-2]

On 05/21/2012 10:32 AM, Adrian Crum wrote:

> On 5/21/2012 4:30 PM, Adam Heath wrote:
>> On 05/21/2012 10:28 AM, Adrian Crum wrote:
>>> I will work on creating Mini-lang unit tests when I am finished with
>>> the overhaul. Unfortunately, for the moment, we have to rely on the
>>> higher level tests and user complaints to detect Mini-language
>>> problems.
>> Speaking of, I've noticed a lot of deprecation warnings during runtime
>> recently; either minilang or widget, I can't recall.  Those shouldn't
>> have been deprecated until all the call sites were fixed.  Then the
>> deprecation tag could be added.
>
> Okay, I can turn Mini-language validation off for now.

Maybe make it verbose/debug, or some such.  Right now, it increases
the size of the log files, which increases the amount written to disk,
which slows things down a little bit.

ps: I can understand deprecating early, then having others help with
the fixes.  That's why I used the word "shouldn't" above, instead of
"must".

pps: I've doing the entity deprecations myself, not because I don't
want/need help, but it gives me a chance to do a code review on every
file I touch; at least, it gives me a gut feeling about what is being
done in the project.