Skip to content

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Oct 11, 2019

https://jira.mongodb.org/browse/PHPC-1347

Since libmongoc rejects an URI with an empty replicaSet URI option, there is different behaviour depending on whether an empty replica set name is passed via the URI string or via the URI options array. The first causes an exception:

Failed to parse MongoDB URI: 'mongodb://localhost:3000/?replicaSet='. Value for URI option "replicaset" cannot be empty string.

The latter lets users create the manager, but causes a connection exception on server selection:

Fatal error: Uncaught MongoDB\Driver\Exception\ConnectionTimeoutException: No suitable servers found (serverSelectionTryOnce set)

This PR changes this and checks the replicaSet option when it is passed via URI options array, also throwing an exception when the option is explicitly given with an empty value. Not passing the option at all does not cause an error.

@alcaeus alcaeus requested a review from jmikola October 11, 2019 12:43
@alcaeus alcaeus self-assigned this Oct 11, 2019
php_phongo.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be better placed within the if (mongoc_uri_option_is_utf8(key)) { condition further down in php_phongo_apply_options_to_uri. That already does the BSON_ITER_HOLDS_UTF8 check, which you're duplicating here. Consider:

if (mongoc_uri_option_is_utf8(key)) {
    if (!BSON_ITER_HOLDS_UTF8(&iter)) {
        PHONGO_URI_INVALID_TYPE(iter, "string");
        return false;
    }

    if (!strcasecmp(key, MONGOC_URI_REPLICASET) && !strcmp("", bson_iter_utf8(&iter, NULL))) {
        phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Value for URI option \"%s\" cannot be empty string.", key);
        return false;
    }

    if (!mongoc_uri_set_option_as_utf8(uri, key, bson_iter_utf8(&iter, NULL))) {
        /* Assignment uses mongoc_uri_set_appname() for the "appname"...

Copy link
Member

Choose a reason for hiding this comment

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

Both errors (libmongoc URI parsing and our new exception) can be exercised without incurring I/O, so I think this test could be better rewritten like the existing Manager::__construct() error tests (e.g. manager-ctor_error-003.phpt). Note that the 003 test already handles the invalid type check, which you had duplicated above, so there's be no need to check that again in your new test.

With the SKIPIF removed, I think this would be as simple as two cases in their own throws() callable:

  • Invalid connection string with ?replicaSet= throws libmongoc's exception
  • Valid connection string with ['replicaSet' => ''] URI options throws our exception

Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this file to manager-ctor_error-003.phpt? I realize we have a lot of existing tests split across various sub-directories, but the general convention is class-method-###.phpt, with an optional _error for error tests.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

LGTM with test file rename.

alcaeus added a commit that referenced this pull request Oct 21, 2019
@alcaeus alcaeus merged commit 0dbadbb into mongodb:master Oct 21, 2019
@alcaeus alcaeus deleted the phpc-1347 branch October 21, 2019 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants