HTTP::Response inconsistency

In a LWP application I'm just writing I have found a surprising
behaviour: Responses created by HTTP::Response::new and written to
disk with HTTP::Response::as_string can't be successfully parsed by
HTTP::Response::Parse.

The reason is simple:

HTTP::Response::new creates a response from its headers, then sets the
code and message as given by the $rc and $msg parameters. It does
*not* define a protocol.

my $self = $class->SUPER::new($header, $content);
$self->code($rc);
$self->message($msg);

However, HTTP::Response::parse expects the first nonblank item to be
the protocol, which hasn't been defined by HTTP::Response::new.

my($protocol, $code, $message) = split(' ', $status_line, 3);

Hence the original code is stored in $response->protocol and the
original message in $response->code - the latter makes methods like
$response->is_success() croak.

I don't see a really *good* solution. In my opinion
HTTP::Response::new ought to allow to add a protocol. But changing
the parameters might break existing code.

As a fallback, HTTP::Response::parse could set the protocol to undef
if it turns out to be a three-digit number, assigning this value to
the code (after assigning to the message what was parsed as the code).

Maybe the best fallback would be to write some "undefined" value in
HTTP::Response::as_string if the protocol is undefined:

my $status_line = "$code";
my $proto = $self->protocol;
- $status_line = "$proto $status_line" if $proto;
+ $status_line = $proto ? "$proto $status_line"
+ : "UNKNOWN $status_line";

But again, this might break existing code.

Currently I'm working around the problem by setting the protocol
in my own code before stringifying like this:

$response->protocol('UNKNOWN') unless $response->protocol();

I don't like that too much either.

I could submit patches for all the fallbacks and workarounds -
what would be The Right Thing To Do?
--
Cheers,
haj

Re: HTTP::Response inconsistency

I wrote:

> In a LWP application I'm just writing I have found a surprising
> behaviour: Responses created by HTTP::Response::new and written to
> disk with HTTP::Response::as_string can't be successfully parsed by
> HTTP::Response::Parse.

Addendum:

HTTP::Response::clone doesn't clone the protocol either.
This, however, can be fixed easily:

--- Response.pm.1.50 2004-12-02 21:36:42.437500000 +0100
+++ Response.pm 2004-12-02 21:37:18.343750000 +0100
@@ -47,4 +47,5 @@
my $self = shift;
my $clone = bless $self->SUPER::clone, ref($self);
+ $clone->protocol($self->protocol);
$clone->code($self->code);
$clone->message($self->message);

--
Cheers,
haj

Re: HTTP::Response inconsistency

Harald Joerg writes:

> HTTP::Response::clone doesn't clone the protocol either.
> This, however, can be fixed easily:

Thanks. Applied this patch to HTTP::Message so that also Requests
clone their protocol attribute.

> --- Response.pm.1.50 2004-12-02 21:36:42.437500000 +0100
> +++ Response.pm 2004-12-02 21:37:18.343750000 +0100
> @@ -47,4 +47,5 @@
> my $self = shift;
> my $clone = bless $self->SUPER::clone, ref($self);
> + $clone->protocol($self->protocol);
> $clone->code($self->code);
> $clone->message($self->message);
>
> --
> Cheers,
> haj

Re: HTTP::Response inconsistency

Harald Joerg writes:

> As a fallback, HTTP::Response::parse could set the protocol to undef
> if it turns out to be a three-digit number, assigning this value to
> the code (after assigning to the message what was parsed as the code).

This is my preferred fix. Just make HTTP::Response::parse deal with
what as_string spits out. I would just make it look at the string
before spliting it. If it starts with /\d/ split in 2 instead of 3.

>
> Maybe the best fallback would be to write some "undefined" value in
> HTTP::Response::as_string if the protocol is undefined:
>
> my $status_line = "$code";
> my $proto = $self->protocol;
> - $status_line = "$proto $status_line" if $proto;
> + $status_line = $proto ? "$proto $status_line"
> + : "UNKNOWN $status_line";
>
> But again, this might break existing code.

I also find this quite ugly.

> I could submit patches for all the fallbacks and workarounds -

That would be very much appreciated.

Regards,
Gisle

Re: HTTP::Response inconsistency

--------------030800040608070202080008
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Gisle Aas writes:

> Harald Joerg writes:
>
>>As a fallback, HTTP::Response::parse could set the protocol to undef
>>if it turns out to be a three-digit number, assigning this value to
>>the code (after assigning to the message what was parsed as the code).
>
> This is my preferred fix. Just make HTTP::Response::parse deal with
> what as_string spits out. I would just make it look at the string
> before spliting it. If it starts with /\d/ split in 2 instead of 3.

Patch is attached.

Should we try to add values for protocol every time when LWP creates
a HTTP::Response object? Given this patch and the fact that nobody
complained so far, I don't think it's worth the effort...

BTW: Thanks for moving my other patch to HTTP::Message. I had looked
at HTTP::Headers::clone, but missed the HTTP::Message base class.
--
Cheers,
haj



--------------030800040608070202080008
Content-Type: text/plain;
name="patch.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="patch.txt"

--- Response.pm.1.50 2004-12-02 21:36:42.437500000 +0100
+++ Response.pm 2004-12-03 22:10:27.421875000 +0100
@@ -35,5 +35,11 @@

my $self = $class->SUPER::parse($str);
- my($protocol, $code, $message) = split(' ', $status_line, 3);
+ my($protocol, $code, $message);
+ if ($status_line =~ /^\d{3} /) {
+ # Looks like a response created by HTTP::Response->new
+ ($code, $message) = split(' ', $status_line, 2);
+ } else {
+ ($protocol, $code, $message) = split(' ', $status_line, 3);
+ }
$self->protocol($protocol) if $protocol;
$self->code($code) if defined($code);


--------------030800040608070202080008--

Re: HTTP::Response inconsistency

Harald Joerg writes:

> Gisle Aas writes:
>
> > Harald Joerg writes:
> >
> >>As a fallback, HTTP::Response::parse could set the protocol to undef
> >>if it turns out to be a three-digit number, assigning this value to
> >>the code (after assigning to the message what was parsed as the code).
> > This is my preferred fix. Just make HTTP::Response::parse deal with
> > what as_string spits out. I would just make it look at the string
> > before spliting it. If it starts with /\d/ split in 2 instead of 3.
>
> Patch is attached.

Thanks. Applied.

Regards,
Gisle

> --- Response.pm.1.50 2004-12-02 21:36:42.437500000 +0100
> +++ Response.pm 2004-12-03 22:10:27.421875000 +0100
> @@ -35,5 +35,11 @@
>
> my $self = $class->SUPER::parse($str);
> - my($protocol, $code, $message) = split(' ', $status_line, 3);
> + my($protocol, $code, $message);
> + if ($status_line =~ /^\d{3} /) {
> + # Looks like a response created by HTTP::Response->new
> + ($code, $message) = split(' ', $status_line, 2);
> + } else {
> + ($protocol, $code, $message) = split(' ', $status_line, 3);
> + }
> $self->protocol($protocol) if $protocol;
> $self->code($code) if defined($code);