MyFaces Core
  1. MyFaces Core
  2. MYFACES-2544

UIViewRoot skips uncorrectly encodeBegin

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-beta-2
    • Fix Version/s: 2.0.0-beta-2
    • Component/s: JSR-314
    • Labels:
      None
    • Environment:
      myfaces trunk

      Description

      javax.faces.component.UIViewRoot.encodeBegin(FacesContext) contains:

      if (!skipPhase)

      { super.encodeBegin(context); }

      but skipPhase = context.getRenderResponse() || context.getResponseComplete() - it
      makes sense for all phases except render response phase itself. That condition
      probably should be:

      if (!context.getResponseComplete())

      { super.encodeBegin(context); }
      1. MYFACES-2544.patch
        1 kB
        Martin Kočí
      2. MYFACES-2544-2.patch
        2 kB
        Leonardo Uribe
      3. MYFACES-2544-3.patch
        4 kB
        Leonardo Uribe
      4. MYFACES-2544-4.patch
        6 kB
        Leonardo Uribe

        Activity

        Hide
        Martin Kočí added a comment -

        This issue and patch are exactly the same I've reported against mojarra: https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1425

        Show
        Martin Kočí added a comment - This issue and patch are exactly the same I've reported against mojarra: https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1425
        Hide
        Jakob Korherr added a comment -

        This is very interesting. Until now the if part in encodeBegin was never executed, because facesContext.getRenderResponse() is always true at this point. So maybe there are some unwanted side effects from the patch. Because of that I added a FIXME in the code.

        Martin, maybe you could check if this is a problem with the spec. I noticed that the if path is not that different from the else path. The only difference is that a PreRenderComponentEvent is published and I don't know if this is ok. Thanks!

        Show
        Jakob Korherr added a comment - This is very interesting. Until now the if part in encodeBegin was never executed, because facesContext.getRenderResponse() is always true at this point. So maybe there are some unwanted side effects from the patch. Because of that I added a FIXME in the code. Martin, maybe you could check if this is a problem with the spec. I noticed that the if path is not that different from the else path. The only difference is that a PreRenderComponentEvent is published and I don't know if this is ok. Thanks!
        Hide
        Martin Kočí added a comment -

        This is probably bug in spec. https://javaserverfaces.dev.java.net/nonav/docs/2.0/javadocs/javax/faces/component/UIViewRoot.html says: "Upon return from the listener, call FacesContext.getResponseComplete() and FacesContext.getRenderResponse(). If either return true set the internal state flag to true. ... Execute any processing for this phase if the internal state flag was not set." But this nonsence for render response phase, because FacesContext.renderResponse() = "Signal the JavaServer faces implementation that, as soon as the current phase of the request processing lifecycle has been completed, control should be passed to the <em>Render Response</em> phase, bypassing any phases that have not been executed yet."

        Delivering PreRenderComponentEvent is ok, because spec says:
        "encodeBegin() must publish a PreRenderComponentEvent" or "PreRenderComponentEvent indicates that the source component is about to be rendered". UIViewRoot is UIComponent too and there is no reason to act differently (although PreRenderViewEvent and PreRenderComponentEvent attached to UIViewRoot have same meaning).

        Show
        Martin Kočí added a comment - This is probably bug in spec. https://javaserverfaces.dev.java.net/nonav/docs/2.0/javadocs/javax/faces/component/UIViewRoot.html says: "Upon return from the listener, call FacesContext.getResponseComplete() and FacesContext.getRenderResponse(). If either return true set the internal state flag to true. ... Execute any processing for this phase if the internal state flag was not set." But this nonsence for render response phase, because FacesContext.renderResponse() = "Signal the JavaServer faces implementation that, as soon as the current phase of the request processing lifecycle has been completed, control should be passed to the <em>Render Response</em> phase, bypassing any phases that have not been executed yet." Delivering PreRenderComponentEvent is ok, because spec says: "encodeBegin() must publish a PreRenderComponentEvent" or "PreRenderComponentEvent indicates that the source component is about to be rendered". UIViewRoot is UIComponent too and there is no reason to act differently (although PreRenderViewEvent and PreRenderComponentEvent attached to UIViewRoot have same meaning).
        Hide
        Leonardo Uribe added a comment -

        I'll revert the previous patch. The use case you have to consider is when there is an error on a beforePhase listener. If there is, context.getResponseComplete() could return true, inclusive on render response phase. Long time ago, I review MYFACES-2374 and It is known ri does not do this, and this one cause an error on a compatibility test, but in this case the previous behavior is the expected one (the javadoc says so and this one comes from jsf 1.2).

        Please note maybe there is a misundersanding about what FacesContext.renderResponse() and FacesContext.getRenderResponse() do. The meaning of this constant is allow jsf to skip phases when an error occur. For example, if a validator throws a RuntimeException, ProcessUpdates and InvokeApplication phases should not be executed, so they will be skipped and RenderResponse phase should occur. Why UIViewRoot.encodeBegin skip render the view? because in jsf 1.2 this is considered an error and encodeBegin should not happen, instead, the error page is published.

        Now, PreRenderViewEvent could be used to change the view to be rendered. Just take a look at org.apache.myfaces.lifecycle.RenderResponseExecutor. Its meaning and behavior are different to PreRenderComponentEvent. Maybe the real bug is UIViewRoot does not call

        context.getApplication().publishEvent(context, PreRenderComponentEvent.class, UIComponent.class, this);

        before call encodeBegin(), like UIComponentBase.encodeBegin() does. Maybe this one was ignored, because usually UIViewRoot does not render anything.

        Note to solve this issue we have also to check if the new behavior introduced with ExceptionHandler api is compatible with the original jsf 1.2 algorithm.

        Jakob, could you take a look at this one again from this point of view?

        Show
        Leonardo Uribe added a comment - I'll revert the previous patch. The use case you have to consider is when there is an error on a beforePhase listener. If there is, context.getResponseComplete() could return true, inclusive on render response phase. Long time ago, I review MYFACES-2374 and It is known ri does not do this, and this one cause an error on a compatibility test, but in this case the previous behavior is the expected one (the javadoc says so and this one comes from jsf 1.2). Please note maybe there is a misundersanding about what FacesContext.renderResponse() and FacesContext.getRenderResponse() do. The meaning of this constant is allow jsf to skip phases when an error occur. For example, if a validator throws a RuntimeException, ProcessUpdates and InvokeApplication phases should not be executed, so they will be skipped and RenderResponse phase should occur. Why UIViewRoot.encodeBegin skip render the view? because in jsf 1.2 this is considered an error and encodeBegin should not happen, instead, the error page is published. Now, PreRenderViewEvent could be used to change the view to be rendered. Just take a look at org.apache.myfaces.lifecycle.RenderResponseExecutor. Its meaning and behavior are different to PreRenderComponentEvent. Maybe the real bug is UIViewRoot does not call context.getApplication().publishEvent(context, PreRenderComponentEvent.class, UIComponent.class, this); before call encodeBegin(), like UIComponentBase.encodeBegin() does. Maybe this one was ignored, because usually UIViewRoot does not render anything. Note to solve this issue we have also to check if the new behavior introduced with ExceptionHandler api is compatible with the original jsf 1.2 algorithm. Jakob, could you take a look at this one again from this point of view?
        Hide
        Martin Kočí added a comment -

        > when there is an error on a beforePhase listener. If there is, context.getResponseComplete() could return true, inclusive on render response phase.

        How is context.getResponseComplete() related to error handling? I don't get the point about error in beforePhase - if there is a error in phaseListener it is logged and/or published with exceptionHandler: "6.2.2 Backwards Compatible ExceptionHandler" and "12.3 PhaseListener"

        > Please note maybe there is a misundersanding about what >FacesContext.renderResponse() and FacesContext.getRenderResponse() do. > The meaning of this constant is allow jsf to skip phases when an error > occur. For example, if a validator throws a RuntimeException,
        > ProcessUpdates and InvokeApplication phases should not be executed, so they will be skipped and RenderResponse phase should occur.

        Yes, but why exclude from executing exactly one method at particular class? Why UIViewRoot.encodeEnd is not exluded? UIViewRoot.encodeBegin represents the beginning of render response phase as component. It is legal for a listener (action or valueChange) to call context FacesContext.renderResponse() but it does not mean that there was a error - listener can simple be smart enough to know that no other phases except for render response are needed.

        > Why UIViewRoot.encodeBegin skip render the view? because in jsf 1.2
        > this is considered an error and encodeBegin should not happen,
        > instead, the error page is published.
        I don't think "FacesContext.getRenderResponse() true = a runtime exception" is a valid expectation - skipping to render response phase is a possible response to runtime exception, but it does not imply that render response phase is executing as response to error.

        Here is example what should work:
        MyUIView extends UIViewRoot {

        encodeBegin()

        { super.encodeBegin() // super notifies phase listeners, puts component to EL and publish PreRenderComponentEvent // rendering specific stuff here }

        }

        This was not working with mojarra and isn't working with myfaces. But clearly it can be fixed with adding

        context.getApplication().publishEvent(PreRenderComponentEvent.class) and it will work together with algorithm requested by spec.

        Show
        Martin Kočí added a comment - > when there is an error on a beforePhase listener. If there is, context.getResponseComplete() could return true, inclusive on render response phase. How is context.getResponseComplete() related to error handling? I don't get the point about error in beforePhase - if there is a error in phaseListener it is logged and/or published with exceptionHandler: "6.2.2 Backwards Compatible ExceptionHandler" and "12.3 PhaseListener" > Please note maybe there is a misundersanding about what >FacesContext.renderResponse() and FacesContext.getRenderResponse() do. > The meaning of this constant is allow jsf to skip phases when an error > occur. For example, if a validator throws a RuntimeException, > ProcessUpdates and InvokeApplication phases should not be executed, so they will be skipped and RenderResponse phase should occur. Yes, but why exclude from executing exactly one method at particular class? Why UIViewRoot.encodeEnd is not exluded? UIViewRoot.encodeBegin represents the beginning of render response phase as component. It is legal for a listener (action or valueChange) to call context FacesContext.renderResponse() but it does not mean that there was a error - listener can simple be smart enough to know that no other phases except for render response are needed. > Why UIViewRoot.encodeBegin skip render the view? because in jsf 1.2 > this is considered an error and encodeBegin should not happen, > instead, the error page is published. I don't think "FacesContext.getRenderResponse() true = a runtime exception" is a valid expectation - skipping to render response phase is a possible response to runtime exception, but it does not imply that render response phase is executing as response to error. Here is example what should work: MyUIView extends UIViewRoot { encodeBegin() { super.encodeBegin() // super notifies phase listeners, puts component to EL and publish PreRenderComponentEvent // rendering specific stuff here } } This was not working with mojarra and isn't working with myfaces. But clearly it can be fixed with adding context.getApplication().publishEvent(PreRenderComponentEvent.class) and it will work together with algorithm requested by spec.
        Hide
        Leonardo Uribe added a comment -

        >How is context.getResponseComplete() related to error handling?

        Look org.apache.myfaces.renderkit.ErrorPageWriter. This one is called after log a page.

        >Yes, but why exclude from executing exactly one method at particular class? Why UIViewRoot.encodeEnd is not exluded?

        This should not happen and it is a bug. This means the code that call the listener inside encodeBegin() should be called overriding UIViewRoot.encodeAll(). Note this is not notice because the typical use case fall into previous phases.

        >I don't think "FacesContext.getRenderResponse() true = a runtime exception" is a valid expectation

        True. My intention here is analize a possible use case that makes fail the previous patch, not say this is the rule.

        I committed an incomplete alternate patch. It is better to check:

        context.getResponseComplete() || (context.getRenderResponse() && !PhaseId.RENDER_RESPONSE.equals(phaseId))

        and move the algorithm that check for the listener to encodeAll. This fix should be done in jsf 1.2 too. I'll commit the full solution soon.

        Show
        Leonardo Uribe added a comment - >How is context.getResponseComplete() related to error handling? Look org.apache.myfaces.renderkit.ErrorPageWriter. This one is called after log a page. >Yes, but why exclude from executing exactly one method at particular class? Why UIViewRoot.encodeEnd is not exluded? This should not happen and it is a bug. This means the code that call the listener inside encodeBegin() should be called overriding UIViewRoot.encodeAll(). Note this is not notice because the typical use case fall into previous phases. >I don't think "FacesContext.getRenderResponse() true = a runtime exception" is a valid expectation True. My intention here is analize a possible use case that makes fail the previous patch, not say this is the rule. I committed an incomplete alternate patch. It is better to check: context.getResponseComplete() || (context.getRenderResponse() && !PhaseId.RENDER_RESPONSE.equals(phaseId)) and move the algorithm that check for the listener to encodeAll. This fix should be done in jsf 1.2 too. I'll commit the full solution soon.
        Hide
        Leonardo Uribe added a comment -

        The latest patch (MYFACES-2544-3.patch) is the final form of the proposed solution. If no objections I'll commit it soon

        Show
        Leonardo Uribe added a comment - The latest patch ( MYFACES-2544 -3.patch) is the final form of the proposed solution. If no objections I'll commit it soon
        Hide
        Leonardo Uribe added a comment -

        Reading the spec javadoc, it is enforced listeners should be called from encodeBegin() and encodeEnd(). We have to use an alternate way to produce the same results.

        Show
        Leonardo Uribe added a comment - Reading the spec javadoc, it is enforced listeners should be called from encodeBegin() and encodeEnd(). We have to use an alternate way to produce the same results.
        Hide
        Leonardo Uribe added a comment -

        The latest patch (MYFACES-2544-4.patch) uses encodeBegin() and encodeEnd(), but before execute encodeChildren() or encodeEnd() it check for getResponseComplete(). If no objections, I'll commit this code soon.

        Show
        Leonardo Uribe added a comment - The latest patch ( MYFACES-2544 -4.patch) uses encodeBegin() and encodeEnd(), but before execute encodeChildren() or encodeEnd() it check for getResponseComplete(). If no objections, I'll commit this code soon.
        Hide
        Leonardo Uribe added a comment -

        Thanks to Martin Koci for provide this patch

        Show
        Leonardo Uribe added a comment - Thanks to Martin Koci for provide this patch
        Hide
        Tibor added a comment -

        I believe there is the same issue in version 1.2 of MyFaces. Could you provide a fix also for that version? Thanks.

        Show
        Tibor added a comment - I believe there is the same issue in version 1.2 of MyFaces. Could you provide a fix also for that version? Thanks.

          People

          • Assignee:
            Leonardo Uribe
            Reporter:
            Martin Kočí
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development