MyFaces Core
  1. MyFaces Core
  2. MYFACES-3039

MyFaces broken in Portlet environment: Fails to support extendable FacesContextFactory/FacesContext/ExternalContext

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.5
    • Component/s: JSR-314
    • Labels:
      None

      Description

      JSF 2.0 improved the definition/handling of the instantiation of the FacesContext allowing non-servlet environments to wrap the base/core impl. This was done because most of the FacesContext apis are inherently runtime environment neutral – allowing the portlet bridge to not have to duplicate/reimplement and maybe get wrong base core function. Unfortunately MyFaces doesn't conform to this change and hence the Portlet Bridge can't run in the MyFaces environment.

      Basically the bridge expects to be able to delegate from its FacesContextFactoryImpl.getFacesContext and then wrap the returned FacesContext with its own. This requires the underlying core impl to be runtime (servlet/portlet) neutral during the creation process. The bridge will wrap the FacesContext and supply its own ExternalContext such that any servlet dependent impl in the core FacesContext/ExternalContext will be hidden by overrides.

      FYI ... until this is addressed I can't begin any testing of the bridge on MyFaces.

        Activity

        Hide
        Leonardo Uribe added a comment -

        I don't understand this one. Could you provide more details about the code you are trying to make it work. For my understanding, MyFaces allows override FacesContext / ExternalContext factories without problem. It has been widely tested, but I know portlet support is a special case.

        Show
        Leonardo Uribe added a comment - I don't understand this one. Could you provide more details about the code you are trying to make it work. For my understanding, MyFaces allows override FacesContext / ExternalContext factories without problem. It has been widely tested, but I know portlet support is a special case.
        Hide
        Michael Freedman added a comment -

        2.0.3 has the following code (at the end of getFacesContext in its FacesContextFactoryImpl):
        if (context instanceof ServletContext)
        {
        FacesContext facesContext;
        if (externalContext instanceof ReleaseableExternalContext)

        { facesContext = new FacesContextImpl(externalContext, (ReleaseableExternalContext) externalContext, this); }

        else if (defaultExternalContext != null && defaultExternalContext instanceof ReleaseableExternalContext)

        { facesContext = new FacesContextImpl(externalContext, (ReleaseableExternalContext) defaultExternalContext, this); }

        else

        { facesContext = new FacesContextImpl(externalContext, null, this); }

        facesContext.setExceptionHandler(_exceptionHandlerFactory.getExceptionHandler());

        return facesContext;
        //return new FacesContextImpl((ServletContext)context, (ServletRequest)request, (ServletResponse)response);
        }

        throw new FacesException("Unsupported context type " + context.getClass().getName());

        As you see from this code – if you aren't running in the servlet environment you throw an Unsupported context exception. So I am suggesting this code be reworked to be context neutral so the portlet bridge can rely on the same delegation support as it does with Mojarra.

        Show
        Michael Freedman added a comment - 2.0.3 has the following code (at the end of getFacesContext in its FacesContextFactoryImpl): if (context instanceof ServletContext) { FacesContext facesContext; if (externalContext instanceof ReleaseableExternalContext) { facesContext = new FacesContextImpl(externalContext, (ReleaseableExternalContext) externalContext, this); } else if (defaultExternalContext != null && defaultExternalContext instanceof ReleaseableExternalContext) { facesContext = new FacesContextImpl(externalContext, (ReleaseableExternalContext) defaultExternalContext, this); } else { facesContext = new FacesContextImpl(externalContext, null, this); } facesContext.setExceptionHandler(_exceptionHandlerFactory.getExceptionHandler()); return facesContext; //return new FacesContextImpl((ServletContext)context, (ServletRequest)request, (ServletResponse)response); } throw new FacesException("Unsupported context type " + context.getClass().getName()); As you see from this code – if you aren't running in the servlet environment you throw an Unsupported context exception. So I am suggesting this code be reworked to be context neutral so the portlet bridge can rely on the same delegation support as it does with Mojarra.
        Hide
        Scott O'Bryan added a comment -

        I think I disagree with this bug. This limitation is not spec'd in JSF and I'm not even sure it's needed provided MyFaces WORKS with a beige provided FacesContext. The Bridge's FacesContextFactory should be of the 0-parameter sort and should implement its own FacesContext and ExternalContext.

        I believe that the FacesContext implementation is fairly trivial. Perhaps we should discuss his some on the list.

        Show
        Scott O'Bryan added a comment - I think I disagree with this bug. This limitation is not spec'd in JSF and I'm not even sure it's needed provided MyFaces WORKS with a beige provided FacesContext. The Bridge's FacesContextFactory should be of the 0-parameter sort and should implement its own FacesContext and ExternalContext. I believe that the FacesContext implementation is fairly trivial. Perhaps we should discuss his some on the list.
        Hide
        Scott O'Bryan added a comment -

        One other idea might be to move a container-neutral implementation of FacesCotextImpl into shared and then put container specific impl's into each container. This will allow us to use a common code base for most Of the FaceContext's stuff, butthen each container can have its own real implementation without Beig dependent on one another.

        Further, this would work in Mojarra as well.

        Show
        Scott O'Bryan added a comment - One other idea might be to move a container-neutral implementation of FacesCotextImpl into shared and then put container specific impl's into each container. This will allow us to use a common code base for most Of the FaceContext's stuff, butthen each container can have its own real implementation without Beig dependent on one another. Further, this would work in Mojarra as well.
        Hide
        Michael Freedman added a comment -

        I have to respectively disagree. The only servlet dependency I see in the MyFaces FacesContextImpl I see is in the "special" constructor code used to self create an external context. Isn't this a holdover from the JSF 1.2 pattern? In JSF 2.0 things were reorganized so the ExternalContext is constructed from a factory and passed to the FacesContext constructor – ala:
        FacesContext ctx =
        new FacesContextImpl(
        externalContextFactory.getExternalContext(sc, request, response),
        lifecycle);

        This allows the FacesContext to be environment neutral (all environmental stuff is within the ExternalContext) – and with the new FacesContext wrapper its now easy for the bridge to wrap the core context and only override the few method it changes.

        What is the rationale for the MyFaces model and runtime dependency? If some of that rationale needs to be maintained, can't it be isolated to the circumstance it pertains to and the more general pattern (runtime neutral) be utilized in other cases (like the bridge)?

        Basically, what I am arguing is that the model supported by the spec and implemented by Mojarra seems to be a general/broad model and should be supported by MyFaces – and if improvements can be made for specific situations, so be it, but handle as such without penalizing the general.

        Show
        Michael Freedman added a comment - I have to respectively disagree. The only servlet dependency I see in the MyFaces FacesContextImpl I see is in the "special" constructor code used to self create an external context. Isn't this a holdover from the JSF 1.2 pattern? In JSF 2.0 things were reorganized so the ExternalContext is constructed from a factory and passed to the FacesContext constructor – ala: FacesContext ctx = new FacesContextImpl( externalContextFactory.getExternalContext(sc, request, response), lifecycle); This allows the FacesContext to be environment neutral (all environmental stuff is within the ExternalContext) – and with the new FacesContext wrapper its now easy for the bridge to wrap the core context and only override the few method it changes. What is the rationale for the MyFaces model and runtime dependency? If some of that rationale needs to be maintained, can't it be isolated to the circumstance it pertains to and the more general pattern (runtime neutral) be utilized in other cases (like the bridge)? Basically, what I am arguing is that the model supported by the spec and implemented by Mojarra seems to be a general/broad model and should be supported by MyFaces – and if improvements can be made for specific situations, so be it, but handle as such without penalizing the general.
        Hide
        Scott O'Bryan added a comment -

        Ahhh yes. Mike, you're one hundred percent right. Pre-JSF 2.0 I don't think we delegated because there was no concept of an ExternalContextFactory. That was set up by the FacesContextFactory. Now, however, the ExternalContextFactory is the "hook point" for the container abstraction. Simply put the FacesContextFactory should not care what objects back the ExternalContext so long as the contracts are followed and, looking at the code above, it is unclear to me why the check is even there.

        I suspect that its there because FacesContext used to instantiate externalContext itself (and there clearly used to be a cast to the native objects)..

        So yeah, the FacesContext and Factory should be container agnostic for JSF 2.0.

        Show
        Scott O'Bryan added a comment - Ahhh yes. Mike, you're one hundred percent right. Pre-JSF 2.0 I don't think we delegated because there was no concept of an ExternalContextFactory. That was set up by the FacesContextFactory. Now, however, the ExternalContextFactory is the "hook point" for the container abstraction. Simply put the FacesContextFactory should not care what objects back the ExternalContext so long as the contracts are followed and, looking at the code above, it is unclear to me why the check is even there. I suspect that its there because FacesContext used to instantiate externalContext itself (and there clearly used to be a cast to the native objects).. So yeah, the FacesContext and Factory should be container agnostic for JSF 2.0.
        Hide
        Leonardo Uribe added a comment -

        Ok, now I understand, it is more simple than it seems to be. The check is there just by historical reasons, so we can just comment it and prevent throw the exception. The current FacesContext implementation should work without problem in portlet case.

        Show
        Leonardo Uribe added a comment - Ok, now I understand, it is more simple than it seems to be. The check is there just by historical reasons, so we can just comment it and prevent throw the exception. The current FacesContext implementation should work without problem in portlet case.
        Hide
        Michael Freedman added a comment -

        Can you clarify what the defaultExternalContext is being used for? On portlet requests this seemingly won't be set/used as its the Bridge's ExternalContext that is in use not the core MyFaces one (which does the attribute put that causes this default stuff to be enabled). Basically, I am trying to ensure there isn't a problem in the portlet env in not participating in this mechanism.

        Show
        Michael Freedman added a comment - Can you clarify what the defaultExternalContext is being used for? On portlet requests this seemingly won't be set/used as its the Bridge's ExternalContext that is in use not the core MyFaces one (which does the attribute put that causes this default stuff to be enabled). Basically, I am trying to ensure there isn't a problem in the portlet env in not participating in this mechanism.
        Hide
        Leonardo Uribe added a comment -

        The objective of defaultExternalContext is work as a fallback when jsf 1.2 or 1.1 libraries with custom ExternalContext wrapper are used. The idea is if the external wrapper does not implement a method introduced on jsf 2.0, it automatically fallback to the defaultExternalContext implementation.

        It is known both myfaces and mojarra has different strategies to handle this case, and there is nothing on the spec related to this behavior.

        In jsf 2.0 it was introduced ExternalContextWrapper, so all wrappers must extends from this class.

        At the end, in my opinion, it should not be a concern in portlet case. It is a known problem in portlets (it happens with jsf 1.2 and some conditions, that's the reason why orchestra has a jsf 1.2 package). I tried once to comunicate this problem to you personally (in JSFDays) but in that time it was not clear why this detail was important (as now), so in that time I was not taken too seriously and really I don't see how this detail could be on the spec in other way. Note the wrapper solves the problem for future jsf versions.

        Show
        Leonardo Uribe added a comment - The objective of defaultExternalContext is work as a fallback when jsf 1.2 or 1.1 libraries with custom ExternalContext wrapper are used. The idea is if the external wrapper does not implement a method introduced on jsf 2.0, it automatically fallback to the defaultExternalContext implementation. It is known both myfaces and mojarra has different strategies to handle this case, and there is nothing on the spec related to this behavior. In jsf 2.0 it was introduced ExternalContextWrapper, so all wrappers must extends from this class. At the end, in my opinion, it should not be a concern in portlet case. It is a known problem in portlets (it happens with jsf 1.2 and some conditions, that's the reason why orchestra has a jsf 1.2 package). I tried once to comunicate this problem to you personally (in JSFDays) but in that time it was not clear why this detail was important (as now), so in that time I was not taken too seriously and really I don't see how this detail could be on the spec in other way. Note the wrapper solves the problem for future jsf versions.

          People

          • Assignee:
            Leonardo Uribe
            Reporter:
            Michael Freedman
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development