Skip to content

PHPC-1960: Do not use BulkWriteException for client-side errors #1708

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

Closed
wants to merge 4 commits into from
Closed

PHPC-1960: Do not use BulkWriteException for client-side errors #1708

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
5a3767e
PHPC-2344 Do not use BulkWriteException for client-side errors
GromNaN Oct 1, 2024
18e2e15
Fix exception class
GromNaN Oct 1, 2024
6260899
Add $writeResult property to ConnectionTimeoutException
GromNaN Oct 1, 2024
73faf65
Simplify
GromNaN Oct 1, 2024
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
15 changes: 15 additions & 0 deletions src/MongoDB/Exception/ConnectionTimeoutException.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,25 @@
#include <php.h>

#include "php_phongo.h"
#include "phongo_error.h"
#include "ConnectionTimeoutException_arginfo.h"

zend_class_entry* php_phongo_connectiontimeoutexception_ce;

/* Returns the WriteResult from the failed write operation. */
static PHP_METHOD(MongoDB_Driver_Exception_ConnectionTimeoutException, getWriteResult)
{
zval* writeresult;
zval rv;

PHONGO_PARSE_PARAMETERS_NONE();

writeresult = zend_read_property(php_phongo_bulkwriteexception_ce, Z_OBJ_P(getThis()), ZEND_STRL("writeResult"), 0, &rv);

RETURN_ZVAL(writeresult, 1, 0);
}


void php_phongo_connectiontimeoutexception_init_ce(INIT_FUNC_ARGS)
{
php_phongo_connectiontimeoutexception_ce = register_class_MongoDB_Driver_Exception_ConnectionTimeoutException(php_phongo_connectionexception_ce);
Expand Down
4 changes: 4 additions & 0 deletions src/MongoDB/Exception/ConnectionTimeoutException.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,8 @@

final class ConnectionTimeoutException extends ConnectionException
{
/** @var \MongoDB\Driver\WriteResult */
protected $writeResult;

final public function getWriteResult(): \MongoDB\Driver\WriteResult {}
}
13 changes: 12 additions & 1 deletion src/MongoDB/Exception/ConnectionTimeoutException_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 2 additions & 17 deletions src/phongo_execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,29 +296,14 @@ bool phongo_execute_bulk_write(zval* manager, const char* namespace, php_phongo_
/* A BulkWriteException is always thrown if mongoc_bulk_operation_execute()
* fails to ensure that the write result is accessible. If the error does
* not originate from the server (e.g. socket error), throw the appropriate
* exception first. It will be included in BulkWriteException's message and
* will also be accessible via Exception::getPrevious(). */
* exception first. It will be thrown directly */
if (!success) {
if (error.domain != MONGOC_ERROR_SERVER && error.domain != MONGOC_ERROR_WRITE_CONCERN) {
phongo_throw_exception_from_bson_error_t_and_reply(&error, &reply);
}

/* Argument errors occur before command execution, so there is no need
* to layer this InvalidArgumentException behind a BulkWriteException.
* In practice, this will be a "Cannot do an empty bulk write" error. */
if (error.domain == MONGOC_ERROR_COMMAND && error.code == MONGOC_ERROR_COMMAND_INVALID_ARG) {
goto cleanup;
}

if (EG(exception)) {
char* message;

(void) spprintf(&message, 0, "Bulk write failed due to previous %s: %s", PHONGO_ZVAL_EXCEPTION_NAME(EG(exception)), error.message);
zend_throw_exception(php_phongo_bulkwriteexception_ce, message, 0);
efree(message);
} else {
zend_throw_exception(php_phongo_bulkwriteexception_ce, error.message, error.code);
}
zend_throw_exception(php_phongo_bulkwriteexception_ce, error.message, error.code);

/* Ensure error labels are added to the final BulkWriteException. If a
* previous exception was also thrown, error labels will already have
Expand Down
39 changes: 4 additions & 35 deletions tests/manager/manager-executeBulkWrite_error-005.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,45 +24,14 @@ $bulk->insert(['x' => 1]);
$bulk->update(['x' => 1], ['$set' => ['y' => 1]]);
$bulk->delete(['x' => 1]);

try {
echo throws(function() use ($server, $bulk) {
$server->executeBulkWrite(NS, $bulk);
} catch (MongoDB\Driver\Exception\BulkWriteException $e) {
printf("%s(%d): %s\n", get_class($e), $e->getCode(), $e->getMessage());
$prev = $e->getPrevious();
printf("%s(%d): %s\n", get_class($prev), $prev->getCode(), $prev->getMessage());
var_dump($e->getWriteResult());
}
}, MongoDB\Driver\Exception\ConnectionTimeoutException::class), "\n";

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
MongoDB\Driver\Exception\BulkWriteException(0): Bulk write failed due to previous MongoDB\Driver\Exception\ConnectionTimeoutException: Failed to send "delete" command with database "%s": Failed to read 4 bytes: socket error or timeout
MongoDB\Driver\Exception\ConnectionTimeoutException(%d): Failed to send "delete" command with database "%s": Failed to read 4 bytes: socket error or timeout
object(MongoDB\Driver\WriteResult)#%d (%d) {
["nInserted"]=>
int(1)
["nMatched"]=>
int(1)
["nModified"]=>
int(1)
["nRemoved"]=>
int(0)
["nUpserted"]=>
int(0)
["upsertedIds"]=>
array(0) {
}
["writeErrors"]=>
array(0) {
}
["writeConcernError"]=>
NULL
["writeConcern"]=>
object(MongoDB\Driver\WriteConcern)#%d (%d) {
}
["errorReplies"]=>
array(0) {
}
}
OK: Got MongoDB\Driver\Exception\ConnectionTimeoutException
Failed to send "delete" command with database "%s": Failed to read 4 bytes: socket error or timeout
Comment on lines +35 to +36
Copy link
Member Author

Choose a reason for hiding this comment

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

The current result is:

Deprecated: Creation of dynamic property MongoDB\Driver\Exception\RuntimeException::$writeResult is deprecated in /Users/jerome/Develop/mongo-php-driver/tests/manager/manager-executeBulkWrite_error-005.php on line 20
ALMOST: Got MongoDB\Driver\Exception\RuntimeException - expected MongoDB\Driver\Exception\ConnectionTimeoutException
Failed to send "delete" command with database "phongo": Failed to read 4 bytes: socket error or timeout

I need to fix the "dynamic property" issue and the exception class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I got the correct exception class by using phongo_throw_exception_from_bson_error_t_and_reply(&error, &reply).

Deprecated: Creation of dynamic property MongoDB\Driver\Exception\ConnectionTimeoutException::$writeResult is deprecated in /Users/jerome/Develop/mongo-php-driver/tests/manager/manager-executeBulkWrite_error-005.php on line 20
OK: Got MongoDB\Driver\Exception\ConnectionTimeoutException
Failed to send "delete" command with database "phongo": Failed to read 4 bytes: socket error or timeout

Now I need to figure where the dynamic property comes from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Test successful after adding ConnectionTimeoutException::$writeResult.

===DONE===
Loading