Bug #95796 for MetaCPAN-Client: $AUTHOR->releases() ignores UA passed to MetaCPAN::Client->new()

Tue May 20 07:38:28 2014 KENTNL [...] cpan.org - Ticket created

Subject:

$AUTHOR->releases() ignores UA passed to MetaCPAN::Client->new()

https://metacpan.org/source/XSAWYERX/MetaCPAN-Client-1.003000/lib/MetaCPAN/Client/Author.pm#L32

This means:

my $client = MetaCPAN::Client->new( ua => $cachingua );
my $author = $client->author("KENTNL");
$author->releases();

Completely ignores the caching, so you get a cached response for ->author(), but not a cached response from ->releases()

As evidenced by testing both with, and without a cache, for me, both take a full 9 seconds.

However, then your still having it not cache due to using Search::ElasticSearch, which has its own UA.

After a bit of source diving I discoved you can do:

    my $es = Search::Elasticsearch->new(
        nodes            => $self->domain,
        cxn_pool         => 'Static::NoPing',
        send_get_body_as => 'POST',
        handle           => $self->ua,
    );

And upon doing this, it becomes much more obvious that it's processing requests through the caching UA.

Though it doesn't seem to give much performance benefit to cache for some reason.

Tue May 20 07:42:04 2014 KENTNL [...] cpan.org - Correspondence added

Show quoted text

> Though it doesn't seem to give much performance benefit to cache for
> some reason.

Scratch that, for some reason unknown to me, after I filed the bug report, my the initial POST request started to respond to caching ( due to my ES hack ), cutting the slow 5 seconds request down to 0.5 s :)

Tue May 20 07:47:06 2014 XSAWYERX [...] cpan.org - Ticket deleted

Tue Jun 10 03:52:38 2014 KENTNL [...] cpan.org - Correspondence added

On 2014-05-20 23:42:04, KENTNL wrote:
> > Though it doesn't seem to give much performance benefit to cache for
> > some reason.
>
> Scratch that, for some reason unknown to me, after I filed the bug
> report, my
> the initial POST request started to respond to caching ( due to my ES
> hack ),
> cutting the slow 5 seconds request down to 0.5 s :)

I didn't mean to say "This entire ticket is invalid".

But instead, I meant to say "After making certain changes I was under the impression the changes weren't sufficient to make caching work, however, I was wrong, my changes *WERE* sufficient to make caching happen".

I'm very much certain that the custom value of UA is still not passed to the infrastructure where it needs to be, and ES doesn't get passed that UA

I'll take another shot at demonstrating it though.

Tue Jun 10 03:52:39 2014 KENTNL [...] cpan.org - Status changed from 'deleted' to 'open'

Tue Jun 10 04:49:19 2014 KENTNL [...] cpan.org - Correspondence added

Here's the smallest test I have ATM that demonstrates the value of UA is useless for at least one query:

https://github.com/kentfredric/metacpan-client/blob/master/t/ua_trap.t

It fails because somewhere in the stack, it gets an HTTP::Tiny object and uses that to do calls instead of the chosen UA object. I'd expand on the test so the static data it uses was actually useful, but its pointless because the code is such presently it can never get that far.

Subject:

ua_trap.t

use strict; use warnings; use Test::More; # ABSTRACT: Make sure passed value of UA gets used for things. { package TrapUA; use Data::Dump qw(pp); use JSON; sub new { my ( $class, @args ) = @_; return bless { args => [@args] }, $class; } my $GET = { 'http://api.metacpan.org/v0/author/KENTNL' => { visits => 0, payload => { success => 1, content => JSON->new->encode( { name => 'Kent Fredric', dir => 'id/K/KE/KENTNL', pauseid => 'KENTNL', } ), }, } }; sub get { my ( $self, $uri, @rest ) = @_; if ( not exists $GET->{$uri} ) { die "No cached value for $uri"; } $GET->{$uri}->{visits}++; return $GET->{$uri}->{payload}; } } { require HTTP::Tiny; use Data::Dump qw(pp); no warnings "redefine"; *HTTP::Tiny::request = sub { my ( $self, @args ) = @_; die "Illegal use of HTTP::Tiny" . pp( \@args ); }; } use MetaCPAN::Client; my $client = MetaCPAN::Client->new( ua => TrapUA->new() ); my $a = $client->author('KENTNL'); my $releases = $a->releases; done_testing;

Fri Jul 25 23:30:04 2014 olaf [...] wundersolutions.com - Correspondence added

On Tue Jun 10 04:49:19 2014, KENTNL wrote: Show quoted text

> Here's the smallest test I have ATM that demonstrates the value of UA > is > useless for at least one query: > > https://github.com/kentfredric/metacpan-client/blob/master/t/ua_trap.t > > It fails because somewhere in the stack, it gets an HTTP::Tiny object > and uses > that to do calls instead of the chosen UA object. I'd expand on the > test so the > static data it uses was actually useful, but its pointless because the > code is > such presently it can never get that far.

I ran up against this just now too. https://metacpan.org/source/MICKEY/MetaCPAN-Client-1.006000/lib/MetaCPAN/Client/Author.pm#L28 creates a new MetaCPAN::Client object instantiated without the UserAgent which was passed to the original object. This is an easy fix, assuming there is not some specific reason for ignoring the UserAgent.

Wed Aug 13 15:47:20 2014 olaf [...] wundersolutions.com - Correspondence added

On Fri Jul 25 23:30:04 2014, OALDERS wrote: Show quoted text

> I ran up against this just now too. > https://metacpan.org/source/MICKEY/MetaCPAN-Client- > 1.006000/lib/MetaCPAN/Client/Author.pm#L28 creates a new > MetaCPAN::Client object instantiated without the UserAgent which was > passed to the original object. This is an easy fix, assuming there is > not some specific reason for ignoring the UserAgent.

After looking into this more, I think this is possible, but maybe not ideal. For example, GET requests for authors use the supplied UA, but Elasticsearch then creates its own UA, so we have no control over scrolled searches. We'd need to specify a custom 'cx' class for Elasticsearch.pm That class would need to subclass Search::Elasticsearch::Cxn::HTTPTiny and (probably) override the _build_handle() method, which is not hard but probably not a good idea either. I'm going to ask Clinton Gormley for some feedback on this.

Wed Aug 13 16:03:26 2014 olaf [...] wundersolutions.com - Correspondence added

kentnl just pointed out to me that I didn't read his initial comments with the appropriate attention to detail. What he has proposed is, I think, a good idea less messing around than what I was suggesting. We could do this in only the cases where the user has supplied her own UserAgent, so the caveat emptor would apply only to those people who are already meddling with things.

Thu Aug 14 22:40:12 2014 KENTNL [...] cpan.org - Fixed in 1.007000 added

Thu Aug 14 22:40:12 2014 KENTNL [...] cpan.org - Correspondence added

This should be fixed now in 1.007

Thu Aug 14 22:40:43 2014 KENTNL [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #95796 for MetaCPAN-Client: $AUTHOR->releases() ignores UA passed to MetaCPAN::Client->new()

Preferred bug tracker