Skip to content
This repository was archived by the owner on Dec 23, 2021. It is now read-only.

HHVM-239: Allow users to set a limit on acceptable staleness #127

Merged
merged 3 commits into from
Sep 7, 2016
Merged

HHVM-239: Allow users to set a limit on acceptable staleness #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b5ad1f
Upgrade to latest libmongoc 1.5-dev
derickr Aug 30, 2016
beae7bf
HHVM-239: Allow users to set a limit on acceptable staleness
derickr Aug 30, 2016
98b9c56
Also allow for the fully lower case version of the option
derickr Aug 31, 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
4 changes: 3 additions & 1 deletion config.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ HHVM_EXTENSION(mongodb
libmongoc/src/mongoc/mongoc-gridfs-file.c libmongoc/src/mongoc/mongoc-gridfs.c
libmongoc/src/mongoc/mongoc-host-list.c
libmongoc/src/mongoc/mongoc-index.c libmongoc/src/mongoc/mongoc-init.c
libmongoc/src/mongoc/mongoc-list.c libmongoc/src/mongoc/mongoc-log.c
libmongoc/src/mongoc/mongoc-list.c
libmongoc/src/mongoc/mongoc-linux-distro-scanner.c
libmongoc/src/mongoc/mongoc-log.c
libmongoc/src/mongoc/mongoc-matcher-op.c libmongoc/src/mongoc/mongoc-matcher.c
libmongoc/src/mongoc/mongoc-memcmp.c
libmongoc/src/mongoc/mongoc-metadata.c
Expand Down
33 changes: 29 additions & 4 deletions src/MongoDB/Driver/Manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@ const StaticString
s_MongoDBDriverManager_slaveok("slaveok"),
s_MongoDBDriverManager_readpreference("readpreference"),
s_MongoDBDriverManager_readpreferencetags("readpreferencetags"),
s_MongoDBDriverManager_maxstalenessms("maxstalenessms"),
s_MongoDBDriverManager_readPreference("readPreference"),
s_MongoDBDriverManager_readPreferenceTags("readPreferenceTags"),
s_MongoDBDriverManager_maxStalenessMS("maxStalenessMS"),
Copy link
Member

@jmikola jmikola Aug 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why do you define these strings with multiple cases? URI option parsing should be case-insensitive, per the spec:

Keys should be normalised and character case should be ignored.

I realize this is for the options array, instead of URI string; however, PHPC appears to do case-insensitive lookups after we convert the options array to BSON.

Copy link
Member

Choose a reason for hiding this comment

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

Accepting one canonical form of the option would be consistent with how most PHP libraries (PHPLIB included) operate, but the logic in mongoc-uri.c for checking if an option is of a certain type before we invoke a setter is case-insensitive. The case-insensitivity does mean we'd permit some weirdness if the user decided to pass a few variants of the same option with different casing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means I can't use the nice HHVM string lookups, and need to do a really annoying loop with conversions and strcasecmps. I am not intending to do that. I'm happy to add the check for all lower case though.

Copy link
Member

Choose a reason for hiding this comment

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

In the interest of consistency, would you like to create a PHPC ticket to ultimately remove the case-insensitive handling of $uriOptions? PHPC-655 is one related ticket, which we actually have a test for parsing the all-caps variant; however, that test is likely obsolete now that I've dropped PHP streams.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to add the check for all lower case though.

That sounds like a good idea, considering you do it for the other options here. If we end up changing PHPC, I can do the same (i.e. allow the canonical camel-case and all-lowercase versions).

s_MongoDBDriverManager_readconcernlevel("readconcernlevel"),
s_MongoDBDriverManager_readConcernLevel("readConcernLevel"),
s_MongoDBDriverManager_mode("mode"),
Expand Down Expand Up @@ -134,8 +136,10 @@ static bool hippo_mongo_driver_manager_apply_rp(mongoc_uri_t *uri, const Array o
!options.exists(s_MongoDBDriverManager_slaveok) &&
!options.exists(s_MongoDBDriverManager_readpreference) &&
!options.exists(s_MongoDBDriverManager_readpreferencetags) &&
!options.exists(s_MongoDBDriverManager_maxstalenessms) &&
!options.exists(s_MongoDBDriverManager_readPreference) &&
!options.exists(s_MongoDBDriverManager_readPreferenceTags)
!options.exists(s_MongoDBDriverManager_readPreferenceTags) &&
!options.exists(s_MongoDBDriverManager_maxStalenessMS)
) {
return true;
}
Expand Down Expand Up @@ -184,17 +188,37 @@ static bool hippo_mongo_driver_manager_apply_rp(mongoc_uri_t *uri, const Array o
mongoc_read_prefs_set_tags(new_rp, b_tags);
}

/* Validate that readPreferenceTags are not used with PRIMARY readPreference */
if (
mongoc_read_prefs_get_mode(new_rp) == MONGOC_READ_PRIMARY &&
!bson_empty(mongoc_read_prefs_get_tags(new_rp))
) {
throw MongoDriver::Utils::throwInvalidArgumentException("Primary read preference mode conflicts with tags");
mongoc_read_prefs_destroy(new_rp);

return false;
}

/* This may be redundant in light of the last check (primary with tags),
/* Handle maxStalenessMS, and make sure it is not combined with PRIMARY readPreference */
if (
(options.exists(s_MongoDBDriverManager_maxstalenessms) && options[s_MongoDBDriverManager_maxstalenessms].isInteger())
||
(options.exists(s_MongoDBDriverManager_maxStalenessMS) && options[s_MongoDBDriverManager_maxStalenessMS].isInteger())
) {
if (mongoc_read_prefs_get_mode(new_rp) == MONGOC_READ_PRIMARY) {
throw MongoDriver::Utils::throwInvalidArgumentException("Primary read preference mode conflicts with maxStalenessMS");
mongoc_read_prefs_destroy(new_rp);

return false;
}

if (options.exists(s_MongoDBDriverManager_maxstalenessms)) {
mongoc_read_prefs_set_max_staleness_ms(new_rp, (int32_t) options[s_MongoDBDriverManager_maxstalenessms].toInt64());
} else {
mongoc_read_prefs_set_max_staleness_ms(new_rp, (int32_t) options[s_MongoDBDriverManager_maxStalenessMS].toInt64());
}
}

/* This may be redundant in light of the check for primary with tags,
* but we'll check anyway in case additional validation is implemented. */
if (!mongoc_read_prefs_is_valid(new_rp)) {
throw MongoDriver::Utils::throwInvalidArgumentException("Read preference is not valid");
Expand Down Expand Up @@ -337,7 +361,8 @@ static mongoc_uri_t *hippo_mongo_driver_manager_make_uri(const char *dsn, const
!strcasecmp(s_key, "safe") ||
!strcasecmp(s_key, "slaveok") ||
!strcasecmp(s_key, "w") ||
!strcasecmp(s_key, "wtimeoutms")
!strcasecmp(s_key, "wtimeoutms") ||
!strcasecmp(s_key, "maxstalenessms")
Copy link
Member

@jmikola jmikola Aug 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Should you be using the defined static strings here, or are the string literals intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StaticStrings are C++, strcasecmp is C. This is correct.

) {
continue;
}
Expand Down
6 changes: 4 additions & 2 deletions tests/MongoDBDriverManager_debugInfo.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ array(2) {
["is_passive"]=>
bool(false)
["last_is_master"]=>
array(8) {
array(9) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to just use %d here, as we typically do for object fields (even when we enumerate all of them in the expected output)?

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 did that for the PHPC one already - the problem is more that different versions of MongoDB include different items. Hence me doing: mongodb/mongo-php-driver@6462468#diff-3bb29f65077b3f49f6e05c7d78fdab99R53

Will fix that here too.

["ismaster"]=>
bool(true)
["maxBsonObjectSize"]=>
Expand All @@ -46,12 +46,14 @@ array(2) {
["localTime"]=>
object(MongoDB\BSON\UTCDateTime)#%d (1) {
["milliseconds"]=>
int(%d)
string(%d) "%d"
}
["maxWireVersion"]=>
int(%d)
["minWireVersion"]=>
int(%d)
["readOnly"]=>
bool(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 assume this will be replaced with %a?

["ok"]=>
float(1)
}
Expand Down
13 changes: 13 additions & 0 deletions tests/MongoDBDriverManager_maxStalenessMS-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
--TEST--
MongoDB\Driver\Manager: maxStalenessMS
--FILE--
<?php
// They both should work, and produce no output. We're not testing whether the flag actually does something
$manager = new MongoDB\Driver\Manager("mongodb://localhost/?readPreference=SECONDARY&maxStalenessMS=1231");
$manager = new MongoDB\Driver\Manager("mongodb://localhost/?readPreference=SECONDARY", [ 'maxStalenessMS' => 1231 ] );
$manager = new MongoDB\Driver\Manager("mongodb://localhost/?readPreference=SECONDARY&maxstalenessms=1231");
$manager = new MongoDB\Driver\Manager("mongodb://localhost/?readPreference=SECONDARY", [ 'maxstalenessms' => 1231 ] );
?>
==DONE==
--EXPECTF--
==DONE==
33 changes: 33 additions & 0 deletions tests/MongoDBDriverManager_maxStalenessMS_error-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
--TEST--
MongoDB\Driver\Manager: maxStalenessMS
--FILE--
<?php
try {
$manager = new MongoDB\Driver\Manager("mongodb://localhost/?maxstalenessms=1231");
} catch ( MongoDB\Driver\Exception\InvalidArgumentException $e ) {
echo $e->getMessage(), "\n";
}

try {
$manager = new MongoDB\Driver\Manager("mongodb://localhost/?maxStalenessMS=1231");
} catch ( MongoDB\Driver\Exception\InvalidArgumentException $e ) {
echo $e->getMessage(), "\n";
}

try {
$manager = new MongoDB\Driver\Manager("mongodb://localhost/", [ 'maxstalenessms' => 1231 ] );
} catch ( MongoDB\Driver\Exception\InvalidArgumentException $e ) {
echo $e->getMessage(), "\n";
}

try {
$manager = new MongoDB\Driver\Manager("mongodb://localhost/", [ 'maxStalenessMS' => 1231 ] );
} catch ( MongoDB\Driver\Exception\InvalidArgumentException $e ) {
echo $e->getMessage(), "\n";
}
?>
--EXPECTF--
Failed to parse MongoDB URI: 'mongodb://localhost/?maxstalenessms=1231'
Failed to parse MongoDB URI: 'mongodb://localhost/?maxStalenessMS=1231'
Primary read preference mode conflicts with maxStalenessMS
Primary read preference mode conflicts with maxStalenessMS