Tricky issue with HttpServletRequest injection

[Brian Pontarelli]

I've got a interesting situation that I can solve using a variety of
methods, all of which are somewhat annoying and I'm wondering if there
is a better solution that anyone on the list has. Here's the issue
I've got:

First a bit of background. The project is an MVC and it uses Guice to
create a Workflow. Each step of the Workflow does part of the work for
the MVC, until finally an MVC action is invoked and then a result is
generated and sent back to the browser. This Workflow is constructed
in a JEE Filter and then invoked, so, all of the MVC objects are
created at the start of each request and injected before being called.

Okay, now the issue:

I'm using the standard ThreadLocal to store the request and allow it
to be injected into any class using a Provider. The class that is
injected might want to wrap the request and change something, such as
the request URI. They do this by wrapping the request and setting it
back into the ThreadLocal. However, since Guice injects my entire
application up front, everything has already been injected with the
original HttpServletRequest upfront.

Some failed attempts at solving this:

First, I figured I would create a proxy to the HttpServletRequest and
inject that rather than the request itself. Whenever a method is
invoked on the proxy it would grab the real request from the
ThreadLocal and use that one to proxy to. If someone wrapped the proxy
and set it back into the ThreadLocal, that would be grabbed by the
next class in the workflow. This causes some nasty infinite recursions
in some edge cases, specifically if the proxy hasn't been
"initialized" and gets wrapped and set back into the ThreadLocal. Bad
stuff happens. I have a fix for this in the ThreadLocal that looks at
the request being set and if it is a HttpServletRequestWrapper and the
request is is wrapping is a proxy it removes the proxy and re-wraps
everything. This is somewhat nasty though.

Second, I figured I would NOT make the request injectable and instead
inject the Provider. The Provider would also have a setter method that
would allow for wrapping to take place. This works and is probably the
best solution because it also provides a good wrapping API. However,
it is a little rough and introduces a wide spread dependency on the
provider.

Next, I could just remove the proxy, make the request NOT injectable
and just have everyone get it from the ThreadLocal. This also
introduces a dependency and to a static method to boot.

Lastly, I change everything to use the JEE Filter methodology and pass
the request around everywhere via method calls. This isn't too bad,
but still somewhat annoying. Plus, the request still NOT injectable.
If you aren't passed the request, you can't ever get it.

None of these are great solutions and I've been brainstorming for a
while trying to think of something better. Anyone have ideas or
solutions they have implemented to solve this?

Thanks,
-bp

[Dhanji R. Prasanna]

Sounds like you're trying to move the work of Filters into the MVC app. What's wrong with Filters ;)

Can you not simply return a request from each workflow component and pass that to the next:

public void dispatchWorkflow() {   //framework method
    HttpServletRequest request = getOriginalRequest();
    request = step1.process(request);
    request = step2.process(request);
    ...
}

Then you don't need to mess with ugly ThreadLocals... This is roughly how warp-servlet decorates the request across registered filters/servlets.

Dhanji.

[Brian Pontarelli]

This is one solution. However, once you "pass" the request around you then have to pass it everywhere. No more injection of the request without providing access to the "latest" wrapper, which is the root of my problem. From what I've seen in the warp code, it still suffers from this request injecting issue. It uses a ThreadLocal to provide injection same as guice-servlet does. If you inject the request into an object that is called at some point later and before it is called someone wraps the request, the object will still be using the older version.

I've been thinking of a new method like this: 

- The ThreadLocal is package protected and only setup in the initial Filter

- It always contains a sub-class of HttpServletRequestWrapper

- Anyone that wants to wrap the request first calls the getRequest method on the wrapper they were injected with, second they wrap that request and last they sent the new wrapper into the wrapper they were injected with. Like this:

public class Foo {

  private HttpServletRequest request;

  @Inject

  public Foo(HttpServletRequest request) {

    this.request = request;

  }

  public void wrapIt() {

    HttpServletRequestWrapper wrapper = (HttpServletRequestWrapper) request;

    HtttpServletRequest local = wrapper.getRequest();

    wrapper.setRequest(new CoolWrapper(local));

  }

}

This would be the "pattern" for wrapping and would be enforced because the ThreadLocal isn't accessible. This works, but is a little bit of overhead. Not bad and doesn't introduce dependencies. Thoughts?    

-bp

[Logan Johnson]

I like Dhanji's solution the best, but it'd be nice to find a good pattern for this that lets pipeline stages leverage guice a little more, too.

Instead of injecting all of the stages into your workflow, can you inject a Provider for each stage?

That would change your workflow from:

void execute() {
   stage1.execute();
   stage2.execute();
   ...
}

to:

void execute() {
   stage1.get().execute();
   stage2.get().execute();
}

Each stage will be instantiated just-in-time, so its dependencies won't be injected until the previous stage is already executed.  You can bind a ThreadLocal-based Provider for the request, and the request itself will still be injectable-- each stage will always get the latest request.

If you want a stage to modify the request for future stages, provide another interface that allows it to do so, and tie its implementation to the same ThreadLocal that the request Provider uses:

class CoolStage {
   @Inject HttpServletRequest request;
   @Inject FutureStages futureStages;

   void execute() {
      futureStages.setRequest(new CoolWrapper(request));

[Logan Johnson]

Actually, if you can do this you don't even need a ThreadLocal. Just stuff the request into a request-scoped holder, and let the Scope handle the details. 

[Brian Pontarelli]

Yeah, I was thinking along the same lines, but not for the workflow steps or anything else in the app, but the request itself:

requestStore.get().getRequestURI();

requestStore.set(new CoolWrapper(requestStore.get()));

The injection would be:

@Inject RequestStore requestStore;

Still, introduces a dependency and it is getting to the same end as the wrapper solution I'm trying.

-bp

[Brian Pontarelli]

You still need a ThreadLocal for the Scope to access the request, right?

[Logan Johnson]

I had assumed you're using GuiceFilter and Guice's ServletScopes.REQUEST, in which case the ThreadLocal work is already done for you.  You don't have to touch it.  If not, then yeah, you'll need to reimplement that.

[Logan Johnson]

Yeah, but you're going to have to introduce some kind of dependency, unless you go with Dhanji's approach.  The RequestStore approach is relatively minimal, and not bad.  The stage-provider approach is a little more minimal (you can inject the request directly), and also I think not bad.

Honestly, I think this is a pretty natural dependency anyway.  The stage needs the request, and it needs to pass a decorated request to the rest of the workflow-- otherwise by definition it can't do its job.  Introduce it in some form, and don't feel bad about it.

On Sun, Jul 6, 2008 at 1:40 PM, Brian Pontarelli <br...@pontarelli.com> wrote:

[Brian Pontarelli]

I've got a working solution with no dependencies. It merely uses the HttpServletRequestWrapper's getRequest and setRequest method. However, I'm still kicking things around and trying to figure out which one I like the best.

-bp