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