Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement an ErrorTransport (#1529) #1592

Merged
merged 3 commits into from Jan 21, 2019
Merged

Implement an ErrorTransport (#1529) #1592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38ca2ce
Implement an ErrorTransport (#1529)
jandom Jan 13, 2019
45b69ae
Update CHANGELOG.md
jandom Jan 14, 2019
7aa541e
Update CHANGELOG.md
jandom Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -10,6 +10,11 @@ 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
Copy link
Owner

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?

Copy link
Contributor Author

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

Copy link
Collaborator

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

Copy link
Contributor Author

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

Copy link
Collaborator

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 ?

Copy link
Owner

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.

Copy link
Contributor Author

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!

[#1529](https://github.com/ruflin/Elastica/pull/1529)
* [Backported] Added a transport class for mocking a HTTP 403 error codes, useful for testing response failures in inheriting clients
[#1592](https://github.com/ruflin/Elastica/pull/1592)

### Improvements

- [Backported] Reduced memory footprint of response by not keeping the raw JSON data when JSON after JSON has been parsed. [#1588](https://github.com/ruflin/Elastica/pull/1588)
Expand Down
4 changes: 3 additions & 1 deletion lib/Elastica/Transport/Guzzle.php
View file
Open in desktop
Expand Up @@ -83,7 +83,8 @@ public function exec(Request $request, array $params)
throw new GuzzleException($ex, $request, new Response($ex->getMessage()));
}

$response = new Response((string) $res->getBody(), $res->getStatusCode());
$responseBody = (string) $res->getBody();
$response = new Response($responseBody, $res->getStatusCode());
$response->setQueryTime($end - $start);
if ($connection->hasConfig('bigintConversion')) {
$response->setJsonBigintConversion($connection->getConfig('bigintConversion'));
Expand All @@ -93,6 +94,7 @@ public function exec(Request $request, array $params)
[
'request_header' => $request->getMethod(),
'http_code' => $res->getStatusCode(),
'body' => $responseBody,
Copy link
Owner

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?

Copy link
Contributor Author

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)

Copy link
Collaborator

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.

Copy link
Collaborator

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"}}]}}
)

Copy link
Owner

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

@jandom jandom Jan 21, 2019 •
edited

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

Copy link
Owner

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?

Copy link
Collaborator

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?

Copy link
Collaborator

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.

]
);

Expand Down
60 changes: 55 additions & 5 deletions lib/Elastica/Transport/NullTransport.php
View file
Open in desktop
Expand Up @@ -12,18 +12,50 @@
* host but still returns a valid response object
*
* @author James Boehmer <james.boehmer@jamesboehmer.com>
* @author Jan Domanski <jandom@gmail.com>
*/
class NullTransport extends AbstractTransport
{
/**
* Null transport.
* Response you want to get from the transport
*
* @var Response Response
*/
protected $_response = null;

/**
* Set response object the transport returns
*
* @param \Elastica\Response $response
*
* @return $this
*/
public function getResponse()
{
return $this->_response;
}

/**
* Set response object the transport returns
*
* @param \Elastica\Response $response
*
* @return $this
*/
public function setResponse(Response $response)
{
$this->_response = $response;
return $this->_response;
}

/**
* Generate an example response object
*
* @param \Elastica\Request $request
* @param array $params Hostname, port, path, ...
*
* @return \Elastica\Response Response empty object
* @return \Elastica\Response $response
*/
public function exec(Request $request, array $params)
public function generateDefaultResponse(array $params)
{
$response = [
'took' => 0,
Expand All @@ -40,7 +72,25 @@ public function exec(Request $request, array $params)
],
'params' => $params,
];

return new Response(JSON::stringify($response));
}

/**
* Null transport.
*
* @param \Elastica\Request $request
* @param array $params Hostname, port, path, ...
*
* @return \Elastica\Response Response empty object
*/
public function exec(Request $request, array $params)
{
$response = $this->getResponse();

if (!$response) {
$response = $this->generateDefaultResponse($params);
}

return $response;
}
}
36 changes: 36 additions & 0 deletions test/Elastica/Transport/NullTransportTest.php
View file
Open in desktop
Expand Up @@ -12,9 +12,20 @@
* Elastica Null Transport Test.
*
* @author James Boehmer <james.boehmer@jamesboehmer.com>
* @author Jan Domanski <jandom@gmail.com>
*/
class NullTransportTest extends BaseTest
{

/** @var NullTransport NullTransport */
protected $transport;

public function setUp()
{
parent::setUp();
$this->transport = new NullTransport();
}

/**
* @group functional
*/
Expand Down Expand Up @@ -96,4 +107,29 @@ public function testOldObject()
$data = $response->getData();
$this->assertEquals($params, $data['params']);
}

/**
* @group unit
*/
public function testResponse()
{
$resposeString = '';
$response = new Response($resposeString);
$this->transport->setResponse($response);
$this->assertEquals($response, $this->transport->getResponse());
}

/**
* @group unit
*/
public function testGenerateDefaultResponse()
{
$params = [ 'blah' => 123 ];
$response = $this->transport->generateDefaultResponse($params);
$this->assertEquals([], $response->getTransferInfo());

$responseData = $response->getData();
$this->assertContains('params', $responseData);
$this->assertEquals($params, $responseData['params']);
}
}