Hi,
Thanks for the comments and modifications. I've added two more:
- cosmetic: rename variable query_string_nvc to query_string
- avoid code duplication: use HttpUtility.ParseQueryString in
HttpRequest
- move core of HttpUtility.ParseQueryString into an internal method
(which fills the NameValueCollection passed to it)
With the new patch applied, the NUnit tests for System.Web show only the
one failure I've already mentioned. If it were up to me, I would prefer
to add a comment and disable it, as we are copying broken behavior. What
do you think?
I will do some more testing with a few websites of our own. If
everything works fine, I would like to commit the changes.
- Juraj
On Fri, 2006-05-26 at 10:50 +0200, Kornél Pál wrote:
> Hi,
>
> I looked at the patch and I have the following comments:
>
> >public HttpRequest (string filename, string url, string queryString)
> >...
> >url_components.Query = queryString.TrimStart('?');
> >encoding = Encoding.Default;
>
> ? should not be trimmed at all. And in fact TrimStart will trim all the ?
> characters not the first one only.
>
> encoding should not be initialized because you should get an exception when
> you later try to get ContentEncoding as there is no worker request. I think
> this is why QueryString is initialized using Encoding.Default in MS.NET.
>
> So QueryString should be initialized using Encoding.Default but
> ContentEncoding should not be set. I think that the cleanest way is to move
> query string parsing to a separate function that takes encoding as a
> parameter and call it using Encoding.Default in this constructor and using
> ContentEncoding in QueryString property.
>
> I attached a patch that implements these modifications. (Please review
> however as I didn't test it much.)
>
> Kornél
>
> ----- Original Message -----
> From: "Juraj Skripsky" <[EMAIL PROTECTED]>
> To: "Kornél Pál" <[EMAIL PROTECTED]>
> Cc: "Miguel de Icaza" <[EMAIL PROTECTED]>;
> <mono-devel-list@lists.ximian.com>
> Sent: Friday, May 26, 2006 2:31 AM
> Subject: Re: [Mono-dev] Patch for HttpRequest.cs
>
>
> > Hi,
> >
> > Sorry for taking so long to get back to you. A new patch is attached
> > which does the following:
> >
> > - get_RootVirtualDir: small cleanup and optimization. get_RootVirtualDir
> > and get_BaseVirtualDir are almost identical, so let the former use the
> > later.
> > - get_QueryString: using ContentEncoding when UrlDecoding
> > - url_components: renamed from uri_builder
> > - UrlComponent: new private property. Its getter does the job of former
> > method InitUriBuilder(). Allows to eliminate most of the ugly
> > "uri_builder == null" checks in HttpRequest.
> > - UrlCompontent.Query is initialized as suggested by Kornél (using
> > HttpWorker.{GetQueryStringRawBytes,GetQueryString},
> > HttpRequest.ContentEncoding).
> >
> > There is one test case failure after applying the patch.
> >
> > Test "U2" in "Test_PropertiesSimpleConstructor ()":
> > ---------------------------------------------------
> > string url = "http://www.gnome.org/";;
> > string qs = "key=value&key2=value%32second";
> > HttpRequest r = new HttpRequest ("file", url, qs);
> > Assert.AreEqual (url, r.Url.ToString (), "U2");
> >
> > Result:
> > -------
> > expected:<"http://www.gnome.org/";>
> > but was:<"http://www.gnome.org/?key=value&key2=value2second";
> >
> > I consider this a bug in MS.NET, "r.Url.ToString ()" should be returning
> > the url including the query string at this point. Are there any known
> > cases where code depends on the presence of this bug? What should we do
> > about it?
> >
> > - Juraj
> >
> >
> > On Mon, 2006-05-08 at 12:57 +0200, Kornél Pál wrote:
> >> Hi,
> >>
> >> You are wrong. HttpRequest.QueryString does the following on MS.NET:
> >>
> >> The only encoding it uses is HttpRequest.ContentEncoding. It tries to
> >> obtain
> >> HttpWorkerRequest.GetQueryStringRawBytes(). If it fails then falls back
> >> to
> >> HttpWorkerRequest.GetQueryString(). When it was able to obtain the byte
> >> array it will decode it using HttpRequest.ContentEncoding.GetString(). As
> >> such query string is decoded correctly. When no byte array is available
> >> in
> >> HttpWorkerRequest or the query string was set either in constructor or
> >> using
> >> HttpContext.RewritePath for example the string is assumed to be decoded
> >> correctly so no decoding is done.
> >>
> >> Now we have a string that still may be URL encoded. MS.NET probably calls
> >> HttpUtility.UrlDecode just like we do but MS.NET passes
> >> HttpRequest.ContentEncoding as well because query string is assumed to be
> >> URL encoded using that encoding.
> >>
> >> Note that obtaining query string from HttpWorkerRequest in the
> >> constructor
> >> as we currently do is a wrong implementation as
> >> HttpRequest.ContentEncoding
> >> can be changed before HttpRequest.QueryString is first accessed.
> >>
> >> We should do the following:
> >> - delay query string processing until it is needed (don't obtain query
> >> string in the constructor)
> >> - try HttpWorkerRequest.GetQueryStringRawBytes() as well
> >> - use HttpRequest.ContentEncoding to decode the byte array and for
> >> HttpUtility.UrlDecode
> >>
> >> Kornél
> >>
> >> ----- Original Message -----
> >> From: "Juraj Skripsky" <[EMAIL PROTECTED]>
> >> To: "Miguel de Icaza" <[EMAIL PROTECTED]>
> >> Cc: <mono-devel-list@lists.ximian.com>
> >> Sent: Monday, May 08, 2006 12:22 PM
> >> Subject: Re: [Mono-dev] Patch for HttpRequest.cs
> >>
> >>
> >> > Hello,
> >> >
> >> > After running more tests, I've found out that on MS.NET the decoding in
> >> > HttpRequest.QueryString does _not_ depend on
> >> > HttpRequest.ContentEncoding. In fact, MS seems to be always using
> >> > Latin1
> >> > here. All other standard encodings fail.
> >> >
> >> > A revised patch is attached, including a NUnit test case. If no one
> >> > objects, I'll commit.
> >> >
> >> > - Juraj
> >> >
> >> >
> >> > On Sat, 2006-05-06 at 13:47 -0400, Miguel de Icaza wrote:
> >> >> Hello Juraj,
> >> >>
> >> >> > The attached patch makes sure that the get-parameters in QueryString
> >> >> > are
> >> >> > url-decoded using the proper encoding (when creating the
> >> >> > NameValueCollection).
> >> >> >
> >> >> > May I commit?
> >> >>
> >> >> Could you provide NUnit tests for this case?
> >> >>
> >> >> Miguel
> >> >>
> >
Index: Test/System.Web/HttpRequestTest.cs
===================================================================
--- Test/System.Web/HttpRequestTest.cs (revision 61128)
+++ Test/System.Web/HttpRequestTest.cs (working copy)
@@ -129,7 +129,18 @@
HttpRequest r = new HttpRequest ("file", url, qs);
string s = r.PhysicalApplicationPath;
}
+
+ [Test]
+ public void Test_QueryStringDecoding()
+ {
+ string url = "http://www.gnome.org/";;
+ string qs = "umlaut=" + HttpUtility.UrlEncode("\u00e4", Encoding.Default);
+
+ HttpRequest r = new HttpRequest ("file", url, qs);
+ Assert.AreEqual("\u00e4", r.QueryString["umlaut"]);
+ }
}
+
[TestFixture]
public class Test_HttpFakeRequest {
Index: System.Web/HttpRequest.cs
===================================================================
--- System.Web/HttpRequest.cs (revision 61128)
+++ System.Web/HttpRequest.cs (working copy)
@@ -48,11 +48,11 @@
public sealed class HttpRequest {
HttpWorkerRequest worker_request;
HttpContext context;
- WebROCollection query_string_nvc;
+ WebROCollection query_string;
//
- string filename, query_string;
- UriBuilder uri_builder;
+ string filename;
+ UriBuilder url_components;
string client_target;
@@ -60,7 +60,7 @@
// On-demand computed values
//
HttpBrowserCapabilities browser_capabilities;
- string file_path, base_virtual_dir, root_virtual_dir;
+ string file_path, base_virtual_dir;
string content_type;
int content_length = -1;
Encoding encoding;
@@ -94,31 +94,39 @@
public HttpRequest (string filename, string url, string queryString)
{
// warning 169: what are we supposed to do with filename?
-
this.filename = filename;
- uri_builder = new UriBuilder (url);
- query_string = queryString;
+ url_components = new UriBuilder (url);
+ url_components.Query = queryString;
+
+ query_string = new WebROCollection ();
+ HttpUtility.ParseQueryString (queryString, Encoding.Default, query_string);
+ query_string.Protect ();
}
- void InitUriBuilder ()
- {
- uri_builder = new UriBuilder ();
- uri_builder.Scheme = worker_request.GetProtocol ();
- uri_builder.Host = worker_request.GetServerName ();
- int port = worker_request.GetLocalPort ();
- uri_builder.Port = port;
- uri_builder.Path = worker_request.GetUriPath ();
- if (query_string != null && query_string != "")
- uri_builder.Query = query_string;
+ UriBuilder UrlComponents {
+ get {
+ if (url_components == null) {
+ url_components = new UriBuilder ();
+ url_components.Scheme = worker_request.GetProtocol ();
+ url_components.Host = worker_request.GetServerName ();
+ url_components.Port = worker_request.GetLocalPort ();
+ url_components.Path = worker_request.GetUriPath ();
+
+ byte[] queryStringRaw = worker_request.GetQueryStringRawBytes();
+ if(queryStringRaw != null)
+ url_components.Query = ContentEncoding.GetString(queryStringRaw);
+ else
+ url_components.Query = worker_request.GetQueryString();
+ }
+ return url_components;
+ }
}
internal HttpRequest (HttpWorkerRequest worker_request, HttpContext context)
{
this.worker_request = worker_request;
this.context = context;
- if (worker_request != null)
- query_string = worker_request.GetQueryString ();
}
string [] SplitHeader (int header_index)
@@ -878,10 +886,7 @@
public string Path {
get {
- if (uri_builder == null)
- InitUriBuilder ();
-
- return uri_builder.Path;
+ return UrlComponents.Path;
}
}
@@ -927,52 +932,32 @@
internal string RootVirtualDir {
get {
- if (root_virtual_dir == null){
- string fp = FilePath;
- int p = fp.LastIndexOf ('/');
-
- if (p == -1)
- root_virtual_dir = "/";
- else
- root_virtual_dir = fp.Substring (0, p);
- }
-
- return root_virtual_dir;
- }
+ string dir = BaseVirtualDir;
+ if (dir.LastIndexOf ('/') == -1)
+ return "/";
+ else
+ return dir;
+ }
}
public NameValueCollection QueryString {
get {
- if (query_string_nvc == null){
- query_string_nvc = new WebROCollection ();
+ if (query_string == null){
+ string q = UrlComponents.Query;
+ if (q.Length != 0)
+ q = q.Remove(0, 1);
- if (uri_builder == null)
- InitUriBuilder ();
-
- string q = query_string;
- if (q != null && q != ""){
- string [] components = q.Split ('&');
- foreach (string kv in components){
- int pos = kv.IndexOf ('=');
- if (pos == -1){
- query_string_nvc.Add (null, HttpUtility.UrlDecode (kv));
- } else {
- string key = HttpUtility.UrlDecode (kv.Substring (0, pos));
- string val = HttpUtility.UrlDecode (kv.Substring (pos+1));
-
- query_string_nvc.Add (key, val);
- }
- }
- }
- query_string_nvc.Protect ();
+ query_string = new WebROCollection ();
+ HttpUtility.ParseQueryString (q, ContentEncoding, query_string);
+ query_string.Protect();
}
if (validate_query_string && !checked_query_string) {
- ValidateNameValueCollection ("QueryString", query_string_nvc);
+ ValidateNameValueCollection ("QueryString", query_string);
checked_query_string = true;
}
- return query_string_nvc;
+ return query_string;
}
}
@@ -980,12 +965,8 @@
get {
if (worker_request != null)
return worker_request.GetRawUrl ();
- else {
- if (query_string != null && query_string != "")
- return uri_builder.Path + "?" + query_string;
- else
- return uri_builder.Path;
- }
+ else
+ return UrlComponents.Path + UrlComponents.Query;
}
}
@@ -1029,14 +1010,10 @@
public Uri Url {
get {
- if (uri_builder == null)
- InitUriBuilder ();
-
- if (cached_url == null) {
- UriBuilder builder = new UriBuilder (uri_builder.Uri);
- cached_url = builder.Uri;
- }
- return cached_url;
+ if (cached_url == null)
+ cached_url = UrlComponents.Uri;
+
+ return cached_url;
}
}
@@ -1185,12 +1162,9 @@
string path = "/";
if (worker_request != null) {
version = worker_request.GetHttpVersion ();
- InitUriBuilder ();
- path = uri_builder.Path;
+ path = UrlComponents.Path;
}
- string qs = null;
- if (query_string != null && query_string != "")
- qs = "?" + query_string;
+ string qs = UrlComponents.Query;
sb.AppendFormat ("{0} {1}{2} {3}\r\n", HttpMethod, path, qs, version);
NameValueCollection coll = Headers;
@@ -1261,19 +1235,16 @@
file_path = path;
}
- internal void SetCurrentExePath (string path)
- {
+ internal void SetCurrentExePath (string path)
+ {
cached_url = null;
current_exe_path = path;
file_path = path;
- if (uri_builder == null)
- InitUriBuilder ();
- uri_builder.Path = path;
+ UrlComponents.Path = path;
// recreated on demand
- root_virtual_dir = null;
base_virtual_dir = null;
physical_path = null;
- }
+ }
internal void SetPathInfo (string pi)
{
@@ -1293,20 +1264,13 @@
// Notice: there is nothing raw about this querystring.
internal string QueryStringRaw {
get {
- if (uri_builder == null)
- InitUriBuilder ();
-
- return query_string;
+ return UrlComponents.Query;
}
set {
- if (uri_builder == null)
- InitUriBuilder ();
-
- query_string = value;
- query_string_nvc = null;
- if (uri_builder != null)
- uri_builder.Query = value;
+ UrlComponents.Query = value;
+ cached_url = null;
+ query_string = null;
}
}
Index: System.Web/HttpUtility.cs
===================================================================
--- System.Web/HttpUtility.cs (revision 61128)
+++ System.Web/HttpUtility.cs (working copy)
@@ -965,7 +965,7 @@
#if NET_2_0
public static NameValueCollection ParseQueryString (string query)
{
- return ParseQueryString (query, Encoding.UTF8);
+ return ParseQueryString (query, Encoding.Default);
}
public static NameValueCollection ParseQueryString (string query, Encoding encoding)
@@ -978,41 +978,30 @@
return new NameValueCollection ();
if (query[0] == '?')
query = query.Substring (1);
+
+ NameValueCollection result = new NameValueCollection ();
+ ParseQueryString (query, encoding, result);
+ return result;
+ }
+#endif
- int namePos = 0;
- NameValueCollection collection = new NameValueCollection ();
- while (namePos <= query.Length) {
- int valuePos = -1, valueEnd = -1;
- for (int q = namePos; q < query.Length; q++) {
- if (valuePos == -1 && query[q] == '=') {
- valuePos = q + 1;
- } else if (query[q] == '&') {
- valueEnd = q;
- break;
+ internal static void ParseQueryString (string query, Encoding encoding, NameValueCollection result)
+ {
+ if (query.Length != 0) {
+ string [] components = query.Split ('&');
+ foreach (string kv in components){
+ int pos = kv.IndexOf ('=');
+ if (pos == -1) {
+ result.Add (null, HttpUtility.UrlDecode (kv, encoding));
+ } else {
+ string key = HttpUtility.UrlDecode (kv.Substring (0, pos), encoding);
+ string val = HttpUtility.UrlDecode (kv.Substring (pos+1), encoding);
+
+ result.Add (key, val);
}
}
-
- string name, value;
- if (valuePos == -1) {
- name = null;
- valuePos = namePos;
- } else {
- name = System.Web.HttpUtility.UrlDecode (query.Substring (namePos, valuePos - namePos - 1), encoding);
- }
- if (valueEnd < 0) {
- namePos = -1;
- valueEnd = query.Length;
- } else {
- namePos = valueEnd + 1;
- }
- value = System.Web.HttpUtility.UrlDecode (query.Substring (valuePos, valueEnd - valuePos), encoding);
-
- collection.Add (name, value);
- if (namePos == -1) break;
}
- return collection;
}
-#endif
#endregion // Methods
}
}
_______________________________________________
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
