Open GoogleCodeExporter opened 9 years ago
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
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
Is the problem somewhere near //noinspection OverlyComplexAnonymousInnerClass in
ServletDefinition.jav?
Original comment by Leigh.Kl...@gmail.com
on 28 Jul 2009 at 10:48
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
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
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
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:
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
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
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
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
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:
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
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
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
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
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
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:
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
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
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
Ok, attached you can find a patch file!
Original comment by valo...@gmail.com
on 5 Jun 2011 at 8:41
Attachments:
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
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
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
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
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
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
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
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
No problem. Happy to have helped :)
Original comment by valo...@gmail.com
on 8 Apr 2014 at 3:17
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
Wow, that was fast. Thanks guys :)
Original comment by arman.va...@gmail.com
on 8 Apr 2014 at 3:22
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
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
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
Fixed by r=c35ebc2ce88f & r=f1abba38c7f5.
Original comment by sberlin
on 10 Apr 2014 at 11:28
Original issue reported on code.google.com by
brianm%s...@gtempaccount.com
on 12 May 2009 at 10:42