Bugzilla – Full Text Bug Listing
| Summary: | async dispatch ignores query parameters in dispatch string | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [RT] Jetty | Reporter: | David Jencks <david.a.jencks> | ||||||||||
| Component: | server | Assignee: | Thomas Becker <tbecker> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | gregw, jetty-inbox | ||||||||||
| Version: | 8.0.0 | ||||||||||||
| Target Milestone: | 7.2.x | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Hmmm I wonder what happens if you have the sequence as follows: dispatch to /foo?a=1 startAsync async.dispatch to /bar?b=2 startAsync async.dispatch to /bob?c=3 On the last dispatch, are the paramaters a=1&b=2&c=3 or a=1&c=3 ? I've committed a fix to jetty-7. It moves the query/parameter merging to a method on the request class that is called both from Dispatcher and the async dispatch. This needs to be tested, the questions about multiple async dispatches resolved, and then merged with jetty-8 Thomas, can you create a test harness for this? Created attachment 184640 [details]
Integration & Unit Tests
Hi Greg,
I did as I'm used to a CodeFormat on RequestTest.java. You'll find the new tests at the bottom of the class.
I also added Mockito to the base pom.xml to provide the version number and to jetty.server with scope test. Not sure if that's ok with you, but I found it very useful for Unit testing.
Cheers,
Thomas
Thomas, normally I try to keep formatting changes in different patches than functional ones, as it makes it harder to see the differences. Note that we have some eclipse formatting styles in svn+ssh://gwilkins@dev.eclipse.org/svnroot/rt/org.eclipse.jetty/admin I'm normally dependency phobic, but less so for testing. However, all our dependencies need to be IP cleared by eclipse. So you need to check if mockito is already approved by eclipse and then we need to enter the right doco to say it is ok to use it. As for the tests themselves, they are a little more fine grained than we normally do (hence the need for mockito). Which is not a bad thing. It is good to test the actual merging that is being done. But I'd also like to see some tests that check an actual async dispatch and make sure the merge is triggered correctly. cheers Created attachment 184651 [details]
New Patch including missing integration test.
Greg, sorry. Forgot to svn add before I did the diff. So the integration test was missing in the first dev. Not used to not being able to commit myself (which is ok, just have to get used to it).
Please find attached an updated diff including the integration test.
Regarding the code format. I usually separate them from functional changes as well. My excuse is the same as with svn add. :) I placed the IntegrationTest in jetty-servlet as I used a Servlet to test it. I can as well check if the same thing is possible with a Handler if you want me to. Then it'll reside in jetty-server which is closer to Continuation API and AsyncContext.java. Created attachment 184703 [details]
Minor update
Changed two minor things. If you didn't integrate that patch yet, use the newest version.
Created attachment 184704 [details]
+ code format with Intalio code templates
I'm not going to commit this patch for now, because there is a 2 month back log to getting new CQ's approved at eclipse - even for things that are already at eclipse. So now is not the time to be adding new dependencies, as it will prevent us being able to make releases. I'll see if there is any of it that can be used without mockito I added AsyncContextDispatchWithQueryStrings, but not the ReqeustTest changes. |
Lets say you start with a request /cr/foo?foo=bar Then you start async and dispatch overriding this parameter AsyncContext ac = request.startAsync(); ac.dispatch(request.getServletContext(), "/bar?foo=baz"); the dispatched-to servlet still sees the foo=bar parameter, not foo=baz. There's some appropriate-looking override code in Dispatcher.forward but I'm not quite sure what the best way of reusing it would be.