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

Throw MongoWriteConcernException on bulk write errors #120

Merged
merged 4 commits into from
Jul 3, 2016
Merged

Throw MongoWriteConcernException on bulk write errors #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ebaff33
Harden tests to expect exception
alcaeus Jun 29, 2016
bcd9792
Allow setting a document in MongoWriteConcernException
alcaeus Jun 29, 2016
80a0fda
Throw MongoWriteConcernException on bulkWrite errors
alcaeus Jun 29, 2016
7e4cf47
Add changelog entry
alcaeus Jul 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG-1.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ milestone.

* [#117](https://github.com/alcaeus/mongo-php-adapter/pull/117) adds a missing
flag to indexes when calling `MongoCollection::getIndexInfo`.
* [#120](https://github.com/alcaeus/mongo-php-adapter/pull/120) throws the proper
`MongoWriteConcernException` when encountering bulk write errors.

1.0.4 (2016-06-22)
------------------
Expand Down
74 changes: 51 additions & 23 deletions lib/Mongo/MongoWriteBatch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

use Alcaeus\MongoDbAdapter\TypeConverter;
use Alcaeus\MongoDbAdapter\Helper\WriteConcernConverter;
use MongoDB\Driver\Exception\BulkWriteException;
use MongoDB\Driver\WriteError;
use MongoDB\Driver\WriteResult;

/**
* MongoWriteBatch allows you to "batch up" multiple operations (of same type)
Expand Down Expand Up @@ -115,55 +118,62 @@ final public function execute(array $writeOptions = [])
$options['ordered'] = $writeOptions['ordered'];
}

$collection = $this->collection->getCollection();

try {
$result = $collection->BulkWrite($this->items, $options);
$writeResult = $this->collection->getCollection()->bulkWrite($this->items, $options);
$resultDocument = [];
$ok = true;
} catch (\MongoDB\Driver\Exception\BulkWriteException $e) {
$result = $e->getWriteResult();
} catch (BulkWriteException $e) {
$writeResult = $e->getWriteResult();
$resultDocument = ['writeErrors' => $this->convertWriteErrors($writeResult)];
$ok = false;
}

if ($ok === true) {
$this->items = [];
}
$this->items = [];

switch ($this->batchType) {
case self::COMMAND_UPDATE:
$upsertedIds = [];
foreach ($result->getUpsertedIds() as $index => $id) {
foreach ($writeResult->getUpsertedIds() as $index => $id) {
$upsertedIds[] = [
'index' => $index,
'_id' => TypeConverter::toLegacy($id)
];
}

$result = [
'nMatched' => $result->getMatchedCount(),
'nModified' => $result->getModifiedCount(),
'nUpserted' => $result->getUpsertedCount(),
'ok' => $ok,
$resultDocument += [
'nMatched' => $writeResult->getMatchedCount(),
'nModified' => $writeResult->getModifiedCount(),
'nUpserted' => $writeResult->getUpsertedCount(),
'ok' => true,
];

if (count($upsertedIds)) {
$result['upserted'] = $upsertedIds;
$resultDocument['upserted'] = $upsertedIds;
}

return $result;
break;

case self::COMMAND_DELETE:
return [
'nRemoved' => $result->getDeletedCount(),
'ok' => $ok,
$resultDocument += [
'nRemoved' => $writeResult->getDeletedCount(),
'ok' => true,
];
break;

case self::COMMAND_INSERT:
return [
'nInserted' => $result->getInsertedCount(),
'ok' => $ok,
$resultDocument += [
'nInserted' => $writeResult->getInsertedCount(),
'ok' => true,
];
break;
}

if (! $ok) {
// Exception code is hardcoded to the value in ext-mongo, see
// https://github.com/mongodb/mongo-php-driver-legacy/blob/ab4bc0d90e93b3f247f6bcb386d0abc8d2fa7d74/batch/write.c#L428
throw new \MongoWriteConcernException('Failed write', 911, null, $resultDocument);
Copy link

Choose a reason for hiding this comment

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

I would not return the error code hardcoded, but use the error code of the previous MongoWriteConcernException.

Copy link

Choose a reason for hiding this comment

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

Also there is a \MongoDuplicateKeyException, would be nice if I would get that specific exception class from this lib, too.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I would not return the error code hardcoded, but use the error code of the previous MongoWriteConcernException.

Yeah, that was the quick hack to ensure it's passing, I'll write more tests adding failures during update and remove batches to ensure proper exception codes are passed along.

Also there is a \MongoDuplicateKeyException, would be nice if I would get that specific exception class from this lib, too.

The check actually produces an error due to a unique key violation but ext-mongo still throws \MongoWriteConcernException. I'm not sure where you'd expect \MongoDuplicateKeyException.

Copy link
Owner Author

@alcaeus alcaeus Jul 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

After checking once more, there is no exception code to pass on. The BulkWriteException thrown by ext-mongodb has code 0 and the legacy driver has the 911 code hardcoded into it: https://github.com/mongodb/mongo-php-driver-legacy/blob/ab4bc0d90e93b3f247f6bcb386d0abc8d2fa7d74/batch/write.c#L428
I'll add a comment specifying why the exception code is hardcoded and leave it at that.

}

return $resultDocument;
}

private function validate(array $item)
Expand Down Expand Up @@ -214,4 +224,22 @@ private function addItem(array $item)
break;
}
}

/**
* @param WriteResult $result
* @return array
*/
private function convertWriteErrors(WriteResult $result)
{
$writeErrors = [];
/** @var WriteError $writeError */
foreach ($result->getWriteErrors() as $writeError) {
$writeErrors[] = [
'index' => $writeError->getIndex(),
'code' => $writeError->getCode(),
'errmsg' => $writeError->getMessage(),
];
}
return $writeErrors;
}
}
27 changes: 25 additions & 2 deletions lib/Mongo/MongoWriteConcernException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,34 @@
* <p>(PECL mongo &gt;= 1.5.0)</p>
* @link http://php.net/manual/en/class.mongowriteconcernexception.php#class.mongowriteconcernexception
*/
class MongoWriteConcernException extends MongoCursorException {
class MongoWriteConcernException extends MongoCursorException
{
private $document;

/**
* MongoWriteConcernException constructor.
*
* @param string $message
* @param int $code
* @param Exception|null $previous
* @param null $document
*
* @internal The $document parameter is not part of the ext-mongo API
*/
public function __construct($message = '', $code = 0, Exception $previous = null, $document = null)
{
parent::__construct($message, $code, $previous);

$this->document = $document;
}

/**
* Get the error document
* @link http://php.net/manual/en/mongowriteconcernexception.getdocument.php
* @return array <p>A MongoDB document, if available, as an array.</p>
*/
public function getDocument() {}
public function getDocument()
{
return $this->document;
}
}
1 change: 0 additions & 1 deletion tests/Alcaeus/MongoDbAdapter/Mongo/MongoDeleteBatchTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ public function testDeleteMany()
$this->assertSame(0, $newCollection->count());
}


public function testValidateItem()
{
$collection = $this->getCollection();
Expand Down
2 changes: 2 additions & 0 deletions tests/Alcaeus/MongoDbAdapter/Mongo/MongoInsertBatchTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@ public function testInsertBatchError()

try {
$batch->execute();
$this->fail('Expected MongoWriteConcernException');
} catch (\MongoWriteConcernException $e) {
$this->assertSame('Failed write', $e->getMessage());
$this->assertSame(911, $e->getCode());
$this->assertArraySubset($expected, $e->getDocument());
}
}
Expand Down
73 changes: 72 additions & 1 deletion tests/Alcaeus/MongoDbAdapter/Mongo/MongoUpdateBatchTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,42 @@ public function testUpdateOne()
$this->assertAttributeSame('foo', 'foo', $record);
}

public function testUpdateOneException()
{
$collection = $this->getCollection();
$batch = new \MongoUpdateBatch($collection);

$document = ['foo' => 'bar'];
$collection->insert($document);
$document = ['foo' => 'foo'];
$collection->insert($document);
$collection->createIndex(['foo' => 1], ['unique' => true]);

$this->assertTrue($batch->add(['q' => ['foo' => 'bar'], 'u' => ['$set' => ['foo' => 'foo']]]));

$expected = [
'writeErrors' => [
[
'index' => 0,
'code' => 11000,
]
],
'nMatched' => 0,
'nModified' => 0,
'nUpserted' => 0,
'ok' => true,
];

try {
$batch->execute();
$this->fail('Expected MongoWriteConcernException');
} catch (\MongoWriteConcernException $e) {
$this->assertSame('Failed write', $e->getMessage());
$this->assertSame(911, $e->getCode());
$this->assertArraySubset($expected, $e->getDocument());
}
}

public function testUpdateMany()
{
$collection = $this->getCollection();
Expand All @@ -49,7 +85,6 @@ public function testUpdateMany()
unset($document['_id']);
$collection->insert($document);


$this->assertTrue($batch->add(['q' => ['foo' => 'bar'], 'u' => ['$set' => ['foo' => 'foo']], 'multi' => true]));

$expected = [
Expand All @@ -69,6 +104,42 @@ public function testUpdateMany()
$this->assertAttributeSame('foo', 'foo', $record);
}

public function testUpdateManyException()
{
$collection = $this->getCollection();
$batch = new \MongoUpdateBatch($collection);

$document = ['foo' => 'bar', 'bar' => 'bar'];
$collection->insert($document);
$document = ['foo' => 'foobar', 'bar' => 'bar'];
$collection->insert($document);
$collection->createIndex(['foo' => 1], ['unique' => true]);

$batch->add(['q' => ['bar' => 'bar'], 'u' => ['$set' => ['foo' => 'foo']], 'multi' => true]);

$expected = [
'writeErrors' => [
[
'index' => 0,
'code' => 11000,
]
],
'nMatched' => 0,
'nModified' => 0,
'nUpserted' => 0,
'ok' => true,
];

try {
$batch->execute();
$this->fail('Expected MongoWriteConcernException');
} catch (\MongoWriteConcernException $e) {
$this->assertSame('Failed write', $e->getMessage());
$this->assertSame(911, $e->getCode());
$this->assertArraySubset($expected, $e->getDocument());
}
}

public function testUpsert()
{
$document = ['foo' => 'foo'];
Expand Down