MyFaces Core
  1. MyFaces Core
  2. MYFACES-2946

composite:attribute "targets" property does not match with ViewDeclarationLanguage.retargetMethodExpressions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.0.3
    • Component/s: JSR-314
    • Labels:
      None

      Description

      Some days ago, it was mentioned on myfaces dev list that "targets"
      attribute does not seem to work as expected. After review the available
      documentation and doing some test, my conclusion was the spec is good,
      but this part needs a little bit more documentation (and fix some
      bugs), because there are use cases that are causing problems to users.
      Some of these problems has been already mentioned (spec issue 755) but
      my intention here is do a full and detailed analysis.

      The property "targets" is used for these tags:

      composite:actionSource
      composite:editableValueHolder
      composite:valueHolder
      composite:clientBehavior
      composite:attribute

      For the first four cases, this property is used by :

      ViewDeclarationLanguage.retargetAttachedObjects(FacesContext context,
      UIComponent topLevelComponent,
      List<AttachedObjectHandler> handlers)

      In JSF 2.0 rev A it was made explicit the need to handle nested
      composite component with this lines:

      "... The implementation must support retargeting attached objects from
      the top level compsite component to targets that are composite and
      non-composite components ...."

      This is ok .

      The problem is the description about how composite:attribute "targets"
      property works does not match with the algorithm for:

      ViewDeclarationLanguage.retargetMethodExpressions(FacesContext context,
      UIComponent topLevelComponent)

      Here is the documentation about composite:attribute "targets":

      "... If this element has a method-signature attribute,

      the value of the targets attribute must be interpreted as a space
      (not tab) separated list of client ids (relative to the top level
      component) of components within the <composite:implementation> section.

      Space is used as the delimiter for compatibility with the IDREFS and
      NMTOKENS data types from the XML Schema.

      Each entry in the list must be interpreted as the id of an inner
      component to which the MethodExpression from the composite component
      tag in the using page must be applied.

      If this element has a method-signature attribute, but no targets
      attribute, the value of the name attribute is used as the single entry
      in the list.

      If the value of the name attribute is not one of the special values
      listed in the description of the name attribute, targets (or its
      derived value) need not correspond to the id of an inner component ...".

      At this point everything seems to be ok. But look this documentation
      (I'll put the important points):

      ".....

      1. Get the value of the targets attribute. If the value is a ValueExpression
        evaluate it. If there is no targets attribute, let the name of the
        metadata element be the evaluated value of the targets attribute.
      2. Interpret targets as a space (not tab) separated list of ids. For each entry
        in the list:
        ......
      3. If name is equal to the string "action" without the quotes, call
        ActionSource2.setActionExpression(javax.el.MethodExpression) on target,
        passing attributeMethodExpression.
      4. If name is equal to the string "actionListener" without the quotes, call
        ActionSource.addActionListener(javax.faces.event.ActionListener) on target,
        passing attributeMethodExpression wrapped in a MethodExpressionActionListener.
      5. If name is equal to the string "validator" without the quotes, call
        EditableValueHolder.addValidator(javax.faces.validator.Validator) on target,
        passing attributeMethodExpression wrapped in a MethodExpressionValidator.
      6. If name is equal to the string "valueChangeListener" without the quotes, call
        EditableValueHolder.addValueChangeListener(javax.faces.event.ValueChangeListener)
        on target, passing attributeMethodExpression wrapped in a
        MethodExpressionValueChangeListener.
      7. Otherwise, assume that the MethodExpression should be placed in the
        components attribute set. The runtme must create the MethodExpression
        instance based on the value of the "method-signature" attribute.
        ....."

      My first interpretation of this javadoc was that "targets" only has sense for "action",
      "actionListener", "validator" and "valueChangeListener". But note if that's true,
      someone could expect something like the description on retargetAttachedObjects, right?

      The current behavior (Mojarra 2.0.3 and Myfaces 2.0.2) is the same, if we have two
      nested composite components, that will throw a ClassCastException. There is a issue already
      open for this one:

      https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=755

      There are two possible alternatives here:

      1. Implement the algorithm proposed for retargetMethodExpressions and ignore
      composite:attribute "targets" property says.
      2. Implement the expected behavior of composite:attribute "targets" property and make
      some changes to retargetMethodExpressions algorithm.

      The intention is take option 2 and include it for JSF 2.0, because in theory should be
      handled as an implementation detail of the algorithm (anyway it could be good to update
      the documentation with the same advice used for retargetAttachedObjects).

      For "action", "actionListener", "validator" and "valueChangeListener" it is clear that
      the action to take is something like this:

      • If target is a composite component and that one "retarget" to other components and
        additionally does not implements [ActionSource2/EditableValueHolder], call:

      targetComponent.getAttributes().put(attributeName, attributeNameValueExpression);

      and call retargetMethodExpressions recursively. Take care of apply the method twice
      and if the property pointed by "attributeName" was already set, revert its effects.

      The tricky part is how to handle generic method expression properties. The javadoc says:

      "....

      1. Otherwise, assume that the MethodExpression should be placed in the
        components attribute set. The runtme must create the MethodExpression
        instance based on the value of the "method-signature" attribute.
        ....."

      But I have identified three valid cases:

      1. testSimpleAttributeMethodExpressionEmpty.xhtml (Using an EL #

      {cc}

      reference)

      <testComposite:simpleAttributeMethodExpressionEmpty id="cc1"
      customMethod="#

      {methodExpressionBean.doSomethingFunny}">
      </testComposite:simpleAttributeMethodExpressionEmpty>

      simpleAttributeMethodExpressionEmpty.xhtml

      <composite:interface>
      <composite:attribute
      name="customMethod"
      method-signature="String doSomething(String)"/>
      </composite:interface>
      <composite:implementation>
      <tc:testComponent id="testComponent" customMethod="#{cc.attrs.customMethod}"/>
      </composite:implementation>

      2. testSimpleAttributeMethodExpressionTarget.xhtml (Using "targets" to call
      a property setter directly)

      <testComposite:simpleAttributeMethodExpressionTarget id="cc1"
      customMethod="#{methodExpressionBean.doSomethingFunny}

      ">
      </testComposite:simpleAttributeMethodExpressionTarget>

      simpleAttributeMethodExpressionTarget.xhtml

      <composite:interface>
      <composite:attribute
      name="customMethod"
      method-signature="String doSomething(String)"
      targets="testComponent"/>
      </composite:interface>
      <composite:implementation>
      <tc:testComponent id="testComponent"/>
      </composite:implementation>

      3. testCompositeAttributeMethodExpressionTarget.xhtml (Using "targets",
      but the target component is a composite one)

      <testComposite:compositeAttributeMethodExpressionTarget id="cc1"
      customMethod="#

      {methodExpressionBean.doSomethingFunny}

      ">
      </testComposite:compositeAttributeMethodExpressionTarget>

      compositeAttributeMethodExpressionTarget.xhtml

      <composite:interface>
      <composite:attribute
      name="customMethod"
      method-signature="String doSomething(String)"
      targets="simpleAttributeMethodExpressionTarget"/>
      </composite:interface>
      <composite:implementation>
      <testComposite:simpleAttributeMethodExpressionTarget id="simpleAttributeMethodExpressionTarget"/>
      </composite:implementation>

      simpleAttributeMethodExpressionTarget.xhtml (could be like in 1 or 2)

      The case (1) actually works but the case (2) and (3) does not work on both
      MyFaces 2.0.2 and Mojarra 2.0.3 .

      1. MYFACES-2946-1.patch
        51 kB
        Leonardo Uribe

        Issue Links

          Activity

          Leonardo Uribe created issue -
          Leonardo Uribe made changes -
          Field Original Value New Value
          Attachment MYFACES-2946-1.patch [ 12457512 ]
          Leonardo Uribe made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Jakob Korherr added a comment -

          The patch looks very good. +1 for committing it!

          Show
          Jakob Korherr added a comment - The patch looks very good. +1 for committing it!
          Hide
          Jakob Korherr added a comment -

          Also I was wondering: shouldn't it be possible to reference the known attributes (e.g. "action") also via #

          {cc.attrs.action}

          . The spec actually does not "allow" this, but I think it really should be possible, because this may really confuse users, because some attributes are available via #

          {cc.attrs}

          and some are not.

          WDYT Leo?

          Show
          Jakob Korherr added a comment - Also I was wondering: shouldn't it be possible to reference the known attributes (e.g. "action") also via # {cc.attrs.action} . The spec actually does not "allow" this, but I think it really should be possible, because this may really confuse users, because some attributes are available via # {cc.attrs} and some are not. WDYT Leo?
          Hide
          Leonardo Uribe added a comment -

          Unfortunately, we can't do that, because that is not explicit on the spec javadoc, and there is a way to do what is expected using "targets" property.

          Show
          Leonardo Uribe added a comment - Unfortunately, we can't do that, because that is not explicit on the spec javadoc, and there is a way to do what is expected using "targets" property.
          Leonardo Uribe made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 2.0.3-SNAPSHOT [ 12315349 ]
          Resolution Fixed [ 1 ]
          Hide
          Leonardo Uribe added a comment -

          Checking how this algorithm works, I found a misunderstanding between two concepts:

          • It is necessary to save the object created for actionListener, validator and valueChangeListener, so we can revert them later when we resolve a nested retargetMethodExpressions. That should be saved for each target component.
          • It is necessary to mark the attribute on the composite component parent, to prevent apply twice the algorithm when a nested retargetMethodExpressions is called.

          For do both I tried to use just one map, but clearly the intention require use a different map for each one of them, so we need to refactor the algorithm properly.

          Show
          Leonardo Uribe added a comment - Checking how this algorithm works, I found a misunderstanding between two concepts: It is necessary to save the object created for actionListener, validator and valueChangeListener, so we can revert them later when we resolve a nested retargetMethodExpressions. That should be saved for each target component. It is necessary to mark the attribute on the composite component parent, to prevent apply twice the algorithm when a nested retargetMethodExpressions is called. For do both I tried to use just one map, but clearly the intention require use a different map for each one of them, so we need to refactor the algorithm properly.
          Leonardo Uribe made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Leonardo Uribe made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Leonardo Uribe made changes -
          Fix Version/s 2.0.3 [ 12315976 ]
          Fix Version/s 2.0.3-SNAPSHOT [ 12315349 ]
          Leonardo Uribe made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Leonardo Uribe made changes -
          Link This issue is duplicated by MYFACES-3112 [ MYFACES-3112 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development