Phpc 887 : Throw exceptions for unexpected types in URI options array

[7snovic]

PHPC-887

[jmikola]

ZVAL_DEREF and SEPARATE_ZVAL_NOREF are only done for "readPreferenceTags" because php_phongo_read_preference_prep_tagsets() may end up modifying the zval and we don't want those changes to affect the argument passed into Manager::__construct(). In this case, you're only reading the option zval's type code with Z_TYPE_P(), so you can skip any dereferencing and separation.

[7snovic]

👍 👍

because php_phongo_read_preference_prep_tagsets() may end up modifying the zval

do you mean by converting it into object convert_to_object(tagSet); ?

[jmikola]

do you mean by converting it into object convert_to_object(tagSet);.

Yup. Without separating the zval, it's possible for that conversion to modify the original argument in the calling scope, regardless of whether it was a literal or a variable. This is similar to what the legacy driver did by injecting an _id key into the MongoCollection::insert() argument, which is not a mistake we want to repeat :)

[jmikola]

Since you continue iteration below, it's possible for several exceptions to be thrown if other validations are added in the future. This would require users to rely on Exception::getPrevious() to view them all, as only the most recently thrown exception will be immediately accessible.

By convention, we'd want to throw an exception and return immediately with a failure code. Additionally, we'd check for a failure in the calling context (i.e. Manager::__construct()) and return there as well, so that we don't proceed with trying to initialize the object any further. Note that php_phongo_manager_prep_uri_options() is void, so that would need to be changed. If you look at php_phongo_manager_merge_context_options() and how it's called from the constructor, you'll see an example of what I'm describing.

Validating types may be outside of the purpose of php_phongo_manager_prep_authmechanismproperties() and php_phongo_manager_prep_uri_options(), since these functions were created to convert PHP values for BSON encoding that will later happen in phongo_manager_init().

I think it may be preferable to introduce validation in the php_phongo.c functions that actually apply the converted BSON values to the libmongoc URI struct. See php_phongo_apply_options_to_uri() for the generic case:

if (mongoc_uri_option_is_int32(key) && BSON_ITER_HOLDS_INT32(&iter)) {
	if (!mongoc_uri_set_option_as_int32(uri, key, bson_iter_int32(&iter))) {
		phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Failed to parse \"%s\" URI option", key);
		return false;
	}

	continue;
}

We currently ignore options unless the name indicates that libmongoc expects a given type and the value matches that type. If we were to add validation here, we'd throw an exception if the BSON iterator did not hold the actual type.

---

Lastly, Query.c has numerous examples of exception messages for expected type errors. For consistency, we'd want to use something like:

phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"FOOBAR\" option to be string, %s given", zend_get_type_by_const(type));

If we're validating types while working with BSON, we can no longer rely on zend_get_type_by_const(); however, we can sort out error messages later.

[7snovic]

Validating types may be outside of the purpose of php_phongo_manager_prep_authmechanismproperties() and php_phongo_manager_prep_uri_options()

so we will not need to change it's type from void to another type , as we will implement a new validation function that applied on BSON values instead of zval - if I got what you mean - and since we are converting the zval to bson inside the phongo_manager_init function which is invoked after php_phongo_manager_prep_uri_options function but in this case we will not stop proceeding the trying to initialize the object.

so that we don't proceed with trying to initialize the object any further.

[7snovic]

another question about validating bson value's types, I went with the source code but I couldn't found any clue about it except the php_phongo_bson_state struct and php_phongo_bson_to_zval_ex function then check the type using the ordinary php way which I'm sure that this is not the proper way to check about the bson value

[jmikola]

as we will implement a new validation function that applied on BSON values instead of zval

I'm suggesting that you not modify anything in Manager.c and instead focus on php_phongo_apply_options_to_uri() (and similar functions). Consider:

if (mongoc_uri_option_is_int32(key)) {
	if (!BSON_ITER_HOLDS_INT32(&iter)) {
		/* phongo_throw_exception() for invalid type */
		return false;
	}

	if (!mongoc_uri_set_option_as_int32(uri, key, bson_iter_int32(&iter))) {
		phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Failed to parse \"%s\" URI option", key);
		return false;
	}

	continue;
}

another question about validating bson value's types

There are macros in libbson that check types. BSON_ITER_HOLDS_INT32(), which appears in the example above, is one of these macros. When we're working with BSON, libbson provides a bson_iter_t that can point to a specific field within a document/array. The common API use case is to move the iter to the field corresponding to a given name, and then we can check its type code or access its value. In cases where the value is another BSON document or array, we can also recurse the iter a level deeper (not relevant to this ticket but I wanted to mention it).

[7snovic]

after trying to move the responsibility of validating the readpreferenceoption from php side to bson side , we will always got that exception thrown when we are trying to pass un-serializeable object like [ MongoDB\Driver\ReadPreference ]

Serialization of 'MongoDB\Driver\ReadPreference' is not allowed

code to re-produce :

$manager = new MongoDB\Driver\Manager('mongodb://localhost:27017', [
    'readpreference' => new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY),
]);

and I think that the problem is that we are proceeding the validation operation from checking an object as a zval to bson value

and that's mean that we will always be stuck in here

if (options) {
    php_phongo_manager_prep_uri_options(options TSRMLS_CC);
}

and will not move forward to phongo_manager_init(intern ....

[jmikola]

This is outside the scope of PHPC-887. The URI's readpreference option is a string (e.g. "primary", "primaryPreferred") and only one of several read preference URI options. MongoDB\Driver\ReadPreference represents a complete read preference (mode, tag sets, max staleness). This is analogous to a MongoDB\Driver\WriteConcern representing multiple write concern URI options.

[7snovic]

Yea, I can see that here , but I think that as mentioned at PHPC-887 that we need to throw some meaningful exception to the user to tell him about the invalid passed option, the issue with validating the option type -in case passing MongoDB\Driver\ReadPreference for example- with bson that bson will not check for zval values like objects.
it's a bit confusing, the example provided in that ticket was about unsupported types, also your reply here

[jmikola]

You make a good point by linking to the mongodb-user thread. There are two facets to this, so I'll explain each separately.

PHP serialization only occurs because we're creating a hash key to persist the libmongoc client for the given MongoDB\Driver\Manager arguments. The "cannot be serialized" exception therefore has more to do with ReadPreference not supporting serialize() than being an invalid argument. If we skipped that hash key creation, a ReadPreference object would serialize to BSON and option validation could report that a document was provided where a string was expected (for readPreference URI option).

My hesitation with validating the options array (i.e. Manager's second argument) is that objects may implement interfaces that affect how they serialize to BSON. At present, authMechanismProperties and readPreferenceTags are the only non-scalar URI options, but imagine an object implemented MongoDB\BSON\Serializable and returned a document or array for those options, respectively. Since BSON is that format we encounter before assigning the options to mongoc_uri_t, I think we definitely need validation there and suggest starting with that.

There is no reason for ReadPreference, WriteConcern, and ReadConcern to not support PHP serialization (see: PHPC-850). Once that is implemented, the "cannot be serialized" exception should no longer be an issue (for the hash key generation) and any BSON-level type validation implemented for PHPC-887 will fully cover us.

[7snovic]

The "cannot be serialized" exception therefore has more to do with ReadPreference not supporting serialize() than being an invalid argument
.......
There is no reason for ReadPreference, WriteConcern, and ReadConcern to not support PHP serialization ....

I see, so basically the problem of the serialization was not with MongoDB\Driver\ReadPreference , so something like this 'readpreference' => new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY), should not cause any serialization exceptions.

---

so does that would be the proper implementation for this -within php_phongo_apply_options_to_uri function ? or should I implement the validation inside php_phongo_apply_rp_options_to_uri as this is more related to readPreference- ?

if (!strcasecmp(key, MONGOC_URI_READPREFERENCE)) {
    if (!BSON_ITER_HOLDS_UTF8(&iter)) {
        phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "some meaningful exception message");
    }
}

[jmikola]

I see, so basically the problem of the serialization was not with MongoDB\Driver\ReadPreference

It was technically due to ReadPreference not supporting PHP serialization (to be fixed by PHPC-850).

so something like this 'readpreference' => new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY), should not cause any serialization exceptions.

If PHPC-850 was implemented, we would get beyond the point where the connection URI options are serialized to a string for the hash string. A ReadPreference object inside of the options array would then get serialized to a BSON document according to its bsonSerialize() method, and I'd expect validation logic for PHPC-887 to throw an exception because a string value was expected for "readPreference".

php_phongo_apply_options_to_uri or php_phongo_apply_rp_options_to_uri?

The read preference URI options are only handled in php_phongo_apply_rp_options_to_uri(). In #601 (comment), I gave an example of how code inside php_phongo_apply_options_to_uri() should be changed. The read concern, read preference, and write concern options have their own functions, but all other URI options are handled by php_phongo_apply_options_to_uri(). You can start with any of these functions, but I think the general php_phongo_apply_options_to_uri() function may be best since you have an example already from my earlier comment.

[jmikola]

Since you're using STANDALONE here, the SKIPIF block should include <?php NEEDS('STANDALONE'); ?> (see other tests for examples). That said, you're not actually connecting to a remote server here, so you can simply pass null for Manager::__construct()'s first argument to fall back to the default connection string ("mongodb://127.0.0.1/").

[jmikola]

EXPECTF is only needed if the expected output uses patterns. In this case, you can use EXPECT.

[jmikola]

Thanks for opening the PR. I tried to provide some verbose feedback to point you in the right direction, but feel free to follow up if you have specific questions and I'll be happy to respond when I get a chance.

[jmikola]

I think this logic belongs in php_phongo_apply_rp_options_to_uri() (see linked position), which is the only place we currently read this option value.

When I suggested to "focus on php_phongo_apply_options_to_uri() (and similar functions)" in #601 (comment), I did not mean to suggest we validate all options in php_phongo_apply_options_to_uri(). The code example I shared in that comment (for mongoc_uri_option_is_int32(key)) can be adapted for the read preference.

Essentially:

if (bson_iter_init_find_case(&iter, options, MONGOC_URI_READPREFERENCE) && BSON_ITER_HOLDS_UTF8(&iter)) {
    const char *str = bson_iter_utf8(&iter, NULL);
    /* ... */
}

Becomes:

if (bson_iter_init_find_case(&iter, options, MONGOC_URI_READPREFERENCE)) {
	if (BSON_ITER_HOLDS_UTF8(&iter)) {
		const char *str = bson_iter_utf8(&iter, NULL);
		/* ... */
	} else {
		/* phongo_throw_exception() for invalid type */
		return false;
	}
}

Notice that we do have to adapt the code a bit from my example in #601 (comment) because const char *str must be declared at the top of its block scope. Therefore, we can't simply precede it with an if that conditionally returns.

[7snovic]

That's why I'd asked about the right context, because I felt that we should use this validation within the php_phongo_apply_rp_options_to_uri() context, but I get confused with

but I think the general php_phongo_apply_options_to_uri() function may be best since ...

so I decided to go with php_phongo_apply_options_to_uri, However I will fix this, but in this case we will pass the key literally in the exception message [readPreference], since we can not use key variable, and I think that it will not be proper to define it const char *key = bson_iter_key(&iter); for only that use

[jmikola]

You can start with any of these functions, but I think the general php_phongo_apply_options_to_uri() function may be best since you have an example already from my earlier comment.

Sorry for any miscommunication on my part. With that, I was suggesting that start with the options handled by that function (not read preference), since I had already provided a code example of how to validate int32 types.

However I will fix this, but in this case we will pass the key literally in the exception message [readPreference], since we can not use key variable, and I think that it will not be proper to define it const char *key = bson_iter_key(&iter); for only that use

If you start with the three basic cases in php_phongo_apply_options_to_uri(), which are bool, int32, and utf8, then you have key available to use in the message already.

In the case of php_phongo_apply_rp_options_to_uri(), we know exactly what options to look and don't have to loop over all fields in the options document and fetch key at each position. While we could use a known constant like MONGOC_URI_READPREFERENCE in the exception message, I think bson_iter_key(&iter); may be preferable because it ensures that we report the exact case for the key of the invalid field.

Consider if the user had both "readPreference" and "readpreference" as keys in the options array. bson_iter_init_find_case() will stop at the first one it finds, so php_phongo_apply_rp_options_to_uri() will only process the first field. I agree that this is an unfortunate edge case, but in this case I'd prefer to tell the user exactly which field we found to be invalid.

[7snovic]

You are right, also in this case we may make use of the bson_iter_key(&iter); by using it with the Unsupported readPreference value ... exception message as follows:

if (bson_iter_init_find_case(&iter, options, MONGOC_URI_READPREFERENCE)) {
	const char *key = bson_iter_key(&iter);
	if (BSON_ITER_HOLDS_UTF8(&iter)) {
		const char *str = bson_iter_utf8(&iter, NULL);
		/* ... */
		} else {
		    phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Unsupported %s value: '%s'", key, str);
		}
	} else {
		/* phongo_throw_exception() for invalid type */
		return false;
	}
}

[jmikola]

I would leave this test as-is for now. Once all of the read preference options are handled in the PR, we can simply delete it.

[jmikola]

I might suggest just adapting the manager-ctor-read_preference-003.phpt test case here. I don't think it's necessary to test all possible types here, as one invalid type for each option should be sufficient to exercise the validation logic.

[7snovic]

should manager-ctor-read_preference-003.phpt moved to an -error test or we may leave this as it and just adapting the test case here?

[jmikola]

Since manager-ctor-read_preference-003.phpt in its current state will no longer apply, I'd suggest deleting the file in git. The new test's filename will be manager-ctor-read_preference-error-004.phpt (4 is next in the sequence) and can be based on the existing contents of manager-ctor-read_preference-003.phpt.

I don't think it matters if you delete manager-ctor-read_preference-003.phpt and add the new test or use git mv to rename the test. Since you'll be modifying the original test anyway, git will decide the change is sufficient to consider them two different files or a rename with modifications.

[7snovic]

hey @jmikola can we go with this?

[jmikola]

@7snovic: Thanks for starting this. I've squashed your commits and cherry-picked the readPreference work into #624.