Skip to content

PHPC-1195: BulkWrite::update() should append arrayFilters as array #842

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 4 commits into from
May 25, 2018
Merged

PHPC-1195: BulkWrite::update() should append arrayFilters as array #842

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
bf5cf8b
PHPC-1179: arrayFilters tests don't need to start servers
jmikola May 24, 2018
a048e0e
PHPC-1195: BulkWrite::update() should append arrayFilters as array
jmikola May 24, 2018
1b082cf
PHPC-1195: Bump libmongoc to 1.10-dev for CDRIVER-2661
jmikola May 25, 2018
3c8fdfd
Factor out redundant parameter in append_document()
jmikola May 25, 2018
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
93 changes: 82 additions & 11 deletions src/MongoDB/BulkWrite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,79 @@ static inline bool php_phongo_bulkwrite_update_has_operators(bson_t* bupdate) /*
return false;
} /* }}} */

/* Returns whether the BSON array's keys are a sequence of integer strings
* starting with "0". BSON_APPEND_ARRAY considers it the caller's responsibility
* to ensure that the array's keys are properly formatted. */
Copy link
Contributor

Choose a reason for hiding this comment

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

And this is an equivalent to bson-encode.c's php_phongo_is_array_or_document? Should they be placed together perhaps? I don't mind, so please do what makes sense for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

php_phongo_is_array_or_document() operates on a zval, while this validates a bson_t.

We can't work with the zval here, as we're not inspecting it until it's already been converted to a bson_t. In theory, this allows a Serializable value to produce a BSON array for this option.

static inline bool php_phongo_bulkwrite_bson_array_has_valid_keys(bson_t* array) /* {{{ */
{
bson_iter_t iter;

if (bson_empty(array)) {
return true;
}

if (bson_iter_init(&iter, array)) {
char key[12];
int count = 0;

while (bson_iter_next(&iter)) {
bson_snprintf(key, sizeof(key), "%d", count);

if (0 != strcmp(key, bson_iter_key(&iter))) {
return false;
}

count++;
}
}

return true;
} /* }}} */

/* Appends an array field for the given opts document and key. Returns true on
* success; otherwise, false is returned and an exception is thrown. */
static bool php_phongo_bulkwrite_opts_append_array(bson_t* opts, const char* key, zval* zarr TSRMLS_DC) /* {{{ */
{
zval* value = php_array_fetch(zarr, key);
bson_t b = BSON_INITIALIZER;

if (Z_TYPE_P(value) != IS_OBJECT && Z_TYPE_P(value) != IS_ARRAY) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"%s\" option to be array or object, %s given", key, zend_get_type_by_const(Z_TYPE_P(value)));
return false;
}

php_phongo_zval_to_bson(value, PHONGO_BSON_NONE, &b, NULL TSRMLS_CC);

if (EG(exception)) {
bson_destroy(&b);
return false;
}

if (!php_phongo_bulkwrite_bson_array_has_valid_keys(&b)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "\"%s\" option has invalid keys for a BSON array", key);
bson_destroy(&b);
return false;
}

if (!BSON_APPEND_ARRAY(opts, key, &b)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error appending \"%s\" option", key);
bson_destroy(&b);
return false;
}

bson_destroy(&b);
return true;
} /* }}} */

/* Appends a document field for the given opts document and key. Returns true on
* success; otherwise, false is returned and an exception is thrown. */
static bool php_phongo_bulkwrite_opts_append_document(bson_t* opts, const char* opts_key, zval* zarr, const char* zarr_key TSRMLS_DC) /* {{{ */
static bool php_phongo_bulkwrite_opts_append_document(bson_t* opts, const char* key, zval* zarr TSRMLS_DC) /* {{{ */
{
zval* value = php_array_fetch(zarr, zarr_key);
zval* value = php_array_fetch(zarr, key);
bson_t b = BSON_INITIALIZER;

if (Z_TYPE_P(value) != IS_OBJECT && Z_TYPE_P(value) != IS_ARRAY) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"%s\" option to be array or object, %s given", zarr_key, zend_get_type_by_const(Z_TYPE_P(value)));
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"%s\" option to be array or object, %s given", key, zend_get_type_by_const(Z_TYPE_P(value)));
return false;
}

Expand All @@ -92,8 +156,8 @@ static bool php_phongo_bulkwrite_opts_append_document(bson_t* opts, const char*
return false;
}

if (!BSON_APPEND_DOCUMENT(opts, opts_key, &b)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error appending \"%s\" option", opts_key);
if (!BSON_APPEND_DOCUMENT(opts, key, &b)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error appending \"%s\" option", key);
bson_destroy(&b);
return false;
}
Expand All @@ -114,11 +178,18 @@ static bool php_phongo_bulkwrite_opts_append_document(bson_t* opts, const char*
return false; \
}

#define PHONGO_BULKWRITE_OPT_DOCUMENT(opt) \
if (zoptions && php_array_existsc(zoptions, (opt))) { \
if (!php_phongo_bulkwrite_opts_append_document(boptions, (opt), zoptions, (opt) TSRMLS_CC)) { \
return false; \
} \
#define PHONGO_BULKWRITE_OPT_ARRAY(opt) \
if (zoptions && php_array_existsc(zoptions, (opt))) { \
if (!php_phongo_bulkwrite_opts_append_array(boptions, (opt), zoptions TSRMLS_CC)) { \
return false; \
} \
}

#define PHONGO_BULKWRITE_OPT_DOCUMENT(opt) \
if (zoptions && php_array_existsc(zoptions, (opt))) { \
if (!php_phongo_bulkwrite_opts_append_document(boptions, (opt), zoptions TSRMLS_CC)) { \
return false; \
} \
}

/* Applies options (including defaults) for an update operation. */
Expand All @@ -137,7 +208,7 @@ static bool php_phongo_bulkwrite_update_apply_options(bson_t* boptions, zval* zo

PHONGO_BULKWRITE_APPEND_BOOL("multi", multi);
PHONGO_BULKWRITE_APPEND_BOOL("upsert", upsert);
PHONGO_BULKWRITE_OPT_DOCUMENT("arrayFilters");
PHONGO_BULKWRITE_OPT_ARRAY("arrayFilters");
PHONGO_BULKWRITE_OPT_DOCUMENT("collation");

return true;
Expand Down
2 changes: 1 addition & 1 deletion src/libmongoc
Open in desktop
Submodule libmongoc updated 8 files
+1 −1 VERSION_CURRENT
+10 −4 build/generate-opts.py
+1 −0 src/libmongoc/doc/includes/bulk-update-many-opts.txt
+1 −0 src/libmongoc/doc/includes/bulk-update-one-opts.txt
+35 −7 src/libmongoc/src/mongoc/mongoc-bulk-operation.c
+2 −0 src/libmongoc/src/mongoc/mongoc-opts-private.h
+22 −0 src/libmongoc/src/mongoc/mongoc-opts.c
+111 −0 src/libmongoc/tests/test-mongoc-bulk.c
12 changes: 4 additions & 8 deletions tests/bulk/bulkwrite-update-arrayFilters-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
--TEST--
MongoDB\Driver\BulkWrite::update with arrayFilters
--XFAIL--
START() tests must be reimplemented (PHPC-1179)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php START('THROWAWAY', ["version" => "36-release"]); CLEANUP(THROWAWAY); ?>
<?php skip_if_not_live(); ?>
<?php skip_if_server_version('<', '3.6'); ?>
<?php skip_if_not_clean(); ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit misleading, innit? skip_if_not_clean will also clean first?

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 change was made during #787.

Technically, the function attempts to drop the test collection and skips the test if that fails for some reason. I'll admit it's unlike other skip_if functions because it attempts to change the state of the system; however, it can actually skip if something goes wrong. In the past, CLEAN() ignored any errors.

If you have some ideas for renaming or revising this, a new ticket would be best 👍

--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(THROWAWAY);
$manager = new MongoDB\Driver\Manager(URI);

$bulk = new MongoDB\Driver\BulkWrite();

Expand All @@ -35,11 +35,7 @@ $cursor = $manager->executeQuery( DATABASE_NAME . '.' . COLLECTION_NAME, new \Mo
var_dump($cursor->toArray());
?>
===DONE===
<?php DELETE("THROWAWAY"); ?>
<?php exit(0); ?>
--CLEAN--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php DELETE("THROWAWAY"); ?>
--EXPECTF--
array(%d) {
[0]=>
Expand Down
14 changes: 14 additions & 0 deletions tests/bulk/bulkwrite-update_error-003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ echo throws(function() use ($bulk) {

echo throws(function() use ($bulk) {
$bulk->update(['x' => 1], ['$set' => ['y' => 1]], ['collation' => 1]);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n\n";

echo throws(function() use ($bulk) {
$bulk->update(['x' => 1], ['$set' => ['y' => 1]], ['arrayFilters' => 1]);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n\n";

echo throws(function() use ($bulk) {
$bulk->update(['x' => 1], ['$set' => ['y' => 1]], ['arrayFilters' => ['foo' => 'bar']]);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

?>
Expand All @@ -31,4 +39,10 @@ Expected "collation" option to be array or object, int%S given

OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected "collation" option to be array or object, int%S given

OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected "arrayFilters" option to be array or object, int%S given

OK: Got MongoDB\Driver\Exception\InvalidArgumentException
"arrayFilters" option has invalid keys for a BSON array
===DONE===
12 changes: 4 additions & 8 deletions tests/command/findAndModify-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
--TEST--
MongoDB\Driver\Command with findAndModify and arrayFilters
--XFAIL--
START() tests must be reimplemented (PHPC-1179)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php START('THROWAWAY', ["version" => "36-release"]); CLEANUP(THROWAWAY); ?>
<?php skip_if_not_live(); ?>
<?php skip_if_server_version('<', '3.6'); ?>
<?php skip_if_not_clean(); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(THROWAWAY);
$manager = new MongoDB\Driver\Manager(URI);

$bulk = new MongoDB\Driver\BulkWrite();

Expand All @@ -35,11 +35,7 @@ $cursor = $manager->executeQuery( DATABASE_NAME . '.' . COLLECTION_NAME, new \Mo
var_dump($cursor->toArray());
?>
===DONE===
<?php DELETE("THROWAWAY"); ?>
<?php exit(0); ?>
--CLEAN--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php DELETE("THROWAWAY"); ?>
--EXPECTF--
array(%d) {
[0]=>
Expand Down
12 changes: 4 additions & 8 deletions tests/command/update-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
--TEST--
MongoDB\Driver\Command with update and arrayFilters
--XFAIL--
START() tests must be reimplemented (PHPC-1179)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php START('THROWAWAY', ["version" => "36-release"]); CLEANUP(THROWAWAY); ?>
<?php skip_if_not_live(); ?>
<?php skip_if_server_version('<', '3.6'); ?>
<?php skip_if_not_clean(); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(THROWAWAY);
$manager = new MongoDB\Driver\Manager(URI);

$bulk = new MongoDB\Driver\BulkWrite();

Expand All @@ -35,11 +35,7 @@ $cursor = $manager->executeQuery( DATABASE_NAME . '.' . COLLECTION_NAME, new \Mo
var_dump($cursor->toArray());
?>
===DONE===
<?php DELETE("THROWAWAY"); ?>
<?php exit(0); ?>
--CLEAN--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php DELETE("THROWAWAY"); ?>
--EXPECTF--
array(%d) {
[0]=>
Expand Down