javaserverfaces
  1. javaserverfaces
  2. JAVASERVERFACES-1866

ParialViewContextImpl does not respect FacesContextWrapper

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1.0
    • Fix Version/s: 2.1_gf31_m7
    • Component/s: ajax
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Issuezilla Id:
      1,866
    • Status Whiteboard:
      Hide

      size_small importance_large

      Show
      size_small importance_large

      Description

      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.

      1. changebundle.txt
        2 kB
        rogerk
      2. PartialViewContextImpl-1866.patch
        3 kB
        Hanspeter Duennenberger
      3. PartialViewContextImpl-1866.patch
        3 kB
        Hanspeter Duennenberger

        Activity

        Hide
        rogerk added a comment -

        triage

        Show
        rogerk added a comment - triage
        Hide
        rogerk added a comment -

        Starting

        Show
        rogerk added a comment - Starting
        Hide
        rogerk added a comment -

        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(); }
        Show
        rogerk added a comment - 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(); }
        Hide
        Hanspeter Duennenberger added a comment -

        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.

        Show
        Hanspeter Duennenberger added a comment - 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.
        Hide
        rogerk added a comment -

        Created an attachment (id=1334)
        First Iteration.

        Show
        rogerk added a comment - Created an attachment (id=1334) First Iteration.
        Hide
        Ed Burns added a comment -

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

        Show
        Ed Burns added a comment - What Hanspeter suggests seems reasonable to me. Roger, what do you think?
        Hide
        Ed Burns added a comment -

        R=edburns for attachment 1334.

        Show
        Ed Burns added a comment - R=edburns for attachment 1334.
        Hide
        rogerk added a comment -

        Committed revision 8720.

        Show
        rogerk added a comment - Committed revision 8720.
        Hide
        Hanspeter Duennenberger added a comment -

        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

        Show
        Hanspeter Duennenberger added a comment - 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
        Hide
        Hanspeter Duennenberger added a comment -

        Created an attachment (id=1335)
        PartialViewContextImpl patch

        Show
        Hanspeter Duennenberger added a comment - Created an attachment (id=1335) PartialViewContextImpl patch
        Hide
        Hanspeter Duennenberger added a comment -

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

        Show
        Hanspeter Duennenberger added a comment - Created an attachment (id=1337) Improved patch with better selfdescribing naming
        Hide
        rogerk added a comment -

        Thanks for the update.
        Testing your latest patch now.

        Show
        rogerk added a comment - Thanks for the update. Testing your latest patch now.
        Hide
        rogerk added a comment -

        Committed revision 8722.

        Show
        rogerk added a comment - Committed revision 8722.
        Hide
        Manfred Riem added a comment -

        Closing issue out

        Show
        Manfred Riem added a comment - Closing issue out

          People

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

            Dates

            • Created:
              Updated:
              Resolved: