Tapestry 5
  1. Tapestry 5
  2. TAP5-1549

ParameterWorker forces evaluation of default method, even if the parameter is bound by other means

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.2.5
    • Fix Version/s: 5.3
    • Component/s: tapestry-core
    • Labels:
      None

      Description

      The revised implementation of ParameterWorker evaluates the defaultXXX method regardless of whether or not the parameter is bound. This can easily lead to application exceptions, particularly when the defaultXXX value is computed rather than static.

      For example, loop's "encoder" parameter attempts to get a ValueEncoder from ValueEncoderSource. This will fail if, for instance, the loop's value is a hibernate entity with a multi-column PK, even if the developer binds a custom ValueEncoder to the parameter to handle the entity. Admittedly, this case has a workaround: contribute the custom encoder to ValueEncoderSource. But that's only one edge case.

      The correct solution is to lazily evaluate defaultXXX methods.

        Activity

        Hide
        Hudson added a comment -

        Integrated in tapestry-trunk-freestyle #412 (See https://builds.apache.org/job/tapestry-trunk-freestyle/412/)
        TAP5-1549: Refactor ParameterWorker and BindParameterWorker to ComponentClassTransformWorker2
        Change the way generics are used in PlasticField.setConduit() and setComputedConduit()
        Change ParameterConduit to extend from FieldConduit, not the old FieldValueConduit
        Add NamedSet.eachValue() to apply an operation to each value stored in the set

        hlship : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1143993
        Files :

        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/InjectContainerWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/BindParameterWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/InjectComponentWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ParameterWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ReadOnlyFieldValueConduit.java
        • /tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/MixinWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TransformConstants.java
        • /tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/plastic/PlasticField.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/EnvironmentalWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/structure/InternalComponentResourcesImpl.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/InjectPageWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ComponentWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/util/NamedSet.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/BridgeClassTransformation.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/UnclaimedFieldWorker.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ParameterConduit.java
        • /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TapestryModule.java
        Show
        Hudson added a comment - Integrated in tapestry-trunk-freestyle #412 (See https://builds.apache.org/job/tapestry-trunk-freestyle/412/ ) TAP5-1549 : Refactor ParameterWorker and BindParameterWorker to ComponentClassTransformWorker2 Change the way generics are used in PlasticField.setConduit() and setComputedConduit() Change ParameterConduit to extend from FieldConduit, not the old FieldValueConduit Add NamedSet.eachValue() to apply an operation to each value stored in the set hlship : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1143993 Files : /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/InjectContainerWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/BindParameterWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/InjectComponentWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ParameterWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ReadOnlyFieldValueConduit.java /tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/MixinWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TransformConstants.java /tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/plastic/PlasticField.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/EnvironmentalWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/structure/InternalComponentResourcesImpl.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/InjectPageWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ComponentWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/util/NamedSet.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/BridgeClassTransformation.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/UnclaimedFieldWorker.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/transform/ParameterConduit.java /tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TapestryModule.java
        Howard M. Lewis Ship made changes -
        Status In Progress [ 3 ] Closed [ 6 ]
        Fix Version/s 5.3 [ 12316024 ]
        Resolution Fixed [ 1 ]
        Howard M. Lewis Ship made changes -
        Summary ParameterWorker forces evaluation of defaultXXX methods, even if the parameter is bound ParameterWorker forces evaluation of default method, even if the parameter is bound by other means
        Hide
        Howard M. Lewis Ship added a comment -

        The code is now simpler and more explicit ... the default method is only invoked when the parameter is explicitly bound, does not have a default, and does not autoconnect.

        Show
        Howard M. Lewis Ship added a comment - The code is now simpler and more explicit ... the default method is only invoked when the parameter is explicitly bound, does not have a default, and does not autoconnect.
        Howard M. Lewis Ship committed 1143993 (18 files)
        Reviews: none

        TAP5-1549: Refactor ParameterWorker and BindParameterWorker to ComponentClassTransformWorker2
        Change the way generics are used in PlasticField.setConduit() and setComputedConduit()
        Change ParameterConduit to extend from FieldConduit, not the old FieldValueConduit
        Add NamedSet.eachValue() to apply an operation to each value stored in the set

        tapestry trunk
        Hide
        Robert Zeigler added a comment -

        I could check in my test if you want. It's a simple component with a parameter named "value" that just prints the value to the page. The component supplies a defaultValue method that throws a Runtime exception. Then there's a page that contains the component and explicitly binds the value parameter. In the current trunk, without my fix (or your code refactoring), you hit the runtime exception, even though the value parameter is explicitly bound by the page because the defaultValue method is needlessly evaluated.

        Show
        Robert Zeigler added a comment - I could check in my test if you want. It's a simple component with a parameter named "value" that just prints the value to the page. The component supplies a defaultValue method that throws a Runtime exception. Then there's a page that contains the component and explicitly binds the value parameter. In the current trunk, without my fix (or your code refactoring), you hit the runtime exception, even though the value parameter is explicitly bound by the page because the defaultValue method is needlessly evaluated.
        Hide
        Howard M. Lewis Ship added a comment -

        Sorry I took this out from under you. I think I have a fix, as part of a fairly big refactoring of ParameterWorker. Side benefit: its much simpler, shifts some responsibility to InternalComponentResourcesImpl, and doesn't have to advise any component methods.

        I'm just trying to figure out what a test will look like to prove that the method is only invoked when needed.

        Show
        Howard M. Lewis Ship added a comment - Sorry I took this out from under you. I think I have a fix, as part of a fairly big refactoring of ParameterWorker. Side benefit: its much simpler, shifts some responsibility to InternalComponentResourcesImpl, and doesn't have to advise any component methods. I'm just trying to figure out what a test will look like to prove that the method is only invoked when needed.
        Hide
        Robert Zeigler added a comment -

        Heh. I forgot to mark this as "In Progress". I have a working fix for this, I was just exploring different possibilities because my solution would wind up calling the default method at least once, any render it was accessed, even if the underlying value didn't change, and I wasn't happy with that. I'll be curious to see what you come up with.

        Show
        Robert Zeigler added a comment - Heh. I forgot to mark this as "In Progress". I have a working fix for this, I was just exploring different possibilities because my solution would wind up calling the default method at least once, any render it was accessed, even if the underlying value didn't change, and I wasn't happy with that. I'll be curious to see what you come up with.
        Howard M. Lewis Ship made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Howard M. Lewis Ship made changes -
        Assignee Robert Zeigler [ ongakugainochi ] Howard M. Lewis Ship [ hlship ]
        Robert Zeigler made changes -
        Field Original Value New Value
        Assignee Robert Zeigler [ ongakugainochi ]
        Robert Zeigler created issue -

          People

          • Assignee:
            Howard M. Lewis Ship
            Reporter:
            Robert Zeigler
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development