Skip to content

PHPC-498: ReadPreference, ReadConcern, and WriteConcern should serialize to BSON #413

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
Sep 19, 2016
Merged

PHPC-498: ReadPreference, ReadConcern, and WriteConcern should serialize to BSON #413

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
59 changes: 31 additions & 28 deletions php_phongo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,8 +931,6 @@ void php_phongo_read_concern_to_zval(zval *retval, const mongoc_read_concern_t *

if (level) {
ADD_ASSOC_STRING(retval, "level", (char *)level);
} else {
ADD_ASSOC_NULL_EX(retval, "level");
}
} /* }}} */

Expand Down Expand Up @@ -962,12 +960,21 @@ bool php_phongo_read_preference_tags_are_valid(const bson_t *tags) /* {{{ */
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);
mongoc_read_mode_t mode = mongoc_read_prefs_get_mode(read_prefs);

array_init_size(retval, 2);

ADD_ASSOC_LONG_EX(retval, "mode", mongoc_read_prefs_get_mode(read_prefs));
switch (mode) {
case MONGOC_READ_PRIMARY: ADD_ASSOC_STRING(retval, "mode", "primary"); break;
case MONGOC_READ_PRIMARY_PREFERRED: ADD_ASSOC_STRING(retval, "mode", "primaryPreferred"); break;
case MONGOC_READ_SECONDARY: ADD_ASSOC_STRING(retval, "mode", "secondary"); break;
case MONGOC_READ_SECONDARY_PREFERRED: ADD_ASSOC_STRING(retval, "mode", "secondaryPreferred"); break;
case MONGOC_READ_NEAREST: ADD_ASSOC_STRING(retval, "mode", "nearest"); break;
Copy link
Member

Choose a reason for hiding this comment

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

add_assoc_string_ex() requires a char * for the value string, so each of these will need (char *) casting. Alternatively, I suppose we can just add that in the macro definition.

default: /* Do nothing */
break;
}

if (tags->len) {
if (!bson_empty0(tags)) {
php_phongo_bson_state state = PHONGO_BSON_STATE_INITIALIZER;
/* Use native arrays for debugging output */
state.map.root_type = PHONGO_TYPEMAP_NATIVE_ARRAY;
Expand All @@ -979,15 +986,14 @@ void php_phongo_read_preference_to_zval(zval *retval, const mongoc_read_prefs_t
#else
ADD_ASSOC_ZVAL_EX(retval, "tags", state.zchild);
#endif
} else {
ADD_ASSOC_NULL_EX(retval, "tags");
}
} /* }}} */

void php_phongo_write_concern_to_zval(zval *retval, const mongoc_write_concern_t *write_concern) /* {{{ */
{
const char *wtag = mongoc_write_concern_get_wtag(write_concern);
const int32_t w = mongoc_write_concern_get_w(write_concern);
const int32_t wtimeout = mongoc_write_concern_get_wtimeout(write_concern);
Copy link
Member

Choose a reason for hiding this comment

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

Does this count as mixing code and declarations? I actually would have thought the compiler would yell about the existing *wtag and w lines.

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 don't think that's a problem for initializers.... but a good point. But we do this in many other places too:

 858 php_phongo_server_description_type_t php_phongo_server_description_type(mongoc_server_description_t *sd)
 859 {
 860     const char* name = mongoc_server_description_type(sd);
 861     int i;
 862 

 872 void php_phongo_server_to_zval(zval *retval, mongoc_server_description_t *sd) /* {{{ */
 873 {
 874     mongoc_host_list_t *host = mongoc_server_description_host(sd);
 875     const bson_t       *is_master = mongoc_server_description_ismaster(sd);
 876     bson_iter_t         iter;
 877

in the same file for example.

Copy link
Member

Choose a reason for hiding this comment

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

Confirming that this doesn't appear to be a problem with my local builds.

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 would like to have a proper definitive answer on this somewhere. Can't be arsed to read the spec though :-)


array_init_size(retval, 4);

Expand All @@ -997,17 +1003,14 @@ void php_phongo_write_concern_to_zval(zval *retval, const mongoc_write_concern_t
ADD_ASSOC_STRING(retval, "w", (char *)PHONGO_WRITE_CONCERN_W_MAJORITY);
} else if (w != MONGOC_WRITE_CONCERN_W_DEFAULT) {
ADD_ASSOC_LONG_EX(retval, "w", w);
} else {
ADD_ASSOC_NULL_EX(retval, "w");
}

ADD_ASSOC_BOOL_EX(retval, "wmajority", mongoc_write_concern_get_wmajority(write_concern));
ADD_ASSOC_LONG_EX(retval, "wtimeout", mongoc_write_concern_get_wtimeout(write_concern));

if (mongoc_write_concern_journal_is_set(write_concern)) {
ADD_ASSOC_BOOL_EX(retval, "journal", mongoc_write_concern_get_journal(write_concern));
} else {
ADD_ASSOC_NULL_EX(retval, "journal");
ADD_ASSOC_BOOL_EX(retval, "j", mongoc_write_concern_get_journal(write_concern));
}

if (wtimeout != 0) {
ADD_ASSOC_LONG_EX(retval, "wtimeout", wtimeout);
}
} /* }}} */
/* }}} */
Expand Down Expand Up @@ -2105,6 +2108,20 @@ PHP_MINIT_FUNCTION(mongodb)

PHP_MINIT(bson)(INIT_FUNC_ARGS_PASSTHRU);

PHP_MINIT(Type)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Serializable)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Unserializable)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Persistable)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Binary)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Decimal128)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Javascript)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(MaxKey)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(MinKey)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(ObjectID)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Regex)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Timestamp)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(UTCDateTime)(INIT_FUNC_ARGS_PASSTHRU);

PHP_MINIT(Command)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Cursor)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(CursorId)(INIT_FUNC_ARGS_PASSTHRU);
Expand Down Expand Up @@ -2132,20 +2149,6 @@ PHP_MINIT_FUNCTION(mongodb)
PHP_MINIT(ExecutionTimeoutException)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(ConnectionTimeoutException)(INIT_FUNC_ARGS_PASSTHRU);

PHP_MINIT(Type)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Serializable)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Unserializable)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Persistable)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Binary)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Decimal128)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Javascript)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(MaxKey)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(MinKey)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(ObjectID)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Regex)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(Timestamp)(INIT_FUNC_ARGS_PASSTHRU);
PHP_MINIT(UTCDateTime)(INIT_FUNC_ARGS_PASSTHRU);

REGISTER_STRING_CONSTANT("MONGODB_VERSION", (char *)MONGODB_VERSION_S, CONST_CS | CONST_PERSISTENT);
REGISTER_STRING_CONSTANT("MONGODB_STABILITY", (char *)MONGODB_STABILITY_S, CONST_CS | CONST_PERSISTENT);

Expand Down
18 changes: 17 additions & 1 deletion src/MongoDB/ReadConcern.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,20 @@ PHP_METHOD(ReadConcern, getLevel)
}
/* }}} */

/* {{{ proto array ReadConcern::bsonSerialize()
*/
PHP_METHOD(ReadConcern, bsonSerialize)
{
const mongoc_read_concern_t *read_concern = phongo_read_concern_from_zval(getThis() 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.

For consistency with our other functions that take no arguments, I'd consider adding this:

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

Also, did we need the following suppressions for PHP 5.x?

SUPPRESS_UNUSED_WARNING(return_value_ptr) SUPPRESS_UNUSED_WARNING(return_value_used)

If these warnings didn't trigger for you, you can always omit this change. I'll add them back if I see them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These warnings are IMO superfluous. I have never noticed this being necessary, and I have never seen this in other extensions either.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you configure phongo with --enable-developer-flags it'll add -Wunused-parameter, which would issue a warning for arguments that aren't used.

The PHP_METHOD() macro defines a c function that has all sorts of arguments, including return_value_ptr and return_value_used which are rarely used, which is why those macros meant to suppress them, so the flag can still be helpful for avoid accidentally declaring superfluous arguments in other functions

Copy link
Member

Choose a reason for hiding this comment

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

It appears that -Wno-unused-parameter was introduced in e3e3c9d#diff-788d457a20b110cc38e571dec9ddc68cR126, builds have been ignoring unused variables since then.

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'm fine with not having warnings for unused function arguments though. Catching locally undefined vars is important though.


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

php_phongo_read_concern_to_zval(return_value, read_concern);
}
/* }}} */

/**
* Value object for read concern used in issuing read operations.
*/
Expand All @@ -116,7 +130,7 @@ ZEND_END_ARG_INFO()
static zend_function_entry php_phongo_readconcern_me[] = {
PHP_ME(ReadConcern, __construct, ai_ReadConcern___construct, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_ME(ReadConcern, getLevel, ai_ReadConcern_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_ME(Manager, __wakeup, ai_ReadConcern_void, ZEND_ACC_PUBLIC)
PHP_ME(ReadConcern, bsonSerialize, ai_ReadConcern_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_FE_END
};

Expand Down Expand Up @@ -192,6 +206,8 @@ PHP_MINIT_FUNCTION(ReadConcern)
PHONGO_CE_FINAL(php_phongo_readconcern_ce);
PHONGO_CE_DISABLE_SERIALIZATION(php_phongo_readconcern_ce);

zend_class_implements(php_phongo_readconcern_ce TSRMLS_CC, 1, php_phongo_serializable_ce);

memcpy(&php_phongo_handler_readconcern, phongo_get_std_object_handlers(), sizeof(zend_object_handlers));
php_phongo_handler_readconcern.get_debug_info = php_phongo_readconcern_get_debug_info;
#if PHP_VERSION_ID >= 70000
Expand Down
18 changes: 17 additions & 1 deletion src/MongoDB/ReadPreference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,20 @@ PHP_METHOD(ReadPreference, getTagSets)
}
/* }}} */

/* {{{ proto array ReadPreference::bsonSerialize()
*/
PHP_METHOD(ReadPreference, bsonSerialize)
{
const mongoc_read_prefs_t *read_preference = phongo_read_preference_from_zval(getThis() 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.

Same changes as ReadConcern::bsonSerialize().


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

php_phongo_read_preference_to_zval(return_value, read_preference);
}
/* }}} */

/**
* Value object for read preferences used in issuing commands and queries.
*/
Expand All @@ -179,7 +193,7 @@ static zend_function_entry php_phongo_readpreference_me[] = {
PHP_ME(ReadPreference, __construct, ai_ReadPreference___construct, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_ME(ReadPreference, getMode, ai_ReadPreference_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_ME(ReadPreference, getTagSets, ai_ReadPreference_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_ME(Manager, __wakeup, ai_ReadPreference_void, ZEND_ACC_PUBLIC)
PHP_ME(ReadPreference, bsonSerialize, ai_ReadPreference_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_FE_END
};

Expand Down Expand Up @@ -258,6 +272,8 @@ PHP_MINIT_FUNCTION(ReadPreference)
PHONGO_CE_FINAL(php_phongo_readpreference_ce);
PHONGO_CE_DISABLE_SERIALIZATION(php_phongo_readpreference_ce);

zend_class_implements(php_phongo_readpreference_ce TSRMLS_CC, 1, php_phongo_serializable_ce);

memcpy(&php_phongo_handler_readpreference, phongo_get_std_object_handlers(), sizeof(zend_object_handlers));
php_phongo_handler_readpreference.get_debug_info = php_phongo_readpreference_get_debug_info;
#if PHP_VERSION_ID >= 70000
Expand Down
18 changes: 17 additions & 1 deletion src/MongoDB/WriteConcern.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,20 @@ PHP_METHOD(WriteConcern, getJournal)
}
/* }}} */

/* {{{ proto array WriteConcern::bsonSerialize()
*/
PHP_METHOD(WriteConcern, bsonSerialize)
{
const mongoc_write_concern_t *write_concern = phongo_write_concern_from_zval(getThis() TSRMLS_CC);
Copy link
Member

@jmikola jmikola Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Same changes as ReadConcern::bsonSerialize().


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

php_phongo_write_concern_to_zval(return_value, write_concern);
}
/* }}} */

/**
* Value object for write concern used in issuing write operations.
*/
Expand All @@ -201,7 +215,7 @@ static zend_function_entry php_phongo_writeconcern_me[] = {
PHP_ME(WriteConcern, getW, ai_WriteConcern_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_ME(WriteConcern, getWtimeout, ai_WriteConcern_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_ME(WriteConcern, getJournal, ai_WriteConcern_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_ME(Manager, __wakeup, ai_WriteConcern_void, ZEND_ACC_PUBLIC)
PHP_ME(WriteConcern, bsonSerialize, ai_WriteConcern_void, ZEND_ACC_PUBLIC|ZEND_ACC_FINAL)
PHP_FE_END
};

Expand Down Expand Up @@ -277,6 +291,8 @@ PHP_MINIT_FUNCTION(WriteConcern)
PHONGO_CE_FINAL(php_phongo_writeconcern_ce);
PHONGO_CE_DISABLE_SERIALIZATION(php_phongo_writeconcern_ce);

zend_class_implements(php_phongo_writeconcern_ce TSRMLS_CC, 1, php_phongo_serializable_ce);

memcpy(&php_phongo_handler_writeconcern, phongo_get_std_object_handlers(), sizeof(zend_object_handlers));
php_phongo_handler_writeconcern.get_debug_info = php_phongo_writeconcern_get_debug_info;
#if PHP_VERSION_ID >= 70000
Expand Down
4 changes: 0 additions & 4 deletions tests/bulk/write-0002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,8 @@ object(MongoDB\Driver\BulkWrite)#%d (%d) {
array(%d) {
["w"]=>
int(1)
["wmajority"]=>
bool(false)
["wtimeout"]=>
int(1000)
["journal"]=>
NULL
}
}
Inserted 2 documents to %s
Expand Down
2 changes: 0 additions & 2 deletions tests/manager/manager-getreadconcern-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ foreach ($tests as $i => $test) {
<?php exit(0); ?>
--EXPECTF--
object(MongoDB\Driver\ReadConcern)#%d (%d) {
["level"]=>
NULL
}
object(MongoDB\Driver\ReadConcern)#%d (%d) {
["level"]=>
Expand Down
26 changes: 7 additions & 19 deletions tests/manager/manager-getreadpreference-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,35 +32,23 @@ foreach ($tests as $i => $test) {
--EXPECTF--
object(MongoDB\Driver\ReadPreference)#%d (%d) {
["mode"]=>
int(1)
["tags"]=>
array(0) {
}
string(7) "primary"
}
object(MongoDB\Driver\ReadPreference)#%d (%d) {
["mode"]=>
int(2)
["tags"]=>
array(0) {
}
string(9) "secondary"
}
object(MongoDB\Driver\ReadPreference)#%d (%d) {
["mode"]=>
int(5)
["tags"]=>
array(0) {
}
string(16) "primaryPreferred"
}
object(MongoDB\Driver\ReadPreference)#%d (%d) {
["mode"]=>
int(6)
["tags"]=>
array(0) {
}
string(18) "secondaryPreferred"
}
object(MongoDB\Driver\ReadPreference)#%d (%d) {
["mode"]=>
int(2)
string(9) "secondary"
["tags"]=>
array(2) {
[0]=>
Expand All @@ -77,7 +65,7 @@ object(MongoDB\Driver\ReadPreference)#%d (%d) {
}
object(MongoDB\Driver\ReadPreference)#%d (%d) {
["mode"]=>
int(2)
string(9) "secondary"
["tags"]=>
array(2) {
[0]=>
Expand All @@ -94,7 +82,7 @@ object(MongoDB\Driver\ReadPreference)#%d (%d) {
}
object(MongoDB\Driver\ReadPreference)#%d (%d) {
["mode"]=>
int(2)
string(9) "secondary"
["tags"]=>
array(1) {
[0]=>
Expand Down
Loading