FilterDefinition should wrap the servlet request like ServletDefinition

[gissuebot]

From James.Moger on May 22, 2014 23:50:06

Perhaps that design suggestion is incorrect.

Here is the scenario:

filter("/dir/).through(MyFilter.class); serve("/dir/).with(MyServlet.class);

MyFilter.doFilter(request, response) {   request.getServletPath() == "/dir/com/google/whatever.java"   request.getPathInfo() == null }

MyServlet.doGet(request, response) {   request.getServletPath() == "/dir"   request.getPathInfo() == "/com/google/whatever.java" }

The filter does not get the same wrapped request that the servlet gets which makes it difficult for me to correctly process request info.  Can the filter also receive a Request that is wrapped correctly like the servlet appears to receive?

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

[gissuebot]

From James.Moger on May 29, 2014 04:55:09

I'd love to be able to use Guice, but this limitation is a killer.  I use filters frequently for security control and I can't do anything useful with a filter instantiated through Guice because it doesn't know it's url mount point.

The fix looks pretty straight-forward since it would mostly involve porting the working code from the ServletDefinition to the FilterDefinition.  I have made an attempt at this, but I couldn't figure out how to setup Eclipse for hacking on guice-servlet.

[gissuebot]

From dhanji on June 11, 2014 18:38:43

Can you not check the request.getURI() for the same details?

Or do you need it to be done per servlet?

[gissuebot]

From James.Moger on June 12, 2014 05:57:45

I tried that... but (using my above example) the result is:

Request URI: /context/dir/com/google/whatever.java

I have probably 10 filters each with different paths.  Properly identifying pathinfo is a critical step.  I could make assumptions and hack on the returned urls, but that is not a good solution.

guice-servlet has the correct servlet path info internally, but the wrapped request isn't constructed correctly for filters. The wrapped request is correct (AFAIK) for servlets 1.

I think I now have a working dev environment. I can attempt a patch (bringing some of the ServletDefinition design to the FilterDefinition), if that would be welcome.

[gissuebot]

From sberlin on June 12, 2014 06:52:13

Yup, patches with regression/unit tests are very welcome.

[gissuebot]

From James.Moger on June 12, 2014 08:52:52

I've attached a patch which wraps the servlet request before filter execution using code hijacked from ServletDefinition.  I'm not sure if this is 100% correct (i.e. REQUEST_DISPATCHER_REQUEST attribute) but this change provides my filters with the correct path info.

This change required a tweak to a unit test to retrieve the inner (I hesitate to say original) request for identity comparison.

Unfortunately, this patch also includes a regression (doh!) in:

    FilterDispatchIntegrationTest.testDispatchFilterPipelineWithRegexMatching

I haven't figured out why this is the case yet.  When debugging the test, successful test conditions are met - but it fails at what seems to be an unusual place - no doubt due to EasyMock proxy generation which I am unfamiliar with.  There is a subtle detail here which is escaping me.

Attachment: gist    _fix_issue_807.patch_

[zlosch]

With Guice 4.0, a wrapped request is correctly passed to other filters in the chain and the servlet. But still, getPathInfo() and getServletPath() return different values when called from the filter or servlet.

My understanding of the servlet spec is that the servlet mapping determines the values, and that they should be the same in the filters.

Servlet mapping:

filter("/dir/*").through(ObserverFilter.class);
serve("/dir/*").with(ObserverServlet.class);

Request: /dir/com/google/whatever.java

Guice 3.0 (injected request):

filter getPathInfo(): /dir/com/google/whatever.java
filter getServletPath(): 
servlet getPathInfo(): /dir/com/google/whatever.java
servlet getServletPath(): 

Guice 3.0 (request passed to doFilter/service methods):

filter getPathInfo(): /dir/com/google/whatever.java
filter getServletPath(): 
servlet getPathInfo(): /com/google/whatever.java
servlet getServletPath(): /dir

Guice 4.0:

filter getPathInfo(): /dir/com/google/whatever.java
filter getServletPath(): 
servlet getPathInfo(): /com/google/whatever.java
servlet getServletPath(): /dir

Expected:

filter getPathInfo(): /com/google/whatever.java
filter getServletPath(): /dir
servlet getPathInfo(): /com/google/whatever.java
servlet getServletPath(): /dir

[gitblit]

@sameb Would it be possible to review this issue and the proposed patch?

Since this was reported 2 years ago I've been maintaining a fork of guice-servlet with my patch applied. My repo predates guice hosted on GitHub so it's not a traditional GH fork. I'd love to drop my fork and have a proper fix upstream.

https://github.com/gitblit/google-guice/commit/60e1acc364902bdd889a402c5de211b5fd192d12

[sameb]

could you open a pull request with your proposed patch + testcases? thanks!