Handle Elastica Transport errors #1465

jandom · 2018-10-28T14:09:42Z

This PR attempts to address problem described in ruflin/Elastica#1508

Transport errors don't throw exceptions but are handled by ruflin/Elastica, however FOSElasticaBundle was never checking if there was a transport error detected by ruflin/Elastica

This PR goes hand-in-hand with the PR here, which improves NullTransport in ruflin/Elastica

ruflin/Elastica#1529

…dn't connect to ES host

…/Elistica

jandom · 2018-11-04T20:10:39Z

Updated the repo to not point at my personal fork, tests should pass now

jandom · 2018-11-04T20:16:12Z

Hmm almost cleaned that up – any idea how to make php-5.6 tests to pass? do i have to back-port into the 5.x branch?

XWB · 2018-11-19T20:40:40Z

composer.json

@@ -19,7 +19,7 @@
        "symfony/property-access": "^3.2|^4",
        "pagerfanta/pagerfanta": "^1.0.5|^2.0",
        "psr/log": "^1.0",
-        "ruflin/elastica": "^5.2.1|^6.0"
+        "ruflin/elastica": "dev-master"


We would like to keep stable packages.

Completely agreed, alas last release from ruflin/elastica is from waaay back

Release 6.0.2 @ruflin ruflin released this on May 29 · 8 commits to master since this release

See link for reference https://github.com/ruflin/Elastica/releases

And the commit that's needed for this PR, is a fairly recent one on master

ruflin/Elastica@d6f1475

I'm not sure what's the best way around this? ask ruflin to release a new version?

@jandom Is a new release needed from master?

@ruflin nothing would make me more happy – but i'm not sure what your release cycle is like/don't want to bother if you're waiting for something else to merge in

@ruflin yeah let's have a new ruflin/Elastica release, if that's okay – it'll push this PR forward

Next week is totally appropriate

@ruflin super-gentle nudge if there are any updates on the new release of ruflin/Elastica?

Thanks for the nudge ;-) Just opened the PR to prepare the release. Now I only need a review for it: ruflin/Elastica#1547

@jandom This should help: https://github.com/ruflin/Elastica/releases/tag/6.1.0

Yaaas thank you!

XWB · 2018-11-19T20:42:16Z

src/Elastica/Client.php

@@ -24,6 +24,8 @@
 */
 class Client extends BaseClient
 {
+    private $forbiddenHttpCodes = [403, 404, 400];


We should make this configurable.

Agreed, let me implement this and add some tests

Please note that by intercepting 404 you break various exists functions in Elastica which send HEAD requests and check for 200 vs 404 responses.

Elastica\Type::exists()

Elastica\Index::exists()

Elastica\IndexTemplate::exists()

Put in configuration instead of hard-coding in the Client class Unit test the configuration Defaults are 400, 403, 404 Refactor unit tests and mocks for the Client

XWB · 2018-12-11T11:59:57Z

src/Elastica/Client.php

+        if (isset($transportInfo['http_code']) && in_array($transportInfo['http_code'], $forbiddenHttpCodes, true)) {
+            $body = isset($transportInfo['body']) ? $transportInfo['body'] : 'blank';
+            $message = sprintf('Error in transportInfo: response code is %s, response body is %s', $transportInfo['http_code'], $body);
+            throw new \Exception($message);


For debugging reasons we should throw a different exception. Maybe one of https://github.com/ruflin/Elastica/tree/master/lib/Elastica/Exception ? @ruflin ?

SGTM. The part I'm worried here is that this could be a breaking change in Elastica? Want to open a PR against Elastica to discuss it there?

Changed the exception from the global Exception to ClientException, good point!

Gents, thank you both so much for your help and let me know if something more is needed

Update unit test

XWB · 2018-12-11T15:20:12Z

@jandom The latest changes are OK, but some tests are still failing:

Fatal error: Call to undefined method Elastica\Transport\NullTransport::setResponse() in /home/travis/build/FriendsOfSymfony/FOSElasticaBundle/tests/Unit/Elastica/ClientTest.php on line 38

jandom · 2019-01-13T12:34:34Z

Yeah, you're right – let me have a look

jandom · 2019-01-13T12:54:45Z

okay, i think i know what's going on – the test fail with the "prefer lowest" which causes the ruflin/elastica version 5.2.1 to be installed. The version obviously doesn't have the improvements I shipped – these are only present in the 6.x branch – I'd have to backport my changes into the other branch.

@XWB is that what has to get done to get this over the line?

jandom · 2019-01-13T20:53:53Z

Backported the changes to ruflin/Elastica 5.x branch – as far as I can tell this is the only way to get the CI green here

ruflin/Elastica#1592

jandom · 2019-01-22T20:46:57Z

Here is some discussion that lead to the latest commits ruflin/Elastica#1592

src/Elastica/Client.php

XWB · 2019-01-25T11:05:49Z

@jandom PR looks good. If @ruflin is willing to make another release then we can bump the minimum Elastica dependency to 5.3.5.

jandom · 2019-01-26T15:19:23Z

@XWB awesome – thanks for guiding the process!

@ruflin if it's not a problem, we'll be all merry and dancing here as soon as the versions are bumped on Elastica master and 5.x branches ;-)

ruflin · 2019-01-28T09:35:13Z

As @thePanz handles mostly 5.x release I'll ping him here. On the master side I think we can to a 6.1.1 (@p365labs objects?).

@jandom I assume that would solve your issues here?

jandom · 2019-01-28T10:22:09Z

@ruflin thanks so much – yes that is correct!

thePanz · 2019-01-28T10:42:43Z

@jandom I prepared the changes for the new release v5.3.5. as soon as I'll get @ruflin OK, I'll tag it.
Get ready for your dances ;)

@jandom merged, and tagged 5.3.5. (nice gif 🤣 )

jandom · 2019-01-28T10:50:31Z

ruflin · 2019-01-29T09:39:12Z

Here you go with the 6.1.1 release: https://github.com/ruflin/Elastica/releases/tag/6.1.1

jandom · 2019-01-29T11:19:17Z

@ruflin thank you so much #SparklesOfJoy #HappyTimes

jandom · 2019-01-29T11:21:49Z

Updated the composer.json with the new dependency tags

XWB · 2019-01-29T14:48:06Z

Thanks for all the effort guys :)

jandom · 2019-01-29T18:28:37Z

Wooohooo, thanks to @XWB for closing 🎉

jandom added 2 commits October 28, 2018 00:02

Elastica/Client uses getTransportInfo and throws an exception if coul…

be5d9fe

…dn't connect to ES host

Pass errored response body in transportInfo

f792d6d

jandom mentioned this pull request Oct 29, 2018

Throw exception on 403 response ruflin/Elastica#1508

Closed

Revert changes is composer.json - no longer point to a fork of ruflin…

2607d66

…/Elistica

XWB requested changes Nov 19, 2018

View reviewed changes

ruflin mentioned this pull request Dec 6, 2018

Prepare for 6.1.0 release ruflin/Elastica#1547

Merged

jandom added 2 commits December 9, 2018 22:10

Switch to ruflin/elastica 6.1 in composer.json

543e31f

Factor out the forbidden HTTP codes

e5386ea

Put in configuration instead of hard-coding in the Client class Unit test the configuration Defaults are 400, 403, 404 Refactor unit tests and mocks for the Client

XWB reviewed Dec 11, 2018

View reviewed changes

Switch from Exception to Elastica\Exception\ClientException

3d5e889

Update unit test

jandom mentioned this pull request Jan 13, 2019

Implement an ErrorTransport (#1529) ruflin/Elastica#1592

Merged

jandom added 2 commits January 19, 2019 11:18

Merge branch 'master' into issue/1508-aws-credentials-403-errors

5cc527d

Refactor following discussion in ruflin/Elastica

7f3de79

XWB requested changes Jan 25, 2019

View reviewed changes

src/Elastica/Client.php Outdated Show resolved Hide resolved

Remove double carrige return

40848c0

Handle responseData being a string or an array

934f812

Update composer.json

92e089f

XWB approved these changes Jan 29, 2019

View reviewed changes

XWB changed the title ~~Issue/1508 aws credentials 403 errors~~ Handle Elastic Transport errors Jan 29, 2019

XWB changed the title ~~Handle Elastic Transport errors~~ Handle Elastica Transport errors Jan 29, 2019

XWB merged commit 8b30ba6 into FriendsOfSymfony:master Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Elastica Transport errors #1465

Handle Elastica Transport errors #1465

jandom commented Oct 28, 2018

jandom commented Nov 4, 2018

jandom commented Nov 4, 2018

XWB Nov 19, 2018

jandom Nov 25, 2018

ruflin Nov 27, 2018

jandom Nov 27, 2018

jandom Nov 28, 2018

jandom Nov 29, 2018

jandom Dec 5, 2018

ruflin Dec 6, 2018

ruflin Dec 7, 2018

jandom Dec 9, 2018

XWB Nov 19, 2018

jandom Nov 25, 2018

NathanBaulch Mar 13, 2019

XWB Dec 11, 2018

ruflin Dec 11, 2018

jandom Dec 11, 2018

XWB commented Dec 11, 2018

jandom commented Jan 13, 2019

jandom commented Jan 13, 2019 •

edited

jandom commented Jan 13, 2019

jandom commented Jan 22, 2019

XWB commented Jan 25, 2019

jandom commented Jan 26, 2019

ruflin commented Jan 28, 2019

jandom commented Jan 28, 2019

thePanz commented Jan 28, 2019 •

edited

jandom commented Jan 28, 2019

ruflin commented Jan 29, 2019

jandom commented Jan 29, 2019

jandom commented Jan 29, 2019

XWB commented Jan 29, 2019

jandom commented Jan 29, 2019

Handle Elastica Transport errors #1465

Handle Elastica Transport errors #1465

Conversation

jandom commented Oct 28, 2018

jandom commented Nov 4, 2018

jandom commented Nov 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XWB commented Dec 11, 2018

jandom commented Jan 13, 2019

jandom commented Jan 13, 2019 • edited

jandom commented Jan 13, 2019

jandom commented Jan 22, 2019

XWB commented Jan 25, 2019

jandom commented Jan 26, 2019

ruflin commented Jan 28, 2019

jandom commented Jan 28, 2019

thePanz commented Jan 28, 2019 • edited

jandom commented Jan 28, 2019

ruflin commented Jan 29, 2019

jandom commented Jan 29, 2019

jandom commented Jan 29, 2019

XWB commented Jan 29, 2019

jandom commented Jan 29, 2019

jandom commented Jan 13, 2019 •

edited

thePanz commented Jan 28, 2019 •

edited