Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed PHP-774: Mark MongoCollection::toIndexString as deprecated. #434

Closed
wants to merge 4 commits into from

2 participants

@derickr
Owner

For this to work, I also had to separate it out as otherwise our own
ensureIndex() would throw a deprecation warning.

@derickr derickr Fixed PHP-774: Mark MongoCollection::toIndexString as deprecated.
For this to work, I also had to separate it out as otherwise our own
ensureIndex() would throw a deprecation warning.
0d60c15
@bjori
Owner

This change caused TEST 73/429 [tests/generic/collection-index-sparse.phpt]
to segfault on travis

collection.c
((9 lines not shown))
- MONGO_METHOD1(MongoCollection, toIndexString, key_str, NULL, keys);
+ key_str = to_index_string(keys);
+ key_str_len = strlen(key_str);
@bjori Owner
bjori added a note

to_index_string() can return NULL.

Also, since to_index_string() has the length of the return value, why not let it set it?

@derickr Owner
derickr added a note

I'll fix the NULL issue, but with the current code figuring out the length is not trivial enough, so I'd like to leave that for the "rewriting of to_index_string" (for which I'll make a ticket shortly).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
collection.c
@@ -1200,11 +1196,12 @@ static void do_safe_op(mongo_con_manager *manager, mongo_connection *connection,
array_init(data);
add_assoc_zval(data, "deleteIndexes", c->name);
zval_add_ref(&c->name);
- add_assoc_zval(data, "index", key_str);
+ add_assoc_string(data, "index", key_str, 0);
@bjori Owner
bjori added a note

if to_index_str() gave you the lengh of key_str you could use add_assoc_stringl() here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bjori
Owner

The travis builds also fail because this breaks zts builds

@bjori bjori commented on the diff
collection.c
((6 lines not shown))
MONGO_CMD(return_value, c->parent);
zval_ptr_dtor(&data);
+ efree(key_str);
@bjori Owner
bjori added a note

Wait, why are you freeing it here?
You didn't duplicate it when you add_assoc_string() it to data, so the zval_ptr_dtor() on data will free it..

@derickr Owner
derickr added a note

Good catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bjori bjori commented on the diff
collection.c
@@ -1200,11 +1202,12 @@ static void do_safe_op(mongo_con_manager *manager, mongo_connection *connection,
array_init(data);
add_assoc_zval(data, "deleteIndexes", c->name);
zval_add_ref(&c->name);
- add_assoc_zval(data, "index", key_str);
+ add_assoc_string(data, "index", key_str, 1);
@bjori Owner
bjori added a note

This has the same problem as the otherone I commented on. Since you are not passing the length here, add_assoc_string() does a strlen(key_str), but to_index_string() can return a NULL, so the result becomes strlen(NULL).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@derickr derickr Fixed another NULL-dereference
And adding missing tests for deleteIndex/deleteIndexes so that we can see that
this failed. However, I noticed a really annoying behaviour for deleteIndex(
string ). Instead of it dropping an index with the name "string", it uses it as
a field name to append "_1" too. So you can not use it at all for user-named
indexes. I've added issue PHP-817 to the 2.x milestone to sort that out.
05c07bd
@derickr
Owner

Ok, I fixed the other NULL reference and added some missing tests.

@bjori bjori commented on the diff
collection.c
((9 lines not shown))
+
+/* {{{ proto protected static string MongoCollection::toIndexString(array|string keys)
+ Converts $keys to an identifying string for an index */
+PHP_METHOD(MongoCollection, toIndexString)
+{
+ zval *zkeys;
+ char *key_str;
+
+ if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z", &zkeys) == FAILURE) {
+ return;
+ }
+
+ key_str = to_index_string(zkeys TSRMLS_CC);
+
+ if (key_str) {
+ RETVAL_STRING(key_str, 0);
@bjori Owner
bjori added a note

is to_indext_string() not \0 safe?
It still feels very odd that we don't get the length from to_index_string()

@derickr Owner
derickr added a note

Keys in mongodb can't contain nulls, so that's not going to work to create an index anyway... I'd like to get the length back, and added a comment to the smart_str part of this patch.

@derickr Owner
derickr added a note

Also checked, the current existing code already doesn't handle it either. I'll fix it when fixing PHP-807.

@bjori Owner
bjori added a note

fair enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bjori
Owner

LGTM

  • Verify how toIndexString() works with NUL
@derickr derickr referenced this pull request from a commit
@derickr derickr Merged pull request #434 f059db6
@derickr derickr closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 7, 2013
  1. @derickr

    Fixed PHP-774: Mark MongoCollection::toIndexString as deprecated.

    derickr authored
    For this to work, I also had to separate it out as otherwise our own
    ensureIndex() would throw a deprecation warning.
Commits on May 8, 2013
  1. @derickr
  2. @derickr

    Fixed tests for PHP 5.2.

    derickr authored
Commits on May 9, 2013
  1. @derickr

    Fixed another NULL-dereference

    derickr authored
    And adding missing tests for deleteIndex/deleteIndexes so that we can see that
    this failed. However, I noticed a really annoying behaviour for deleteIndex(
    string ). Instead of it dropping an index with the name "string", it uses it as
    a field name to append "_1" too. So you can not use it at all for user-named
    indexes. I've added issue PHP-817 to the 2.x milestone to sort that out.
This page is out of date. Refresh to see the latest.
View
76 collection.c
@@ -42,6 +42,7 @@ static int is_gle_op(zval *options, mongo_server_options *server_options TSRMLS_
static void do_safe_op(mongo_con_manager *manager, mongo_connection *connection, zval *cursor_z, buffer *buf, zval *return_value TSRMLS_DC);
static zval* append_getlasterror(zval *coll, buffer *buf, zval *options, mongo_connection *connection TSRMLS_DC);
static int php_mongo_trigger_error_on_command_failure(zval *document TSRMLS_DC);
+static char *to_index_string(zval *zkeys TSRMLS_DC);
/* {{{ proto MongoCollection MongoCollection::__construct(MongoDB db, string name)
Initializes a new MongoCollection */
@@ -1048,7 +1049,7 @@ PHP_METHOD(MongoCollection, remove)
Create the $keys index if it does not already exist */
PHP_METHOD(MongoCollection, ensureIndex)
{
- zval *keys, *options = 0, *db, *system_indexes, *collection, *data, *key_str;
+ zval *keys, *options = 0, *db, *system_indexes, *collection, *data;
mongo_collection *c;
zend_bool done_name = 0;
@@ -1148,22 +1149,27 @@ PHP_METHOD(MongoCollection, ensureIndex)
}
if (!done_name) {
- /* turn keys into a string */
- MAKE_STD_ZVAL(key_str);
- ZVAL_NULL(key_str);
+ char *key_str;
+ int key_str_len;
- MONGO_METHOD1(MongoCollection, toIndexString, key_str, NULL, keys);
+ key_str = to_index_string(keys TSRMLS_CC);
+ if (!key_str) {
+ zval_ptr_dtor(&data);
+ zval_ptr_dtor(&options);
+ return;
+ }
+
+ key_str_len = strlen(key_str);
- if (Z_STRLEN_P(key_str) > MAX_INDEX_NAME_LEN) {
+ if (key_str_len > MAX_INDEX_NAME_LEN) {
zval_ptr_dtor(&data);
- zend_throw_exception_ex(mongo_ce_Exception, 14 TSRMLS_CC, "index name too long: %d, max %d characters", Z_STRLEN_P(key_str), MAX_INDEX_NAME_LEN);
- zval_ptr_dtor(&key_str);
+ zend_throw_exception_ex(mongo_ce_Exception, 14 TSRMLS_CC, "index name too long: %d, max %d characters", key_str_len, MAX_INDEX_NAME_LEN);
+ efree(key_str);
zval_ptr_dtor(&options);
return;
}
- add_assoc_zval(data, "name", key_str);
- zval_add_ref(&key_str);
+ add_assoc_stringl(data, "name", key_str, key_str_len, 0);
}
MONGO_METHOD2(MongoCollection, insert, return_value, collection, data, options);
@@ -1173,10 +1179,6 @@ PHP_METHOD(MongoCollection, ensureIndex)
zval_ptr_dtor(&system_indexes);
zval_ptr_dtor(&collection);
zval_ptr_dtor(&keys);
-
- if (!done_name) {
- zval_ptr_dtor(&key_str);
- }
}
/* }}} */
@@ -1184,15 +1186,18 @@ PHP_METHOD(MongoCollection, ensureIndex)
Remove the $keys index */
PHP_METHOD(MongoCollection, deleteIndex)
{
- zval *keys, *key_str, *data;
+ zval *keys, *data;
+ char *key_str;
mongo_collection *c;
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z", &keys) == FAILURE) {
return;
}
- MAKE_STD_ZVAL(key_str);
- MONGO_METHOD1(MongoCollection, toIndexString, key_str, NULL, keys);
+ key_str = to_index_string(keys TSRMLS_CC);
+ if (!key_str) {
+ return;
+ }
PHP_MONGO_GET_COLLECTION(getThis());
@@ -1200,11 +1205,12 @@ PHP_METHOD(MongoCollection, deleteIndex)
array_init(data);
add_assoc_zval(data, "deleteIndexes", c->name);
zval_add_ref(&c->name);
- add_assoc_zval(data, "index", key_str);
+ add_assoc_string(data, "index", key_str, 1);
@bjori Owner
bjori added a note

This has the same problem as the otherone I commented on. Since you are not passing the length here, add_assoc_string() does a strlen(key_str), but to_index_string() can return a NULL, so the result becomes strlen(NULL).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
MONGO_CMD(return_value, c->parent);
zval_ptr_dtor(&data);
+ efree(key_str);
@bjori Owner
bjori added a note

Wait, why are you freeing it here?
You didn't duplicate it when you add_assoc_string() it to data, so the zval_ptr_dtor() on data will free it..

@derickr Owner
derickr added a note

Good catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
/* }}} */
@@ -1422,18 +1428,11 @@ static char *replace_dots(char *key, int key_len, char *position)
return position;
}
-/* {{{ proto protected static string MongoCollection::toIndexString(array|string keys)
- Converts $keys to an identifying string for an index */
-PHP_METHOD(MongoCollection, toIndexString)
+static char *to_index_string(zval *zkeys TSRMLS_DC)
{
- zval *zkeys;
char *name, *position;
int len = 0;
- if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z", &zkeys) == FAILURE) {
- return;
- }
-
if (Z_TYPE_P(zkeys) == IS_ARRAY || Z_TYPE_P(zkeys) == IS_OBJECT) {
HashTable *hindex = HASH_P(zkeys);
HashPosition pointer;
@@ -1532,9 +1531,30 @@ PHP_METHOD(MongoCollection, toIndexString)
*(position) = '\0';
} else {
php_error_docref(NULL TSRMLS_CC, E_WARNING, "The key needs to be either a string or an array");
+ return NULL;
+ }
+
+ return name;
+}
+
+/* {{{ proto protected static string MongoCollection::toIndexString(array|string keys)
+ Converts $keys to an identifying string for an index */
+PHP_METHOD(MongoCollection, toIndexString)
+{
+ zval *zkeys;
+ char *key_str;
+
+ if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z", &zkeys) == FAILURE) {
+ return;
+ }
+
+ key_str = to_index_string(zkeys TSRMLS_CC);
+
+ if (key_str) {
+ RETVAL_STRING(key_str, 0);
@bjori Owner
bjori added a note

is to_indext_string() not \0 safe?
It still feels very odd that we don't get the length from to_index_string()

@derickr Owner
derickr added a note

Keys in mongodb can't contain nulls, so that's not going to work to create an index anyway... I'd like to get the length back, and added a comment to the smart_str part of this patch.

@derickr Owner
derickr added a note

Also checked, the current existing code already doesn't handle it either. I'll fix it when fixing PHP-807.

@bjori Owner
bjori added a note

fair enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ } else {
return;
}
- RETURN_STRING(name, 0)
}
/* }}} */
@@ -1901,7 +1921,7 @@ static zend_function_entry MongoCollection_methods[] = {
PHP_ME(MongoCollection, save, arginfo_insert, ZEND_ACC_PUBLIC)
PHP_ME(MongoCollection, createDBRef, arginfo_createDBRef, ZEND_ACC_PUBLIC)
PHP_ME(MongoCollection, getDBRef, arginfo_getDBRef, ZEND_ACC_PUBLIC)
- PHP_ME(MongoCollection, toIndexString, arginfo_toIndexString, ZEND_ACC_PROTECTED|ZEND_ACC_STATIC)
+ PHP_ME(MongoCollection, toIndexString, arginfo_toIndexString, ZEND_ACC_PROTECTED|ZEND_ACC_DEPRECATED|ZEND_ACC_STATIC)
PHP_ME(MongoCollection, group, arginfo_group, ZEND_ACC_PUBLIC)
PHP_ME(MongoCollection, distinct, arginfo_distinct, ZEND_ACC_PUBLIC)
PHP_ME(MongoCollection, aggregate, arginfo_aggregate, ZEND_ACC_PUBLIC)
View
35 tests/generic/mongocollection-deleteindex.phpt
@@ -0,0 +1,35 @@
+--TEST--
+MongoCollection::deleteIndex()
+--SKIPIF--
+<?php require_once "tests/utils/standalone.inc"; ?>
+--FILE--
+<?php
+require_once "tests/utils/server.inc";
+$m = mongo_standalone();
+$name = dbname();
+$d = $m->$name;
+$c = $d->deleteIndex;
+$ns = $d->selectCollection('system.indexes');
+$c->drop();
+
+echo "Delete with keys\n";
+$c->ensureIndex( array( 'surname' => 1, 'name' => 1 ) );
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+$c->deleteIndex( array( 'surname' => 1, 'name' => 1 ) );
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+
+echo "Delete with name\n";
+$c->ensureIndex( array( 'surname' => 1, 'name' => 1 ) );
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+/* This is actually cheating, as for some annoying (but documented) reason, a
+ * string is not an index name, but a field name. */
+$c->deleteIndex( 'surname_1_name');
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+?>
+--EXPECT--
+Delete with keys
+int(2)
+int(1)
+Delete with name
+int(2)
+int(1)
View
27 tests/generic/mongocollection-deleteindex_error.phpt
@@ -0,0 +1,27 @@
+--TEST--
+MongoCollection::deleteIndex() error with invalid params
+--SKIPIF--
+<?php require_once "tests/utils/standalone.inc"; ?>
+--FILE--
+<?php
+require_once "tests/utils/server.inc";
+$m = mongo_standalone();
+$name = dbname();
+$d = $m->$name;
+$c = $d->deleteIndex;
+$ns = $d->selectCollection('system.indexes');
+$c->drop();
+
+$c->ensureIndex( array( 'surname' => 1, 'name' => 1 ) );
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+
+echo "Wrong argument\n";
+$c->deleteIndex( 42 );
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+?>
+--EXPECTF--
+int(2)
+Wrong argument
+
+Warning: MongoCollection::deleteIndex(): The key needs to be either a string or an array in %smongocollection-deleteindex_error.php on line %d
+int(2)
View
34 tests/generic/mongocollection-deleteindexes.phpt
@@ -0,0 +1,34 @@
+--TEST--
+MongoCollection::deleteIndexes()
+--SKIPIF--
+<?php require_once "tests/utils/standalone.inc"; ?>
+--FILE--
+<?php
+require_once "tests/utils/server.inc";
+$m = mongo_standalone();
+$name = dbname();
+$d = $m->$name;
+$c = $d->deleteIndex;
+$ns = $d->selectCollection('system.indexes');
+$c->drop();
+
+echo "Delete all (except the _id index)\n";
+$c->ensureIndex( array( 'surname' => 1, 'name' => 1 ) );
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+$c->deleteIndexes();
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+
+echo "Delete two\n";
+$c->ensureIndex( array( 'name' => 1 ) );
+$c->ensureIndex( array( 'surname' => 1, 'name' => 1 ) );
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+$c->deleteIndexes();
+var_dump( count(iterator_to_array( $ns->find( array( 'ns' => "$name.deleteIndex" ) ) ) ) );
+?>
+--EXPECT--
+Delete all (except the _id index)
+int(2)
+int(1)
+Delete two
+int(3)
+int(1)
View
2  tests/no-servers/mongocollection-toindexstring-broken.phpt
@@ -12,5 +12,7 @@ class MyCollection extends MongoCollection
var_dump(MyCollection::toIndexString(null));
?>
--EXPECTF--
+%s: Function MongoCollection::toIndexString() is deprecated in %smongocollection-toindexstring-broken.php on line 6
+
%s: MongoCollection::toIndexString(): The key needs to be either a string or an array in %smongocollection-toindexstring-broken.php on line 6
NULL
View
13 tests/no-servers/mongocollection-toindexstring.phpt
@@ -18,10 +18,21 @@ var_dump(MyCollection::toIndexString(array('x' => 1)));
var_dump(MyCollection::toIndexString(array('x' => -1)));
var_dump(MyCollection::toIndexString(array('x' => 1, 'y' => -1)));
?>
---EXPECT--
+--EXPECTF--
+%s: Function MongoCollection::toIndexString() is deprecated in %smongocollection-toindexstring.php on line %d
string(3) "x_1"
+
+%s: Function MongoCollection::toIndexString() is deprecated in %smongocollection-toindexstring.php on line %d
string(7) "x_y_z_1"
+
+%s: Function MongoCollection::toIndexString() is deprecated in %smongocollection-toindexstring.php on line %d
string(7) "x_y_z_1"
+
+%s: Function MongoCollection::toIndexString() is deprecated in %smongocollection-toindexstring.php on line %d
string(3) "x_1"
+
+%s: Function MongoCollection::toIndexString() is deprecated in %smongocollection-toindexstring.php on line %d
string(4) "x_-1"
+
+%s: Function MongoCollection::toIndexString() is deprecated in %smongocollection-toindexstring.php on line %d
string(8) "x_1_y_-1"
Something went wrong with that request. Please try again.