Forwarding to Servlet Broken w/ Parameters

[gissuebot]

From stevekeogh on May 29, 2009 03:13:57

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/597d1cfbc2017722

Original issue: http://code.google.com/p/google-guice/issues/detail?id=379

[gissuebot]

From dhanji on June 26, 2009 18:08:45

Is this even legal in the servlet spec?

Note to self: check the servlet spec for RequestDispatcher.

[gissuebot]

From sberlin on February 21, 2011 17:41:31

(No comment was entered for this change.)

Labels: Extension-Servlet

[gissuebot]

From nick.lothian on May 20, 2011 21:54:52

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.

[gissuebot]

From arman.vartan on April 11, 2014 05:16:43

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...

[gissuebot]

From sberlin on April 11, 2014 06:00:12

Patches with tests are gladly accepted.

[gissuebot]

From arman.vartan on April 11, 2014 10:39:45

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.

Attachment: gist
   uriPatternTypePatch.diff
   UriPatterTypeTest.java

[gissuebot]

From sberlin on April 11, 2014 12:34:41

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.

[gissuebot]

From arman.vartan on April 11, 2014 17:45:19

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 ;)

Attachment: gist
   uriPatternTypePatch2.diff

[gissuebot]

From sberlin on May 02, 2014 21:54:35

fixed by r269f2f69279d

Status: Fixed