Manager getters and options for read prefs and write concerns #77
Conversation
jmikola
commented
Aug 5, 2015
jmikola
commented
- https://jira.mongodb.org/browse/PHPC-196
- https://jira.mongodb.org/browse/PHPC-353
| size_t str_len; | ||
|
|
||
| str = bson_as_json(&bson_options, &str_len); | ||
| phongo_throw_exception(PHONGO_ERROR_RUNTIME TSRMLS_CC, "Failed to create Manager from URI '%s' and options: %s", uri_string, str); |
There was a problem hiding this comment.
I don't think its a good idea to serialize the entire options array as json and include it in the exception...
There was a problem hiding this comment.
I reverted to just printing the URI for the two original exceptions (URI parsing fails and client initialization fails). I did change the URI parsing exception to use InvalidArgumentException, which I think makes more sense.
For the later two exceptions, I've changed them to InvalidArgumentException and just report that we couldn't apply the RP or WC options to the Manager.
jmikola
force-pushed
the
phpc-196
branch
5 times, most recently
from
9e7f683 to
ec9a4a8
Compare
|
@bjori: see my replies on jmikola@2b20d34 and jmikola@80ef0e9 and let me know if anything remains. |
| const mongoc_read_prefs_t *old_rp; | ||
|
|
||
| if (!(old_rp = mongoc_client_get_read_prefs(client))) { | ||
| MONGOC_WARNING("Client does not have a read preference."); |
There was a problem hiding this comment.
I'm really not a fan of using MONGOC_WARNING(). I've used MONGOC_DEBUG() before for.. debugging. But this feels backwards..
It turns out php_phongo_log() doesn't actually throw exception or anything -- doesn't even raise a php-error by default -- for _WARNING.
So something doesn't work and the user has no idea why?
There was a problem hiding this comment.
This is no difference than the warnings raised by libmongoc during URI parsing, though. They end up logged (if logging is enabled) or ignored. I can certainly remove these if you think it has no value, though.
This particular line (checking old_rp) is more of an assertion, because the client should always have a valid RP and WC struct. The warnings below (e.g. "Unsupported readPreference value") are more important.
There was a problem hiding this comment.
The mongoc use of MONGOC_WARNING() for error messaging is problematic.
All we can do is log it and fail. And the failure contains no information as to why it failed. It just failed.
For example, monogc will issue MONGOC_WARNING() as to why a mongoc_bulk_operation_replace_one() was not added to the bulk. The function declaration is void, so its really difficult for a user to determine if an bulk operation has been added to the bulk or not, and doing so programmatically is a nightmare.
From phongo perspective, these log messages are just that.. logs. That are sent to /dev/null unless you turn on 'mongodb.debug' php.ini
I think, when something fails, or we don't do something, or interrupt early, or whatever, we should throw. throw an exception with why that happened.
If it isn't an exception worthy situation, then we should make it a debug or info level log message.
Why? Simply because WARNING runs the risk of the next person after you seeing this code assuming it means something that it doesn't. It doesn't trigger exception. It doesn't abort. It doesn't indicate any sorts of failure, and it doesn't tell the user anything by default.
jmikola
force-pushed
the
phpc-196
branch
2 times, most recently
from
a21e7e5 to
1e9766b
Compare
|
lgtm What happened to the coveralls reporting? Would be nice to see the test coverage for this |
The string returned by bson_as_json() must be freed.
If the URI cannot be parsed, we should throw an InvalidArgumentException. Failing to initialize the client from a valid URI can remain a RuntimeException.
The read preference and write concern are complex structures, so we can't simply set their options on the URI as we do for other things (e.g. auth credentials).
This avoids copying the client's existing RP or WC struct for no reason.
The extra arguments test can be skipped for HHVM (as was done in mongodb#74). The read preference and write concern tests attempt to cover all permutations of invalid arguments. Detailed exceptions are only possible where we apply array options to the structures, since libmongoc currently provides no detailed errors when URI parsing fails (see: CDRIVER-782).
