kaare / role-rest-client Goto Github PK

Role::REST::Client appears to have the intent to support other user agents besides HTTP::Tiny, but in practice it requires HTTP::Tiny's uncommon call signature to the request() method for compatibility.

Role::REST::Client also contains some code to handle the case where a user agent returns a hashref like HTTP::Tiny vs the more common HTTP::Request objects.

I propose updating the internals to use HTTP::Thin as the default user agent instead of HTTP::Tiny. HTTP::Thin sub-classes HTTP::Tiny and just wraps it's request method, so that it can accept a HTTP::Request object as input, like LWP::UserAgent-compatible user agents, and it also returns a HTTP::Response object like LWP::UserAgent-compatible agents.

This has a couple of advantages, while still retaining the bulk of performance and simplicity of HTTP::Tiny:

1. All LWP::Useragent-compatible agents can now be used as alternatives for those that want or need them.
2. A bit of internal code can be simplified as we can now expect all user agents to follow the convention of returning an HTTP::Response object directly.

@kaare: What do you think?

LWP::UserAgent considers 4xx responses as "errors", but here only 5xx responses are considered to be "failed". Is that intentional? Here's how HTTP response codes map to statuses from HTTP::Status:

sub is_success      ($) { $_[0] >= 200 && $_[0] < 300; }
sub is_redirect     ($) { $_[0] >= 300 && $_[0] < 400; }
sub is_error        ($) { $_[0] >= 400 && $_[0] < 600; }
sub is_client_error ($) { $_[0] >= 400 && $_[0] < 500; }
sub is_server_error ($) { $_[0] >= 500 && $_[0] < 600; }

I think I agree with LWP here. If the server responds with a '404 Not Found' or '403 Forbidden', that I would consider the API request to have failed.

If the API provider specifies the wrong Content-Type, Role::REST::Client will emit a warning and presume 'application/json' is correct. The warning can avoided by passing an explicit deserializer to every API call.

I'm dealing with iContacts API, which brokenly returns "text/html" when they send JSON. I would like to be able to specify the deserializer I want to use in one place, rather than in every call.

Today I ran into a problem, so I checked $res->error as the docs suggest. It told me the problem was:

Internal Exception

... which wasn't so helpful. By dumping the whole response object, I found the helpful response was located in $res->response->content:

IO::Socket::SSL 1.56 must be installed for https support

Ah! Helpful! So, I suggest this be included all the time, or stronger hints be added about looking here in case of an error.

Some examples:

Internal Exception: IO::Socket::SSL 1.56 must be installed for https support

Or in the POD:

=head2 error

Returns the returned reason from HTTP::Tiny where the status is 500 or higher. For more detail, check C<< $res->response->content >>.

Commit 5a4b8cd fixed an annoying issue. Any chance you could get a new release out? Thanks!

I think there is a typo in your email address in POD. It ends with ".net", but should be ".org" probably. I tried sending the mail, before finding the GitHub repo and it bounced.

Thanks! :)

The docs show these pieces of the URI:

server =>      'http://localhost:3000',

and

my $res = $self->post('foo/bar/baz', {foo => 'bar'});

But they are actually combined in the source code like this:

my $uri = $self->server.$endpoint;

So, if someone followed the documentation, they would end up with an invalid request like this:

http://localhost:3000foo/bar/baz

I'm sure how you want to resolve this. Adding a leading "/" to the documented examples would mean the code remained backwards compatible and would also add a signifier that these arguments are URI fragments, since "foo" looks like just a word, while "/foo" looks like it might be part of a URL.

Thansk for this module. Here's some feedback on it.

The docs contain this statement:

"An UA object which can do ->request method, for instance: HTTP::Tiny, LWP::UserAgent, etc."

This sounds like LWP::UserAgent could work suiltably well, but does not. Simply having a request() method turns out not to be enough. The User Agent has accept the same arguments to request() as HTTP::Tiny does.

Role::REST::Client calls the request() method like this:

$self->user_agent->request($method, $uri, $opts);

However, the method signature for LWP::UserAgent is rather different:

$ua->request( $request, $content_cb, $read_size_hint )

I'm not sure how you want to resolve this. One option is drop support for LWP::UserAgent, updating the docs to clarify that any alternative user agent must have a request() method that is compatible with HTTP::Tiny.

Alternatively, you could explicitly support LWP::UserAgent.

Hi, I have a server (which I do not control) which expects some certain body data on a DELETE call.
Currently, I use

$self->_request_with_body("DELETE", $path, $data);

Do you have any better idea? Should this be supported by Role::REST::Client?
According to a Stackoverflow Question it is allowed to have a body on DELETE.

I'm dealing with an API that returns application/json, charset=UTF-8 as content type. It is normally a semi colon, and the module correctly handles that, but the comma confuses. Need to modify the regex here:

https://github.com/kaare/Role-REST-Client/blob/master/lib/Role/REST/Client.pm#L129

Thanks!

This is probably the same as https://rt.cpan.org/Public/Bug/Display.html?id=81803,
but I was able to reduce test to this: http://paste.scsys.co.uk/236176

You probably need to run it more than once to see failure.

Hey, I need my deserialized object in a specific format. Therefore I need to have serializer set-up like follows:

my $s = Data::Serializer::Raw->new(
    serializer => 'XML::Simple',
    options    => { RootName => 'object' } );

Could there be an option to add custom config options to the serializer object?

Hi, I am currently doing as shown below, to use LWP::UserAgent as a user-agent for Role::REST::Client. (Basically, I am making the request method compatible to the style required by Role::REST::Client. The result is an ordinary HTTP::Response and therefore already supported.)

Do you have an idea on how to incorporate its interface into Role::REST::Client so it can be used without glue code?

{
    package LWP_Wrapper;
    use Moo;
    use HTTP::Request;
    use HTTP::Headers;
    extends 'LWP::UserAgent';

    around request => sub {
            my ($next, $self, $method, $uri, $opts) = @_;
            my @headers;
            if ($opts->{headers}) {
                @headers = HTTP::Headers->new(%{ $opts->{headers} });
            }
            my $req = HTTP::Request->new($method, $uri, @headers);
            if ($opts->{content}) {
                $req->content($opts->{content});
            }
            my $res = $self->$next($req);
            return $res;
    };
}

sub _build_user_agent {
    my $ua = LWP_Wrapper->new(
            ssl_opts => { verify_hostname => 0 },
            timeout => 20,
        );
    return $ua;
}