v1: pool (CXX-3237, CXX-3238)#1566
Conversation
kevinAlbs
left a comment
There was a problem hiding this comment.
LGTM. Only minor comments.
|
|
||
| using code = mongocxx::v1::pool::errc; | ||
|
|
||
| TEST_CASE("error code", "[bsoncxx][v1][pool][error]") { |
There was a problem hiding this comment.
| TEST_CASE("error code", "[bsoncxx][v1][pool][error]") { | |
| TEST_CASE("error code", "[mongocxx][v1][pool][error]") { |
|
|
||
| struct identity_type {}; | ||
|
|
||
| // Represent a successfully-constructed mocked `v1::client` object. |
There was a problem hiding this comment.
| // Represent a successfully-constructed mocked `v1::client` object. | |
| // Represent a successfully-constructed mocked `v1::pool` object. |
| auto const tls_opts_set = GENERATE(false, true); | ||
| CAPTURE(tls_opts_set); |
There was a problem hiding this comment.
| auto const tls_opts_set = GENERATE(false, true); | |
| CAPTURE(tls_opts_set); |
Remove tls_opts_set? Appears redundant with tls_enabled.
There was a problem hiding this comment.
Whoops. This input value was intended to exercise the "URI does not enable TLS, but TLS options were still set" scenario, but the opts.tls_opts() was not guarded on this properly. Updated both the client and pool tests accordingly.
| @@ -13,17 +13,3 @@ | |||
| // limitations under the License. | |||
|
|
|||
| #include <mongocxx/options/pool.hpp> | |||
There was a problem hiding this comment.
Remove file? Or is this intended to isolate compile coverage of mongoc/options/pool.hpp?
There was a problem hiding this comment.
Yes, this file is acting as the standalone compilation test of the mongocxx/options/pool component.
Resolves CXX-3237 and CXX-3238 for the
v1::poolcomponent and implements dependent functions inv1::auto_encryption_optionsandv1::client.On further inspection, opted to make the
v1::poolAPI more consistent withv1::clientAPI by:v1::uri(e.g.v1::client c = v1::uri{"abc"};<->v1::pool p = v1::uri{"abc"};)..database()to complementoperator[](e.g.entry["db"]+entry.database("db")).The same
T const& -> Tchange made for thev1::client::optionsparameter in #1559 is applied to thev1::pool::optionsparameter to permit moving the APM callback functions. Additionally, database name parameters are changed fromstd::stringtobsoncxx::v1::stdx::string_viewfor consistency with the removal of the owning internal data member in #1564.The formerly
v1::client-specific internal helpers for translating option classes into BSON fields are refactored into their respective components for reusability byv1::pool. The implementation of thev1::poolconstructor is deliberately made as symmetric as possible with thev1::clientconstructor.Although
v1::poolis moveable, we do not support v_noabi <-> v1 conversion due to the potential invalidation of associated client objects (a similar concern to client objects associated withv1::client_session). Similarly, there is no v_noabi <-> conversions forv_noabi::options::pooldue to no conversions forv_noabi::options::client(due to.auto_encryption_opts()) or forv_noabi::pool::entry(due toT const&accessors). However, users who want to convert pool options between v_noabi <-> v1 can do so by converting between v_noabi <-> client options instead.Nevertheless, because the pool API does not expose the client or auto encryption options post-initialization, the initialization of
v_noabi::poolcan still be refactored to be (partially) implemented in terms ofv1::pool; pool options are still handled separately to avoid intermediate deep-copies.Because pool options are essentially identical to client options, test coverage for the behavior of
v1::pool::optionsmostly deferred to equivalentv1::client:optionstest cases. The pool option test cases are reduced to ensuring pool-specific code paths are exercised. The option field handlers are already exhaustively tested byv1::client::optionstest cases added by #1559.