Search::Elasticsearch 5.x compatibility

[pghmcfc]

As a follow-on from #48, I have got my own usage of MetaCPAN::Client working again in conjunction with Search::Elasticsearch 5.x by installing Search-Elasticsearch-Client-2_0 and making the following changes in MetaCPAN::Client:

diff -up /usr/share/perl5/vendor_perl/MetaCPAN/Client/Request.pm.orig /usr/share/perl5/vendor_perl/MetaCPAN/Client/Request.pm
--- /usr/share/perl5/vendor_perl/MetaCPAN/Client/Request.pm.orig
+++ /usr/share/perl5/vendor_perl/MetaCPAN/Client/Request.pm
@@ -129,6 +129,7 @@ sub ssearch {
     my $params = shift;
 
     my $es = Search::Elasticsearch->new(
+        client           => '2_0::Direct',
         nodes            => [ 'https://' . $self->domain ],
         cxn_pool         => 'Static::NoPing',
         send_get_body_as => 'POST',
diff -up /usr/share/perl5/vendor_perl/MetaCPAN/Client/ResultSet.pm.orig /usr/share/perl5/vendor_perl/MetaCPAN/Client/ResultSet.pm
--- /usr/share/perl5/vendor_perl/MetaCPAN/Client/ResultSet.pm.orig
+++ /usr/share/perl5/vendor_perl/MetaCPAN/Client/ResultSet.pm
@@ -22,7 +22,7 @@ has scroller => (
     is        => 'ro',
     isa       => sub {
         use Safe::Isa;
-        $_[0]->$_isa('Search::Elasticsearch::Scroll')
+        $_[0]->$_isa('Search::Elasticsearch::Client::2_0::Scroll')
             or croak 'scroller must be an Search::Elasticsearch::Scroll object';
     },
     predicate => 'has_scroller',

I've tried it successfully with both the v0 and v1 APIs. I haven't tried the 5_0 or older clients to see if they would also work. Obviously this is just a proof of concept and a proper fix would probably entail supporting different client versions, but that's currently beyond my coding ability.

[pghmcfc]

Actually I did try the 5_0 client because that's the default in Search::Elasticsearch, and that was what led to me raising #48 in the first place. So the 2_0 client looks like the best bet for now.

[mickeyn]

Thanks @pghmcfc

I know the 2_0 client works for v1, and though it does seem to work (not enough tests :)) for v0 - I can't trust it as v0 is ES 0.20 and not 2.x.

We can potentially use the 0_90 client conditionally for it (even though it's off by many releases as well) but after the migration of metacpan-api to ES v1 (coming soon) we'll have only one version to support and won't need this conditioning (and will be able to clean up some other version related conditions as well).

I'll keep this ticket open until after the migration and then get back to it.

[mickeyn]

a quick update: after the removal of 0.20 support I tried to upgrade and use the 2_0 client - doesn't work out of the box (failing tests), maybe something stupid but the point is that it's not fully backwards compatible and I'm reluctant of wasting time on it as I'm not yet convinced of any added value by this upgrade (please correct me if you see any).
what I have started looking into is the complete removal of this module as a dependency.

[pghmcfc]

The MetaCPAN-Client tests all work for me, with the following changes to the dist:

--- lib/MetaCPAN/Client/Request.pm
+++ lib/MetaCPAN/Client/Request.pm
@@ -128,6 +128,7 @@ sub ssearch {
     my $params = shift;
 
     my $es = Search::Elasticsearch->new(
+        client           => '2_0::Direct',
         nodes            => [ $self->base_url ],
         cxn_pool         => 'Static::NoPing',
         send_get_body_as => 'POST',
--- lib/MetaCPAN/Client/ResultSet.pm
+++ lib/MetaCPAN/Client/ResultSet.pm
@@ -22,8 +22,8 @@ has scroller => (
     is        => 'ro',
     isa       => sub {
         use Safe::Isa;
-        $_[0]->$_isa('Search::Elasticsearch::Scroll')
-            or croak 'scroller must be an Search::Elasticsearch::Scroll object';
+        $_[0]->$_isa('Search::Elasticsearch::Client::2_0::Scroll')
+            or croak 'scroller must be an Search::Elasticsearch::Client::2_0::Scroll object';
     },
     predicate => 'has_scroller',
 );
--- t/api/favorite.t
+++ t/api/favorite.t
@@ -15,6 +15,6 @@ foreach my $option ( { author => 'XSAWYE
     isa_ok( $rs, 'MetaCPAN::Client::ResultSet' );
     can_ok( $rs, qw<type scroller> );
     is( $rs->type, 'favorite', 'Correct resultset type' );
-    isa_ok( $rs->scroller, 'Search::Elasticsearch::Scroll' );
+    isa_ok( $rs->scroller, 'Search::Elasticsearch::Client::2_0::Scroll' );
 }
 
--- t/resultset.t
+++ t/resultset.t
@@ -9,7 +9,7 @@ use MetaCPAN::Client::ResultSet;
 
 {
     package MetaCPAN::Client::Test::ScrollerZ;
-    use base 'Search::Elasticsearch::Scroll'; # < 5.10 FTW (except, no)
+    use base 'Search::Elasticsearch::Client::2_0::Scroll'; # < 5.10 FTW (except, no)
     sub total {0}
 }
 
@@ -26,7 +26,7 @@ like(
 
 my $rs = MetaCPAN::Client::ResultSet->new(
     type     => 'author',
-    scroller => bless {}, 'Search::Elasticsearch::Scroll',
+    scroller => bless {}, 'Search::Elasticsearch::Client::2_0::Scroll',
 );
 
 isa_ok( $rs, 'MetaCPAN::Client::ResultSet' );

[mickeyn]

@pghmcfc that patch doesn't work for me, with it I get errors of the form:

t/api/author.t .......... 1/? [Param] ** Not a HASH reference at /usr/local/share/perl/5.22.1/Search/Elasticsearch/Role/Client/Direct.pm line 116. in (search) request. See docs at: http://www.elastic.co/guide/en/elasticsearch/reference/current/search-search.html, called from sub Search::Elasticsearch::Role::Client::Direct::Main::scroll_helper at /home/mickey/git_tree/metacpan-client/lib/MetaCPAN/Client/Request.pm line 146.# Tests were run but no plan was declared and done_testing() was not seen.

I'm sure it's solvable, but I'm yet not convinced there is a good enough reason to make the effort and deal with all the issues people will run into following this change.

Unfortunately, ES, and in this case ES module changes come with a lot of pain and breakage so they need to be justified - so far, supporting a new version that doesn't add features we need and is optimized for an ES version we don't use is not good enough for me.
I specifically don't like the last version of Search::Elasticsearch as it was released without warning with breaking changes that immediately broke every depending release on CPAN with a normal (default >=) prerequisite. therefore I rather remove it completely as a dependency rather than force users to upgrade to it (and basically break the client for who needs to keep the current version installed for no good reason, as it is the version matching the ES version we are using).

Having said that, if and when we move the API to 5.x it would make total sense to upgrade.
If you have any arguments in its favor - I'm open to be convinced I'm wrong.

[pghmcfc]

@mickeyn, from a downstream distributor point of view (I maintain the perl-MetaCPAN-Client package in Fedora), the current situation with the module requiring a not-latest version of S:ES is a problem because it's effectively blocking us from updating S:ES itself. I'd been looking at resolving this by being able to use the new API but removing the dependency works equally well from my point of view.

Are you going to clone the bits you need of the previous S:ES into your own namespace?

[mickeyn]

I understand the problem. but I'm afraid I can't think of a proper solution, as long as "latest" (breaking) version is not fully compatible I can't force every user to use it and then have to support every issue they encounter because of it.

As I cannot guarantee the latest version of S::Es (current or future) will work nicely with my code, I rather take the approach of removing it completely as a dependency (as I know others do, given the trouble they run into with ES).

I'm not cloning any of the code, but rather have created an internal scroller class for the client that will just do its own book-keeping and avoid using S:Es for that (we don't use most of its features anyway) -
it's just an API to an API, so I rather use that end API directly.

I hope this will resolve soon.

[ranguard]

@mickeyn is there a ticket in S::ES that we could link to, so once it is not broken we can follow up again once that is resolved, then @pghmcfc can chase that as well

[mickeyn]

@ranguard I opened one but it was closed. It was argued that setting the client will "just work"... so far I tried it and it doesn't.
see #64 for the direction I'm trying to take.

[pghmcfc]

Just for the record, I'm perfectly happy with a solution that removes S::ES as a dependency of MetaCPAN::Client; it resolves the issue of MetaCPAN::Client blocking S::ES updates (other dependencies of S::ES may also have issues, but that's not relevant here).

[mickeyn]

@pghmcfc the latest version (2.004000) of MetaCPAN::Client no longer uses Search::Elasticsearch so I guess this issue is resolved.
Can you please confirm this resolves the issue and close it?

[mickeyn]

BTW, thanks for bringing up this issue. I'm happy with the change it drove.

[pghmcfc]

@mickeyn, 2.004000 works fine for me.

[pghmcfc]

Oh, and you should be able to drop the EU::MM version requirement now you don't need fancy version range support.