Issue Details (XML | Word | Printable)

Key: JAVASERVERFACES-1866
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Blocker Blocker
Assignee: rogerk
Reporter: Hanspeter Duennenberger
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
javaserverfaces

ParialViewContextImpl does not respect FacesContextWrapper

Created: 12/Nov/10 02:43 AM   Updated: 15/Feb/12 02:02 PM   Resolved: 15/Nov/10 08:58 AM
Component/s: ajax
Affects Version/s: 2.1.0
Fix Version/s: 2.1_gf31_m7

Time Tracking:
Not Specified

File Attachments: 1. Text File changebundle.txt (2 kB) 12/Nov/10 09:51 AM - rogerk
2. Text File PartialViewContextImpl-1866.patch (3 kB) 14/Nov/10 01:30 PM - Hanspeter Duennenberger
3. Text File PartialViewContextImpl-1866.patch (3 kB) 12/Nov/10 05:51 PM - Hanspeter Duennenberger

Environment:

Operating System: All
Platform: All


Issuezilla Id: 1,866
Status Whiteboard:

size_small importance_large

Tags:
Participants: Ed Burns, Hanspeter Duennenberger, Manfred Riem and rogerk


 Description  « Hide

PartialViewContextImple gets the impl's FacesContext reference from
FacesContextImpl directly and always uses this FacesContextImpl for partial
request processing (processPartial() always uses ctx that was passed in at
construcion time).

Therefore it's not possible to use wrapped FacesContext for partial request
processing, unless wrapped FacesContext's override the method
getPartialViewContext().

I think there are two possible solutions:
1. make FacesContextWrapper.getPartialViewContext() methot abstract so wrappers
are forced to override this method.
2. on first access PartialViewContextImpl should acquire
FacesContext.getCurrentInstance() and cache it locally instead of using the
passed in instance.



rogerk added a comment - 12/Nov/10 06:50 AM

triage


rogerk added a comment - 12/Nov/10 06:52 AM

Starting


rogerk added a comment - 12/Nov/10 08:02 AM

Although option 1 (making FacesContextWrapper.getPartialViewContext abstract)
seems like it may be an easier solution, it is an api change and may be late to
do at this stage - plus backwards compatibility issues (all of a sudden forcing
people to override this method). So I'm inclined I guess to go with option 2.
For option 2, would introducing a PartialViewContextImp() no arg constructor
suffice (in addition to keeping the constructor accepting a FacsContext)?
So, something like:

public PartialViewContextImpl() { this.ctx = FacesContext.getCurrentInstance(); }


Hanspeter Duennenberger added a comment - 12/Nov/10 08:37 AM

Hi Roger,

I think it would be better to introduce a method like this:
private FacesContext getFacesContext() {
if (ctx == null || ctx.isReleased()) { ctx = FacesContext.getCurrentInstance(); }
return ctx;
}

and use that method inside PartialViewContextImpl wherever ctx was used
directly. That way the caching of ctx happens as late as possible and it would
also respect a possibly changing FacesContext in case of portal integration
where ctx might change between execute and render phase.

This kind of caching FacesContext could also be implemented in UIComponentBase -
but that would be another issue.


rogerk added a comment - 12/Nov/10 09:51 AM

Created an attachment (id=1334)
First Iteration.


Ed Burns added a comment - 12/Nov/10 10:03 AM

What Hanspeter suggests seems reasonable to me. Roger, what do you think?


Ed Burns added a comment - 12/Nov/10 10:06 AM

R=edburns for attachment 1334.


rogerk added a comment - 12/Nov/10 10:22 AM

Committed revision 8720.


Hanspeter Duennenberger added a comment - 12/Nov/10 05:49 PM

Hi Roger.

Sorry, I had no time to review before. I reviewed and it was not exactly what I
meant, but then I changed locally how I meant and it also did not work.

The problem is, that the first access to isAjaxRequest() happens while
FacesContext is still created (setup of ExternalContext). After a few tries I
realized that ctx must be updated with FacesContext.getCurrentInstance() when
the phase processing has started. I tested with that and then it worked fine.

I'll add the patch file with that solution.

Probably it would be good to have a test-case too, is should make shure that
PartialViewContextWrappers are used and that processPartial(PhaseId) uses a
possibly existing FacesContextWrapper. I don't know if there is already a
TestCase for FacesContextWrapper - maybe this test could be added there.

Sorry for the late review.

regards
Hanspeter


Hanspeter Duennenberger added a comment - 12/Nov/10 05:51 PM

Created an attachment (id=1335)
PartialViewContextImpl patch


Hanspeter Duennenberger added a comment - 14/Nov/10 01:30 PM

Created an attachment (id=1337)
Improved patch with better selfdescribing naming


rogerk added a comment - 15/Nov/10 06:40 AM

Thanks for the update.
Testing your latest patch now.


rogerk added a comment - 15/Nov/10 08:58 AM

Committed revision 8722.


Manfred Riem added a comment - 15/Feb/12 02:02 PM

Closing issue out