MyFaces Core
  1. MyFaces Core
  2. MYFACES-3169

ui:param and c:set implementations does not work as expected

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.7, 2.1.1
    • Fix Version/s: 2.0.8, 2.1.2
    • Component/s: JSR-314
    • Labels:
      None

      Description

      Original message sent to jsr344-experts and users mailing list:

      Checking how ui:param and c:set works and its relationship with facelets
      VariableMapper, I notice the original implementation that comes from facelets
      does not work like a page developer should expect. In fact, in some situations
      ui:param and c:set implementation is just the same.

      The consequence of this situation is there are ways to write code that just
      should not be allowed, because it breaks the intention of those tags. JSF
      implementations should fix this, and maybe if it is required add more
      documentation specifying these tags better.

      The first case is c:set. This is the description on facelets taglibdoc:

      "... Sets the result of an expression evaluation based on the value of the
      attributes. If 'scope' the is present, but has a zero length or is equal
      to the string 'page', TagException is thrown with an informative error
      message, ensuring page location information is saved. ..."

      "... This handler operates in one of two ways.

      1. The user has set 'var', 'value' and (optionally) 'scope' attributes.

      2. The user has set 'target', 'property', and 'value' attributes.

      The first case takes precedence over the second ..."

      The buggy behavior of this tag can be seen when it is used in this way:

      <c:set var="someVar" value="someValue"/>

      Look the part that says "... if 'scope' the is present, but has zero length or
      is equal to the string 'page' ..." . In JSP, it exists a page context and
      in that context, the variable has scope to the current page. Since in that
      case there are no template tags, this variable cannot be located on other
      pages included with jsp:include or whatever you use.

      The current implementation of c:set that comes from facelets 1.1.x does not
      implements the original intention of this tag in jsp, and instead it uses
      VariableMapper instance obtained through FaceletContext (which is instance
      of ELContext). Since both JSF 2.0 implementations comes from the original
      facelets 1.1.x code, you can see the following problems:

      cset1.xhtml

      <c:set var="someVar" value="someValue"/>
      <ui:decorate template="cset1_1.xhtml">
      <!-- ... -->
      </ui:decorate>

      cset1_1.xhtml

      <ui:composition>
      <h:outputText value="#

      {someVar}"/>
      </ui:composition>

      The previous code in practice will output "someValue", but it should not,
      because "someVar" should be only available on the current .xhtml page, in
      this case cset1.xhtml.

      Now consider this more realistic example:

      cset2.xhtml

      <c:set var="someVar" value="someValue"/>
      <ui:decorate template="cset2_1.xhtml">
      <!-- ... -->
      <ui:define name="header">
      <h:outputText value="#{someVar}

      "/>
      </ui:define>
      </ui:decorate>

      cset2_1.xhtml

      <ui:composition>
      <c:set var="someVar" value="badValue"/>

      <!-- ... something with someVar ... -->

      <ui:insert name="header"/>

      </ui:composition>

      In practice the output will be "badValue", but it should be "someValue",
      again because c:set intention is to define a value that should be
      available only on the current page. This problem is also valid if you
      replace ui tags with a composite component and cc:insertXXX tags.

      Now take a look at this one:

      cset3.xhtml

      <c:set var="someVar" value="someValue"/>
      <ui:decorate template="cset3_1.xhtml">
      <!-- ... code without use ui:param ... -->
      </ui:decorate>
      <h:outputText value="#

      {someVar}

      "/>

      cset3_1.xhtml

      <ui:composition>
      <c:set var="someVar" value="badValue"/>
      <!-- ... something with someVar ... -->
      </ui:composition>

      In practice the output will be again "badValue", but it is interesting to note
      that if you use ui:param, the example will work again, because a
      VariableMapperWrapper is used, discarding the bad value after ui:decorate
      ends.

      Things start to get worse when you see how ui:param works:

      String nameStr = this.name.getValue(ctx);
      ValueExpression valueVE = this.value.getValueExpression(ctx, Object.class);
      ctx.getVariableMapper().setVariable(nameStr, valueVE);

      It is the same thing as c:set!!!!! .

      if (this.scope != null)
      {
      /* ... Does this exception really has sense ??? ... */
      if ("page".equals(scopeStr))

      { throw new TagException(tag, "page scope is not allowed"); }

      /* ... some stuff that works well ...*/
      } else

      { ctx.getVariableMapper().setVariable(varStr, veObj); }

      Why this code hasn't been broken before? because nobody has ever use c:set and
      ui:param with exactly the same var name. Maybe because on facelets dev
      documentation:

      http://facelets.java.net/nonav/docs/dev/docbook.html

      says this:

      "... Table 3.5. <c:set/> (Avoid if Possible) ..."

      Really there are some particular situations where c:set is useful.

      There are a lot more examples I tried on ui:param that just don't work. But
      the big question is how c:set and ui:param should work?

      • c:set using only 'var' and 'value' should define the variable only as
        "page" scoped, just like the old jstl tag does and the current spec javadoc
        says.
      • ui:param should define a variable "template" scoped, that means it applies
        to both template client and templates. It should be propagated through
        ui:composition, ui:decorate, ui:define and ui:insert, but it should not pass
        composite components, because this one defines a different template resolution
        context (hack only on MyFaces, but valid for JSF). It should not pass on
        nested templates case (nested ui:composition or ui:decorate).
      • The facelets taglibdoc of ui:param says:

      "... Use this tag to pass parameters to an included file (using ui:include),
      or a template (linked to either a composition or decorator). Embed ui:param
      tags in either ui:include, ui:composition, or ui:decorate to pass the
      parameters. ..."

      JSF implementation should do exactly what it says here.

      Note all this should work using a more elaborate hack over VariableMapper,
      because it is not possible to use another alternative here. One idea is
      create a component that defines a "local" page scope just like {} does
      in java code, but maybe is too much for something than in practice should
      be simple.

      I think this should be fixed. Obviously I'm doing an interpretation of the
      few documentation available, but note at the end a "final word" should be
      done here at spec level to keep compatibility between JSF implementations.

      1. MYFACES-3169-2.patch
        141 kB
        Leonardo Uribe

        Issue Links

          Activity

          Hide
          Leonardo Uribe added a comment -

          VariableMapper strategy still works, so I think you can create a custom tag handler (<x:globalParam>?) that set the variable on the VariableMapper just like the previous implementation did. It will work without problem.

          If you have a param that applies to all views in an application, maybe it is better to use an EL Resolver that handle this special name, just like JSF does for handle implicit objects ( request, facesContext, externalContext, session, application, component, cc, .... ). After all, this is the pattern used for JSF to deal with this kind of scenarios.

          c:set with var and value properties only define vars on "page" scope, just like the old jsp tag did and more according to the spec description. So it works on any page, but variables defined will only apply to the page this tag is used.

          I know this fix could cause these kind of problems, but in my personal opinion it is worst rely on the previous behavior, because that is not clear and does not match with the spec documentation. Anyway, I'm open to discussion this stuff on dev list if the behavior proposed seems to be very inconvenient.

          Show
          Leonardo Uribe added a comment - VariableMapper strategy still works, so I think you can create a custom tag handler (<x:globalParam>?) that set the variable on the VariableMapper just like the previous implementation did. It will work without problem. If you have a param that applies to all views in an application, maybe it is better to use an EL Resolver that handle this special name, just like JSF does for handle implicit objects ( request, facesContext, externalContext, session, application, component, cc, .... ). After all, this is the pattern used for JSF to deal with this kind of scenarios. c:set with var and value properties only define vars on "page" scope, just like the old jsp tag did and more according to the spec description. So it works on any page, but variables defined will only apply to the page this tag is used. I know this fix could cause these kind of problems, but in my personal opinion it is worst rely on the previous behavior, because that is not clear and does not match with the spec documentation. Anyway, I'm open to discussion this stuff on dev list if the behavior proposed seems to be very inconvenient.
          Hide
          Martin Kočí added a comment -

          Yes, I'm aware of this solution but we cannot use this, because it means modification of hundreds views and of course we don't use only one ui:param per view but many more -> propagating variable in this manner means thousands of modifications in source code of views, many of then are already deployed in production.

          I can do a global change from ui:param to c:set but c:set is unsupported inside decorate tag. But It this ok? If I look at tld docs for ui:decorate it mentions c:forEach inside ui:decorate

          Show
          Martin Kočí added a comment - Yes, I'm aware of this solution but we cannot use this, because it means modification of hundreds views and of course we don't use only one ui:param per view but many more -> propagating variable in this manner means thousands of modifications in source code of views, many of then are already deployed in production. I can do a global change from ui:param to c:set but c:set is unsupported inside decorate tag. But It this ok? If I look at tld docs for ui:decorate it mentions c:forEach inside ui:decorate
          Hide
          Leonardo Uribe added a comment -

          In this case you can do this:

          <ui:decorate xmlns:ui="..."
          template="Template1.xhtml">

          <ui:param name="localization" value="#

          {aSpecificLocalizationSource}

          " />
          <ui:define name="content">
          <ui:include src="TemplateClient2.xhtml" >
          <ui:param name="localization" value="#

          {localization}

          "
          </ui:include>

          </ui:define>

          Look the additional declaration inside ui:include. In theory, ui:include creates another template context, that means ui:define fragments defined outside it will not be passed. The same happen for ui:param. All params defined outside will be ignored, because those ones are bound to the previous template context.

          So, ui:param declarations can be lookup on nested ui:composition and ui:decorate tags, but cannot for ui:include. In that case you should declare which params the included page requires, which is preferred because in this way it is easier to see which params are relevant for the template.

          Show
          Leonardo Uribe added a comment - In this case you can do this: <ui:decorate xmlns:ui="..." template="Template1.xhtml"> <ui:param name="localization" value="# {aSpecificLocalizationSource} " /> <ui:define name="content"> <ui:include src="TemplateClient2.xhtml" > <ui:param name="localization" value="# {localization} " </ui:include> </ui:define> Look the additional declaration inside ui:include. In theory, ui:include creates another template context, that means ui:define fragments defined outside it will not be passed. The same happen for ui:param. All params defined outside will be ignored, because those ones are bound to the previous template context. So, ui:param declarations can be lookup on nested ui:composition and ui:decorate tags, but cannot for ui:include. In that case you should declare which params the included page requires, which is preferred because in this way it is easier to see which params are relevant for the template.
          Hide
          Martin Kočí added a comment -

          hmm, we have many (over 1500!) views where ui:param is used. Structure looks:
          <ui:decorate xmlns:ui="..."
          template="Template1.xhtml">

          <ui:param name="localization" value="#

          {aSpecificLocalizationSource}

          " />
          <ui:define name="content">
          <ui:include src="TemplateClient2.xhtml" />

          </ui:define>

          TemplateClient2.xhtml itself uses another template2.xhtml and uses ui:include etc.

          The point is that all parts this facelet use variable "localization".

          Now with this change, this stops working and included parts of facelets do not resolve this variable. I agree that ui:param is was misused here and relied on internal VariableMapper - usage and c:set as "page scope" variable is better.

          But simple replacing ui:param with c:set does not solve this problem, because DecorateHandler support only define and param!

          How to solve this? Please notice that ui:decorate is root element and has ui:param inside, not outside the ui:decorate.

          Show
          Martin Kočí added a comment - hmm, we have many (over 1500!) views where ui:param is used. Structure looks: <ui:decorate xmlns:ui="..." template="Template1.xhtml"> <ui:param name="localization" value="# {aSpecificLocalizationSource} " /> <ui:define name="content"> <ui:include src="TemplateClient2.xhtml" /> </ui:define> TemplateClient2.xhtml itself uses another template2.xhtml and uses ui:include etc. The point is that all parts this facelet use variable "localization". Now with this change, this stops working and included parts of facelets do not resolve this variable. I agree that ui:param is was misused here and relied on internal VariableMapper - usage and c:set as "page scope" variable is better. But simple replacing ui:param with c:set does not solve this problem, because DecorateHandler support only define and param! How to solve this? Please notice that ui:decorate is root element and has ui:param inside, not outside the ui:decorate.
          Hide
          Leonardo Uribe added a comment -

          Ok, I commited the solution. We need to insert that template manager to resolve parameters properly (because resolve parameters follows the same rules), so I added some checks for null over Facelet owner, so in such case apply method just return false.

          Please try again and let me know if you found another bug.

          Show
          Leonardo Uribe added a comment - Ok, I commited the solution. We need to insert that template manager to resolve parameters properly (because resolve parameters follows the same rules), so I added some checks for null over Facelet owner, so in such case apply method just return false. Please try again and let me know if you found another bug.
          Hide
          Martin Kočí added a comment -

          I have problem with NPEs and it's probably caused by:

          new TemplateManagerImpl(null, INITIAL_TEMPLATE_CLIENT, true, INITIAL_PAGE_CONTEXT)

          the first param is null - is that ok? Docs says "dummy template client" but can be Facelet owner null really? With complex
          template-based view I'm getting this NPE:
          java.lang.NullPointerException
          at org.apache.myfaces.view.facelets.impl.DefaultFaceletContext.getExpressionFactory(DefaultFaceletContext.java:218)

          because null is propagated from TemplateManagerImpl to DefaultFaceletContext

          Show
          Martin Kočí added a comment - I have problem with NPEs and it's probably caused by: new TemplateManagerImpl(null, INITIAL_TEMPLATE_CLIENT, true, INITIAL_PAGE_CONTEXT) the first param is null - is that ok? Docs says "dummy template client" but can be Facelet owner null really? With complex template-based view I'm getting this NPE: java.lang.NullPointerException at org.apache.myfaces.view.facelets.impl.DefaultFaceletContext.getExpressionFactory(DefaultFaceletContext.java:218) because null is propagated from TemplateManagerImpl to DefaultFaceletContext
          Hide
          Leonardo Uribe added a comment -

          Attached patch that adds PageContext, update TemplateContext stuff and fix c:set and ui:param. JUnit tests included to check correct and bad syntax using those tags.

          Show
          Leonardo Uribe added a comment - Attached patch that adds PageContext, update TemplateContext stuff and fix c:set and ui:param. JUnit tests included to check correct and bad syntax using those tags.

            People

            • Assignee:
              Leonardo Uribe
              Reporter:
              Leonardo Uribe
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development