Bugzilla – Full Text Bug Listing
| Summary: | Passing empty query string to UrlEncoded#decodeTo(String, MultiMap, String) does not yield an empty map | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [RT] Jetty | Reporter: | Stephan Mising name <stephan202> | ||||
| Component: | other | Assignee: | Michael Gorovoy <mgorovoy> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | minor | ||||||
| Priority: | P3 | CC: | mgorovoy | ||||
| Version: | unspecified | ||||||
| Target Milestone: | 7.2.x | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
Created attachment 197964 [details]
Proposed patch along with a unit test
I attached a unit test for this case, as well as a proposed fix. This applies to the jetty-8 branch as well.
Committed r3390 Hi Michael,
Thanks for handling this so rapidly! Indeed the commit fixes the bug, but I
would like to note that,
1. The corresponding unit test was not committed.
2. The use of spaces around operators, though in general good practice,
deviates from the style of the code in the rest of the file.
3. Unless I'm missing something, decodeString(...) never returns null. As
such the extra 'key != null' check is redundant. (That's the reason I left
it out.)
Having never contributed to Jetty before, I guess I'm just curious to know
whether the above issues were considered. (More so because I took extra care to
provide a patch that could be applied as-is. :)
Thanks,
Stephan
Stephan, We absolutely love patches, and appreciate your effort. In this particular instance your patch did not apply cleanly for some reason, and I chose to re-type it since it was just a few lines of code as opposed to rolling back my development environment. I did commit the unit test as you can see in the diff http://goo.gl/xPi2x. The use of spaces around the operators in this particular source file is inconsistent at best (see line 223 vs line 299 in http://goo.gl/fk9Gv), so I chose to use this style. As far as the check for null goes, you are correct that decodeString() never explicitly returns null. However I believe it is a good practice to check for null before calling a method on an object anyway to protect the code from an NPE that might be caused by either invalid parameters or future code changes. Cheers, Michael Hi Michael, The patch was made w.r.t jetty-util, not the root of the project; that might explain why it couldn't be applied. As for the unit test: I stand corrected :) After a local `svn up && svn diff` the test was still listed in the diff; I didn't actually open the file to inspect it. (Something I should have done, as this confusion makes perfectly clear.) Thanks for the feedback! Cheers, Stephan |
Build Identifier: N/A; verified trunk Consider the following code: final MultiMap<String> params = new MultiMap<String>(); UrlEncoded.decodeTo("", params, "UTF-8"); Upon completion the map is _not_ empty. Rather, it contains a single key/value pair, both of which are the empty string. This makes little sense. Reproducible: Always Steps to Reproduce: See the example in the description.