Struts 2
  1. Struts 2
  2. WW-2726

FilterDispatcher: Catch RedirectException to issue 30x code

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: Dispatch Filter
    • Labels:
      None

      Description

      When creating my own implementation of ActionMapper, I have no way to tell the FilterDispatcher to redirect to a different page. I am creating an alias in effect.

      Here's what is going on:
      1. I invoke the superclass to match an action mapping. if found, return.
      2. Match again adding "/index.action" to the servlet path. if found, return.
      3. Match again adding "index.action" (no slash) to the servlet path. if found, return.

      Aside from the obvious inefficiencies to be resolve, it should be plain what my intent is. I am executing the index action for a directory path, but there's no good way to get #3 to issue a redirect. URIs ending in /foo should become /foo/ in the browser.

      Therefore, I pull a page from the Tapestry book. You can throw a RedirectException with a path and a temporary/permanent flag to set the correct HTTP status.

        Activity

        Hide
        Paul Benedict added a comment -

        I will submit a patch for 2.1.

        Show
        Paul Benedict added a comment - I will submit a patch for 2.1.
        Hide
        Don Brown added a comment -

        I really hate to see exceptions used for control flow, but I can see your need. Is there any other way we could support this?

        Show
        Don Brown added a comment - I really hate to see exceptions used for control flow, but I can see your need. Is there any other way we could support this?
        Hide
        Paul Benedict added a comment - - edited

        I know what you mean. I found it very unnatural to use an exception when I had to with Tapestry. Thanks for the pushback, because I believe there is a better way:

        FilterDispatcher#doFilter() should refactor out its Exception handling and action handling into a protected methods for customization. Since what we really want to do is at the servlet level, it would make more sense to subclass the filter and do the HTTP stuff there. After all, it is not the responsibility of the ActionMapper to do redirects, right?

        PS: Methods in DefaultActionMapper should be more accessible to subclasses. I would mark these as protected and final: isSlashesInActionNames, getDefaultExtensions.

        Show
        Paul Benedict added a comment - - edited I know what you mean. I found it very unnatural to use an exception when I had to with Tapestry. Thanks for the pushback, because I believe there is a better way: FilterDispatcher#doFilter() should refactor out its Exception handling and action handling into a protected methods for customization. Since what we really want to do is at the servlet level, it would make more sense to subclass the filter and do the HTTP stuff there. After all, it is not the responsibility of the ActionMapper to do redirects, right? PS: Methods in DefaultActionMapper should be more accessible to subclasses. I would mark these as protected and final: isSlashesInActionNames, getDefaultExtensions.
        Hide
        Paul Benedict added a comment -

        I spent today trying alternative solutions, but I don't think I can find a better way than throwing an Exception. The fact is even with planned refactoring of FilterDispatcher to make things easier, there is no systematic way of triggering the redirect elsewhere.

        Show
        Paul Benedict added a comment - I spent today trying alternative solutions, but I don't think I can find a better way than throwing an Exception. The fact is even with planned refactoring of FilterDispatcher to make things easier, there is no systematic way of triggering the redirect elsewhere.
        Hide
        Jeromy Evans added a comment -

        I don't have an alternate solution, but rather than restrict this to a redirect couldn't it support any http status code and headers?

        If we allow redirects to creep into the ActionMapper, there's probably some 4xx responses that are also valid to return rather than creating an invocation.

        eg. if it must throw an exception, throw one with a HttpHeaders argument (borrowed from the rest plugin) to set the status code and headers.

        Show
        Jeromy Evans added a comment - I don't have an alternate solution, but rather than restrict this to a redirect couldn't it support any http status code and headers? If we allow redirects to creep into the ActionMapper, there's probably some 4xx responses that are also valid to return rather than creating an invocation. eg. if it must throw an exception, throw one with a HttpHeaders argument (borrowed from the rest plugin) to set the status code and headers.
        Hide
        Paul Benedict added a comment -

        Ah... You are right on track, Jeromy. I was going to recommend from the start a subclass that supports any HTTP Status Code. However, I didn't want to sell the idea too much without others buying in first. If the exception were to accept a collection of HttpHeaders, that would be nice too.

        Would another alternative be to extend the ActionMapper interface? Maybe HttpAwareActionMapper that has custom lifecycle methods to rid Exception throwing?

        Show
        Paul Benedict added a comment - Ah... You are right on track, Jeromy. I was going to recommend from the start a subclass that supports any HTTP Status Code. However, I didn't want to sell the idea too much without others buying in first. If the exception were to accept a collection of HttpHeaders, that would be nice too. Would another alternative be to extend the ActionMapper interface? Maybe HttpAwareActionMapper that has custom lifecycle methods to rid Exception throwing?

          People

          • Assignee:
            Unassigned
            Reporter:
            Paul Benedict
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development