[Mono-dev] Patch for HttpRequest.cs

[Kornél Pál]

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" <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


> 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" <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...
Name: HttpRequest_mod.patch
Type: application/octet-stream
Size: 7800 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20060526/79dc0a20/attachment.obj