search

StripesFramework / stripes

Stripes is a Java framework with the goal of making Servlet/JSP based web development in Java as easy, intuitive and straight-forward as it should be. It's stripey and it doesn't suck.
http://www.stripesframework.org/
172 stars 72 forks source link

Fix Tomcat crossContext #63

Open shaneargo opened 8 years ago

shaneargo commented 8 years ago

This pull request replaced pull request #62. After discussions with @rgrashel, I believe this is a better resolution to the issue.

This resolves issues related to the use of Tomcat's crossContext configuration item. When this config item is set to true a single request may include/forward control across servlets from different contexts. Which results in two different issues.

The first is that the final actionBean in the stack is left in the actionBean request attribute at the end, so when Stripes (running in a second context) attempts to service a request there is already a bean in this attribute. This is resolved by clearing the request attribute when the stack is empty. This make sense as the idea behind the restoreActionBean method is to remove the current action bean and restore the existing one. This change means that it clears the current bean (as expected) even when there is nothing left in the stack to put back in its place.

The second issue is that URL bindings may not be unique across contexts. This is a problem when stripes stores the actionBean for a URL in the session. This is resolved by prepending the context path to the request attribute name.

As with the last pull request, it is difficult to write tests for this as none of the mock round trip infrastructure was written with crossContext in mind. I could write some tests that check that the actionBean request attribute has been unset after servicing a request, but I feel this would be testing implementation, instead of behaviour. Please let me know your feelings on this.

All existing tests pass.

Please let me know if there is anything you'd like me to change or clarify.

Cheers, Shane.

rgrashel commented 8 years ago

Hi Shane,

I actually did some digging in old mailing list posts regarding cross-context and the actionbean stack. As you originally stated, while this code probably deals with the crossContext issue better than the previous pull request, I think we really need to look more deeply at the consequences of this. After doing some research, the actionbean stack was very intentionally made to behave the way it does. I also believe that leaving the actionbean in the request was very intentional. Likewise, UrlBinding is rock-solid right now and I am very hesitant to change this behavior right before a release.

Based on what I read in some old mailing list issues and looking through old JIRA tickets, we could definitely see a problem if a "parent" JSP page does a jsp:include of a stripes action bean... and then the "parent"... tries to access the actionbean created by the child. In that situation, it would appear that the actionbean would be null. Which we don't want. So I think deleting the actionbean from the request in the DispatcherServlet is probably not correct.

I also need to look closer at your proposal of unconditionally prepending the context to the UrlBinding -- after the Dispatcher does all of its work. Again, I am just unsure of the side effects or possible bugs. We're basically saying, a UrlBinding will never exist without a contextName as a prefix. There is a lot of code that people have already written which depends on the behavior of UrlBinding, so it really makes me uneasy trying to change that in the core. It's just scary to do this right before release.

I'm actually thinking a less invasive solution that meets all of your needs could be the following:

1) Instead of changing DispatcherServlet to do the hardcoded prefixing, write your own ActionResolver that extends NameBasedActionResolver. Override the getUrlBinding() method so that it always prepends the contextName to the UrlBinding. Then, put that ActionResolver in your web.xml as a custom action resolver -- using the ActionResolver.Class init-param.

2) If you want to remove the actionbean from the request after it completes, then write your own Interceptor which triggers on LifecycleStage.RequestComplete. In your interceptor, delete the actionbean stack from the request -- and delete the actionbean itself if you want. That will result in DispaterServlet.restoreActionBean() having no effect. But I still think you may have issues if you use jsp:include ... but I'm not sure.

So basically, I think a custom action resolver and a very small interceptor gives you the custom behavior for your portlet/cross-context situation, and it uses the power of Stripes pluggable componentry to implement that behavior without disrupting the core code at all.

shaneargo commented 8 years ago

Hi Rick,

I understand your desire not to break things.

For documentation sake, this change was to prevent other peoples apps, whose source I do not control, which are running in the same container, from breaking mine. Unless each discrete developer implements the changes that you've suggested then it will break when running those apps with crossContext=true.

Not supporting crossContext=true very well might be a limitation you are willing to accept, and I'd understand that decision.

Cheers, Shane.