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/
171 stars 73 forks source link

Fix issues when a single request is forwarded to multiple ActionBeans. #62

Closed shaneargo closed 7 years ago

shaneargo commented 7 years ago

I develop plugins for a product which provides a portal solution. In this software a plugin is basically a standalone java webapp that runs in the Tomcat container alongside the core product. Stripes works well in most scenarios except one.

The product has portlets which are basically boxes on the page that load the contents of other pages. Tomcat forwards control to each module's URL to render each portlets the content into the response. I.e. the DispatcherServelet is called multiple times to fulfill a single request

It appears from the source that this is a scenario that Stripes attempts to handle. When the DispatcherServlet encounters an Object already in the actionBean request parameter, it casts the object to an ActionBean and pushes it onto a stack. It then restores it back again after processing. This works well, except when the ActionBean that is already in the request was loaded by an inaccessible class loader.

When Stripes attempts to cast the Object already in the actionBean request parameter to an ActionBean it can't because it doesn't have the class data. In this instance, a ClassCastException is thrown.

There is no need to cast this to an ActionBean at all. As described above, the Object in the request parameter is simply pushed onto a stack and restored back after. It is never actually used as an ActionBean during its time on the stack. The cast to ActionBean is unnecessary work even ignoring this issue.

There is one other issue with the scenario I have described. Specifically the AnnotatedClassActionResolver assumes that the UrlBinding of an ActionBean is going to be unique, but this is not necessarily the case. Stripes may be running in multiple separate webapp contexts. It is the combination of context path and UrlBinding that is unique. This pull request also addresses this.

Combining everything into a single webapp is not an option, as we do not have control over what clients install. Other developers may also choose to use Stripes.

Please let me know if there is any other information or clarification I can offer.

rgrashel commented 7 years ago

Hi Shane, did this pass the unit test suite in master? Also, are there any unit tests you can add for this particular behavior change? BTW, thanks for this pull request. Awesome!

shaneargo commented 7 years ago

Hi Rick,

Sorry, I should have mentioned that. All existing unit tests passed.

Adding new unit tests is very difficult. In order to produce the ClassCastException, the test would need to establish multiple class loaders. These would probably have to be custom class loaders too as the root class loader would still have access to the test ActionBean.

Plus none of the existing test fixtures support firing the Dispatcher multiple times for a single request. I'm pretty sure it would require some pretty major hacking of this existing infrastructure.

I'd be happy to take feedback on this if you think there is a simple way to deal with it.

Cheers, Shane.

shaneargo commented 7 years ago

I'm closing this as I've submitted pull request #63 which resolves the issues in a better way.