You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Paulo Gaspar <pa...@krankikom.de> on 2002/02/17 05:29:37 UTC
[PATCH] To AbstractContext BUG (methinks)
Hi,
After coding a Context using the AbstractContext I noticed
that on #foreach cycles null values were being replaced by
the previous non-null value.
After trying to find the problem at my (aham! flawless) code
and a bit of navigation trough Velocity code, my top suspect
was this bit at the AbstractContext class:
public Object put(String key, Object value)
{
/*
* don't even continue if key or value is null
*/
if (key == null)
{
return null;
}
else if (value == null)
{
return null;
}
return internalPut(key, value);
}
See? When the value is null it does NOTHING, which means
that the previous non null value remains for that key!!!
(Argh, not even a RuntimeException!)
Since this might be used with some kind of Map also
allergic to null values, I propose to fix it by replacing
this (do-nothing-on-null-value) bit:
else if (value == null)
{
return null;
}
...with this (remove-key-on-null-value) one:
else if (value == null)
{
return internalRemove(key);
// was: return null;
}
Leaving that "was" comment to give a clue to some potential
victim of the change.
IMHO, the current behavior is not acceptable since it does
produce output that does not match its original data and
this is the most reliable fix I can think of.
Better ideas someone?
(This kind of core class scares me a bit.)
The patch is attached.
Have fun,
Paulo Gaspar
RE: [PATCH] To AbstractContext BUG (methinks)
Posted by Paulo Gaspar <pa...@krankikom.de>.
Hi Geir,
Sorry for the delay, answer inline:
> -----Original Message-----
> From: Geir Magnusson Jr. [mailto:geirm@optonline.net]
> Sent: Monday, February 18, 2002 12:10 AM
>
>
> On 2/17/02 6:18 PM, "Paulo Gaspar" <pa...@krankikom.de> wrote:
>
> > Thanks Geir,
> >
> > I noticed that thread but did not pay enough attention to
> > understand it.
> >
> > I just HAD to fix my version or I would get bad output, but I
> > understand that this could break somethink elsewhere.
>
> Musing over this issue a bit more in response to your note, we will have a
> snag we need to deal with -
>
> Suppose that I wrap one context in another.
>
> VelocityContext vc = new VelocityContext( another );
>
> And then use the new one in rendering a template.
>
> Then what do we do? The idea of chaining is to make the 'inner' ctx
> protected.
>
> This can produce some interesting and spooky effects.
>
> To illustrate : with a #set(), it won't work :
>
> #set( $somethingInner = $somethingNull)
>
> Results in $somethingInner being unchanged because this would affect the
> outermost context, which already may not have anything in it, so it
> delegates to the inner. We have to continue to support that contract of
> protection.
>
> geir
Let me see if I understand, the wrapped context is never modified. But if
we have an entry called $somethingInner that is an entry for both the
wrapper and wrapped contexts and we set it to null in the template, the
$somethingInner entry would disapear from the wrapper showing to the
template the value of the one still existeing in the wrapped!!!
Argh! The only way to deal with that would be to be sure that the wrapper
Map would accept null value entries but that an entry with a null value
would still be considered no-value.
Interesting complication.
Another alternative is a kind of hierarchical map I have that des not let
you add a value if the value already exists in the wrapped Map(s). But
there the bahavior becomes TOO different since you are then unable to
"overwrite" an entry of the wrapped context.
=:o/
Really interesting.
Have fun,
Paulo Gaspar
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] To AbstractContext BUG (methinks)
Posted by "Geir Magnusson Jr." <ge...@optonline.net>.
On 2/17/02 6:18 PM, "Paulo Gaspar" <pa...@krankikom.de> wrote:
> Thanks Geir,
>
> I noticed that thread but did not pay enough attention to
> understand it.
>
> I just HAD to fix my version or I would get bad output, but I
> understand that this could break somethink elsewhere.
Musing over this issue a bit more in response to your note, we will have a
snag we need to deal with -
Suppose that I wrap one context in another.
VelocityContext vc = new VelocityContext( another );
And then use the new one in rendering a template.
Then what do we do? The idea of chaining is to make the 'inner' ctx
protected.
This can produce some interesting and spooky effects.
To illustrate : with a #set(), it won't work :
#set( $somethingInner = $somethingNull)
Results in $somethingInner being unchanged because this would affect the
outermost context, which already may not have anything in it, so it
delegates to the inner. We have to continue to support that contract of
protection.
geir
--
Geir Magnusson Jr. geirm@optonline.net
System and Software Consulting
"Whoever would overthrow the liberty of a nation must begin by subduing the
freeness of speech." - Benjamin Franklin
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] To AbstractContext BUG (methinks)
Posted by Paulo Gaspar <pa...@krankikom.de>.
Thanks Geir,
I noticed that thread but did not pay enough attention to
understand it.
I just HAD to fix my version or I would get bad output, but I
understand that this could break somethink elsewhere.
Have fun,
Paulo
> -----Original Message-----
> From: Geir Magnusson Jr. [mailto:geirm@optonline.net]
> Sent: Sunday, February 17, 2002 2:55 PM
> To: Velocity Developer's List
> Subject: Re: [PATCH] To AbstractContext BUG (methinks)
>
>
> We just went through this earlier in the week, and we are going to wait
> until after 1.3.
>
> This is basically a specific case of the
>
> #set($foo = $somethingNotInContext )
>
> problem, where the previous value of $foo is left in the context.
>
> So yes, if/when we decide to do it, this will be along the lines
> of the fix,
> and will be applied universally.
>
> geir
>
>
>
> On 2/16/02 11:29 PM, "Paulo Gaspar" <pa...@krankikom.de> wrote:
>
> > Hi,
> >
> >
> > After coding a Context using the AbstractContext I noticed
> > that on #foreach cycles null values were being replaced by
> > the previous non-null value.
> >
> > After trying to find the problem at my (aham! flawless) code
> > and a bit of navigation trough Velocity code, my top suspect
> > was this bit at the AbstractContext class:
> >
> > public Object put(String key, Object value)
> > {
> > /*
> > * don't even continue if key or value is null
> > */
> >
> > if (key == null)
> > {
> > return null;
> > }
> > else if (value == null)
> > {
> > return null;
> > }
> >
> > return internalPut(key, value);
> > }
> >
> > See? When the value is null it does NOTHING, which means
> > that the previous non null value remains for that key!!!
> > (Argh, not even a RuntimeException!)
> >
> >
> > Since this might be used with some kind of Map also
> > allergic to null values, I propose to fix it by replacing
> > this (do-nothing-on-null-value) bit:
> >
> > else if (value == null)
> > {
> > return null;
> > }
> >
> > ...with this (remove-key-on-null-value) one:
> >
> > else if (value == null)
> > {
> > return internalRemove(key);
> > // was: return null;
> > }
> >
> > Leaving that "was" comment to give a clue to some potential
> > victim of the change.
> >
> >
> > IMHO, the current behavior is not acceptable since it does
> > produce output that does not match its original data and
> > this is the most reliable fix I can think of.
> >
> > Better ideas someone?
> > (This kind of core class scares me a bit.)
> >
> >
> > The patch is attached.
> >
> >
> > Have fun,
> > Paulo Gaspar
> >
> > --
> > To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
>
--
Geir Magnusson Jr. geirm@optonline.net
System and Software Consulting
Be a giant. Take giant steps. Do giant things...
--
To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] To AbstractContext BUG (methinks)
Posted by "Geir Magnusson Jr." <ge...@optonline.net>.
We just went through this earlier in the week, and we are going to wait
until after 1.3.
This is basically a specific case of the
#set($foo = $somethingNotInContext )
problem, where the previous value of $foo is left in the context.
So yes, if/when we decide to do it, this will be along the lines of the fix,
and will be applied universally.
geir
On 2/16/02 11:29 PM, "Paulo Gaspar" <pa...@krankikom.de> wrote:
> Hi,
>
>
> After coding a Context using the AbstractContext I noticed
> that on #foreach cycles null values were being replaced by
> the previous non-null value.
>
> After trying to find the problem at my (aham! flawless) code
> and a bit of navigation trough Velocity code, my top suspect
> was this bit at the AbstractContext class:
>
> public Object put(String key, Object value)
> {
> /*
> * don't even continue if key or value is null
> */
>
> if (key == null)
> {
> return null;
> }
> else if (value == null)
> {
> return null;
> }
>
> return internalPut(key, value);
> }
>
> See? When the value is null it does NOTHING, which means
> that the previous non null value remains for that key!!!
> (Argh, not even a RuntimeException!)
>
>
> Since this might be used with some kind of Map also
> allergic to null values, I propose to fix it by replacing
> this (do-nothing-on-null-value) bit:
>
> else if (value == null)
> {
> return null;
> }
>
> ...with this (remove-key-on-null-value) one:
>
> else if (value == null)
> {
> return internalRemove(key);
> // was: return null;
> }
>
> Leaving that "was" comment to give a clue to some potential
> victim of the change.
>
>
> IMHO, the current behavior is not acceptable since it does
> produce output that does not match its original data and
> this is the most reliable fix I can think of.
>
> Better ideas someone?
> (This kind of core class scares me a bit.)
>
>
> The patch is attached.
>
>
> Have fun,
> Paulo Gaspar
>
> --
> To unsubscribe, e-mail: <ma...@jakarta.apache.org>
> For additional commands, e-mail: <ma...@jakarta.apache.org>
>
--
Geir Magnusson Jr. geirm@optonline.net
System and Software Consulting
Be a giant. Take giant steps. Do giant things...
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>