MyFaces Core
  1. MyFaces Core
  2. MYFACES-2454

Adapt default error page generation to new spec

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha-2
    • Fix Version/s: 2.0.0-beta
    • Component/s: JSR-314
    • Labels:
      None

      Description

      see spec section »6.2.3 Default Error Page« for details.

      This includes the following tasks:

      • <ui:include src="javax.faces.error.xhtml" /> should include the standard error page (created by _ErrorPageWriter in MyFaces) in any facelet error page defined in web.xml
      • <error-page> entries in web.xml should take priority over MyFaces' default error page (currently you have to disable it via org.apache.myfaces.ERROR_HANDLING first)
      • UIInput should create an UpdateModelException and publish it, if an exception occurs in updateModel()
      • The error page should also include the view and the flash scope attributes
      1. myfaces-2454-error-handling-ErrorPageWriter.patch
        4 kB
        Jakob Korherr
      2. myfaces-2454-error-handling.patch
        8 kB
        Jakob Korherr
      3. myfaces-2454.patch
        126 kB
        Jakob Korherr

        Issue Links

          Activity

          Hide
          Jakob Korherr added a comment -

          I am stuck at the first task, input would be appreciated!!

          Spec section 6.2.3 explains the Default Error Page, which is generated in MyFaces via the package private class javax.faces.webapp._ErrorPageWriter. The spec says, if we have an <error-page> definition in web.xml that points to a facelet page, this facelet page must be able to include the information produced by (in case of MyFaces) _ErrorPageWriter via <ui:include src="javax.faces.error.xhtml" />.

          <ui:include> is handled by org.apache.myfaces.view.facelets.tag.ui.IncludeHandler, thus it is part of myfaces-impl and it cannot access _ErrorPageWriter.

          So there a few ways to solve this problem:

          1) Duplicate _ErrorPageWriter to myfaces-impl, which is just ugly

          2) Move _ErrorPageWriter to myfaces-impl and make it public. This would also allow custom error handlers (org.apache.myfaces.ERROR_HANDLER) to fall back to the default behaviour (there was a request for that in the myfaces-user-list recently).
          However, we would not be able to invoke the ErrorPageWriter in FacesServlet's service method any more. So we would have to invoke it either from LifecycleImpl or from ResourceHandlerImpl, if the current request is a resource request. Looking at the code in FacesServlet maybe makes things clearer:

          ...
          try
          {

          ResourceHandler resourceHandler = facesContext.getApplication().getResourceHandler();

          if (resourceHandler.isResourceRequest(facesContext))

          { resourceHandler.handleResourceRequest(facesContext); }

          else

          { _handleStandardRequest(facesContext); }

          }
          catch (Exception e)

          { handleLifecycleException(facesContext, e); }

          ...

          This solution would also allow us to generate different error pages for resource requests, because currently we always a text/html error page, also if the request was for example image/jpg.

          3) Invoke the _ErrorPageWriter in FacesServlet in either way, but if there is an <error-page> in web.xml, save the output in a request-scoped variable, which can be accessed by IncludeHandler, instead of writing it directly to the request writer.

          Input would be highly appreciated!!!!!

          Show
          Jakob Korherr added a comment - I am stuck at the first task, input would be appreciated!! Spec section 6.2.3 explains the Default Error Page, which is generated in MyFaces via the package private class javax.faces.webapp._ErrorPageWriter. The spec says, if we have an <error-page> definition in web.xml that points to a facelet page, this facelet page must be able to include the information produced by (in case of MyFaces) _ErrorPageWriter via <ui:include src="javax.faces.error.xhtml" />. <ui:include> is handled by org.apache.myfaces.view.facelets.tag.ui.IncludeHandler, thus it is part of myfaces-impl and it cannot access _ErrorPageWriter. So there a few ways to solve this problem: 1) Duplicate _ErrorPageWriter to myfaces-impl, which is just ugly 2) Move _ErrorPageWriter to myfaces-impl and make it public. This would also allow custom error handlers (org.apache.myfaces.ERROR_HANDLER) to fall back to the default behaviour (there was a request for that in the myfaces-user-list recently). However, we would not be able to invoke the ErrorPageWriter in FacesServlet's service method any more. So we would have to invoke it either from LifecycleImpl or from ResourceHandlerImpl, if the current request is a resource request. Looking at the code in FacesServlet maybe makes things clearer: ... try { ResourceHandler resourceHandler = facesContext.getApplication().getResourceHandler(); if (resourceHandler.isResourceRequest(facesContext)) { resourceHandler.handleResourceRequest(facesContext); } else { _handleStandardRequest(facesContext); } } catch (Exception e) { handleLifecycleException(facesContext, e); } ... This solution would also allow us to generate different error pages for resource requests, because currently we always a text/html error page, also if the request was for example image/jpg. 3) Invoke the _ErrorPageWriter in FacesServlet in either way, but if there is an <error-page> in web.xml, save the output in a request-scoped variable, which can be accessed by IncludeHandler, instead of writing it directly to the request writer. Input would be highly appreciated!!!!!
          Hide
          Leonardo Uribe added a comment -

          I think move _ErrorPageWriter to myfaces-impl and make it public is the best choice. Keep it simple. In this version the way how to deal with errors was standardized and this way should be preferred over any other previous hack. It could be good to keep the previous hack works, but it is not mandatory. The spec takes precedence.

          Show
          Leonardo Uribe added a comment - I think move _ErrorPageWriter to myfaces-impl and make it public is the best choice. Keep it simple. In this version the way how to deal with errors was standardized and this way should be preferred over any other previous hack. It could be good to keep the previous hack works, but it is not mandatory. The spec takes precedence.
          Hide
          Jakob Korherr added a comment -

          I came up with the following solution for the backwards compatibility:

          • org.apache.myfaces.ERROR_HANDLING will be dismissed by MyFaces 2.0, because we will always handle the Exception with the ExceptionHandler (the spec tells us to).
          • org.apache.myfaces.ERROR_HANDLER can still be used, but only in conjunction with javax.faces.webapp.PreJsf2ExceptionHandlerFactory. However, only the method handleException(FacesContext context, Exception e) has to be provided.
          Show
          Jakob Korherr added a comment - I came up with the following solution for the backwards compatibility: org.apache.myfaces.ERROR_HANDLING will be dismissed by MyFaces 2.0, because we will always handle the Exception with the ExceptionHandler (the spec tells us to). org.apache.myfaces.ERROR_HANDLER can still be used, but only in conjunction with javax.faces.webapp.PreJsf2ExceptionHandlerFactory. However, only the method handleException(FacesContext context, Exception e) has to be provided.
          Hide
          Jakob Korherr added a comment -

          Here is my patch for this issue.

          The patch introduces the class org.apache.myfaces.renderkit.ErrorPageWriter. This class handles the tasks from javax.faces.webapp._ErrorPageWriter and org.apache.myfaces.view.facelets.util.DevTools, so it reduces redundancy.

          The patch also dismisses the init parameter org.apache.myfaces.ERROR_HANDLING, because we will now always handle the Exception with the ExceptionHandler (the spec tells us to). However, if someone really wants to fully disable any error handling, he only has to provide a custom ExceptionHandler which rethrows every Exception.

          I will document all these changes in http://wiki.apache.org/myfaces/Handling_Server_Errors, after the patch was committed.

          Show
          Jakob Korherr added a comment - Here is my patch for this issue. The patch introduces the class org.apache.myfaces.renderkit.ErrorPageWriter. This class handles the tasks from javax.faces.webapp._ErrorPageWriter and org.apache.myfaces.view.facelets.util.DevTools, so it reduces redundancy. The patch also dismisses the init parameter org.apache.myfaces.ERROR_HANDLING, because we will now always handle the Exception with the ExceptionHandler (the spec tells us to). However, if someone really wants to fully disable any error handling, he only has to provide a custom ExceptionHandler which rethrows every Exception. I will document all these changes in http://wiki.apache.org/myfaces/Handling_Server_Errors , after the patch was committed.
          Hide
          Leonardo Uribe added a comment -

          I committed this patch. But after take a look at it again (and the committed solution) it seems maybe we should not dismiss the init parameter org.apache.myfaces.ERROR_HANDLING. The reason why is because "....to fully disable any error handling...." is a very common case (many messages on users and dev list are probe of it). It is more simple to set this param than write a custom ExceptionHandler.

          Show
          Leonardo Uribe added a comment - I committed this patch. But after take a look at it again (and the committed solution) it seems maybe we should not dismiss the init parameter org.apache.myfaces.ERROR_HANDLING. The reason why is because "....to fully disable any error handling...." is a very common case (many messages on users and dev list are probe of it). It is more simple to set this param than write a custom ExceptionHandler.
          Hide
          Jakob Korherr added a comment - - edited

          OK, I'll provide a patch which handles org.apache.myfaces.ERROR_HANDLING. It may simplify the development of a jsf application.

          I will change it to this behaviour: If org.apache.myfaces.ERROR_HANDLING is false, a custom ExceptionHandler will be automatically installed, which rethrows every exception. Furthermore no error page will be created.

          Show
          Jakob Korherr added a comment - - edited OK, I'll provide a patch which handles org.apache.myfaces.ERROR_HANDLING. It may simplify the development of a jsf application. I will change it to this behaviour: If org.apache.myfaces.ERROR_HANDLING is false, a custom ExceptionHandler will be automatically installed, which rethrows every exception. Furthermore no error page will be created.
          Hide
          Jakob Korherr added a comment -

          Here is my patch which reintroduces org.apache.myfaces.ERROR_HANDLING.

          If org.apache.myfaces.ERROR_HANDLING is set to false, only the (default) ExceptionHandlerFactoryImpl will be installed, regardless of any exception handler factory entries in the faces-config. So every Exception will be handled by ExceptionHandlerImpl, which will rethrow the first unhandled Exception in this case. Furthermore ErrorPageWriter will not generate an error page in this scenario.

          Show
          Jakob Korherr added a comment - Here is my patch which reintroduces org.apache.myfaces.ERROR_HANDLING. If org.apache.myfaces.ERROR_HANDLING is set to false, only the (default) ExceptionHandlerFactoryImpl will be installed, regardless of any exception handler factory entries in the faces-config. So every Exception will be handled by ExceptionHandlerImpl, which will rethrow the first unhandled Exception in this case. Furthermore ErrorPageWriter will not generate an error page in this scenario.
          Hide
          Jakob Korherr added a comment -

          Reviewing my solution for org.apache.myfaces.ERROR_HANDLING again, I think that it would be better just to disable the ErrorPageWriter when it is false rather than disabling any custom ExceptionHandler, because the developers may use a custom ExceptionHandler, but still don't want an error page to be created.

          Furthermore the default ExceptionHandler rethrows almost every exception anyway.

          Show
          Jakob Korherr added a comment - Reviewing my solution for org.apache.myfaces.ERROR_HANDLING again, I think that it would be better just to disable the ErrorPageWriter when it is false rather than disabling any custom ExceptionHandler, because the developers may use a custom ExceptionHandler, but still don't want an error page to be created. Furthermore the default ExceptionHandler rethrows almost every exception anyway.
          Hide
          Leonardo Uribe added a comment -

          Thanks to Jakob Korherr for this patch

          Show
          Leonardo Uribe added a comment - Thanks to Jakob Korherr for this patch

            People

            • Assignee:
              Leonardo Uribe
              Reporter:
              Jakob Korherr
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development