[Mono-dev] Patch for HttpRequest.cs
kornelpal at gmail.com
Fri May 26 04:50:36 EDT 2006
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.)
----- Original Message -----
From: "Juraj Skripsky" <js at hotfeet.ch>
To: "Kornél Pál" <kornelpal at gmail.com>
Cc: "Miguel de Icaza" <miguel at ximian.com>;
<mono-devel-list at lists.ximian.com>
Sent: Friday, May 26, 2006 2:31 AM
Subject: Re: [Mono-dev] Patch for HttpRequest.cs
> 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
> - 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
> 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");
> 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:
>> You are wrong. HttpRequest.QueryString does the following on MS.NET:
>> The only encoding it uses is HttpRequest.ContentEncoding. It tries to
>> HttpWorkerRequest.GetQueryStringRawBytes(). If it fails then falls back
>> 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
>> HttpWorkerRequest or the query string was set either in constructor or
>> 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
>> as we currently do is a wrong implementation as
>> 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
>> ----- Original Message -----
>> From: "Juraj Skripsky" <js at hotfeet.ch>
>> To: "Miguel de Icaza" <miguel at ximian.com>
>> Cc: <mono-devel-list at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 7800 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20060526/79dc0a20/attachment.obj
More information about the Mono-devel-list