Implement an ErrorTransport (#1529) #1592

jandom · 2019-01-13T14:38:49Z

This is backport of ErrorTransport onto the 5.x branch, all tests pass for me locally

Need this PR merged to close another one FriendsOfSymfony/FOSElasticaBundle#1465

Also requesting a version bump on 5.x otherwise I won't be able to configure composer.json correctly in FOSElasticaBundle

When using AwsAuthV4 transport, it is possible that the user provides incorrect credentials.

This means that the Guzzle client will get a 403 from AWS.

By default Guzzle silently ignores all exceptions and stores them in `$response->setTransferInfo()`

This feature allows a downstream client/library to handle them however.

Currently, FOSElastica does nothing to process 403 or other errors that come back from ruflin/Elastica (transferInfo is never used).

This causes a problem because 403 are silently ignored and the progress bar appears to be moving unhindered ;-)

This PR is a pre-requisite for unit tests that will accompany this fix on FOSElastica
# Conflicts:
#	test/Elastica/Transport/NullTransportTest.php

When using AwsAuthV4 transport, it is possible that the user provides incorrect credentials. This means that the Guzzle client will get a 403 from AWS. By default Guzzle silently ignores all exceptions and stores them in `$response->setTransferInfo()` This feature allows a downstream client/library to handle them however. Currently, FOSElastica does nothing to process 403 or other errors that come back from ruflin/Elastica (transferInfo is never used). This causes a problem because 403 are silently ignored and the progress bar appears to be moving unhindered ;-) This PR is a pre-requisite for unit tests that will accompany this fix on FOSElastica # Conflicts: # test/Elastica/Transport/NullTransportTest.php

ruflin · 2019-01-14T12:45:51Z

lib/Elastica/Transport/Guzzle.php

@@ -93,6 +94,7 @@ public function exec(Request $request, array $params)
            [
                'request_header' => $request->getMethod(),
                'http_code' => $res->getStatusCode(),
+                'body' => $responseBody,


I wonder if this could have some side effects?

No idea – it's my first contribution to this package. Don't think anything in FOSElastica is trying to read this (if i remember correctly)

@ruflin it should break anything, i'm doing some tests on that and then I let u know.

i had a look a bit more deeply, and if you look the value of $info here
the value contains also body.

( [request_header] => GET [http_code] => 200 [body] => {"took":4,"timed_out":false,"_shards":{"total":5,"successful":5,"skipped":0,"failed":0},"hits":{"total":1,"max_score":0.2876821,"hits":[{"_index":"dynamic_http_method_test","_type":"_doc","_id":"1","_score":0.2876821,"_source":{"test":"test"}}]}} )

@p365labs Not sure I can follow. So this is good?

In general it's just a gut feeling I have that the above could impact other things. If we don't find something I'm good with moving forward.

The problem I see here: the whole response will be stored twice, first as the response itself, and the second time in the TransferInfo object. When fetching huge responses from ES, this could be a significant overhead.
@jandom : which is your use-case? why do you need the whole JSON response body here too?

Ha, you're right – i'm doing something completely stupid here!

The use-case was to pass the body to have useful error messages (in case the AwsAuth is passed incorrect credentials) in FOSElasticaBundle

Can totally factor this bit out

Does this mean we get a follow up PR?

@jandom if I got it correctly, you're using the body to display the response received from the client. In here: https://github.com/FriendsOfSymfony/FOSElasticaBundle/pull/1465/files/e5386eac9a4b1ee22d7d215ebe32afc5fec046af#diff-dbbc2aef2d5d1dd159ad264fb54364dcL51, why don't you keep the response data and use it in case of an exception?

The $responseData = $response->getData(); can be used instead of storing twice the body contents.

ruflin · 2019-01-14T12:46:04Z

CHANGELOG.md

@@ -10,6 +10,9 @@ All notable changes to this project will be documented in this file based on the

 ### Added

+* Added a transport class for mocking a HTTP 403 error codes, useful for testing response failures in inheriting clients
+* [Field](https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-function-score-query.html#function-random) param for `Elastica\Query\FunctionScore::addRandomScoreFunction`


Is this in this PR?

ruflin · 2019-01-14T12:46:18Z

CHANGELOG.md

@@ -10,6 +10,9 @@ All notable changes to this project will be documented in this file based on the

 ### Added

+* Added a transport class for mocking a HTTP 403 error codes, useful for testing response failures in inheriting clients


Could you add a link to the PR at the end of the line?

Done – let me know if that's what you had in mind

Looks like the comment has been added twice, we already had a backport and usually we just mention the backport PR. Don't know if we have any "standard" here, but those 2 log entries are confusing IMHO

In terms of chronology, first came the PR into master – then the PR into the backward compatibility branch. Happy to go either way here – this is super easy

Thus, we did not add 2 new features, just backported something. In this case I'd like to have the [backport] ... log message, pointing to this PR. WDYT @ruflin ?

Both options work for me, whatever you prefer.

Thanks for cleaning this up gents, super-fast work!

jandom · 2019-01-14T23:04:53Z

This PR is currently blocking FriendsOfSymfony/FOSElasticaBundle#1465

ruflin · 2019-01-16T08:08:37Z

@thePanz Perhaps you could also have a quick look at this PR as you were rather active on the 5.x branch recently. I just want to make sure we don't break anything here.

jandom · 2019-01-19T09:46:52Z

@thePanz a quick look would save the day ;-)

ruflin · 2019-01-21T12:21:33Z

@jandom Merged for now. 🤞 that it didn't cause any issues.

@thePanz Feedback still welcome ;-)

thePanz · 2019-01-21T12:27:40Z

I started reviewing it, no comments so far (just a minor about the changelog ;) )

jandom · 2019-01-21T14:22:45Z

Thanks for the merge @ruflin and for the review @thePanz

One last quick request – can we get a new minor version published @ruflin ? Otherwise i'm unable to close FriendsOfSymfony/FOSElasticaBundle#1465 - composer.json has to contain a stable version for maintainers to merge it in

ruflin · 2019-01-22T09:16:59Z

I leave it to @thePanz to decide if we need a follow up / improvement PR or not. If not, @thePanz feel free to cut a release.

thePanz · 2019-01-22T11:50:07Z

@ruflin @jandom let's continue this discussion on #1595
I'd like to clean this up, before tagging a new release.
Btw: the body was only added to the Guzzle implementation, and we missed 2 other clients (HttpClient, ad an instance).

ruflin · 2019-01-22T12:33:42Z

@thePanz Ups, I think I was too fast here. Already merged #1595 :-(

ruflin · 2019-01-22T12:33:47Z

++ on cleaning it up first.

#1592 (#1596) - update CHANGELOG, link to PR - remove 'body' from TransferInfo

jandom mentioned this pull request Jan 13, 2019

Handle Elastica Transport errors FriendsOfSymfony/FOSElasticaBundle#1465

Merged

ruflin reviewed Jan 14, 2019

View reviewed changes

jandom added 2 commits January 14, 2019 23:01

Update CHANGELOG.md

45b69ae

Update CHANGELOG.md

7aa541e

ruflin merged commit f012d97 into ruflin:5.x Jan 21, 2019

thePanz mentioned this pull request Jan 22, 2019

Fix PR-1592 #1595

Merged

2 tasks

jandom deleted the backport/error-transport branch January 22, 2019 19:59

p365labs pushed a commit that referenced this pull request Jan 23, 2019

remove 'body' from TransferInfo on Guzzle Transport - Following on from

b300234

#1592 (#1596) - update CHANGELOG, link to PR - remove 'body' from TransferInfo

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement an ErrorTransport (#1529) #1592

Implement an ErrorTransport (#1529) #1592

jandom commented Jan 13, 2019

ruflin Jan 14, 2019

jandom Jan 14, 2019

p365labs Jan 15, 2019

p365labs Jan 15, 2019

ruflin Jan 15, 2019

thePanz Jan 21, 2019

jandom Jan 21, 2019 •

edited

ruflin Jan 22, 2019

thePanz Jan 22, 2019

thePanz Jan 22, 2019

ruflin Jan 14, 2019

ruflin Jan 14, 2019

jandom Jan 14, 2019

thePanz Jan 21, 2019

jandom Jan 21, 2019

thePanz Jan 22, 2019

ruflin Jan 22, 2019

jandom Jan 22, 2019

jandom commented Jan 14, 2019

ruflin commented Jan 16, 2019

jandom commented Jan 19, 2019

ruflin commented Jan 21, 2019

thePanz commented Jan 21, 2019 via email

jandom commented Jan 21, 2019

ruflin commented Jan 22, 2019

thePanz commented Jan 22, 2019 •

edited

ruflin commented Jan 22, 2019

ruflin commented Jan 22, 2019

		@@ -10,6 +10,9 @@ All notable changes to this project will be documented in this file based on the

		### Added

		* Added a transport class for mocking a HTTP 403 error codes, useful for testing response failures in inheriting clients

Implement an ErrorTransport (#1529) #1592

Implement an ErrorTransport (#1529) #1592

Conversation

jandom commented Jan 13, 2019

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

jandom Jan 21, 2019 • edited

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

jandom commented Jan 14, 2019

ruflin commented Jan 16, 2019

jandom commented Jan 19, 2019

ruflin commented Jan 21, 2019

thePanz commented Jan 21, 2019 via email

jandom commented Jan 21, 2019

ruflin commented Jan 22, 2019

thePanz commented Jan 22, 2019 • edited

ruflin commented Jan 22, 2019

ruflin commented Jan 22, 2019

jandom Jan 21, 2019 •

edited

thePanz commented Jan 22, 2019 •

edited