jsr372-experts@javaserverfaces-spec-public.java.net

[jsr372-experts] Potential issue with hierarchical variable mapper in Facelets

From: arjan tijms <arjan.tijms_at_gmail.com>
Date: Thu, 8 Jan 2015 12:38:23 +0100

Hi,

I just stumbled upon an issue that I'd like to hear your opinion on.

In Facelets the EL variable mapper is setup to resolve partially
hierarchical; every nested Facelet gets a new instance of the Facelets
context, which holds the variable mapper.

This variable mapper resolves against its own instance first, and if
it turns out to be null there, delegates to the parent, which does the
same thing, etc.

E.g. in com.sun.faces.facelets.el.VariableMapperWrapper:

   public ValueExpression resolveVariable(String variable) {
        ValueExpression ve = null;
        try {
            if (this.vars != null) {
                ve = (ValueExpression) this.vars.get(variable);
            }
            if (ve == null) {
                return this.target.resolveVariable(variable);
            }
            return ve;
        } catch (StackOverflowError e) {
            throw new ELException("Could not Resolve Variable
[Overflow]: " + variable, e);
        }
    }


Now there's what seems to be an optimization in place. Initially the
context of a nested Facelet gets a direct reference to the mapper of
the context of the outer Facelet as seen in
com.sun.faces.facelets.impl.DefaultFaceletContext:

  public DefaultFaceletContext(DefaultFaceletContext ctx,
DefaultFacelet facelet) {
      // [...]
      this.varMapper = ctx.varMapper;


Only when Facelets detects there are attributes available, is the
shared variable mapper replaced by the delegating one shown above:

E.g. in com.sun.faces.facelets.tag.UserTagHandler.apply(FaceletContext,
UIComponent):

        FaceletContextImplBase ctx = (FaceletContextImplBase) ctxObj;
        VariableMapper orig = ctx.getVariableMapper();

        // setup a variable map
        if (this.vars.length > 0) {
            VariableMapper varMapper = new VariableMapperWrapper(orig);
            for (int i = 0; i < this.vars.length; i++) {
                varMapper.setVariable(this.vars[i].getLocalName(),
this.vars[i].getValueExpression(ctx, Object.class));
            }
            ctx.setVariableMapper(varMapper);
        }


This makes sense from a performance point of view and is a well known pattern.

However, when setAttribute is later called on the nested
FaceletContext, there is NO attempt made to create a new mapper at
that point. In other words, the setAttribute call would end up
influencing the outer FaceletContext's variable mapper, which I think
is not correct. As with many other instances of this design pattern, I
think some state should be kept that indicates if a mapper is shared
or not and whenever a write is attempted a wrapper like shown in the
apply method above should be created. A small complication is that
writes can go either through the FaceletContext, or could go directly
to the variable mapper obtained from that context. Because of this it
might even be better/easier to always wrap.

Example of an effect that I think will now happen:

Suppose we have a Facelets tag mytag.xhtml, with a Java based tag
handler for "sometag" that sets a value on the variable mapper.

mytag.xhtml
...
<x:sometag var="foo" value="bar"/>

On a Facelet:

<test:mytag>

#{foo}

This will print "bar", mytag was able to modify its outer scope

But on the same Facelet when used instead:

<test:mytag dummy="unrelated">

#{foo}

This will now print nothing, the mere presence of the totally
unrelated attribute "dummy' changed the tag's behaviour. I checked
both Mojarra and MyFaces and from a glance at the source they both
seemed to do pretty much the same thing (obviously they both inherited
from the same original Facelets code).

What do you guys think?

Kind regards,
Arjan Tijms