Tapestry 5
  1. Tapestry 5
  2. TAP5-2101

BeanEditor should always provide a new BeanValidationContext (JSR-303)

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.3.7, 5.4
    • Component/s: tapestry-beanvalidator
    • Labels:
      None

      Description

      We found that the current BeanEditor implementation:

      • fails to beanvalidate (JSR-303) complex beans (beans that contain other beans)
      • fails to validate more than one bean in the same page (when more than one BeanEditor is present in teh page)

      The problem is that BeanEditor doesn't provide the correct BeanValidationContext to the validation framework.

      Given the following beans:

      public class ComplexBean

      { private SomeSimpleBean someSimpleBean; @NotNull private String simpleNotNullProperty; ... }

      public class SimpleBean

      { @Min(6) private int minValue; .. }

      One would expect that the following page would validate all the constraint from both ComplexBean and SimpleBean:
      <t:form validate="complexBean">
      <t:BeanEditor object="complexBean" />
      <t:BeanEditor object="complexBean.someSimpleBean" />
      ...

      Instead only ComplexBean.simpleNotNullProperty constraint is validated.

      Moreover not even:
      <t:form validate="this">
      <t:BeanEditor object="beanA" />
      <t:BeanEditor object=" beanB " />
      <t:form/>

      Is (bean)validating properly....

      BeanEditor should provide the validation framework with a new BeanValidationContext bound to the object being validated all the times.

      1. beaneditor-jsr303.patch
        8 kB
        Luca Menegus
      2. TAP5-2101-2.patch
        4 kB
        Alejandro Scandroli

        Activity

        Hide
        Luca Menegus added a comment -

        Patch with fix AND test case

        Show
        Luca Menegus added a comment - Patch with fix AND test case
        Hide
        Kalle Korhonen added a comment -

        Applied, thanks Luca!

        Show
        Kalle Korhonen added a comment - Applied, thanks Luca!
        Hide
        Hudson added a comment -

        Integrated in tapestry-trunk-freestyle #1044 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1044/)
        FIXED - TAP5-2101: BeanEditor should always provide a new (Revision cb95e2236873b497211427e89f96e54282919e19)

        Result = FAILURE
        kaosko :
        Files :

        • tapestry-beanvalidator/src/test/java/org/apache/tapestry5/beanvalidator/integration/TapestryBeanValidationIntegrationTests.java
        • tapestry-beanvalidator/src/test/java/org/example/testapp/entities/SomeOtherSimpleBean.java
        • tapestry-beanvalidator/src/test/webapp/Index.tml
        • tapestry-beanvalidator/src/test/java/org/example/testapp/entities/ComplexBean.java
        • tapestry-beanvalidator/src/test/java/org/example/testapp/entities/SomeSimpleBean.java
        • tapestry-beanvalidator/src/test/java/org/example/testapp/pages/ComplexBeanDemo.java
        • tapestry-beanvalidator/src/test/webapp/ComplexBeanDemo.tml
        • tapestry-core/src/main/java/org/apache/tapestry5/corelib/components/BeanEditor.java
        Show
        Hudson added a comment - Integrated in tapestry-trunk-freestyle #1044 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1044/ ) FIXED - TAP5-2101 : BeanEditor should always provide a new (Revision cb95e2236873b497211427e89f96e54282919e19) Result = FAILURE kaosko : Files : tapestry-beanvalidator/src/test/java/org/apache/tapestry5/beanvalidator/integration/TapestryBeanValidationIntegrationTests.java tapestry-beanvalidator/src/test/java/org/example/testapp/entities/SomeOtherSimpleBean.java tapestry-beanvalidator/src/test/webapp/Index.tml tapestry-beanvalidator/src/test/java/org/example/testapp/entities/ComplexBean.java tapestry-beanvalidator/src/test/java/org/example/testapp/entities/SomeSimpleBean.java tapestry-beanvalidator/src/test/java/org/example/testapp/pages/ComplexBeanDemo.java tapestry-beanvalidator/src/test/webapp/ComplexBeanDemo.tml tapestry-core/src/main/java/org/apache/tapestry5/corelib/components/BeanEditor.java
        Hide
        Hudson added a comment -

        Integrated in tapestry-trunk-freestyle #1049 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1049/)
        RESOLVED - TAP5-2101: BeanEditor should always provide a new (Revision e954445f990d6528ee3f2f0bc23a31b2a361ecd5)
        RESOLVED - TAP5-2101: BeanEditor should always provide a new (Revision 18b6f2be1c6c6a1842300944dd3bb18da27d91d0)
        RESOLVED - TAP5-2101: BeanEditor should always provide a new (Revision cc0c541ab87970bf31be04c177c016fe16274628)

        Result = SUCCESS
        kaosko :
        Files :

        • tapestry-core/src/test/java/org/apache/tapestry5/corelib/components/BeanEditorTest.java

        kaosko :
        Files :

        • tapestry-core/src/test/java/org/apache/tapestry5/corelib/components/BeanEditorTest.java

        kaosko :
        Files :

        • tapestry-core/src/test/java/org/apache/tapestry5/corelib/components/BeanEditorTest.java
        Show
        Hudson added a comment - Integrated in tapestry-trunk-freestyle #1049 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1049/ ) RESOLVED - TAP5-2101 : BeanEditor should always provide a new (Revision e954445f990d6528ee3f2f0bc23a31b2a361ecd5) RESOLVED - TAP5-2101 : BeanEditor should always provide a new (Revision 18b6f2be1c6c6a1842300944dd3bb18da27d91d0) RESOLVED - TAP5-2101 : BeanEditor should always provide a new (Revision cc0c541ab87970bf31be04c177c016fe16274628) Result = SUCCESS kaosko : Files : tapestry-core/src/test/java/org/apache/tapestry5/corelib/components/BeanEditorTest.java kaosko : Files : tapestry-core/src/test/java/org/apache/tapestry5/corelib/components/BeanEditorTest.java kaosko : Files : tapestry-core/src/test/java/org/apache/tapestry5/corelib/components/BeanEditorTest.java
        Hide
        Alejandro Scandroli added a comment -

        I'm trying to leverage this to create an edit block for @Embedded properties (http://jira.codehaus.org/browse/TYNAMO-22).

        One issue I found is that BeanEditContext and BeanValidationContext get out of sync on submit. When working with nested BeanEditors they return different classes, that is:
        BeanEditContext.getBeanClass() != BeanValidationContext.getBeanType()

        The issue is that the BeanEditContext anonymous inner class is "delegating" the call to the BeanEditor's model on the fly and the model changes. This could be easily solved by creating a BeanEditContextImpl that takes a Class in its constructor.

        One other thing, I've also noticed that the Environment ends up with more BeanValidationContext in the stack than it should. If I'm not mistaken the stack should end up completely empty after the request. If that's the case, I think that the originalBeanValidationContext push in BeanEditor.cleanupEnvironment() it's unnecessary. After all it is a stack so the previous BeanValidationContext is still there.

        Show
        Alejandro Scandroli added a comment - I'm trying to leverage this to create an edit block for @Embedded properties ( http://jira.codehaus.org/browse/TYNAMO-22 ). One issue I found is that BeanEditContext and BeanValidationContext get out of sync on submit. When working with nested BeanEditors they return different classes, that is: BeanEditContext.getBeanClass() != BeanValidationContext.getBeanType() The issue is that the BeanEditContext anonymous inner class is "delegating" the call to the BeanEditor's model on the fly and the model changes. This could be easily solved by creating a BeanEditContextImpl that takes a Class in its constructor. One other thing, I've also noticed that the Environment ends up with more BeanValidationContext in the stack than it should. If I'm not mistaken the stack should end up completely empty after the request. If that's the case, I think that the originalBeanValidationContext push in BeanEditor.cleanupEnvironment() it's unnecessary. After all it is a stack so the previous BeanValidationContext is still there.
        Hide
        Kalle Korhonen added a comment -

        Reopening based on Alejandro's comments. Alejandro, if you have time, please create a patch, otherwise I'll take a look at it (tomorrow at the earliest).

        Show
        Kalle Korhonen added a comment - Reopening based on Alejandro's comments. Alejandro, if you have time, please create a patch, otherwise I'll take a look at it (tomorrow at the earliest).
        Hide
        Alejandro Scandroli added a comment - - edited

        Here is the patch (against master):

        • removed originalBeanValidationContext
        • refactored BeanEditContext anonymous inner class to BeanEditContextImpl
        Show
        Alejandro Scandroli added a comment - - edited Here is the patch (against master): removed originalBeanValidationContext refactored BeanEditContext anonymous inner class to BeanEditContextImpl
        Hide
        Hudson added a comment -

        Integrated in tapestry-trunk-freestyle #1052 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1052/)
        FIXED - TAP5-2101: BeanEditor should always provide a new (Revision 349ef622030ed5f1bfb098829321f753b4e19843)

        Result = SUCCESS
        kaosko :
        Files :

        • tapestry-core/src/main/java/org/apache/tapestry5/internal/BeanEditContextImpl.java
        • tapestry-core/src/main/java/org/apache/tapestry5/corelib/components/BeanEditor.java
        • tapestry-core/src/test/java/org/apache/tapestry5/corelib/components/BeanEditorTest.java
        Show
        Hudson added a comment - Integrated in tapestry-trunk-freestyle #1052 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1052/ ) FIXED - TAP5-2101 : BeanEditor should always provide a new (Revision 349ef622030ed5f1bfb098829321f753b4e19843) Result = SUCCESS kaosko : Files : tapestry-core/src/main/java/org/apache/tapestry5/internal/BeanEditContextImpl.java tapestry-core/src/main/java/org/apache/tapestry5/corelib/components/BeanEditor.java tapestry-core/src/test/java/org/apache/tapestry5/corelib/components/BeanEditorTest.java
        Hide
        Kalle Korhonen added a comment -

        Applied Alejandro's patch and cherry-picked to 5.3, thanks!

        Show
        Kalle Korhonen added a comment - Applied Alejandro's patch and cherry-picked to 5.3, thanks!

          People

          • Assignee:
            Kalle Korhonen
            Reporter:
            Luca Menegus
          • Votes:
            4 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development