[Mono-dev] Patch for HttpRequest.cs

[Kornél Pál]

Hi,

I didn't do performance tests but the original core ParseQueryString 
implementation may be faster. You should give it a try. I didn't evaluate 
whether movig string query_string to UrlComponents is good so I don't know 
but I like the other modifications. Of course I think you should wait for an 
approval.

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 3:26 PM
Subject: Re: [Mono-dev] Patch for HttpRequest.cs


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