Skip to content

[WAIT] PHPC-1076: Expose result document for failed commands via CommandFailedEvent #818

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 1 commit into from
Closed

[WAIT] PHPC-1076: Expose result document for failed commands via CommandFailedEvent #818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions php_phongo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2214,11 +2214,13 @@ static void php_phongo_command_failed(const mongoc_apm_command_failed_t* event)
p_event->operation_id = mongoc_apm_command_failed_get_operation_id(event);
p_event->request_id = mongoc_apm_command_failed_get_request_id(event);
p_event->duration_micros = mongoc_apm_command_failed_get_duration(event);
p_event->reply = bson_copy(mongoc_apm_command_failed_get_reply(event));
Copy link
Member

@jmikola jmikola Apr 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The build currently fails because mongoc_apm_command_failed_get_reply() does not exist in the libmongoc submodule. You'll need to do the following to address this:

  • Bump the libmongoc submodule to its master branch (or the commit where this function first shows up). The PHPC commit for this can use this issue number. For prior art, see: a49c71c#diff-25d902c24283ab8cfbac54dfa101ad31
  • In config.m4, you'll also need to bump lines that check the version of libmongoc when used as a system library. See:

    mongo-php-driver/config.m4

    Line 289 in 94c7487

    if test -x "$PKG_CONFIG" && $PKG_CONFIG --exists libmongoc-1.0; then

Note that libmongoc's master branch (1.10-dev at the moment) incorporates libbson, so I'm not suggesting you bump the libbson submodule since there is nothing to bump to. I assume libmongoc 1.10-dev will still play nice with libbson 1.9 (for now), but if only updating libmongoc causes issues, we can work through a solution in more detail.


/* We need to process and convert the error right here, otherwise
* debug_info will turn into a recursive loop, and with the wrong trace
* locations */
mongoc_apm_command_failed_get_error(event, &tmp_error);

{
#if PHP_VERSION_ID < 70000
MAKE_STD_ZVAL(p_event->z_error);
Expand All @@ -2229,6 +2231,7 @@ static void php_phongo_command_failed(const mongoc_apm_command_failed_t* event)
object_init_ex(&p_event->z_error, phongo_exception_from_mongoc_domain(tmp_error.domain, tmp_error.code));
zend_update_property_string(default_exception_ce, &p_event->z_error, ZEND_STRL("message"), tmp_error.message TSRMLS_CC);
zend_update_property_long(default_exception_ce, &p_event->z_error, ZEND_STRL("code"), tmp_error.code TSRMLS_CC);

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you should add this extra white space line here.

Copy link
Member

Choose a reason for hiding this comment

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

I removed this in #840.

#endif
}

Expand Down
1 change: 1 addition & 0 deletions php_phongo_structs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ typedef struct {
uint64_t operation_id;
uint64_t request_id;
uint64_t duration_micros;
bson_t* reply;
PHONGO_STRUCT_ZVAL z_error;
PHONGO_ZEND_OBJECT_POST
} php_phongo_commandfailedevent_t;
Expand Down
36 changes: 36 additions & 0 deletions src/MongoDB/Monitoring/CommandFailedEvent.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,29 @@ PHP_METHOD(CommandFailedEvent, getOperationId)
PHONGO_RETVAL_STRING(int_as_string);
} /* }}} */

/* {{{ proto stdClass CommandFailedEvent::getReply()
Returns the reply document associated with the event */
PHP_METHOD(CommandFailedEvent, getReply)
{
php_phongo_commandfailedevent_t* intern;
php_phongo_bson_state state = PHONGO_BSON_STATE_INITIALIZER;
SUPPRESS_UNUSED_WARNING(return_value_ptr)
SUPPRESS_UNUSED_WARNING(return_value_used)

intern = Z_COMMANDFAILEDEVENT_OBJ_P(getThis());

if (zend_parse_parameters_none() == FAILURE) {
return;
}

php_phongo_bson_to_zval_ex(bson_get_data(intern->reply), intern->reply->len, &state);
#if PHP_VERSION_ID >= 70000
RETURN_ZVAL(&state.zchild, 0, 1);
#else
RETURN_ZVAL(state.zchild, 0, 1);
#endif
} /* }}} */

/* {{{ proto string CommandFailedEvent::getRequestId()
Returns the event's request ID */
PHP_METHOD(CommandFailedEvent, getRequestId)
Expand Down Expand Up @@ -152,6 +175,7 @@ static zend_function_entry php_phongo_commandfailedevent_me[] = {
PHP_ME(CommandFailedEvent, getError, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(CommandFailedEvent, getDurationMicros, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(CommandFailedEvent, getOperationId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(CommandFailedEvent, getReply, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you did here, indenting it nicely in between the one before and after — however, I don't think our intention was ever to stair-case-indent these in the first place, and it might just have slipped by our review radar when we introduced clang-format.

This can be addressed by using the following patch:

--- a/src/MongoDB/Monitoring/CommandFailedEvent.c
+++ b/src/MongoDB/Monitoring/CommandFailedEvent.c
@@ -147,15 +147,17 @@ ZEND_BEGIN_ARG_INFO_EX(ai_CommandFailedEvent_void, 0, 0, 0)
 ZEND_END_ARG_INFO()
 
 static zend_function_entry php_phongo_commandfailedevent_me[] = {
+       /* clang-format off */
        ZEND_NAMED_ME(__construct, PHP_FN(MongoDB_disabled___construct), ai_CommandFailedEvent_void, ZEND_ACC_PRIVATE | ZEND_ACC_FINAL)
-               PHP_ME(CommandFailedEvent, getCommandName, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
-                       PHP_ME(CommandFailedEvent, getError, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
-                               PHP_ME(CommandFailedEvent, getDurationMicros, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
-                                       PHP_ME(CommandFailedEvent, getOperationId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
-                                               PHP_ME(CommandFailedEvent, getRequestId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
-                                                       PHP_ME(CommandFailedEvent, getServer, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
-                                                               ZEND_NAMED_ME(__wakeup, PHP_FN(MongoDB_disabled___wakeup), ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
-                                                                       PHP_FE_END
+       PHP_ME(CommandFailedEvent, getCommandName, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+       PHP_ME(CommandFailedEvent, getError, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+       PHP_ME(CommandFailedEvent, getDurationMicros, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+       PHP_ME(CommandFailedEvent, getOperationId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+       PHP_ME(CommandFailedEvent, getRequestId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+       PHP_ME(CommandFailedEvent, getServer, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+       ZEND_NAMED_ME(__wakeup, PHP_FN(MongoDB_disabled___wakeup), ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+       PHP_FE_END
+       /* clang-format on */
 };
 /* }}} */

I will make a patch to do that for all places — but that might mean a conflict here. For now, I think you can keep it and I'll deal with it in my formatting patch.

PHP_ME(CommandFailedEvent, getRequestId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_ME(CommandFailedEvent, getServer, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
ZEND_NAMED_ME(__wakeup, PHP_FN(MongoDB_disabled___wakeup), ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
Expand All @@ -172,6 +196,10 @@ static void php_phongo_commandfailedevent_free_object(phongo_free_object_arg* ob
zval_ptr_dtor(&intern->z_error);
}

if (intern->reply) {
bson_destroy(intern->reply);
}

if (intern->command_name) {
efree(intern->command_name);
}
Expand Down Expand Up @@ -210,6 +238,7 @@ static HashTable* php_phongo_commandfailedevent_get_debug_info(zval* object, int
php_phongo_commandfailedevent_t* intern;
zval retval = ZVAL_STATIC_INIT;
char operation_id[20], request_id[20];
php_phongo_bson_state reply_state = PHONGO_BSON_STATE_INITIALIZER;

intern = Z_COMMANDFAILEDEVENT_OBJ_P(object);
*is_temp = 1;
Expand All @@ -229,6 +258,13 @@ static HashTable* php_phongo_commandfailedevent_get_debug_info(zval* object, int
sprintf(operation_id, "%" PRIu64, intern->operation_id);
ADD_ASSOC_STRING(&retval, "operationId", operation_id);

php_phongo_bson_to_zval_ex(bson_get_data(intern->reply), intern->reply->len, &reply_state);
#if PHP_VERSION_ID >= 70000
ADD_ASSOC_ZVAL(&retval, "reply", &reply_state.zchild);
#else
ADD_ASSOC_ZVAL(&retval, "reply", reply_state.zchild);
#endif

sprintf(request_id, "%" PRIu64, intern->request_id);
ADD_ASSOC_STRING(&retval, "requestId", request_id);

Expand Down
59 changes: 59 additions & 0 deletions tests/apm/monitoring-commandFailed-003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
--TEST--
MongoDB\Driver\Monitoring\CommandFailedEvent
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php NEEDS('STANDALONE'); NEEDS_ATLEAST_MONGODB_VERSION(STANDALONE, "3.4"); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(STANDALONE);

class MySubscriber implements MongoDB\Driver\Monitoring\CommandSubscriber
{
public function commandStarted( \MongoDB\Driver\Monitoring\CommandStartedEvent $event )
{
echo "started: ", $event->getCommandName(), "\n";
}

public function commandSucceeded( \MongoDB\Driver\Monitoring\CommandSucceededEvent $event )
{
var_dump($event);
}

public function commandFailed( \MongoDB\Driver\Monitoring\CommandFailedEvent $event )
{
echo "failed: ", $event->getCommandName(), "\n";
var_dump($event->getReply());
}
}

$subscriber = new MySubscriber;

MongoDB\Driver\Monitoring\addSubscriber( $subscriber );

$command = new MongoDB\Driver\Command([
'findAndModify' => COLLECTION_NAME,
'query' => ['_id' => 'foo'],
'upsert' => true,
'new' => true,
]);

try {
$manager->executeWriteCommand(DATABASE_NAME, $command);
} catch (MongoDB\Driver\Exception\CommandException $e) {}

?>
--EXPECTF--
started: findAndModify
failed: findAndModify
object(stdClass)#%d (4) {
["ok"]=>
float(0)
["errmsg"]=>
string(49) "Either an update or remove=true must be specified"
["code"]=>
int(9)
["codeName"]=>
string(13) "FailedToParse"
}