Forwarding to Servlet Broken w/ Parameters

[GoogleCodeExporter]

I believe there is a bug in Guice-Servlet when grabbing a RequestDispatcher
to a servlet served by Guice.

For example:

with: serve("/download").with(DownloadServlet.class); 

request.getRequestDispatcher("/download") will return a
ManagedServletPipeline and forwarding with this dispatcher functions correctly.

however request.getRequestDispatcher("/download?
code=3vif9&msisdn=46733710212&language=sv") returns an
ApplicationDispatcher and forwarding results in a 404 error.

This issue is discussed in more detail here:

http://groups.google.com/group/google-guice/browse_thread/thread/597d1cfbc201772
2

Original issue reported on code.google.com by steveke...@gmail.com on 29 May 2009 at 7:13

[GoogleCodeExporter]

Is this even legal in the servlet spec?

Note to self: check the servlet spec for RequestDispatcher.

Original comment by dha...@gmail.com on 27 Jun 2009 at 1:08

[GoogleCodeExporter]

Original comment by sberlin on 22 Feb 2011 at 1:41

• Added labels: Extension-Servlet

[GoogleCodeExporter]

It is legal according to the servlet spec. In 2.4 (which is what I happened to 
have) it says:

SRV.8.1.1

Query Strings in Request Dispatcher Paths
The ServletContext and ServletRequest methods that create RequestDispatcher
objects using path information allow the optional attachment of query string
information to the path. For example, a Developer may obtain a RequestDispatcher
by using the following code:

String path = “/raisins.jsp?orderno=5”;
RequestDispatcher rd = context.getRequestDispatcher(path);
rd.include(request, response);

Parameters specified in the query string used to create the RequestDispatcher
take precedence over other parameters of the same name passed to the included
servlet. The parameters associated with a RequestDispatcher are scoped to apply
only for the duration of the include or forward call.

Original comment by nick.lot...@gmail.com on 21 May 2011 at 4:54

[GoogleCodeExporter]

This issue is similar to 
https://code.google.com/p/google-guice/issues/detail?id=372, but is not fixed 
by it...

Simple example:

* Create a GuiceServletContextListener with
    filter("/*").through(Filter1.class);
    serve("/getfile").with(Servlet1.class);
* Filter1 implementation
    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
        request.getRequestDispatcher("/getfile?foo=bar").forward(request, response);
    }
* Servlet1 implementation
    protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
        super.doGet(req, resp);
    }

* Guice: Requesting any url will go through Filter1, but won't call doGet on 
Servlet1
* Classic web.xml: Requesting any url will go through FIlter1 and 
Servlet1.doGet()

In our real-world use-case, we currently utilize the urlrewritefilter and have 
rules like:
<rule enabled="true">
    <from>^(/extern/.*)$</from>
    <to>/getfile?file=$1</to>
</rule> 
But the servlet bound to /getfile is never called when using guice-servlet...

Original comment by arman.va...@gmail.com on 11 Apr 2014 at 12:16

[GoogleCodeExporter]

Patches with tests are gladly accepted.

Original comment by sberlin on 11 Apr 2014 at 1:00

[GoogleCodeExporter]

In principle the UriPatternType.SERVLET matcher works as specified in the 
servlet-3.0 specs 12.2.
But the current implementation does not take into consideration that according 
to 9.1.1 the request dispatcher can dispatch a path with a query string 
attached to it.

As for the matter of matching a filter/servlet, my patch will create an URI 
object out of the uri string and check against the path.

Original comment by arman.va...@gmail.com on 11 Apr 2014 at 5:39

Attachments:

• uriPatternTypePatch.diff
• UriPatterTypeTest.java

[GoogleCodeExporter]

Thanks for the patch!  I'm very wary of putting a java.net.URI in the critical 
path.  It's not very forgiving in terms of throwing exceptions on certain 
inputs, and it's a bit heavyweight in construction.

Original comment by sberlin on 11 Apr 2014 at 7:34

[GoogleCodeExporter]

I guess a cheaper implementation is just to strip out the fragment and query.
You might want to change the getPath() method name into something more 
meaningful though ;)

Original comment by arman.va...@gmail.com on 12 Apr 2014 at 12:45

Attachments:

• uriPatternTypePatch2.diff

[GoogleCodeExporter]

fixed by r269f2f69279d

Original comment by sberlin on 3 May 2014 at 4:54

• Changed state: Fixed