Repository: netty/netty
Issue: QueryStringDecoder replace all ; using &
Labels: defect

If the url is /hello;pathparameter

Then System.out.println(decoder.getPath()); 

will replace ; to & :  /hello&pathparameter

Simply replace ; use & does not work. 


Issue comments:
@jianjin can you give some details on what not work and what you expect it to do ?

If an url like this : http://foo.com/images;num=10?query=name;value=123
the first ";" should not be replaced by &, because it is part of path but NOT query.
Host:foo.com
Path:/images
Path Parameter:num=10
QueryParameters:query=name  and value=123

The getPath() should be /images;num=10 but not  /images&num=10

https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java#L117

^ that should probably only be applied to the query string instead of the whole URI

Yep. Or we could change the code @ https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java#L225

to  (c == '&' || c == ';') {

Could you submit a pull-req ?
Am 12.09.2013 um 12:05 schrieb jian jin notifications@github.com:

> Yep. Or we could change the code @ https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java#L225
> 
> to (c == '&' || c == ';') {
> 
> —
> Reply to this email directly or view it on GitHub.

@jianjin @normanmaurer ,the ;in URL is just like &

see this:

```
According to the W3C: http://www.w3.org/TR/html401/appendix/notes.html#h-B.2.2

We recommend that HTTP server implementors, and in particular, CGI implementors support the use of ";" in place of "&" to save authors the trouble of escaping "&" characters in this manner.
The query string decoder that Netty provides follows this recommendation, which I think is reasonable. The best thing to do is probably to use a separator other than ; or URL-encode your data.
```

so 
localhost/get？key=a;b;c 
will make key=a ,and treat b and c as keys which will be null.
key=a
a=null
c=null

@jianjin I am wandering ,why you make you url pattern like that?

@hepin1989 Netty's decoder does it wrong at the moment, that's the issue here. Netty replaces ; with & in the whole URI instead of just the query string.

@quickwango really?okay,just send a pull request.

just did it via the browser, I don't expect anything to break though

cool. Would we fix it in 3.x also?

Sure. the fix should go to both 3.x and 4.x.

Fixed in branches 3, 4.0 and master


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
index 792452f44e3..4075efa25b1 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
@@ -113,8 +113,7 @@ public QueryStringDecoder(String uri, Charset charset, boolean hasPath, int maxP
                     "maxParams: " + maxParams + " (expected: a positive integer)");
         }
 
-        // http://en.wikipedia.org/wiki/Query_string
-        this.uri = uri.replace(';', '&');
+        this.uri = uri;
         this.charset = charset;
         this.maxParams = maxParams;
         this.hasPath = hasPath;
@@ -160,10 +159,8 @@ public QueryStringDecoder(URI uri, Charset charset, int maxParams) {
             hasPath = false;
         }
         // Also take care of cut of things like "http://localhost"
-        String newUri = rawPath + '?' + uri.getRawQuery();
+        this.uri = rawPath + '?' + uri.getRawQuery();
 
-        // http://en.wikipedia.org/wiki/Query_string
-        this.uri = newUri.replace(';', '&');
         this.charset = charset;
         this.maxParams = maxParams;
     }
@@ -222,7 +219,8 @@ private void decodeParams(String s) {
                     name = decodeComponent(s.substring(pos, i), charset);
                 }
                 pos = i + 1;
-            } else if (c == '&') {
+            // http://www.w3.org/TR/html401/appendix/notes.html#h-B.2.2
+            } else if (c == '&' || c == ';') {
                 if (name == null && pos != i) {
                     // We haven't seen an `=' so far but moved forward.
                     // Must be a param of the form '&a&' so add it with


Test patch: