[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 Created: 23/Dec/10  Updated: 01/Aug/14  Resolved: 19/May/11

Status: Closed
Project: javaserverfaces-spec-public
Component/s: Uncategorized
Affects Version/s: 2.1
Fix Version/s: 2.1, 2.2

Type: Task Priority: Major
Reporter: Hanspeter Duennenberger Assignee: Hanspeter Duennenberger
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File spec-917-FacesWrapper-testcase-ready-for-commit.patch     Text File spec-917-FacesWrapper-testcase-with-docs.patch     Text File spec-917-FacesWrapper-testcase.patch    
Issue Links:
Dependency
depends on JAVASERVERFACES_SPEC_PUBLIC-914 add ResourceWrapper#getContentType() ... Closed
Related
is related to JAVASERVERFACES_SPEC_PUBLIC-938 PartialViewContextWrapper is missing ... Closed

 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.



 Comments   
Comment by Hanspeter Duennenberger [ 23/Dec/10 ]

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

Comment by Ed Burns [ 20/Jan/11 ]

Make this a task.

Comment by Hanspeter Duennenberger [ 15/Feb/11 ]

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

Comment by Hanspeter Duennenberger [ 03/May/11 ]

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

Comment by i_oss [ 03/May/11 ]

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

Comment by Ed Burns [ 10/May/11 ]

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/>.

Comment by Hanspeter Duennenberger [ 17/May/11 ]

updated patch with changed_added_2_2/changed_modified_2_2 markers

Comment by Hanspeter Duennenberger [ 19/May/11 ]

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

Comment by Hanspeter Duennenberger [ 19/May/11 ]

Patch created immediately before commit.

Comment by Manfred Riem [ 01/Aug/14 ]

Closing resolved issue out

Generated at Sat Feb 28 14:11:38 UTC 2015 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.