Skip to content

PHPC-1222: Implement accessor for error labels on exception classes #870

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

Merged
merged 1 commit into from
Jul 3, 2018
Merged

PHPC-1222: Implement accessor for error labels on exception classes #870

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
54 changes: 50 additions & 4 deletions php_phongo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,49 @@ void phongo_throw_exception(php_phongo_error_domain_t domain TSRMLS_DC, const ch
va_end(args);
}

void phongo_throw_exception_from_bson_error_t(bson_error_t* error TSRMLS_DC)
static void phongo_exception_add_error_labels(bson_t* reply TSRMLS_DC)
{
zend_throw_exception(phongo_exception_from_mongoc_domain(error->domain, error->code), error->message, error->code TSRMLS_CC);
bson_iter_t iter;

if (!reply) {
return;
}

if (bson_iter_init_find(&iter, reply, "errorLabels")) {
bson_iter_t error_labels;
#if PHP_VERSION_ID >= 70000
zval labels;

array_init(&labels);
#else
zval* labels = NULL;

ALLOC_INIT_ZVAL(labels);
array_init(labels);
#endif

bson_iter_recurse(&iter, &error_labels);
while (bson_iter_next(&error_labels)) {
if (BSON_ITER_HOLDS_UTF8(&error_labels)) {
const char* error_label;
uint32_t error_label_len;

error_label = bson_iter_utf8(&error_labels, &error_label_len);
#if PHP_VERSION_ID >= 70000
ADD_NEXT_INDEX_STRINGL(&labels, error_label, error_label_len);
#else
ADD_NEXT_INDEX_STRINGL(labels, error_label, error_label_len);
#endif
}
}

#if PHP_VERSION_ID >= 70000
phongo_add_exception_prop(ZEND_STRL("errorLabels"), &labels);
#else
phongo_add_exception_prop(ZEND_STRL("errorLabels"), labels TSRMLS_CC);
#endif
zval_ptr_dtor(&labels);
}
}

void phongo_throw_exception_from_bson_error_and_reply_t(bson_error_t* error, bson_t* reply TSRMLS_DC)
Expand All @@ -192,7 +232,7 @@ void phongo_throw_exception_from_bson_error_and_reply_t(bson_error_t* error, bso
* may use CommandException and report the result document for the
* failed command. For BC, ExceededTimeLimit errors will continue to use
* ExcecutionTimeoutException and omit the result document. */
if ((error->domain == MONGOC_ERROR_SERVER && error->code != PHONGO_SERVER_ERROR_EXCEEDED_TIME_LIMIT) || error->domain == MONGOC_ERROR_WRITE_CONCERN) {
if (reply && ((error->domain == MONGOC_ERROR_SERVER && error->code != PHONGO_SERVER_ERROR_EXCEEDED_TIME_LIMIT) || error->domain == MONGOC_ERROR_WRITE_CONCERN)) {
#if PHP_VERSION_ID >= 70000
zval zv;
#else
Expand All @@ -209,8 +249,14 @@ void phongo_throw_exception_from_bson_error_and_reply_t(bson_error_t* error, bso
#endif
zval_ptr_dtor(&zv);
} else {
phongo_throw_exception_from_bson_error_t(error TSRMLS_CC);
zend_throw_exception(phongo_exception_from_mongoc_domain(error->domain, error->code), error->message, error->code TSRMLS_CC);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to use phongo_throw_exception_from_bson_error_t(error TSRMLS_CC); here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because phongo_throw_exception_from_bson_error_t is now a wrapper for phongo_throw_exception_from_bson_error_and_reply_t.

Copy link
Member

Choose a reason for hiding this comment

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

Now that you added back phongo_throw_exception_from_bson_error_t() I think we can use it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we can't, as phongo_throw_exception_from_bson_error_t is now a wrapper for phongo_throw_exception_from_bson_error_and_reply_t:

void phongo_throw_exception_from_bson_error_t(bson_error_t* error TSRMLS_DC)
{
    phongo_throw_exception_from_bson_error_and_reply_t(error, NULL TSRMLS_CC);
}

}
phongo_exception_add_error_labels(reply TSRMLS_CC);
}

void phongo_throw_exception_from_bson_error_t(bson_error_t* error TSRMLS_DC)
{
phongo_throw_exception_from_bson_error_and_reply_t(error, NULL TSRMLS_CC);
}

static void php_phongo_log(mongoc_log_level_t log_level, const char* log_domain, const char* message, void* user_data)
Expand Down
78 changes: 78 additions & 0 deletions src/MongoDB/Exception/RuntimeException.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,88 @@

#include "phongo_compat.h"
#include "php_phongo.h"
#include "php_array_api.h"

zend_class_entry* php_phongo_runtimeexception_ce;

static bool php_phongo_has_string_array_element(zval* labels, char* label TSRMLS_DC)
{
HashTable* ht_data;

if (Z_TYPE_P(labels) != IS_ARRAY) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

I pondered whether a LogicException or something similar would work here, but it's probably best to keep hasErrorLabel() as simple as possible. The edge case I was thinking of was if someone uses reflection to re-assign our private property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Copy link
Member

@jmikola jmikola Jun 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What is "added" referring to? I wasn't suggesting actually throwing, just explaining my thought process. I do think it's preferable to just return false here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added "added" to the wrong place I think. Want to say I added the test cases, perhaps? /me shrugs

}

ht_data = HASH_OF(labels);

#if PHP_VERSION_ID >= 70000
{
zval* z_label;

ZEND_HASH_FOREACH_VAL(ht_data, z_label)
{
if ((Z_TYPE_P(z_label) == IS_STRING) && (strcmp(Z_STRVAL_P(z_label), label) == 0)) {
return true;
}
}
ZEND_HASH_FOREACH_END();
}
#else
{
HashPosition pos;
zval** z_label;

for (
zend_hash_internal_pointer_reset_ex(ht_data, &pos);
zend_hash_get_current_data_ex(ht_data, (void**) &z_label, &pos) == SUCCESS;
zend_hash_move_forward_ex(ht_data, &pos)) {

if (Z_TYPE_PP(z_label) == IS_STRING) {
if (strcmp(Z_STRVAL_PP(z_label), label) == 0) {
return true;
}
}
}
}
#endif

return false;
}

/* {{{ proto bool MongoDB\Driver\Exception\RuntimeException::hasErrorLabel(string $label)
Returns whether a specific error label has been set */
static PHP_METHOD(RuntimeException, hasErrorLabel)
{
char* label;
phongo_zpp_char_len label_len;
zval* error_labels;
#if PHP_VERSION_ID >= 70000
zval rv;
#endif

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &label, &label_len) == FAILURE) {
return;
}

#if PHP_VERSION_ID >= 70000
error_labels = zend_read_property(php_phongo_runtimeexception_ce, getThis(), ZEND_STRL("errorLabels"), 0, &rv TSRMLS_CC);
#else
error_labels = zend_read_property(php_phongo_runtimeexception_ce, getThis(), ZEND_STRL("errorLabels"), 0 TSRMLS_CC);
#endif

RETURN_BOOL(php_phongo_has_string_array_element(error_labels, label TSRMLS_CC));
} /* }}} */

ZEND_BEGIN_ARG_INFO_EX(ai_RuntimeException_hasErrorLabel, 0, 0, 1)
ZEND_ARG_INFO(0, label)
ZEND_END_ARG_INFO()

/* {{{ MongoDB\Driver\Exception\RuntimeException function entries */
static zend_function_entry php_phongo_runtimeexception_me[] = {
/* clang-format off */
PHP_ME(RuntimeException, hasErrorLabel, ai_RuntimeException_hasErrorLabel, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
PHP_FE_END
/* clang-format on */
};
/* }}} */

Expand All @@ -43,6 +119,8 @@ void php_phongo_runtimeexception_init_ce(INIT_FUNC_ARGS) /* {{{ */
php_phongo_runtimeexception_ce = zend_register_internal_class_ex(&ce, spl_ce_RuntimeException, NULL TSRMLS_CC);
#endif
zend_class_implements(php_phongo_runtimeexception_ce TSRMLS_CC, 1, php_phongo_exception_ce);

zend_declare_property_null(php_phongo_runtimeexception_ce, ZEND_STRL("errorLabels"), ZEND_ACC_PROTECTED TSRMLS_CC);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason this can't be private? hasErrorLabel() is final on this class and should be the only method that accesses it.

I realize WriteException::$writeResult is also protected (and only accessed by getWriteResult(). Perhaps protected is needed so that we can update the property on any class? If so, I wonder if that's simply a problem with phongo_add_exception_prop() not using the base class entry when it calls zend_update_property().

Copy link
Contributor Author

@derickr derickr Jul 2, 2018 •
edited by jmikola
Loading

Choose a reason for hiding this comment

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

I realize WriteException::$writeResult is also protected (and only accessed by getWriteResult(). Perhaps protected is needed so that we can update the property on any class?

Yes, that's the case:

001+ Fatal error: Uncaught ReflectionException: Property errorLabels does not exist in /home/derick/dev/php/derickr-mongo-php-driver/tests/exception/bulkwriteexception-haserrorlabel-001.php

If so, I wonder if that's simply a problem with phongo_add_exception_prop() not using the base class entry when it calls zend_update_property().

It doesn't matter when I change it to use the base class (php_phongo_runtimeexception_ce). So, I think we're stuck with this.

} /* }}} */

/*
Expand Down
2 changes: 2 additions & 0 deletions tests/apm/overview.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ object(MongoDB\Driver\Monitoring\CommandFailedEvent)#%d (%d) {
%a
["previous":"Exception":private]=>
NULL
["errorLabels":protected]=>
NULL
}
["operationId"]=>
string(%d) "%s"
Expand Down
24 changes: 24 additions & 0 deletions tests/exception/bulkwriteexception-haserrorlabel-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
MongoDB\Driver\Exception\BulkWriteException::hasErrorLabel()
--FILE--
<?php

$exception = new MongoDB\Driver\Exception\BulkWriteException();
$labels = ['test', 'foo'];

$reflection = new ReflectionClass($exception);

$resultDocumentProperty = $reflection->getProperty('errorLabels');
$resultDocumentProperty->setAccessible(true);
$resultDocumentProperty->setValue($exception, $labels);

var_dump($exception->hasErrorLabel('foo'));
var_dump($exception->hasErrorLabel('bar'));

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
bool(true)
bool(false)
===DONE===
22 changes: 22 additions & 0 deletions tests/exception/bulkwriteexception-haserrorlabel_error-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
MongoDB\Driver\Exception\BulkWriteException::hasErrorLabel() with non-array values
--FILE--
<?php

$exception = new MongoDB\Driver\Exception\BulkWriteException();
$labels = 'shouldBeAnArray';

$reflection = new ReflectionClass($exception);

$resultDocumentProperty = $reflection->getProperty('errorLabels');
$resultDocumentProperty->setAccessible(true);
$resultDocumentProperty->setValue($exception, $labels);

var_dump($exception->hasErrorLabel('bar'));

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
bool(false)
===DONE===
24 changes: 24 additions & 0 deletions tests/exception/commandexception-haserrorlabel-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
MongoDB\Driver\Exception\CommandException::hasErrorLabel()
--FILE--
<?php

$exception = new MongoDB\Driver\Exception\CommandException();
$labels = ['test', 'foo'];

$reflection = new ReflectionClass($exception);

$resultDocumentProperty = $reflection->getProperty('errorLabels');
$resultDocumentProperty->setAccessible(true);
$resultDocumentProperty->setValue($exception, $labels);

var_dump($exception->hasErrorLabel('foo'));
var_dump($exception->hasErrorLabel('bar'));

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
bool(true)
bool(false)
===DONE===
22 changes: 22 additions & 0 deletions tests/exception/commandexception-haserrorlabel_error-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
MongoDB\Driver\Exception\CommandException::hasErrorLabel() with non-array values
--FILE--
<?php

$exception = new MongoDB\Driver\Exception\CommandException();
$labels = 'shouldBeAnArray';

$reflection = new ReflectionClass($exception);

$resultDocumentProperty = $reflection->getProperty('errorLabels');
$resultDocumentProperty->setAccessible(true);
$resultDocumentProperty->setValue($exception, $labels);

var_dump($exception->hasErrorLabel('bar'));

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
bool(false)
===DONE===
24 changes: 24 additions & 0 deletions tests/exception/runtimeexception-haserrorlabel-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
MongoDB\Driver\Exception\RuntimeException::hasErrorLabel()
--FILE--
<?php

$exception = new MongoDB\Driver\Exception\RuntimeException();
$labels = ['test', 'foo'];

$reflection = new ReflectionClass($exception);

$resultDocumentProperty = $reflection->getProperty('errorLabels');
$resultDocumentProperty->setAccessible(true);
$resultDocumentProperty->setValue($exception, $labels);

var_dump($exception->hasErrorLabel('foo'));
var_dump($exception->hasErrorLabel('bar'));

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
bool(true)
bool(false)
===DONE===
22 changes: 22 additions & 0 deletions tests/exception/runtimeexception-haserrorlabel_error-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
MongoDB\Driver\Exception\RuntimeException::hasErrorLabel() with non-array values
--FILE--
<?php

$exception = new MongoDB\Driver\Exception\RuntimeException();
$labels = 'shouldBeAnArray';

$reflection = new ReflectionClass($exception);

$resultDocumentProperty = $reflection->getProperty('errorLabels');
$resultDocumentProperty->setAccessible(true);
$resultDocumentProperty->setValue($exception, $labels);

var_dump($exception->hasErrorLabel('bar'));

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
bool(false)
===DONE===
4 changes: 1 addition & 3 deletions tests/session/transaction-integration-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,7 @@ try {
] );
$manager->executeCommand(DATABASE_NAME, $cmd, ['session' => $sessionB]);
} catch (MongoDB\Driver\Exception\CommandException $e) {
$rd = $e->getResultDocument();

echo (isset($rd->errorLabels) && in_array("TransientTransactionError", $rd->errorLabels)) ?
echo $e->hasErrorLabel('TransientTransactionError') ?
Copy link
Member

Choose a reason for hiding this comment

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

Before merging, will this PR have some tests demonstrating that we can call hasErrorLabel() on a basic RuntimeException (or at least things lower in the stack than a CommandException)? BulkWriteException would be important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had written these, but forgotten to git add them.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this test was only using executeCommand() so that you could inspect the error label by calling CommandException::getResultDocument(). I think this should be rewritten to execute a bulk write and then call hasErrorLabel on the BulkWriteException.

That may not be possible in this PR, as phongo_execute_bulk_write() doesn't appear to be considering the reply document when throwing its exception. If so, please link a new ticket to PHPC-1222 to populate error labels for bulk writes and make a note about updating this test.

"found a TransientTransactionError" : "did NOT get a TransientTransactionError", "\n";
}
?>
Expand Down