MyFaces Core
  1. MyFaces Core
  2. MYFACES-2734

Character encoding not set correctly before Restore View

    Details

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

      Description

      In my examples I have a phase listener that outputs all request parameters. I accidentially did this before restore view and got some strange behaviour. With MyFaces 2.0, reading the request parameters before the restore view phase kills german umlauts. This happens because the character encoding is calculated and set in the request at the beginning of restore view but after the before phase listeners are executed.

      As this is not happening with Mojarra, I set a breakpoint in ServletRequest.setCharacterEncoding and saw that they are setting this somewhere at the beginning of the lifecycle.

      I quickly checked the spec but the only thing I found regarding the character encoding was at the beginning of restore view (which is done correctly in MyFaces). But I wonder if it should not be set earlier as, like in my case, an earlier access to the request parameters kills umlauts. This might also be necessary for extensions doing something with request parameters in a before restore view listener.

      1. MYFACES-2734.patch
        2 kB
        Michael Kurz
      2. MYFACES-2734-test-app.zip
        8 kB
        Michael Kurz
      3. MYFACES-2734-doPrePhaseActions.patch
        11 kB
        Jakob Korherr

        Activity

        Hide
        Michael Kurz added a comment -

        This patch sets the character encoding in the FacesServlet. I don't know if there are any reasons why this should not be done, but it works for me. Additionaly, I am not sure if this has to be done for resource requests or if it is sufficient to just do it before starting the lifecycle.

        Show
        Michael Kurz added a comment - This patch sets the character encoding in the FacesServlet. I don't know if there are any reasons why this should not be done, but it works for me. Additionaly, I am not sure if this has to be done for resource requests or if it is sufficient to just do it before starting the lifecycle.
        Hide
        Michael Kurz added a comment -

        Added a small application to test this behaviour. With the patch everything works fine, but with a current version of MyFaces all umlauts will be killed.

        Show
        Michael Kurz added a comment - Added a small application to test this behaviour. With the patch everything works fine, but with a current version of MyFaces all umlauts will be killed.
        Hide
        Jakob Korherr added a comment -

        I did some research about this issue and I found out that, as you said, the restore view phase is the only point where this is mentioned in the spec. However I agree that this should happen earlier (before the before-PhaseListeners of restore view are executed).

        Furthermore I think it is sufficient to do a viewHandler.initView(facesContext); somewhere in LifecycleImpl.execute() before the phases are executed, so we do not need the code on FacesServlet.

        The problem is that the spec dictates that this has to be the very first thing that happens in the restore view phase, but (implicitly) after the before-PhaseListeners where executed:

        Spec section 2.2.1: "The JSF implementation must perform the following tasks during the Restore View phase of the request processing lifecycle: Call initView() on the ViewHandler. This will set the character encoding properly for this request. ....."

        Spec section 7.5.1: "The initView() method must be called as the first method in the implementation of the Restore View Phase of the request processing lifecycle, immediately after checking for the existence of the FacesContext parameter...."

        I also tried to move the call to initView() from RestoreViewExecutor to LifecycleImpl and the demo worked fine, however 4 tests in RestoreViewExecutorTest failed, because they expected the call to viewHandler.initView() in the RestoreViewExecutor.

        In addition I also did some black box tests of Mojarra and you are right, they don't do that on Restore View, but somewhere earlier. I guess this is yet another undocumented spec-change. I recommend that you create a spec issue for this and when we get response from the EG, we can move the code to LifecycleImpl to solve this issue.

        Show
        Jakob Korherr added a comment - I did some research about this issue and I found out that, as you said, the restore view phase is the only point where this is mentioned in the spec. However I agree that this should happen earlier (before the before-PhaseListeners of restore view are executed). Furthermore I think it is sufficient to do a viewHandler.initView(facesContext); somewhere in LifecycleImpl.execute() before the phases are executed, so we do not need the code on FacesServlet. The problem is that the spec dictates that this has to be the very first thing that happens in the restore view phase, but (implicitly) after the before-PhaseListeners where executed: Spec section 2.2.1: "The JSF implementation must perform the following tasks during the Restore View phase of the request processing lifecycle: Call initView() on the ViewHandler. This will set the character encoding properly for this request. ....." Spec section 7.5.1: "The initView() method must be called as the first method in the implementation of the Restore View Phase of the request processing lifecycle, immediately after checking for the existence of the FacesContext parameter...." I also tried to move the call to initView() from RestoreViewExecutor to LifecycleImpl and the demo worked fine, however 4 tests in RestoreViewExecutorTest failed, because they expected the call to viewHandler.initView() in the RestoreViewExecutor. In addition I also did some black box tests of Mojarra and you are right, they don't do that on Restore View, but somewhere earlier. I guess this is yet another undocumented spec-change. I recommend that you create a spec issue for this and when we get response from the EG, we can move the code to LifecycleImpl to solve this issue.
        Hide
        Leonardo Uribe added a comment -

        I think we should do the following:

        1. Add a new method org.apache.myfaces.lifecycle.PhaseExecutor called doPrePhaseActions or something like that, that will give the chance to RestoreViewExecutor to call ViewHandler.initView.
        2. Change PhaseExecutor from interface to abstract class.

        Show
        Leonardo Uribe added a comment - I think we should do the following: 1. Add a new method org.apache.myfaces.lifecycle.PhaseExecutor called doPrePhaseActions or something like that, that will give the chance to RestoreViewExecutor to call ViewHandler.initView. 2. Change PhaseExecutor from interface to abstract class.
        Hide
        Michael Kurz added a comment -

        Leonardo's suggestion seems to be a good idea.

        Nevertheless, I think that this is a spec issue, as Jakob also pointed out. As I consider phase listeners as belonging to the lifecycle and not the phases themselves, the beginning of the phase (as mentioned in the spec) is after the invocation of the before phase listeners.

        I submitted this spec issue:

        https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=816

        Show
        Michael Kurz added a comment - Leonardo's suggestion seems to be a good idea. Nevertheless, I think that this is a spec issue, as Jakob also pointed out. As I consider phase listeners as belonging to the lifecycle and not the phases themselves, the beginning of the phase (as mentioned in the spec) is after the invocation of the before phase listeners. I submitted this spec issue: https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=816
        Hide
        Jakob Korherr added a comment -

        That's a really good idea Leonardo!

        However this might be tested by the TCK, so I don't know if we should change it now or wait for the EG to respond. What do you think?

        Show
        Jakob Korherr added a comment - That's a really good idea Leonardo! However this might be tested by the TCK, so I don't know if we should change it now or wait for the EG to respond. What do you think?
        Hide
        Michael Kurz added a comment -

        Good question, but as Mojarra is doing it differently anyway, we might be on the save side.

        Show
        Michael Kurz added a comment - Good question, but as Mojarra is doing it differently anyway, we might be on the save side.
        Hide
        Jakob Korherr added a comment -

        Yes, right. I will implement the changes proposed by Leonardo and provide a patch for it, so we can talk about it before committing!

        Show
        Jakob Korherr added a comment - Yes, right. I will implement the changes proposed by Leonardo and provide a patch for it, so we can talk about it before committing!
        Hide
        Jakob Korherr added a comment -

        The attached patch (MYFACES-2734-doPrePhaseActions.patch) does exactly what Leonardo proposed. Furthermore it cleans up some javadoc and adds the call to doPrePhaseActions() in RestoreViewExecutorTest.

        If no objections I'll commit this patch soon!

        Show
        Jakob Korherr added a comment - The attached patch ( MYFACES-2734 -doPrePhaseActions.patch) does exactly what Leonardo proposed. Furthermore it cleans up some javadoc and adds the call to doPrePhaseActions() in RestoreViewExecutorTest. If no objections I'll commit this patch soon!

          People

          • Assignee:
            Jakob Korherr
            Reporter:
            Michael Kurz
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development