GuiceFilter breaks dispatching to jsp if jasper is being used to compile the jsp

[GoogleCodeExporter]

The Jasper JSP compiler uses request.getServletPath() to name the JSP file.
When dispatched to a JSP under the filter the servlet path is either empty
or /, which leads to an error when generating a class name for the jsp file
before it can be compiled.

This is happening in 2.0-r943

Original issue reported on code.google.com by brianm%s...@gtempaccount.com on 12 May 2009 at 10:42

[GoogleCodeExporter]

probably, if dispatching to a servlet, the request and response should be 
wrapped and
made to look like the dispatch was actually to a servlet by the container.

I am seeing this under jetty 6.1.16

Original comment by brianm%s...@gtempaccount.com on 12 May 2009 at 10:43

[GoogleCodeExporter]

Yea this is a known issue--I've just not had any time to fix it up =(

Note that this is a side effect of returning the wrong path in the 
requestDispatcher for JSP forwards. It affects 
Guice Servlets using request dispatcher to forward to JSPs too, for example...

Brownie points if you willing to submit a patch!

Original comment by dha...@gmail.com on 12 May 2009 at 11:07

[GoogleCodeExporter]

Is the problem somewhere near //noinspection OverlyComplexAnonymousInnerClass in
ServletDefinition.jav?

Original comment by Leigh.Kl...@gmail.com on 28 Jul 2009 at 10:48

[GoogleCodeExporter]

Yeah this is very annoying. I have been trying to solve it for a while now.Are 
there
any work arounds that I can use in the mean time to direct the response to a 
JSP?

Original comment by landon.w...@gmail.com on 24 Aug 2009 at 6:37

[GoogleCodeExporter]

I've come up with a work around that works for Jasper.  Jasper looks for a 
request
attribute "org.apache.catalina.jsp_file" and uses that before it checks 
request.getServletPath().  I tried simply setting the attribute but it gets 
blown
away at some point during during the request.

public class JSPFixGuiceFilter extends GuiceFilter{
       @Override
       public void doFilter(ServletRequest request,
                       ServletResponse response, FilterChain filterChain)
                       throws IOException, ServletException {
                       request = new
HttpServletRequestWrapper((HttpServletRequest)request){
                               @Override
                               public Object getAttribute(String name) {
                                       if("org.apache.catalina.jsp_file".equals(name)){
                                               return super.getServletPath();
                                       }
                                       return super.getAttribute(name);
                               }        
                       };

                       super.doFilter(request, response, filterChain);

       }
}

Original comment by tobias...@gmail.com on 8 Dec 2009 at 11:01

[GoogleCodeExporter]

This fails if MyServlet is managed by Guice. If this is just a normal unmanaged 
servlet configured in web.xml, it works fine:

@Singleton
public class MyServlet extends HttpServlet {
    protected void doGet(HttpServletRequest req, HttpServletResponse res)
            throws ServletException, IOException {
        req.getRequestDispatcher("/WEB-INF/protected_page.html").forward(req, res);
    }
}

In Tomcat I receive "HTTP Status 404 - /WEB-INF/protected_page.html" with 
description 
"The requested resource (/WEB-INF/protected_page.html) is not available."

Original comment by burke.e...@gmail.com on 21 Dec 2009 at 9:56

[GoogleCodeExporter]

I'm attaching a web app that demonstrates a problem dispatching to a static 
HTML file 
under the WEB-INF directory. I tried to make it easy to compile and run, see 
the 
README.txt in the ZIP file. The embedded JARs are from trunk in Guice 
Subversion on 
12/21/2009, revision 1134

Original comment by burke.e...@gmail.com on 21 Dec 2009 at 10:42

Attachments:

• guice_bug.zip
• guice_bug.war

[GoogleCodeExporter]

Can this be considered fixed (I believe r1135 fixes it, but plz verify)?

I will mark as fixed, plz reopen if there is still a problem.

Original comment by dha...@gmail.com on 31 Dec 2009 at 12:06

• Changed state: Fixed

[GoogleCodeExporter]

I just tested this out. It does not work as intended. 
I checked out the source today from trunk, built it and used it for the test. 

I have a filter that dispatches the request to a servlet. 
RequestDispatcher reqDispatcher = 
req.getRequestDispatcher("/servletName/c2/?param1=1);
reqDispatcher.forward(request, response);

The dispatch almost works, except, when I check the value of 
request.getPathInfo(), I get "/servletName/c2/?param1=1" but should get 
"/servletName/c2/", as the request parameters should not be included...

Original comment by reynirhu...@gmail.com on 24 Jun 2010 at 3:46

[GoogleCodeExporter]

Can you submit a test case for this please? That would be the best way for us 
to verify.

Thanks!

Original comment by dha...@gmail.com on 8 Oct 2010 at 1:20

• Changed state: Accepted

[GoogleCodeExporter]

I've encountered this too, on Jetty 7.2.2 + Guice 3.0 + Stripes 1.5.3.

The original request matches a pattern ("/admin/*" in my case), and gets 
wrapped in a guice ServletDefinition$2 object. When the handling servlet does a 
forward, jetty wraps the guice request wrapper and then invokes setServletPath 
to point it to the new servlet. In my case, the forward is to 
"/WEB-INF/jsp/overview.jsp", and that string is what is passed to 
request.setServletPath(...).

The jasper JspServlet then calls request.getServletPath() to find the jsp to 
render; this is handled by ServletDefinition$2 which calls 
super.getServletPath() to get the *correct new* path, but it then calls 
UriPatternType.extract("/WEB-INF/jsp/overview.jsp") which returns the path to 
the original servlet, ie "/admin"!

Result is that the jsp cannot be found.

Original comment by simon.ki...@airnz.co.nz on 4 Apr 2011 at 2:38

[GoogleCodeExporter]

There's definitely a bug here.

Here's the simplest test case (I'm using tomcat 5.5.33):

* Use Guice ServletModule to configure two servlets, one of which just 
dispatches to the other:

    protected void configureServlets()
    {
        serve( "/dispatcher" ).with( DispatcherServlet.class );
        serve( "/dispatchee" ).with( DispatcheeServlet.class );
    }

* Dispatchee outputs text for verification, dispatcher redirects like this:

        protected void doGet( HttpServletRequest req, HttpServletResponse resp )
            throws ServletException, IOException
        {
            this.getServletContext().getRequestDispatcher( "/dispatchee" )
                .forward( req, resp );
        }

* Invoking /dispatcher yields a blank page with a 404 status that is 
incorrectly served up by Tomcat's default servlet.

* When configuring via traditional web.xml instead of guice, everything works 
as expected.

* I did some debugging and it looks like Tomcat's internal pattern matching 
inside its RequestDispatcher implementation isn't even aware of the servlet 
mapping that Guice configured.

Original comment by ori.schw...@gmail.com on 5 Apr 2011 at 2:42

Attachments:

• GuiceRequestDispatcherBug.java

[GoogleCodeExporter]

re#12: I think that's a different issue from the one discussed in the other 
comments on this bugreport.

Tomcat definitely isn't aware of the mappings define via serve(x).with(y). The 
whole design is that when the original request comes in, the servlet container 
will not find a matching servlet (because there is no matching entry in 
web.xml), so will select the "default servlet" as the target. However it will 
run the Guice filter first, as that is mapped to "/*". And the guice filter 
then arranges for the request to be sent to the configured servlet instead of 
passing it to the (default) servlet that the web container chose.

I would have thought that forwarding from one Guice-configured servlet to 
another would be supported (eg by creating a custom RequestDispatcher), but it 
isn't really necessary for most users.

Forwarding from a guice-configured servlet to the container's JSP or 
static-resource servlet *is* critical, and this is the issue discussed in this 
bugreport. Note that comment#11 is about using a wildcard-suffix (eg 
"/admin/*"). The existing code works fine with wildcard-prefixes (eg "*.do"), 
because UriPatternType.extract returns null in this case, causing 
ServletDefinition$2.getServletPath() to return the (correct) path from the 
parent class.

Original comment by simon.ki...@airnz.co.nz on 5 Apr 2011 at 3:01

[GoogleCodeExporter]

Thanks Simon (#13), that explains it. You are right, this is not the same as 
the JSP issue discussed in this report.

Should I open a separate issue for the behavior I reported? It seems like an 
important fix to me. Lots of apps use ServletContext#getRequestDispatcher to 
forward requests internally. 

Original comment by ori.schw...@gmail.com on 5 Apr 2011 at 4:00

[GoogleCodeExporter]

Yes, please open a separate issue.  If someone is able to also attach a patch 
with a testcase & fix that:
 a) Fails before applying the fix
and
 b) Succeeds after applying the fix

That will help very much in fixing this.

Original comment by sberlin on 5 Apr 2011 at 4:46

[GoogleCodeExporter]

I've opened up issue 621 for the above comment #12 (forward method in 
ServeltContext#getRequestDispatcher). 

Original comment by ori.schw...@gmail.com on 5 Apr 2011 at 4:57

[GoogleCodeExporter]

re#11: serveRegex("/admin/.*").with(YourAdminServlet.class) should work. At 
least that solved the same problem I had with a servlet mapped to a 
wildcard-suffix path.

Original comment by valo...@gmail.com on 3 Jun 2011 at 8:59

[GoogleCodeExporter]

Attached a possible solution with its test. 

I'm not sure if it is the best possible one as I do not know if a custom 
HttpServletRequest that tries to mimic servlet container behavior of computing 
stuff like the request paths is needed by the guice-servlet module internals.

Original comment by valo...@gmail.com on 5 Jun 2011 at 7:54

Attachments:

• ServletDefinition.java
• ServletDefinitionPathsTest.java

[GoogleCodeExporter]

Would you be able to attach a patch instead of the new file as a whole?  
Thanks!  (Patches are much easier to review.)

Original comment by sberlin on 5 Jun 2011 at 7:59

[GoogleCodeExporter]

If you can help me on how to do that! Never had to do it before!

Original comment by valo...@gmail.com on 5 Jun 2011 at 8:09

[GoogleCodeExporter]

If you're using Eclipse there should be a Team -> Create Patch option on the 
file or project (right-click to find it).  If you're using SVN directly, use 
'svn diff > patch.txt'.  You have to have the code checked out from SVN (either 
directly, through Eclipse, or through whatever IDE you're using).  It's harder 
if you just are modifying the src jars (but possible.. you need a diff tool & 
the original & modified files).

See: http://code.google.com/p/google-guice/source/checkout for how to get the 
source from SVN.

Original comment by sberlin on 5 Jun 2011 at 8:16

[GoogleCodeExporter]

Ok, attached you can find a patch file!

Original comment by valo...@gmail.com on 5 Jun 2011 at 8:41

Attachments:

• 372patch.txt

[GoogleCodeExporter]

It's been a while! Has this been fixed? Is there any problem (or any other 
problem) regarding the patch provided? I would be glad to investigate more if 
that is the case!

Original comment by valo...@gmail.com on 11 Sep 2011 at 5:38

[GoogleCodeExporter]

Is it possible to write a test-case also?  I prefer not to apply patches 
without tests attached, especially when I don't know the code as well.  The 
test will make sure the problem doesn't regress, and also highlight what was 
wrong in the first place.

Original comment by sberlin on 16 Oct 2011 at 4:33

[GoogleCodeExporter]

Isn't the ServletDefinitionPathsTest enough? 

This is what I added:
pathInfoWithServletStyleMatching("/path/some/path/of.jsp", "/path", "/thing/*", 
"/some/path/of.jsp", "/some/path/of.jsp");

It should fail if you try to run it without the ServletDefinition changes

Original comment by valo...@gmail.com on 16 Oct 2011 at 8:08

[GoogleCodeExporter]

After 3.5 years this patch has neither found its way into any minor v3 release 
nor any work as been done to fix this issue in the v4 betas.

As this is not a minor bug and has been addressed in several other tickets, 
please revise the status of this ticket.

Original comment by arman.va...@gmail.com on 8 Apr 2014 at 2:50

[GoogleCodeExporter]

It looks like the servlet module is not a high prio one :(

Original comment by valo...@gmail.com on 8 Apr 2014 at 2:56

[GoogleCodeExporter]

Sorry, this fell off our radar.  (3+ years without anyone commenting on it 
helps do that.)  Is the patch from #22 still valid against the latest servlet 
code?

Original comment by sberlin on 8 Apr 2014 at 2:59

[GoogleCodeExporter]

Well, last comment was mine, explaining where to find the test requested. The 
fact that you requested a test case means that you did not review the patch 
that you requested me :)

Anyway, I can not confirm now that the patch will work against the latest code 
at the trunk (as mentioned by the development team, there are quite some 
changes to the guice internals).

Original comment by valo...@gmail.com on 8 Apr 2014 at 3:08

[GoogleCodeExporter]

I remember looking at it, but missed the fact that a test was added (given that 
the test was just a single line) -- I was expecting a whole method.  I'll try 
to find some time to apply it & confirm it doesn't expose other problems.

Original comment by sberlin on 8 Apr 2014 at 3:09

[GoogleCodeExporter]

No problem. Happy to have helped :)

Original comment by valo...@gmail.com on 8 Apr 2014 at 3:17

[GoogleCodeExporter]

Have though in mind what I've said to my previous response:

"I'm not sure if it is the best possible one as I do not know if a custom 
HttpServletRequest that tries to mimic servlet container behavior of computing 
stuff like the request paths is needed by the guice-servlet module internals"

I really do not know of a better way, but in order to make sure that the 
guice-servlet does not break the servlet specifications is to write tests 
against the servlet specifications :/

Original comment by valo...@gmail.com on 8 Apr 2014 at 3:22

[GoogleCodeExporter]

Wow, that was fast. Thanks guys :)

Original comment by arman.va...@gmail.com on 8 Apr 2014 at 3:22

[GoogleCodeExporter]

I have a few questions about the patch:
  1) Should pathInfo.contains(servletPath) instead be pathInfo.startsWith(servletPath)?  That seems safer and it passes the test, but I don't know the intent.
  2) The comment says (paraphrased) "If pathInfo doesn't contain servletPath ... then return null".  But the code doesn't do that.  The code returns pathInfo as-is (with the contextPath stripped) if it doesn't contain the servlet path.  Is that intended?

Original comment by sberlin on 10 Apr 2014 at 1:31

[GoogleCodeExporter]

I think that you are right. I can not remember my intention on the code I wrote 
3y ago :)

I'll try to have a look at it the following days

Original comment by valo...@gmail.com on 10 Apr 2014 at 2:50

[GoogleCodeExporter]

Can you confirm if putting 
 else { // !pathInfo.startsWith(servletPath)
   pathInfo = null;
 }

works for you too?  Based on the javadoc for getPathInfo, it seems incorrect to 
have pathInfo be the same as the servletPath.

Original comment by sberlin on 10 Apr 2014 at 8:28

[GoogleCodeExporter]

Fixed by r=c35ebc2ce88f & r=f1abba38c7f5.

Original comment by sberlin on 10 Apr 2014 at 11:28

• Changed state: Fixed