hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1396793
Date Fri, 12 Oct 2012 23:37:32 GMT
On 12 October 2012 20:24, Oleg Kalnichevski <olegk@apache.org> wrote:
> On Fri, 2012-10-12 at 03:09 +0100, sebb wrote:
>> On 11 October 2012 15:47, Oleg Kalnichevski <olegk@apache.org> wrote:
>> > On Wed, 2012-10-10 at 22:11 +0100, sebb wrote:
>> >> On 10 October 2012 21:51,  <olegk@apache.org> wrote:
>> >> > Author: olegk
>> >> > Date: Wed Oct 10 20:51:30 2012
>> >> > New Revision: 1396793
>> >> >
>> >> > URL: http://svn.apache.org/viewvc?rev=1396793&view=rev
>> >> > Log:
>> >> > HTTPCLIENT-1248: Default and lax redirect strategies should not convert
requests redirected with 307 status to GET method
>> >> >
>> >
>> > ...
>> >
>> >> The above code converts unknown methods into GET.
>> >> If a new method is added, that will be incorrect.
>> >>
>> >> It might be useful to know when this is happening; maybe an assert
>> >> would be useful here?
>> >> Or a log message?
>> >>
>> >> Or is it possible to write a clone/copy method that works for all
>> >> possible methods?
>> >> Maybe methods should know how to clone themselves?
>> >>
>> >
>> > This is a good point. I reworked the method to make use of the new
>> > RequestBuilder class to create a mutable copy of the original request
>> > prior to setting the request URI.
>> >
>> > http://svn.apache.org/viewvc?rev=1397085&view=rev
>> >
>> > Please review
>>
>> Looks much better and less fragile.
>>
>> Not quite sure why getMethod should default to POST/GET if the method
>> is null - why should the method ever be null? Isn't that a bug?
>>
>
> I think these are most commonly used methods and as such are reasonable
> defaults if someone explicitly sets request method to null. I believe it
> is should be ok to default to POSt for entity closing requests and GET
> otherwise.
>

Well, yes, if a null method is allowed then the defaults are fine.

But I question whether one should allow a null method in the first place.
Seems to me that could hide bugs, and is a bit confusing unless the
default is universally applied.
I think it would be safer to throw NPE or IAE.

>> Also, method names are case-significant, so should probably not upcase.
>>
>
> Good catch. Corrected.

Thanks - it was only recently I saw the comment on the Tomcat list.

> Oleg
>
>> Per the Tomcat issues list:
>>
>> >>
>> RFCs 2616, 2068, and 1945 all agree that method name is case-sensitive:
>> http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.1
>> <<
>> > Oleg
>> >
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
>> > For additional commands, e-mail: dev-help@hc.apache.org
>> >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
>> For additional commands, e-mail: dev-help@hc.apache.org
>>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> For additional commands, e-mail: dev-help@hc.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org


Mime
View raw message