javaserverfaces-spec-public
  1. javaserverfaces-spec-public
  2. JAVASERVERFACES_SPEC_PUBLIC-917

add a JUnit test to make sure that classes implementing FacesWrapper do wrap all public and protected methods of the wrapped class

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1, 2.2
    • Component/s: Uncategorized
    • Labels:
      None

      Description

      Often it gets forgotten to change the wrapper classes whe a method is changed or added to the wrapped class. This can result in unexpected behavior.

      Therfore I developed the attached JUnit test to scan all classes implementing FacesWrapper and make sure the wrappers do wrap all public and protected methods of the wrapped class.

      On the current 2.1.0 development stage as of 23. dec 2010, the following missing wrapper methods where detected:

      ResourceWrapper:

      • getContentType()
      • getLibraryName()
      • getResourceName()
      • setContentType(String)
      • setLibraryName(String)
      • setResourceName(String)

      ExternalContextWrapper:

      • getSessionMaxInactiveInterval()
      • isSecure()
      • setSessionMaxInactiveInterval()

      PartialViewContextWrapper

      • setPartialRequest(boolean)

      Since the above methods would cause API change and 2.1 closed, this has to be planned for 2.2.

      See also attached patch file providing the FacesWrapperTestCase and the minimal change for the wrapper classes.

      1. spec-917-FacesWrapper-testcase.patch
        13 kB
        Hanspeter Duennenberger
      2. spec-917-FacesWrapper-testcase-ready-for-commit.patch
        21 kB
        Hanspeter Duennenberger
      3. spec-917-FacesWrapper-testcase-with-docs.patch
        16 kB
        Hanspeter Duennenberger

        Issue Links

          Activity

          Hide
          Hanspeter Duennenberger added a comment -

          914 is related, but this one goes the full way to make sure wrappers do wrap all methods.

          Show
          Hanspeter Duennenberger added a comment - 914 is related, but this one goes the full way to make sure wrappers do wrap all methods.
          Hide
          Ed Burns added a comment -

          Make this a task.

          Show
          Ed Burns added a comment - Make this a task.
          Hide
          Hanspeter Duennenberger added a comment -

          Hi Ed,

          I'd like to contribute that - please review patches and let me commit these changes.
          While updating on this issue I also fixed some missing/wrong javadocs on ExteralContextWrapper - see latest attachement.

          regards
          Hanspeter

          Show
          Hanspeter Duennenberger added a comment - Hi Ed, I'd like to contribute that - please review patches and let me commit these changes. While updating on this issue I also fixed some missing/wrong javadocs on ExteralContextWrapper - see latest attachement. regards Hanspeter
          Hide
          Hanspeter Duennenberger added a comment -

          Hi Ed,

          just updated the patch to also include the build.xml changes to run the FacesWrapperTestCase.

          I was able to run "ant clean main test.with.container.refresh" with no error. Please review the patch or let someone test it so I can commit the changes.

          Since these changes also affect the TCK for JSF 2.2, how are the changes sent to the TCK developers?

          regards
          Hanspeter

          Show
          Hanspeter Duennenberger added a comment - Hi Ed, just updated the patch to also include the build.xml changes to run the FacesWrapperTestCase. I was able to run "ant clean main test.with.container.refresh" with no error. Please review the patch or let someone test it so I can commit the changes. Since these changes also affect the TCK for JSF 2.2, how are the changes sent to the TCK developers? regards Hanspeter
          Hide
          i_oss added a comment -

          Hi Hanspeter,
          nice solution!!!

          Ed: maybe you want to add this one: http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-938 as a duplicate or "going to be fixed by this" .

          Thank you,
          Imre

          Show
          i_oss added a comment - Hi Hanspeter, nice solution!!! Ed: maybe you want to add this one: http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-938 as a duplicate or "going to be fixed by this" . Thank you, Imre
          Hide
          Ed Burns added a comment -

          I like this. r=edburns.

          This is the first change to any class in package javax.faces.application in JSF 2.2. Therefore, make sure to modify javax/faces/application/package.html to include changed_modified_2_2 in the span there where the rest of the changes are flagged.

          Please commit to trunk when the extenal hudson jobs for trunk are all clean <http://hudson.glassfish.org/view/JSF%20Mojarra/>.

          Show
          Ed Burns added a comment - I like this. r=edburns. This is the first change to any class in package javax.faces.application in JSF 2.2. Therefore, make sure to modify javax/faces/application/package.html to include changed_modified_2_2 in the span there where the rest of the changes are flagged. Please commit to trunk when the extenal hudson jobs for trunk are all clean < http://hudson.glassfish.org/view/JSF%20Mojarra/ >.
          Hide
          Hanspeter Duennenberger added a comment -

          updated patch with changed_added_2_2/changed_modified_2_2 markers

          Show
          Hanspeter Duennenberger added a comment - updated patch with changed_added_2_2/changed_modified_2_2 markers
          Hide
          Hanspeter Duennenberger added a comment -

          Added FacesWrapperTestCase to ensure wraper classes do wrap all the public/protected methods of the wrapped class.
          By adding this test missing wrapper methods on 3 existing wrappers where detected:

          javax.faces.application.ResourceWrapper:

          • getContentType()
          • getLibraryName()
          • getResourceName()
          • setContentType(String)
          • setLibraryName(String)
          • setResourceName(String)
            javax.faces.context.ExternalContextWrapper:
          • getSessionMaxInactiveInterval()
          • isSecure()
          • setSessionMaxInactiveInterval()
            javax.faces.context.PartialViewContextWrapper
          • setPartialRequest(boolean)

          r=edburns

          SECTION: Modified files

          M jsf-api/build.xml
          M jsf-api/build-source.xml
          M jsf-api/src/main/java/javax/faces/application/package.html
          M jsf-api/src/main/java/javax/faces/application/ResourceWrapper.java
          M jsf-api/src/main/java/javax/faces/context/ExternalContextWrapper.java
          M jsf-api/src/main/java/javax/faces/context/package.html
          M jsf-api/src/main/java/javax/faces/context/PartialViewContextWrapper.java
          A jsf-api/src/test/java/javax/faces/context/FacesWrapperTestCase.java

          Show
          Hanspeter Duennenberger added a comment - Added FacesWrapperTestCase to ensure wraper classes do wrap all the public/protected methods of the wrapped class. By adding this test missing wrapper methods on 3 existing wrappers where detected: javax.faces.application.ResourceWrapper: getContentType() getLibraryName() getResourceName() setContentType(String) setLibraryName(String) setResourceName(String) javax.faces.context.ExternalContextWrapper: getSessionMaxInactiveInterval() isSecure() setSessionMaxInactiveInterval() javax.faces.context.PartialViewContextWrapper setPartialRequest(boolean) r=edburns SECTION: Modified files M jsf-api/build.xml M jsf-api/build-source.xml M jsf-api/src/main/java/javax/faces/application/package.html M jsf-api/src/main/java/javax/faces/application/ResourceWrapper.java M jsf-api/src/main/java/javax/faces/context/ExternalContextWrapper.java M jsf-api/src/main/java/javax/faces/context/package.html M jsf-api/src/main/java/javax/faces/context/PartialViewContextWrapper.java A jsf-api/src/test/java/javax/faces/context/FacesWrapperTestCase.java
          Hide
          Hanspeter Duennenberger added a comment -

          Patch created immediately before commit.

          Show
          Hanspeter Duennenberger added a comment - Patch created immediately before commit.
          Hide
          Manfred Riem added a comment -

          Closing resolved issue out

          Show
          Manfred Riem added a comment - Closing resolved issue out

            People

            • Assignee:
              Hanspeter Duennenberger
              Reporter:
              Hanspeter Duennenberger
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: