Re: Maybe bug in ZnHttpClient

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Maybe bug in ZnHttpClient

Sven Van Caekenberghe
Hello Tony,

(I am CC'ing the Pharo list)

On 11 Jan 2011, at 23:48, Tony Fleig wrote:

> Hi Sven,
>
> At Stef's suggestion I am trying Zinc in place of WebClient in my
> automated testing of my Seaside apps.
>
> I like Zinc better already and I can see that I will need fewer lines
> of code using Zinc than WebClients for my purposes.

That is great! Thanks for trying Zn and for giving feedback.

BTW, I really liked your writeup about testing (and I was thinking/hoping all the time, he should be using Zn ;-)

> That said, I have found I think a bug.
>
> Pharo 1.1.1 Seaside 3.0.3 Zinc latest
>
> In ZnHttpClient>>#method:for:headers:data:limit, if a 302 response is
> received, the Location header field may not contain a complete URL
> (i.e. it may not contain scheme://host:port but only the path). When
> this happens, the next ZnHttpClient>>#openConnection fails in
> NetNameResolver trying to resolve a nil hostname.
>
> This occurs when the server is Seaside.
>
> I have solved this by setting followRedirect to false and handling the
> redirect myself (as I as doing with WebClient anyway.)

Well, strictly speaking it isn't a bug (the specs say that the Location URL has to be absolute), but you are right: there is common support for relative redirect locations, so I added that (did not yet test it though).

> Some other observations:
>
> 1. It is my understanding that many browsers respond to a 302/303
> response from a POST with a following GET to the address specified in
> the 302/303 Location header rather than repeating the POST with its
> payload as is done in ZnHttpClient. (See
> http://en.wikipedia.org/wiki/HTTP_303).

I'll have to study/think about that for a while, it depends on what the specs says. It doesn't make much sense for an automated client to do a GET I think. What would you suggest ?

> 2. The arguments to the settings accessors in ZnUserAgentSettings are
> all anObject. Wouldn't aBoolean and anInteger be more informative
> names (for followRedirect: and redirectLimit: for example)? I was
> stopped when I saw anObject and went perusing the sources to be sure
> that what was wanted was a boolean and not something more complex.

You're right, proper parameter names are helpful, I changed some of them.

I went over the ZnUserAgent and ZnClient classes (they were originally written by Matt Kennedy). I made various simplifications and cleanups (most related to newer API) which might help when reading the code. I also made an important change/bugfix regarding query parameters (originally reported by Cédric Béler I think).

Thanks again for the feedback, if you have any more questions, feel free to ask them.

Sven

> Sven Van Caekenberghe uploaded a new version of Zinc-HTTP to project Zinc HTTP Components:
> http://www.squeaksource.com/ZincHTTPComponents/Zinc-HTTP-SvenVanCaekenberghe.114.mcz
>
> ==================== Summary ====================
>
> Name: Zinc-HTTP-SvenVanCaekenberghe.114
> Author: SvenVanCaekenberghe
> Time: 12 January 2011, 2:03:44 pm
> UUID: e1a49d00-d9f0-4800-8cd7-cb354e86d671
> Ancestors: Zinc-HTTP-SvenVanCaekenberghe.113
>
> ZnUserAgent (and ZnClient) now can follow relative redirect locations;
> introduced ZnMultiValueDictionary to allow multiple values to be stored under one key as an array;
> using ZnMultiValueDictionary for queries and headers;
> ZnUrl now uses ZnUtils>>parseQueryFrom: again;
> various simplifications and cleanups which might help when reading the code in ZnUserAgent (and ZnClient);
> ZnUserAgent (and ZnClient) now handle parameter encoding differently



Reply | Threaded
Open this post in threaded view
|

Re: Maybe bug in ZnHttpClient

Stéphane Ducasse
Thanks a lot sven.
I really like your spirit. Engaging, addressing problems.... so great.

Stef

On Jan 12, 2011, at 2:10 PM, Sven Van Caekenberghe wrote:

> Hello Tony,
>
> (I am CC'ing the Pharo list)
>
> On 11 Jan 2011, at 23:48, Tony Fleig wrote:
>
>> Hi Sven,
>>
>> At Stef's suggestion I am trying Zinc in place of WebClient in my
>> automated testing of my Seaside apps.
>>
>> I like Zinc better already and I can see that I will need fewer lines
>> of code using Zinc than WebClients for my purposes.
>
> That is great! Thanks for trying Zn and for giving feedback.
>
> BTW, I really liked your writeup about testing (and I was thinking/hoping all the time, he should be using Zn ;-)
>
>> That said, I have found I think a bug.
>>
>> Pharo 1.1.1 Seaside 3.0.3 Zinc latest
>>
>> In ZnHttpClient>>#method:for:headers:data:limit, if a 302 response is
>> received, the Location header field may not contain a complete URL
>> (i.e. it may not contain scheme://host:port but only the path). When
>> this happens, the next ZnHttpClient>>#openConnection fails in
>> NetNameResolver trying to resolve a nil hostname.
>>
>> This occurs when the server is Seaside.
>>
>> I have solved this by setting followRedirect to false and handling the
>> redirect myself (as I as doing with WebClient anyway.)
>
> Well, strictly speaking it isn't a bug (the specs say that the Location URL has to be absolute), but you are right: there is common support for relative redirect locations, so I added that (did not yet test it though).
>
>> Some other observations:
>>
>> 1. It is my understanding that many browsers respond to a 302/303
>> response from a POST with a following GET to the address specified in
>> the 302/303 Location header rather than repeating the POST with its
>> payload as is done in ZnHttpClient. (See
>> http://en.wikipedia.org/wiki/HTTP_303).
>
> I'll have to study/think about that for a while, it depends on what the specs says. It doesn't make much sense for an automated client to do a GET I think. What would you suggest ?
>
>> 2. The arguments to the settings accessors in ZnUserAgentSettings are
>> all anObject. Wouldn't aBoolean and anInteger be more informative
>> names (for followRedirect: and redirectLimit: for example)? I was
>> stopped when I saw anObject and went perusing the sources to be sure
>> that what was wanted was a boolean and not something more complex.
>
> You're right, proper parameter names are helpful, I changed some of them.
>
> I went over the ZnUserAgent and ZnClient classes (they were originally written by Matt Kennedy). I made various simplifications and cleanups (most related to newer API) which might help when reading the code. I also made an important change/bugfix regarding query parameters (originally reported by Cédric Béler I think).
>
> Thanks again for the feedback, if you have any more questions, feel free to ask them.
>
> Sven
>
>> Sven Van Caekenberghe uploaded a new version of Zinc-HTTP to project Zinc HTTP Components:
>> http://www.squeaksource.com/ZincHTTPComponents/Zinc-HTTP-SvenVanCaekenberghe.114.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Zinc-HTTP-SvenVanCaekenberghe.114
>> Author: SvenVanCaekenberghe
>> Time: 12 January 2011, 2:03:44 pm
>> UUID: e1a49d00-d9f0-4800-8cd7-cb354e86d671
>> Ancestors: Zinc-HTTP-SvenVanCaekenberghe.113
>>
>> ZnUserAgent (and ZnClient) now can follow relative redirect locations;
>> introduced ZnMultiValueDictionary to allow multiple values to be stored under one key as an array;
>> using ZnMultiValueDictionary for queries and headers;
>> ZnUrl now uses ZnUtils>>parseQueryFrom: again;
>> various simplifications and cleanups which might help when reading the code in ZnUserAgent (and ZnClient);
>> ZnUserAgent (and ZnClient) now handle parameter encoding differently
>
>
>