-
Notifications
You must be signed in to change notification settings - Fork 126
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
Throw MongoWriteConcernException on bulk write errors #120
Conversation
👍 |
} | ||
|
||
if (! $ok) { | ||
throw new \MongoWriteConcernException('Failed write', 911, null, $resultDocument); |
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 would not return the error code hardcoded, but use the error code of the previous MongoWriteConcernException.
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.
Also there is a \MongoDuplicateKeyException, would be nice if I would get that specific exception class from this lib, 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.
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
.
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.
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.
a1bebfc
to
80a0fda
Compare
@prolic Added some tests for update exceptions, please let me know if this works for you. |
looks really good! thanks for your work! |
This PR supersedes #118 and adds the proper result document to the exception. Since I didn't want to expose a setter for the document property in the exception I decided to not include this in the ExceptionConverter but instead convert this exception manually within the BulkWriteClass.
Todos: