Re: [PATCH] Bug 1961 pt1: Use URL object in HttpRequest and cleanup scheme handling

On 27/04/2014 8:12 a.m., Alex Rousskov wrote:
> On 04/26/2014 01:59 AM, Amos Jeffries wrote:
>
>> Also document HttpMsg::http_ver which is a copy of the HTTP-Version
>> field in the "on-wire" message syntax. It is unrelated to the socket
>> transport protocol and the URL scheme protocol.
>
> I understand the above, but
>
>> + /**
>> + * The HTTP-Version label of this message.
>> + */
>> Http::ProtocolVersion http_ver;
>
> the actual comment wording, especially its "label" part, sounds
> confusing to me. Consider (quoting RFC 2616):
>
> /// HTTP-Version field in the first line of the message.
>

Done.

>
>> AnyP::UriScheme const & getScheme() const {return scheme_;}
>>
>> + /// convert the URL scheme to that given
>> + void makeScheme(const AnyP::ProtocolType &p) {scheme_=p;}
>> +
>
> I would call this setScheme() for consistency with other
> setters/getters, to preserve symmetry with the existing getScheme(), and
> because the method does not really "make" schemes.

Done.

>
> Alternatively, consider just making the scheme data member public and
> deleting setters/getters. They are totally redundant right now can can
> be added later if they become needed.
>

I am avoiding this option since the getter helps enforce const-ness and
later updates for this "bug" will require some validation in the setter.

>
>> - %PROTO Requested protocol
>> + %PROTO Requested URL scheme (protocol)
>
> I think the old description covered more cases because many URLs do not
> have a scheme and some future FTP gatewayed requests may not even have
> true URLs.

Indeed, %PROTO is expected to produce "none" or "-" if the URL has no
known scheme.

class URL is based on
http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-26#section-2.7

And %PROTO has always presented only the URL scheme value with no
alternative data sources. There are already alternative data soruces
available for the reeiving transport protocol
(AnyP::PortCfg::transport), message format protocol (HttpMsg::http_ver).

The "which protocol" situation is about to get a bit tricky as you note,
but current trunk code which this patch is altering is specifically
about the URL request-line field scheme value. I am not altering that.

> How about something like:
>
> %PROTO Requested origin server protocol (URL scheme)
>

This is wrong because we may be mapping the URL into some other protocol
for the origin server (ie COAP, COAPS or HTTPS) even when teh URL says
"http://" or "https://"

"Requested URL scheme" follows the other "Requested URL path"

>
> None of the polishing touches above require another round of reviews as
> far as I am concerned.
>

Thank you. Applied.

Amos
Received on Sun Apr 27 2014 - 07:27:35 MDT