Exception handling #20

RageZBla · 2016-01-05T12:59:07Z

Try to implement exception converter. I don't think we'll be able to use in 100% of the time but should cover easy situations.

Let me know your feedback.

Reference #7

alcaeus · 2016-01-05T17:16:55Z

As a first start, this seems ok, there are a couple of special cases we can't quite cover yet. For example, inserting a document that violates a unique index normally causes a MongoDuplicateKeyException:

PHP Warning:  Uncaught exception 'MongoDuplicateKeyException' with message 'localhost:27017: E11000 duplicate key error collection: mongo-php-adapter.test index: _id_ dup key: { : "foo" }' in php shell code:1

The new driver apparently throws a MongoDB\Driver\Exception\BulkWriteException with the message BulkWrite error, so we can't even check for a code or message and convert to the appropriate exception class. I'll have to see what we can do about that.

RageZBla · 2016-01-09T08:59:08Z

@alcaeus I have done most of the exception conversion. This got hairy real quick, the exception in the new driver does not make much sense.

Anyway, your feedback would be appreciated.

alcaeus · 2016-01-09T09:03:14Z

Thanks, I'll take a look at this soon.

Can you list examples where the exceptions don't make sense? Some of the exceptions contain additional information about the error. For example, the WriteException and BulkWriteException expose a writeResult object that can be used to get more information about the error(s) that occured. See the php docs for more information.

RageZBla · 2016-01-09T09:08:14Z

Actually it's more a semantic issues and things that you won't expect for example trying to connect to a server not responding throw a RuntimeException rather than ConnectionException.

On the same idea setting w to 2 on a standalone server actually throw a ConnectionException, I would more expected some invalid argument or runtime.

alcaeus · 2016-01-09T09:22:33Z

I haven't looked at stuff yet, but from what I've seen in the driver I'm sure @jmikola et al. had a reason for this ;)

RageZBla · 2016-01-09T09:42:25Z

Probably me, maybe I should read the manual a bit more :-)

On 2016/01/09, at 18:22, Andreas notifications@github.com wrote:

I haven't looked at stuff yet, but from what I've seen in the driver I'm sure @jmikola et al. had a reason for this ;)

―
Reply to this email directly or view it on GitHub.

alcaeus · 2016-01-13T18:46:13Z

lib/Mongo/MongoCollection.php

+        $indexes = $this->collection->listIndexes();
+        foreach ($indexes as $index) {
+            if ($index->getKey() === $keys) {
+                throw new \MongoDuplicateKeyException();


A quick test shows that this exception is not actually thrown:

php > $client = new MongoClient(); php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1])); array(4) { 'createdCollectionAutomatically' => bool(true) 'numIndexesBefore' => int(1) 'numIndexesAfter' => int(2) 'ok' => double(1) } php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1])); array(5) { 'createdCollectionAutomatically' => bool(false) 'numIndexesBefore' => int(2) 'numIndexesAfter' => int(2) 'note' => string(25) "all indexes already exist" 'ok' => double(1) }

There is an exception if the index already exists with different options:

php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1], ['unique' => true])); PHP Warning: Uncaught exception 'MongoResultException' with message 'localhost:27017: Index with name: foo_1 already exists with different options' in php shell code:1 Stack trace: #0 php shell code(1): MongoCollection->createIndex(Array, Array) #1 {main} thrown in php shell code on line 1 Warning: Uncaught exception 'MongoResultException' with message 'localhost:27017: Index with name: foo_1 already exists with different options' in php shell code:1 Stack trace: #0 php shell code(1): MongoCollection->createIndex(Array, Array) #1 {main} thrown in php shell code on line 1

That's odd because I have a working test case and I had to catch it in gridfs or the tests would not pass. Maybe because of the ===.

On 2016/01/14, at 3:46, Andreas notifications@github.com wrote:

In lib/Mongo/MongoCollection.php:

// Note: this is what the result array should look like
-// $expected = [
-// 'createdCollectionAutomatically' => true,
-// 'numIndexesBefore' => 1,
-// 'numIndexesAfter' => 2,
-// 'ok' => 1.0
-// ];

if (! is_array($keys) || ! count($keys)) {

throw new MongoException('keys cannot be empty');

}

// duplicate

$indexes = $this->collection->listIndexes();

foreach ($indexes as $index) {

if ($index->getKey() === $keys) {

throw new \MongoDuplicateKeyException();
A quick test shows that this exception is not actually thrown:

php > $client = new MongoClient();
php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1]));
array(4) {
'createdCollectionAutomatically' =>
bool(true)
'numIndexesBefore' =>
int(1)
'numIndexesAfter' =>
int(2)
'ok' =>
double(1)
}
php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1]));
array(5) {
'createdCollectionAutomatically' =>
bool(false)
'numIndexesBefore' =>
int(2)
'numIndexesAfter' =>
int(2)
'note' =>
string(25) "all indexes already exist"
'ok' =>
double(1)
}
There is an exception if the index already exists with different options:

php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1], ['unique' => true]));
PHP Warning: Uncaught exception 'MongoResultException' with message 'localhost:27017: Index with name: foo_1 already exists with different options' in php shell code:1
Stack trace:
#0 php shell code(1): MongoCollection->createIndex(Array, Array)
#1 {main}
thrown in php shell code on line 1

Warning: Uncaught exception 'MongoResultException' with message 'localhost:27017: Index with name: foo_1 already exists with different options' in php shell code:1
Stack trace:
#0 php shell code(1): MongoCollection->createIndex(Array, Array)
#1 {main}
thrown in php shell code on line 1
―
Reply to this email directly or view it on GitHub.

Gotcha, I miss read your comment. Ok, I'll modify this to match more closely the real extension.

On 2016/01/14, at 3:46, Andreas notifications@github.com wrote:

In lib/Mongo/MongoCollection.php:

// Note: this is what the result array should look like
-// $expected = [
-// 'createdCollectionAutomatically' => true,
-// 'numIndexesBefore' => 1,
-// 'numIndexesAfter' => 2,
-// 'ok' => 1.0
-// ];

if (! is_array($keys) || ! count($keys)) {

throw new MongoException('keys cannot be empty');

}

// duplicate

$indexes = $this->collection->listIndexes();

foreach ($indexes as $index) {

if ($index->getKey() === $keys) {

throw new \MongoDuplicateKeyException();
A quick test shows that this exception is not actually thrown:

php > $client = new MongoClient();
php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1]));
array(4) {
'createdCollectionAutomatically' =>
bool(true)
'numIndexesBefore' =>
int(1)
'numIndexesAfter' =>
int(2)
'ok' =>
double(1)
}
php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1]));
array(5) {
'createdCollectionAutomatically' =>
bool(false)
'numIndexesBefore' =>
int(2)
'numIndexesAfter' =>
int(2)
'note' =>
string(25) "all indexes already exist"
'ok' =>
double(1)
}
There is an exception if the index already exists with different options:

php > var_dump($client->selectCollection('mongo-php-adapter', 'test')->createIndex(['foo' => 1], ['unique' => true]));
PHP Warning: Uncaught exception 'MongoResultException' with message 'localhost:27017: Index with name: foo_1 already exists with different options' in php shell code:1
Stack trace:
#0 php shell code(1): MongoCollection->createIndex(Array, Array)
#1 {main}
thrown in php shell code on line 1

Warning: Uncaught exception 'MongoResultException' with message 'localhost:27017: Index with name: foo_1 already exists with different options' in php shell code:1
Stack trace:
#0 php shell code(1): MongoCollection->createIndex(Array, Array)
#1 {main}
thrown in php shell code on line 1
―
Reply to this email directly or view it on GitHub.

Regarding, this the code is still throwing MongoDuplicateKeyException rather than MongoResultException. I kept that way because I don't know if duplicate exception would be ever use and in exactly what situation. If you have the chance to look at the original driver code and tell me. Worst case scenario, I can take a look on my own tomorrow night.

alcaeus · 2016-01-13T18:54:31Z

Apart from the comments above, can you rebase this on top of master? There have been changes to insert, batchInsert and save methods, so the check against empty documents must be revised as well.

RageZBla · 2016-01-14T16:14:28Z

@alcaeus I think I have addressed most of your feedback, if you have few minutes to spare to check.

RageZBla · 2016-01-14T16:19:10Z

lib/Mongo/MongoCollection.php

@@ -772,5 +881,31 @@ private function ensureDocumentHasMongoId(&$document)

        return null;
    }
+
+    private function validateDocument(&$document)


@alcaeus let me know if you think that's overkill.

I think we can incorporate these checks into ensureDocumentHasMongoId since that will return false if the object given is not an object or array. We can just expand those checks to see if there are any empty documents there.

RageZBla · 2016-01-15T12:05:58Z

If you have more feedback!

alcaeus · 2016-01-16T15:17:13Z

Looks good to me. Can you rebase the PR and squash some commits (e.g. the "enable failing test" commits)? I'll set aside some time for this in the next couple of days and will then start to stabilize everything in #28.

RageZBla · 2016-01-17T06:35:00Z

I have cleaned the branch. Regarding the failing test, it's still the same one. Let me know if I can be any help with #28.

alcaeus · 2016-01-17T17:27:02Z

I've changed the failing test around - I don't have 2.6 (the version running on Travis) around to see the actual error thrown.

RageZBla · 2016-01-18T01:04:14Z

I tried with mongo 2.6 and the test still fails somehow.

On 2016/01/18, at 2:27, Andreas notifications@github.com wrote:

I've changed the failing test around - I don't have 2.6 (the version running on Travis) around to see the actual error thrown.

―
Reply to this email directly or view it on GitHub.

jmikola · 2016-02-12T22:16:01Z

Actually it's more a semantic issues and things that you won't expect for example trying to connect to a server not responding throw a RuntimeException rather than ConnectionException.

I actually noticed this today while looking into superfluous warnings emitted during stream initialization. PHPC-581 aims to fix this by throwing ConnectionTimeoutException in this case. We do have a specificity issue with libmongoc, because connections all happen as part of the server selection component. This means that while we might encountered actual socket timeouts or DNS resolution errors internally, this manifests itself as a server selection timeout. We'll hopefully be able to sort this out down the line, but it likely won't happen until libmongoc 1.5 or later.

On the same idea setting w to 2 on a standalone server actually throw a ConnectionException, I would more expected some invalid argument or runtime.

This doesn't sound correct. If you have a reproducible test case, I'd appreciate it if you could open a JIRA issue for this.

RageZBla force-pushed the feature/7/exceptions-converter branch from 26fba08 to 1b64f6d Compare January 5, 2016 13:10

RageZBla changed the title ~~Exception converter~~ [WIP] Exception handling Jan 6, 2016

RageZBla force-pushed the feature/7/exceptions-converter branch from fca982d to df5c55d Compare January 9, 2016 08:50

RageZBla changed the title ~~[WIP] Exception handling~~ Exception handling Jan 9, 2016

RageZBla force-pushed the feature/7/exceptions-converter branch from d5d64ea to 658bb79 Compare January 10, 2016 04:18

alcaeus reviewed Jan 13, 2016
View reviewed changes

RageZBla force-pushed the feature/7/exceptions-converter branch from c73c6c2 to 78c00cf Compare January 14, 2016 16:11

RageZBla reviewed Jan 14, 2016
View reviewed changes

RageZBla force-pushed the feature/7/exceptions-converter branch from c005297 to 736b839 Compare January 17, 2016 06:26

Olivier Lechevalier added 4 commits January 17, 2016 15:27

Implement ExceptionConverter

f7d4aad

Add exceptions handling to MongoDB.

ee7a501

Add exceptions conversion to MongoCollection

ad66fc0

Add exception handling to MongoClient

eab426b

Olivier Lechevalier added 3 commits January 17, 2016 15:27

Add exceptions to MongoCursor

65c2fbf

Corrected issue with MongoGridFSCursor::key method

ec20a6d

Add exceptions to GridFS

666dc29

RageZBla force-pushed the feature/7/exceptions-converter branch from 736b839 to 666dc29 Compare January 17, 2016 06:30

alcaeus closed this in ee831b8 Jan 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exception handling #20

Exception handling #20

RageZBla commented Jan 5, 2016

alcaeus commented Jan 5, 2016

RageZBla commented Jan 9, 2016

alcaeus commented Jan 9, 2016

RageZBla commented Jan 9, 2016

alcaeus commented Jan 9, 2016

RageZBla commented Jan 9, 2016

alcaeus Jan 13, 2016

RageZBla Jan 14, 2016

RageZBla Jan 14, 2016

RageZBla Jan 14, 2016

alcaeus commented Jan 13, 2016

RageZBla commented Jan 14, 2016

RageZBla Jan 14, 2016

alcaeus Jan 14, 2016

RageZBla commented Jan 15, 2016

alcaeus commented Jan 16, 2016

RageZBla commented Jan 17, 2016

alcaeus commented Jan 17, 2016

RageZBla commented Jan 18, 2016

jmikola commented Feb 12, 2016

Exception handling #20

Exception handling #20

Conversation

RageZBla commented Jan 5, 2016

alcaeus commented Jan 5, 2016

RageZBla commented Jan 9, 2016

alcaeus commented Jan 9, 2016

RageZBla commented Jan 9, 2016

alcaeus commented Jan 9, 2016

RageZBla commented Jan 9, 2016

alcaeus Jan 13, 2016

Choose a reason for hiding this comment

RageZBla Jan 14, 2016

Choose a reason for hiding this comment

RageZBla Jan 14, 2016

Choose a reason for hiding this comment

RageZBla Jan 14, 2016

Choose a reason for hiding this comment

alcaeus commented Jan 13, 2016

RageZBla commented Jan 14, 2016

RageZBla Jan 14, 2016

Choose a reason for hiding this comment

alcaeus Jan 14, 2016

Choose a reason for hiding this comment

RageZBla commented Jan 15, 2016

alcaeus commented Jan 16, 2016

RageZBla commented Jan 17, 2016

alcaeus commented Jan 17, 2016

RageZBla commented Jan 18, 2016

jmikola commented Feb 12, 2016