Emit a warning if HTTP status code implies success but X-Died header has been set

[oalders]

Test code mostly stolen from
@eserte in https://rt.cpan.org/Public/Bug/Display.html?id=101990 which is now libwww-perl/libwww-perl#242

I've been lazy with the test. I can clean that up not to use LWP::UserAgent if desired. All feedback welcome. I'm happy to change completely.

[oalders]

Does anyone have any thoughts on this?

[genio]

Since LWP requires HTTP-Message, can we require LWP from HTTP-Message for the test? circular deps problem?

[oalders]

I've switched from Test::Requires to Test::Needs. There should be no circular deps since the test will be skipped if that module is not available. I've also added the actual message from the X-Died header in order to make the warning more informative. I force-pushed to keep this down to one commit.

[skaji]

• Should we care about Client-Warning header too?
• Actually I'm not a big fun fan of emitting warnings. I would suggest that document clearly recommend people checking X-Died / Client-Warning headers by themselves.

[oalders]

Should we care about Client-Warning header too?

Yes, I think so. I could add that.

Actually I'm not a big fun of emitting warnings. I would suggest that document clearly recommend people checking X-Died / Client-Warning headers by themselves.

I'm not a huge fan of it either, but in think in this case it's a hassle to ask people to check two different headers in addition to the response code on each request. What I'd like to do is unmask a hard to debug error. In this case if you check those headers before calling is_success() you can bypass the warning.

Other options:

• add the header checks via HTTP::Response::is_success( strict => 1)
• add the header checks via LWP::UserAgent::is_success( strict => 1)

OR

• add HTTP::Response::is_error() which would check is_client_error(), is_server_error() and then check the headers. You would have to check success via !$response->is_error() && $response->is_success()

I still like the idea of baking this into LWP::UserAgent via something like is_success( strict => 1) as that seems much less verbose than checking two different status for the success of each HTTP request.

@autarch, what do you think?

[oalders]

To be clear, I'm fine with moving this logic outside of this package. I just want it to be easy to use.

[skaji]

+1 for HTTP::Response::is_success( strict => 1)

[autarch]

I think a new is_error method is better than a strict param, especially since it's not at all clear what "strict" means here. But I'm not sure that will help LWP::Simple, which just returns a status code.

[eserte]

What about $resp->is_complete_success?

[oalders]

What about $resp->is_complete_success?

👍

[oalders]

Something like $res->is_complete_success wouldn't exclude also adding an is_error method, I think. I could see how both would be helpful.

[karenetheridge]

I'm concerned about the introduction of extra warnings into users' code. Since this sub is new anyway, we can make it do anything we want -- it might be more clear if we returned false when the X-Died header was set?

[oalders]

@karenetheridge right, that's what we're talking about here. You can ignore this PR. We're talking about fixing this in HTTP::Response.

[vanHoesel]

@karenetheridge right, that's what we're talking about here. You can ignore this PR. We're talking about fixing this in HTTP::Response.

I don't see any resolution in HTTP::Response ...

what about ->warning, such that it can be used to set it or return the warning message if set, otherwise undef

[grr]

It doesn't look like this will be resolved anytime soon. How about updating the docs with an example of how to check for 'X-Died'. Something like:

if (! $res->is_success or defined(my $msg = $res->header('X-Died'))) {
    $res->message($msg) if defined $msg;
    die $res->status_line;
}

and maybe offer this simplified solution so users don't have to modify all their existing error checks:

$ua->set_my_handler(response_done => sub {
    my ($res, $ua, $handler) = @_;
    if (defined(my $msg = $res->header('X-Died'))) {
        $res->code(499);
        $res->message($msg);
    }
});

[oalders]

How about updating the docs with an example of how to check for 'X-Died'

Sure! We would accept a doc patch for this.