You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Oleg Kalnichevski <ol...@apache.org> on 2012/10/11 16:47:31 UTC
Re: svn commit: r1396793
On Wed, 2012-10-10 at 22:11 +0100, sebb wrote:
> On 10 October 2012 21:51, <ol...@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
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: svn commit: r1396793
Posted by sebb <se...@gmail.com>.
On 12 October 2012 20:24, Oleg Kalnichevski <ol...@apache.org> wrote:
> On Fri, 2012-10-12 at 03:09 +0100, sebb wrote:
>> On 11 October 2012 15:47, Oleg Kalnichevski <ol...@apache.org> wrote:
>> > On Wed, 2012-10-10 at 22:11 +0100, sebb wrote:
>> >> On 10 October 2012 21:51, <ol...@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
Re: svn commit: r1396793
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Fri, 2012-10-12 at 03:09 +0100, sebb wrote:
> On 11 October 2012 15:47, Oleg Kalnichevski <ol...@apache.org> wrote:
> > On Wed, 2012-10-10 at 22:11 +0100, sebb wrote:
> >> On 10 October 2012 21:51, <ol...@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.
> Also, method names are case-significant, so should probably not upcase.
>
Good catch. Corrected.
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
Re: svn commit: r1396793
Posted by sebb <se...@gmail.com>.
On 11 October 2012 15:47, Oleg Kalnichevski <ol...@apache.org> wrote:
> On Wed, 2012-10-10 at 22:11 +0100, sebb wrote:
>> On 10 October 2012 21:51, <ol...@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?
Also, method names are case-significant, so should probably not upcase.
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