-
Notifications
You must be signed in to change notification settings - Fork 733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement an ErrorTransport (#1529) #1592
Conversation
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
@@ -93,6 +94,7 @@ public function exec(Request $request, array $params) | |||
[ | |||
'request_header' => $request->getMethod(), | |||
'http_code' => $res->getStatusCode(), | |||
'body' => $responseBody, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could have some side effects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruflin it should break anything, i'm doing some tests on that and then I let u know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"}}]}}
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we get a follow up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $responseData = $response->getData();
can be used instead of storing twice the body contents.
CHANGELOG.md
Outdated
@@ -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` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in this PR?
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a link to the PR at the end of the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done – let me know if that's what you had in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both options work for me, whatever you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning this up gents, super-fast work!
This PR is currently blocking FriendsOfSymfony/FOSElasticaBundle#1465 |
@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. |
@thePanz a quick look would save the day ;-) |
I started reviewing it, no comments so far (just a minor about the
changelog ;) )
|
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 |
++ on cleaning it up first. |
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