Skip to content

Commit

Permalink
Fix exception tests with new error object
Browse files Browse the repository at this point in the history
  • Loading branch information
@ruflin
ruflin committed Nov 4, 2015
1 parent 1dc4195 commit 41a7a20
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 14 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ All notable changes to this project will be documented in this file based on the
- Remove ansible scripts for tests setup and Vagrantfile as not needed anymore.
All is based on docker contaienrs now
- Support for PHP 5.3 removed
- Elastica\Reponse::getError() now returns and array instead of a string

### Bugfixes

### Added
- Elastica\Reponse::getErrorMessage was added as getError is now an object

### Improvements
- Travis builds were moved to docker-compose setup. Ansible scripts and Vagrant files were removed
Expand Down
2 changes: 1 addition & 1 deletion Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,4 +184,4 @@ tmp:
make elastica-image
make setup
mkdir -p build
docker-compose run elastica phpunit -c test/ test/lib/Elastica/Test/ClientTest.php
docker-compose run elastica phpunit -c test/ test/lib/Elastica/Test/Exception
20 changes: 17 additions & 3 deletions lib/Elastica/Exception/ElasticsearchException.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,23 @@ class ElasticsearchException extends \Exception implements ExceptionInterface
*/
private $_isRemote = false;

/**
* @var array Error array
*/
protected $_error = array();

/**
* Constructs elasticsearch exception.
*
* @param int $code Error code
* @param string $error Error message from elasticsearch
* @param array $error Error object from elasticsearch
*/
public function __construct($code, $error)
{
$this->_parseError($error);
parent::__construct($error, $code);
$this->_error = $error;
// TODO: es2 improve as now an array
$this->_parseError(json_encode($error));
parent::__construct(json_encode($error), $code);
}

/**
Expand Down Expand Up @@ -88,4 +95,11 @@ public function isRemoteTransportException()
{
return $this->_isRemote;
}

/**
* @return array Error array
*/
public function getError() {
return $this->_error;
}
}
4 changes: 2 additions & 2 deletions lib/Elastica/Exception/ResponseException.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public function __construct(Request $request, Response $response)
{
$this->_request = $request;
$this->_response = $response;
parent::__construct($response->getError());
parent::__construct($response->getErrorMessage());
}

/**
Expand Down Expand Up @@ -65,6 +65,6 @@ public function getElasticsearchException()
$transfer = $response->getTransferInfo();
$code = array_key_exists('http_code', $transfer) ? $transfer['http_code'] : 0;

return new ElasticsearchException($code, $response->getError());
return new ElasticsearchException($code, $response->getErrorMessage());
}
}
19 changes: 14 additions & 5 deletions lib/Elastica/Response.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,28 @@ public function __construct($responseString, $responseStatus = null)
/**
* Error message.
*
* @return string Error message
* @return array Error object
*/
public function getError()
{
$message = '';
$error = array();
$response = $this->getData();

if (isset($response['error'])) {
$message = $response['error'];
$error = $response['error'];
}
return $error;
}

/**
* @return string Error string based on the error object
*/
public function getErrorMessage() {
$error = $this->getError();

$message = "";

// Encoding needed as much more information inside now
if (!is_string($message)) {
if (!is_string($error)) {
$message = json_encode($message);
}

Expand Down
9 changes: 6 additions & 3 deletions test/lib/Elastica/Test/Exception/ResponseExceptionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ public function testCreateExistingIndex()
$this->_createIndex('woo', false);
$this->fail('Index created when it should fail');
} catch (ResponseException $ex) {
$this->assertEquals('IndexAlreadyExistsException', $ex->getElasticsearchException()->getExceptionName());
$error = $ex->getResponse()->getError();
$this->assertEquals('index_already_exists_exception', $error['type']);
$this->assertEquals(400, $ex->getElasticsearchException()->getCode());
}
}
Expand All @@ -42,7 +43,8 @@ public function testBadType()
)));
$this->fail('Indexing with wrong type should fail');
} catch (ResponseException $ex) {
$this->assertEquals('MapperParsingException', $ex->getElasticsearchException()->getExceptionName());
$error = $ex->getResponse()->getError();
$this->assertEquals('mapper_parsing_exception', $error['type']);
$this->assertEquals(400, $ex->getElasticsearchException()->getCode());
}
}
Expand All @@ -58,7 +60,8 @@ public function testWhatever()
try {
$index->search();
} catch (ResponseException $ex) {
$this->assertEquals('IndexMissingException', $ex->getElasticsearchException()->getExceptionName());
$error = $ex->getResponse()->getError();
$this->assertEquals('index_not_found_exception', $error['type']);
$this->assertEquals(404, $ex->getElasticsearchException()->getCode());
}
}
Expand Down

8 comments on commit 41a7a20

@nickshanks
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong fix. I encountered this issue myself and fixed it locally, but my fix ensures the one inviolate contract, your API interface, is not changed. In short, you can rename getError() to getFullError() or getErrorObject() (or something of that ilk), and rename getErrorMessage() back to getError() — then you don't need to change ResponseException.php, and everybody using the Elastica\Response class won't have to update their code. I do think though that if I were starting from scratch, getError() and getErrorMessage() would be the right names for these methods. Please don't break existing code when there is another way. Also, I don't believe you should be returning an empty array if there is no error in the response data. Return null as this will indicate that there is no error, rather than an error with no information (as implied by the empty array). Finally, line 93 needs a new line before the opening brace.

@nickshanks
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW here's what I did (it met my need of finding out the index name for the exception I was having, and uses the early-return paradigm to avoid nesting):

    /**
     * Error message.
     *
     * @return string Error message
     */
    public function getError()
    {
        $error = $this->getFullError();

        if (!$error) {
            return '';
        }

        if (isset($error['root_cause'][0])) {
            $error = $error['root_cause'][0];
        }

        $message = $error['reason'];
        if (isset($error['index'])) {
            $message .= ' [index: ' . $error['index'] . ']';
        }

        return $message;
    }

    /**
     * A keyed array representing any errors that occured.
     *
     * @return array Error data
     */
    public function getFullError()
    {
        $response = $this->getData();

        if (isset($response['error'])) {
            return $response['error'];
        }

        return null;
    }

@ruflin
Copy link
Owner Author

@ruflin ruflin commented on 41a7a20 Dec 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickshanks I like this change. The above was more a "fast fix" to get it working. Any chance you could open a PR with your changes? @ewgRa What do you think?

@ewgRa
Copy link
Contributor

@ewgRa ewgRa commented on 41a7a20 Dec 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know background of this changes so I can't say something valued. From code perspective @nickshanks fix looks reasonable and haven't BC.

Maybe there is a reason rename getFullError to getRawError, to show that this is an error that ES return.

@ruflin
Copy link
Owner Author

@ruflin ruflin commented on 41a7a20 Dec 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #1016 as I would like to get this change into beta2

@ruflin
Copy link
Owner Author

@ruflin ruflin commented on 41a7a20 Jan 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickshanks Can you have a look at #1016 and let me know if that is as expected for you? I took your code, made some small changes and fixed the tests.

@nickshanks
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ruflin Happy new year. I'm taking a look now…

@nickshanks
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see no problems. Have commented on a couple of style niggles but I'm not sure what your coding style guidelines are.

Please sign in to comment.