Skip to content

PHPC-424: Validate that RP tag set is an array of documents #399

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 2 commits into from
Sep 15, 2016
Merged

PHPC-424: Validate that RP tag set is an array of documents #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bfde7d
Use correct type and format for ReadPreference ctor mode
jmikola Sep 12, 2016
56d8b77
PHPC-424: Validate that RP tag set is an array of documents
jmikola Sep 12, 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
39 changes: 37 additions & 2 deletions php_phongo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,6 +936,29 @@ void php_phongo_read_concern_to_zval(zval *retval, const mongoc_read_concern_t *
}
} /* }}} */

/* Checks if tags is valid to set on a mongoc_read_prefs_t. It may be null or an
* array of one or more documents. */
bool php_phongo_read_preference_tags_are_valid(const bson_t *tags) /* {{{ */
{
bson_iter_t iter;

if (bson_empty0(tags)) {
return true;
}

if (!bson_iter_init(&iter, tags)) {
return false;
}

while (bson_iter_next(&iter)) {
if (!BSON_ITER_HOLDS_DOCUMENT(&iter) && !BSON_ITER_HOLDS_ARRAY(&iter)) {
return false;
}
}

return true;
} /* }}} */

void php_phongo_read_preference_to_zval(zval *retval, const mongoc_read_prefs_t *read_prefs) /* {{{ */
{
const bson_t *tags = mongoc_read_prefs_get_tags(read_prefs);
Expand Down Expand Up @@ -1143,9 +1166,21 @@ static bool php_phongo_apply_rp_options_to_uri(mongoc_uri_t *uri, bson_t *option

bson_iter_array(&iter, &len, &data);

if (bson_init_static(&tags, data, len)) {
mongoc_read_prefs_set_tags(new_rp, &tags);
if (!bson_init_static(&tags, data, len)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Could not initialize BSON structure for read preference tags");
mongoc_read_prefs_destroy(new_rp);

return false;
}

if (!php_phongo_read_preference_tags_are_valid(&tags)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Read preference tags must be an array of zero or more documents");
mongoc_read_prefs_destroy(new_rp);

return false;
}

mongoc_read_prefs_set_tags(new_rp, &tags);
}

if (mongoc_read_prefs_get_mode(new_rp) == MONGOC_READ_PRIMARY &&
Expand Down
2 changes: 2 additions & 0 deletions php_phongo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ const mongoc_write_concern_t* phongo_write_concern_from_zval (zval *zwrite_conc

php_phongo_server_description_type_t php_phongo_server_description_type(mongoc_server_description_t *sd);

bool php_phongo_read_preference_tags_are_valid(const bson_t *tags);

void php_phongo_server_to_zval(zval *retval, mongoc_server_description_t *sd);
void php_phongo_read_concern_to_zval(zval *retval, const mongoc_read_concern_t *read_concern);
void php_phongo_read_preference_to_zval(zval *retval, const mongoc_read_prefs_t *read_prefs);
Expand Down
35 changes: 26 additions & 9 deletions src/MongoDB/ReadPreference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ PHP_METHOD(ReadPreference, __construct)
{
php_phongo_readpreference_t *intern;
zend_error_handling error_handling;
long mode;
phongo_long mode;
zval *tagSets = NULL;
SUPPRESS_UNUSED_WARNING(return_value_ptr) SUPPRESS_UNUSED_WARNING(return_value) SUPPRESS_UNUSED_WARNING(return_value_used)

Expand All @@ -74,22 +74,39 @@ PHP_METHOD(ReadPreference, __construct)
case MONGOC_READ_SECONDARY_PREFERRED:
case MONGOC_READ_NEAREST:
intern->read_preference = mongoc_read_prefs_new(mode);
break;
default:
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Invalid mode: %" PHONGO_LONG_FORMAT, mode);
return;
}

switch(ZEND_NUM_ARGS()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This single-case switch should make sense once you see #400. Since I was refactoring the tagSet validation already, I opted to restructure this early. It should also make the diff for PHPC-752's commit easier to understand.

case 2:
if (tagSets) {
bson_t *tags = bson_new();

phongo_zval_to_bson(tagSets, PHONGO_BSON_NONE, (bson_t *)tags, NULL TSRMLS_CC);
mongoc_read_prefs_set_tags(intern->read_preference, tags);
bson_destroy(tags);
if (!mongoc_read_prefs_is_valid(intern->read_preference)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Invalid tagSets");

if (!php_phongo_read_preference_tags_are_valid(tags)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "tagSets must be an array of zero or more documents");
bson_destroy(tags);
return;
}

if (!bson_empty(tags) && mode == MONGOC_READ_PRIMARY) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "tagSets may not be used with primary mode");
bson_destroy(tags);
return;
}

mongoc_read_prefs_set_tags(intern->read_preference, tags);
bson_destroy(tags);
}
break;
default:
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Invalid mode: %ld", mode);
return;
}

if (!mongoc_read_prefs_is_valid(intern->read_preference)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Read preference is not valid");
return;
}
}
/* }}} */
Expand Down
10 changes: 8 additions & 2 deletions tests/manager/manager-ctor_error-003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,15 @@ echo throws(function() {
}, "MongoDB\Driver\Exception\InvalidArgumentException"), "\n";

echo throws(function() {
$manager = new MongoDB\Driver\Manager(STANDALONE, array('readPreference' => 'nothing'));
$manager = new MongoDB\Driver\Manager(STANDALONE, ['readPreference' => 'nothing']);
}, "MongoDB\Driver\Exception\InvalidArgumentException"), "\n";

echo throws(function() {
$manager = new MongoDB\Driver\Manager(STANDALONE . '/?readPreference=primary', array('readPreferenceTags' => array(array())));
$manager = new MongoDB\Driver\Manager(STANDALONE . '/?readPreference=primary', ['readPreferenceTags' => [[]]]);
}, "MongoDB\Driver\Exception\InvalidArgumentException"), "\n";

echo throws(function() {
$manager = new MongoDB\Driver\Manager(STANDALONE . '/?readPreference=primary', ['readPreferenceTags' => ['invalid']]);
}, "MongoDB\Driver\Exception\InvalidArgumentException"), "\n";

?>
Expand All @@ -28,4 +32,6 @@ OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Unsupported readPreference value: 'nothing'
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Primary read preference mode conflicts with tags
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Read preference tags must be an array of zero or more documents
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should say "Primary read preference mode conflicts with tags". It doesn't matter whether they're valid or not yet.

Copy link
Member Author

@jmikola jmikola Sep 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This is more consistent with the ReadPreference constructor. We're validating individual values before we check the combined state of the read preference. Of course, they'll end up seeing both exceptions in due time (the second will appear after they resolve the first issue).

On that note, I think there's an oversight in php_phongo_apply_rp_options_to_uri() where we're never checking if "maxstalenessms" is a positive value. As-is, I think a negative value would only trigger a generic "Read preference is not valid" error.

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 added another case to the ReadPreference ctor test to demonstrate that it also reports invalid tag set structure first (before the primary conflict).

===DONE===
10 changes: 2 additions & 8 deletions tests/readPreference/readpreference-ctor_error-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,19 @@
--TEST--
MongoDB\Driver\ReadPreference construction (invalid arguments)
MongoDB\Driver\ReadPreference construction (invalid mode)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

echo throws(function() {
new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_PRIMARY, array(array("tag" => "one")));
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
new MongoDB\Driver\ReadPreference(42);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Invalid tagSets
--EXPECT--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Invalid mode: 42
===DONE===
31 changes: 31 additions & 0 deletions tests/readPreference/readpreference-ctor_error-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
--TEST--
MongoDB\Driver\ReadPreference construction (invalid tagSets)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

echo throws(function() {
new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_PRIMARY, [['tag' => 'one']]);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_PRIMARY, ['invalid']);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY, ['invalid']);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
tagSets may not be used with primary mode
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
tagSets must be an array of zero or more documents
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
tagSets must be an array of zero or more documents
===DONE===