From fa37dd88c0992f429720a7c9699064b2938f0043 Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Tue, 31 Aug 2021 16:23:36 -0700 Subject: [PATCH 01/10] Set up bsoncxx spuport for catch, etc.. Signed-off-by: Jesse Williamson --- src/bsoncxx/private/helpers.hh | 17 ++++++++++++++++- src/bsoncxx/stdx/optional.hpp | 1 + src/bsoncxx/test_util/catch.hh | 9 +++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/bsoncxx/private/helpers.hh b/src/bsoncxx/private/helpers.hh index ff5b92dd29..9f2ee71aff 100644 --- a/src/bsoncxx/private/helpers.hh +++ b/src/bsoncxx/private/helpers.hh @@ -1,4 +1,4 @@ -// Copyright 2015 MongoDB Inc. +// Copyright 2015-present MongoDB Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -33,6 +34,20 @@ inline document::value value_from_bson_t(const bson_t* bson) { return document::value{view_from_bson_t(bson)}; } +/* +Construct an oid from a bson_oid_t (which is a C API type that we don't want to +expose to the world). + +Note: passing a nullptr is unguarded +Note: Deduction guides aren't yet available to us, so a factory it is! This is +something that can be improved as part of CXX-2350 (migration to more recent C++ +standards). +*/ +inline bsoncxx::oid make_oid(const bson_oid_t* bson_oid) { + return bsoncxx::oid(reinterpret_cast(const_cast(bson_oid)), + bsoncxx::oid::size()); +} + } // namespace helpers BSONCXX_INLINE_NAMESPACE_END } // namespace bsoncxx diff --git a/src/bsoncxx/stdx/optional.hpp b/src/bsoncxx/stdx/optional.hpp index d41ffb12dd..acf63f0fc5 100644 --- a/src/bsoncxx/stdx/optional.hpp +++ b/src/bsoncxx/stdx/optional.hpp @@ -40,6 +40,7 @@ BSONCXX_INLINE_NAMESPACE_END #include #include +#include namespace bsoncxx { BSONCXX_INLINE_NAMESPACE_BEGIN diff --git a/src/bsoncxx/test_util/catch.hh b/src/bsoncxx/test_util/catch.hh index 9a6d358403..382ef4073d 100644 --- a/src/bsoncxx/test_util/catch.hh +++ b/src/bsoncxx/test_util/catch.hh @@ -17,6 +17,7 @@ #include "catch.hpp" #include #include +#include #include #include @@ -25,6 +26,14 @@ namespace Catch { using namespace bsoncxx; // Catch2 must be able to stringify documents, optionals, etc. if they're used in Catch2 macros. + +template <> +struct StringMaker { + static std::string convert(const bsoncxx::oid& value) { + return value.to_string(); + } +}; + template <> struct StringMaker { static std::string convert(const bsoncxx::document::view& value) { From f5e9ee1408c0bd6df6b7ab6096e490f68164b078 Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Tue, 31 Aug 2021 16:26:28 -0700 Subject: [PATCH 02/10] Update event commands. Signed-off-by: Jesse Williamson --- src/mongocxx/events/command_failed_event.cpp | 14 ++++++++++++++ src/mongocxx/events/command_failed_event.hpp | 9 +++++++++ src/mongocxx/events/command_started_event.cpp | 11 +++++++++++ src/mongocxx/events/command_started_event.hpp | 9 +++++++++ src/mongocxx/events/command_succeeded_event.cpp | 11 +++++++++++ src/mongocxx/events/command_succeeded_event.hpp | 9 +++++++++ 6 files changed, 63 insertions(+) diff --git a/src/mongocxx/events/command_failed_event.cpp b/src/mongocxx/events/command_failed_event.cpp index 3ca07ac5ca..33640bed28 100644 --- a/src/mongocxx/events/command_failed_event.cpp +++ b/src/mongocxx/events/command_failed_event.cpp @@ -12,7 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include +#include +#include #include +#include #include #include @@ -52,6 +56,16 @@ std::int64_t command_failed_event::operation_id() const { static_cast(_failed_event)); } +bsoncxx::stdx::optional command_failed_event::service_id() const { + const bson_oid_t* bson_oid = libmongoc::apm_command_failed_get_service_id( + static_cast(_failed_event)); + + if (nullptr == bson_oid) + return {}; + + return {bsoncxx::helpers::make_oid(bson_oid)}; +} + bsoncxx::stdx::string_view command_failed_event::host() const { return libmongoc::apm_command_failed_get_host( static_cast(_failed_event)) diff --git a/src/mongocxx/events/command_failed_event.hpp b/src/mongocxx/events/command_failed_event.hpp index 88ce7d55cf..2837ae935d 100644 --- a/src/mongocxx/events/command_failed_event.hpp +++ b/src/mongocxx/events/command_failed_event.hpp @@ -17,6 +17,8 @@ #include #include +#include +#include #include @@ -75,6 +77,13 @@ class MONGOCXX_API command_failed_event { /// std::int64_t operation_id() const; + /// + /// Optionally returns the service id. + /// + /// @return No contained value, or contains the service id if load balancing is enabled. + /// + bsoncxx::stdx::optional service_id() const; + /// /// Returns the host name. /// diff --git a/src/mongocxx/events/command_started_event.cpp b/src/mongocxx/events/command_started_event.cpp index f731115380..c2aa909e9e 100644 --- a/src/mongocxx/events/command_started_event.cpp +++ b/src/mongocxx/events/command_started_event.cpp @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include @@ -52,6 +53,16 @@ std::int64_t command_started_event::operation_id() const { static_cast(_started_event)); } +bsoncxx::stdx::optional command_started_event::service_id() const { + const bson_oid_t* bson_oid = libmongoc::apm_command_started_get_service_id( + static_cast(_started_event)); + + if (nullptr == bson_oid) + return bsoncxx::stdx::optional{}; + + return bsoncxx::stdx::optional{bsoncxx::helpers::make_oid(bson_oid)}; +} + bsoncxx::stdx::string_view command_started_event::host() const { return libmongoc::apm_command_started_get_host( static_cast(_started_event)) diff --git a/src/mongocxx/events/command_started_event.hpp b/src/mongocxx/events/command_started_event.hpp index c3bb33752f..ebae4a9b48 100644 --- a/src/mongocxx/events/command_started_event.hpp +++ b/src/mongocxx/events/command_started_event.hpp @@ -17,6 +17,8 @@ #include #include +#include +#include #include @@ -75,6 +77,13 @@ class MONGOCXX_API command_started_event { /// std::int64_t operation_id() const; + /// + /// Optionally returns the service id. + /// + /// @return No contained value, or contains the service id if load balancing is enabled. + /// + bsoncxx::stdx::optional service_id() const; + /// /// Returns the host name. /// diff --git a/src/mongocxx/events/command_succeeded_event.cpp b/src/mongocxx/events/command_succeeded_event.cpp index 8322c050fd..64f83f907a 100644 --- a/src/mongocxx/events/command_succeeded_event.cpp +++ b/src/mongocxx/events/command_succeeded_event.cpp @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include @@ -52,6 +53,16 @@ std::int64_t command_succeeded_event::operation_id() const { static_cast(_succeeded_event)); } +bsoncxx::stdx::optional command_succeeded_event::service_id() const { + const bson_oid_t* bson_oid = libmongoc::apm_command_succeeded_get_service_id( + static_cast(_succeeded_event)); + + if (nullptr == bson_oid) + return bsoncxx::stdx::optional{}; + + return bsoncxx::stdx::optional{bsoncxx::helpers::make_oid(bson_oid)}; +} + bsoncxx::stdx::string_view command_succeeded_event::host() const { return libmongoc::apm_command_succeeded_get_host( static_cast(_succeeded_event)) diff --git a/src/mongocxx/events/command_succeeded_event.hpp b/src/mongocxx/events/command_succeeded_event.hpp index ef066c18cd..f3afd971e8 100644 --- a/src/mongocxx/events/command_succeeded_event.hpp +++ b/src/mongocxx/events/command_succeeded_event.hpp @@ -17,6 +17,8 @@ #include #include +#include +#include #include @@ -75,6 +77,13 @@ class MONGOCXX_API command_succeeded_event { /// std::int64_t operation_id() const; + /// + /// Optionally returns the service id. + /// + /// @return No contained value, or contains the service id if load balancing is enabled. + /// + bsoncxx::stdx::optional service_id() const; + /// /// Returns the host name. /// From f1afa85be90618af7ff9910bc2ca8e68dd666502 Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Tue, 31 Aug 2021 16:45:28 -0700 Subject: [PATCH 03/10] Import rewritten mongoc_symbols Signed-off-by: Jesse Williamson --- src/mongocxx/private/libmongoc_symbols.hh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/mongocxx/private/libmongoc_symbols.hh b/src/mongocxx/private/libmongoc_symbols.hh index 018fc9317b..ca3c546dac 100644 --- a/src/mongocxx/private/libmongoc_symbols.hh +++ b/src/mongocxx/private/libmongoc_symbols.hh @@ -25,6 +25,7 @@ MONGOCXX_LIBMONGOC_SYMBOL(apm_command_failed_get_operation_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_failed_get_reply) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_failed_get_request_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_failed_get_server_id) +MONGOCXX_LIBMONGOC_SYMBOL(apm_command_failed_get_service_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_started_get_command) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_started_get_command_name) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_started_get_context) @@ -33,6 +34,7 @@ MONGOCXX_LIBMONGOC_SYMBOL(apm_command_started_get_host) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_started_get_operation_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_started_get_request_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_started_get_server_id) +MONGOCXX_LIBMONGOC_SYMBOL(apm_command_started_get_service_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_succeeded_get_command_name) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_succeeded_get_context) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_succeeded_get_duration) @@ -41,6 +43,7 @@ MONGOCXX_LIBMONGOC_SYMBOL(apm_command_succeeded_get_operation_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_succeeded_get_reply) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_succeeded_get_request_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_command_succeeded_get_server_id) +MONGOCXX_LIBMONGOC_SYMBOL(apm_command_succeeded_get_service_id) MONGOCXX_LIBMONGOC_SYMBOL(apm_server_changed_get_context) MONGOCXX_LIBMONGOC_SYMBOL(apm_server_changed_get_host) MONGOCXX_LIBMONGOC_SYMBOL(apm_server_changed_get_new_description) From 05e1338bc37d9e22eac49d25bce9bd852e133d8d Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Tue, 31 Aug 2021 16:46:16 -0700 Subject: [PATCH 04/10] Add unit test for serviceId availability. Signed-off-by: Jesse Williamson --- src/mongocxx/test/database.cpp | 58 +++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/src/mongocxx/test/database.cpp b/src/mongocxx/test/database.cpp index 7c1e88590e..e343560a96 100644 --- a/src/mongocxx/test/database.cpp +++ b/src/mongocxx/test/database.cpp @@ -1,4 +1,4 @@ -// Copyright 2014 MongoDB Inc. +// Copyright 2014-present MongoDB Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -492,4 +492,60 @@ TEST_CASE("Database integration tests", "[database]") { } } +// As C++11 lacks generic lambdas, and "ordinary" templates can't appear at block scope, +// we'll have to define our helper for the serviceId tests here. This implementation would +// be more straightforward in newer versions of C++ (C++14 and on allow generic lambdas), +// see CXX-2350 (migration to more recent C++ standards): +template +struct check_service_id +{ + void operator()(const EventT& event) { + + // We MUST NOT report a service_id() outside of load-balancing mode, but in + // load-balancing mode, a value is required: + INFO("checking for service_id()") + CAPTURE(event.command_name(), test_util::is_load_balanced()); + auto service_id = event.service_id(); + if (test_util::is_load_balanced()) + CHECK(service_id); + else + CHECK_FALSE(service_id); + } +}; + + +TEST_CASE("Test serviceId is included in command monitoring events") { + + instance::current(); + + auto client_opts = test_util::add_test_server_api(); + + // Set Application Performance Monitoring options (APM): + mongocxx::options::apm apm_opts; + apm_opts.on_command_started ( check_service_id{} ); + apm_opts.on_command_succeeded ( check_service_id {} ); + apm_opts.on_command_failed ( check_service_id {} ); + + client_opts.apm_opts (apm_opts); + // Adding ?loadBalanced=true results in an error in the C driver because + // the initial hello response from the server does not include serviceId. + client mongo_client(uri("mongodb://localhost:27017/"), client_opts); + stdx::string_view database_name{"database"}; + database database = mongo_client[database_name]; + + // Mock mongoc_apm_command_started_get_service_id + auto apm_command_started_get_service_id = libmongoc::apm_command_started_get_service_id.create_instance(); + + // We have no actual way of knowing if load-balancing is enabled, so we'll have to satisfy ourselves + // with checking for the positive case for now: + apm_command_started_get_service_id->interpose([&] (const mongoc_apm_command_started_t *) { + static bson_oid_t tmp = {}; + return &tmp; + }); + + // Run a command, triggering start and completion: + auto cmd = make_document (kvp ("ping", 1)); + database.run_command (cmd.view()); +} + } // namespace From f228dc8f3e9a3f861c4b7993cc800d3811f5dda5 Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Tue, 31 Aug 2021 16:47:11 -0700 Subject: [PATCH 05/10] Various cleanup; fix nullopt_t usage; remove is_load_balanced() Signed-off-by: Jesse Williamson --- src/mongocxx/events/command_failed_event.cpp | 2 +- src/mongocxx/events/command_started_event.cpp | 4 +- .../events/command_succeeded_event.cpp | 4 +- src/mongocxx/test/database.cpp | 65 +++++++++++++------ 4 files changed, 51 insertions(+), 24 deletions(-) diff --git a/src/mongocxx/events/command_failed_event.cpp b/src/mongocxx/events/command_failed_event.cpp index 33640bed28..5433d9b45b 100644 --- a/src/mongocxx/events/command_failed_event.cpp +++ b/src/mongocxx/events/command_failed_event.cpp @@ -61,7 +61,7 @@ bsoncxx::stdx::optional command_failed_event::service_id() const { static_cast(_failed_event)); if (nullptr == bson_oid) - return {}; + return { bsoncxx::stdx::nullopt }; return {bsoncxx::helpers::make_oid(bson_oid)}; } diff --git a/src/mongocxx/events/command_started_event.cpp b/src/mongocxx/events/command_started_event.cpp index c2aa909e9e..dafb0788ba 100644 --- a/src/mongocxx/events/command_started_event.cpp +++ b/src/mongocxx/events/command_started_event.cpp @@ -58,9 +58,9 @@ bsoncxx::stdx::optional command_started_event::service_id() const static_cast(_started_event)); if (nullptr == bson_oid) - return bsoncxx::stdx::optional{}; + return { bsoncxx::stdx::nullopt }; - return bsoncxx::stdx::optional{bsoncxx::helpers::make_oid(bson_oid)}; + return {bsoncxx::helpers::make_oid(bson_oid)}; } bsoncxx::stdx::string_view command_started_event::host() const { diff --git a/src/mongocxx/events/command_succeeded_event.cpp b/src/mongocxx/events/command_succeeded_event.cpp index 64f83f907a..234a602c95 100644 --- a/src/mongocxx/events/command_succeeded_event.cpp +++ b/src/mongocxx/events/command_succeeded_event.cpp @@ -58,9 +58,9 @@ bsoncxx::stdx::optional command_succeeded_event::service_id() cons static_cast(_succeeded_event)); if (nullptr == bson_oid) - return bsoncxx::stdx::optional{}; + return { bsoncxx::stdx::nullopt }; - return bsoncxx::stdx::optional{bsoncxx::helpers::make_oid(bson_oid)}; + return {bsoncxx::helpers::make_oid(bson_oid)}; } bsoncxx::stdx::string_view command_succeeded_event::host() const { diff --git a/src/mongocxx/test/database.cpp b/src/mongocxx/test/database.cpp index e343560a96..1e8275993f 100644 --- a/src/mongocxx/test/database.cpp +++ b/src/mongocxx/test/database.cpp @@ -501,20 +501,18 @@ struct check_service_id { void operator()(const EventT& event) { - // We MUST NOT report a service_id() outside of load-balancing mode, but in - // load-balancing mode, a value is required: - INFO("checking for service_id()") - CAPTURE(event.command_name(), test_util::is_load_balanced()); - auto service_id = event.service_id(); - if (test_util::is_load_balanced()) - CHECK(service_id); - else + // We MUST NOT report a service_id() outside of load-balancing mode, but in + // load-balancing mode, a value is required: + INFO("checking for service_id()") + CAPTURE(event.command_name(), test_util::is_load_balanced()); + auto service_id = event.service_id(); + + // We do NOT expect a service_id when NOT IN load-balanced mode: CHECK_FALSE(service_id); } }; - -TEST_CASE("Test serviceId is included in command monitoring events") { +TEST_CASE("Test serviceId is not included in command monitoring events when not in load-balancing mode") { instance::current(); @@ -526,26 +524,55 @@ TEST_CASE("Test serviceId is included in command monitoring events") { apm_opts.on_command_succeeded ( check_service_id {} ); apm_opts.on_command_failed ( check_service_id {} ); - client_opts.apm_opts (apm_opts); - // Adding ?loadBalanced=true results in an error in the C driver because - // the initial hello response from the server does not include serviceId. - client mongo_client(uri("mongodb://localhost:27017/"), client_opts); - stdx::string_view database_name{"database"}; - database database = mongo_client[database_name]; - - // Mock mongoc_apm_command_started_get_service_id + // Set up mocking for mongoc_apm_command_started_get_service_id: auto apm_command_started_get_service_id = libmongoc::apm_command_started_get_service_id.create_instance(); + auto apm_command_succeeded_get_service_id = libmongoc::apm_command_succeeded_get_service_id.create_instance(); + auto apm_command_failed_get_service_id = libmongoc::apm_command_failed_get_service_id.create_instance(); // We have no actual way of knowing if load-balancing is enabled, so we'll have to satisfy ourselves // with checking for the positive case for now: + + struct + { + bson_oid_t *operator()(const void *) { + static bson_oid_t tmp = {}; + return &tmp; + } + } make_empty_bson_oid_t; + + apm_command_started_get_service_id->interpose(make_empty_bson_oid_t); + +/* apm_command_started_get_service_id->interpose([&] (const mongoc_apm_command_started_t *) { static bson_oid_t tmp = {}; return &tmp; }); +*/ + apm_command_succeeded_get_service_id->interpose([&] (const mongoc_apm_command_succeeded_t *) { + static bson_oid_t tmp = {}; + return &tmp; + }); + + apm_command_failed_get_service_id->interpose([&] (const mongoc_apm_command_failed_t *) { + static bson_oid_t tmp = {}; + return &tmp; + }); + + // Set up our connection: + client_opts.apm_opts (apm_opts); + // Adding ?loadBalanced=true results in an error in the C driver because + // the initial hello response from the server does not include serviceId. + client mongo_client(uri("mongodb://localhost:27017/"), client_opts); + stdx::string_view database_name{"database"}; + database database = mongo_client[database_name]; - // Run a command, triggering start and completion: + // Run a command, triggering start and completion events: auto cmd = make_document (kvp ("ping", 1)); database.run_command (cmd.view()); + + // Attempt to trigger failure: + cmd = make_document (kvp ("some_sort_of_invalid_command_that_should_never_happen", 1)); + database.run_command (cmd.view()); } } // namespace From 0bc1ae576dbb3413ce6732d9089a9e41326a09ef Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Fri, 10 Sep 2021 17:50:19 -0700 Subject: [PATCH 06/10] Tests for mocked load-balanced or non-load-balanced service_id. Signed-off-by: Jesse Williamson --- src/mongocxx/test/database.cpp | 82 +++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 37 deletions(-) diff --git a/src/mongocxx/test/database.cpp b/src/mongocxx/test/database.cpp index 1e8275993f..9a1caeda51 100644 --- a/src/mongocxx/test/database.cpp +++ b/src/mongocxx/test/database.cpp @@ -495,24 +495,35 @@ TEST_CASE("Database integration tests", "[database]") { // As C++11 lacks generic lambdas, and "ordinary" templates can't appear at block scope, // we'll have to define our helper for the serviceId tests here. This implementation would // be more straightforward in newer versions of C++ (C++14 and on allow generic lambdas), -// see CXX-2350 (migration to more recent C++ standards): +// see CXX-2350 (migration to more recent C++ standards); our C++17 optional<> implmentation +// also has a few inconsistencies with the standard, which appear to vary across platforms: template struct check_service_id { + const bool expect_service_id; + + check_service_id(const bool expect_service_id) + : expect_service_id(expect_service_id) + {} + void operator()(const EventT& event) { - // We MUST NOT report a service_id() outside of load-balancing mode, but in - // load-balancing mode, a value is required: INFO("checking for service_id()") - CAPTURE(event.command_name(), test_util::is_load_balanced()); + CAPTURE(event.command_name(), expect_service_id); + auto service_id = event.service_id(); - // We do NOT expect a service_id when NOT IN load-balanced mode: - CHECK_FALSE(service_id); + if(expect_service_id) + CHECK(service_id); + else + CHECK_FALSE(service_id); } }; -TEST_CASE("Test serviceId is not included in command monitoring events when not in load-balancing mode") { +TEST_CASE("serviceId presence depends on load-balancing mode") { + + // Repeat this test case for a situation in which service_id should and should not be returned: + auto expect_service_id = GENERATE(true, false); instance::current(); @@ -520,43 +531,40 @@ TEST_CASE("Test serviceId is not included in command monitoring events when not // Set Application Performance Monitoring options (APM): mongocxx::options::apm apm_opts; - apm_opts.on_command_started ( check_service_id{} ); - apm_opts.on_command_succeeded ( check_service_id {} ); - apm_opts.on_command_failed ( check_service_id {} ); + apm_opts.on_command_started ( check_service_id{ expect_service_id } ); + apm_opts.on_command_succeeded ( check_service_id { expect_service_id } ); + apm_opts.on_command_failed ( check_service_id { expect_service_id } ); // Set up mocking for mongoc_apm_command_started_get_service_id: auto apm_command_started_get_service_id = libmongoc::apm_command_started_get_service_id.create_instance(); auto apm_command_succeeded_get_service_id = libmongoc::apm_command_succeeded_get_service_id.create_instance(); auto apm_command_failed_get_service_id = libmongoc::apm_command_failed_get_service_id.create_instance(); - // We have no actual way of knowing if load-balancing is enabled, so we'll have to satisfy ourselves - // with checking for the positive case for now: + // Set up mocked functions that DO emit a service_id: + if(expect_service_id) + { + // Return a bson_oid_t with data where the service_id has some value: + struct { bson_oid_t *operator()(const void *) { + static bson_oid_t tmp = { 0x65 }; + return &tmp; + } + } make_service_id_bson_oid_t; + + // Add forever() so that the mock function also extends to endSession (thanks, kalbertson!): + apm_command_started_get_service_id->interpose(make_service_id_bson_oid_t).forever(); + apm_command_succeeded_get_service_id->interpose(make_service_id_bson_oid_t).forever(); + apm_command_failed_get_service_id->interpose(make_service_id_bson_oid_t).forever(); + } - struct + // Set up mocked functions that DO NOT emit a service_id: + if(!expect_service_id) { - bson_oid_t *operator()(const void *) { - static bson_oid_t tmp = {}; - return &tmp; - } - } make_empty_bson_oid_t; - - apm_command_started_get_service_id->interpose(make_empty_bson_oid_t); - -/* - apm_command_started_get_service_id->interpose([&] (const mongoc_apm_command_started_t *) { - static bson_oid_t tmp = {}; - return &tmp; - }); -*/ - apm_command_succeeded_get_service_id->interpose([&] (const mongoc_apm_command_succeeded_t *) { - static bson_oid_t tmp = {}; - return &tmp; - }); - - apm_command_failed_get_service_id->interpose([&] (const mongoc_apm_command_failed_t *) { - static bson_oid_t tmp = {}; - return &tmp; - }); + auto make_empty_bson_oid_t = [](const void *) -> bson_oid_t * { return nullptr; }; + + apm_command_started_get_service_id->interpose(make_empty_bson_oid_t).forever(); + apm_command_succeeded_get_service_id->interpose(make_empty_bson_oid_t).forever(); + apm_command_failed_get_service_id->interpose(make_empty_bson_oid_t).forever(); + } // Set up our connection: client_opts.apm_opts (apm_opts); @@ -572,7 +580,7 @@ TEST_CASE("Test serviceId is not included in command monitoring events when not // Attempt to trigger failure: cmd = make_document (kvp ("some_sort_of_invalid_command_that_should_never_happen", 1)); - database.run_command (cmd.view()); + CHECK_THROWS(database.run_command (cmd.view())); } } // namespace From 32cdfae8eda8a712ce03ff02d73b5fd6a20275be Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Mon, 13 Sep 2021 17:57:29 -0700 Subject: [PATCH 07/10] Apply clang-format. Signed-off-by: Jesse Williamson --- src/mongocxx/events/command_failed_event.cpp | 2 +- src/mongocxx/events/command_started_event.cpp | 2 +- .../events/command_succeeded_event.cpp | 2 +- src/mongocxx/test/database.cpp | 82 +++++++++---------- 4 files changed, 44 insertions(+), 44 deletions(-) diff --git a/src/mongocxx/events/command_failed_event.cpp b/src/mongocxx/events/command_failed_event.cpp index 5433d9b45b..e9139a7120 100644 --- a/src/mongocxx/events/command_failed_event.cpp +++ b/src/mongocxx/events/command_failed_event.cpp @@ -61,7 +61,7 @@ bsoncxx::stdx::optional command_failed_event::service_id() const { static_cast(_failed_event)); if (nullptr == bson_oid) - return { bsoncxx::stdx::nullopt }; + return {bsoncxx::stdx::nullopt}; return {bsoncxx::helpers::make_oid(bson_oid)}; } diff --git a/src/mongocxx/events/command_started_event.cpp b/src/mongocxx/events/command_started_event.cpp index dafb0788ba..8076f1ed3b 100644 --- a/src/mongocxx/events/command_started_event.cpp +++ b/src/mongocxx/events/command_started_event.cpp @@ -58,7 +58,7 @@ bsoncxx::stdx::optional command_started_event::service_id() const static_cast(_started_event)); if (nullptr == bson_oid) - return { bsoncxx::stdx::nullopt }; + return {bsoncxx::stdx::nullopt}; return {bsoncxx::helpers::make_oid(bson_oid)}; } diff --git a/src/mongocxx/events/command_succeeded_event.cpp b/src/mongocxx/events/command_succeeded_event.cpp index 234a602c95..4c1ffd38f4 100644 --- a/src/mongocxx/events/command_succeeded_event.cpp +++ b/src/mongocxx/events/command_succeeded_event.cpp @@ -58,7 +58,7 @@ bsoncxx::stdx::optional command_succeeded_event::service_id() cons static_cast(_succeeded_event)); if (nullptr == bson_oid) - return { bsoncxx::stdx::nullopt }; + return {bsoncxx::stdx::nullopt}; return {bsoncxx::helpers::make_oid(bson_oid)}; } diff --git a/src/mongocxx/test/database.cpp b/src/mongocxx/test/database.cpp index 9a1caeda51..6975129efb 100644 --- a/src/mongocxx/test/database.cpp +++ b/src/mongocxx/test/database.cpp @@ -494,34 +494,29 @@ TEST_CASE("Database integration tests", "[database]") { // As C++11 lacks generic lambdas, and "ordinary" templates can't appear at block scope, // we'll have to define our helper for the serviceId tests here. This implementation would -// be more straightforward in newer versions of C++ (C++14 and on allow generic lambdas), +// be more straightforward in newer versions of C++ (C++14 and on allow generic lambdas), // see CXX-2350 (migration to more recent C++ standards); our C++17 optional<> implmentation // also has a few inconsistencies with the standard, which appear to vary across platforms: template -struct check_service_id -{ +struct check_service_id { const bool expect_service_id; - check_service_id(const bool expect_service_id) - : expect_service_id(expect_service_id) - {} + check_service_id(const bool expect_service_id) : expect_service_id(expect_service_id) {} void operator()(const EventT& event) { - INFO("checking for service_id()") CAPTURE(event.command_name(), expect_service_id); auto service_id = event.service_id(); - if(expect_service_id) - CHECK(service_id); + if (expect_service_id) + CHECK(service_id); else - CHECK_FALSE(service_id); + CHECK_FALSE(service_id); } }; TEST_CASE("serviceId presence depends on load-balancing mode") { - // Repeat this test case for a situation in which service_id should and should not be returned: auto expect_service_id = GENERATE(true, false); @@ -531,43 +526,48 @@ TEST_CASE("serviceId presence depends on load-balancing mode") { // Set Application Performance Monitoring options (APM): mongocxx::options::apm apm_opts; - apm_opts.on_command_started ( check_service_id{ expect_service_id } ); - apm_opts.on_command_succeeded ( check_service_id { expect_service_id } ); - apm_opts.on_command_failed ( check_service_id { expect_service_id } ); + apm_opts.on_command_started( + check_service_id{expect_service_id}); + apm_opts.on_command_succeeded( + check_service_id{expect_service_id}); + apm_opts.on_command_failed( + check_service_id{expect_service_id}); // Set up mocking for mongoc_apm_command_started_get_service_id: - auto apm_command_started_get_service_id = libmongoc::apm_command_started_get_service_id.create_instance(); - auto apm_command_succeeded_get_service_id = libmongoc::apm_command_succeeded_get_service_id.create_instance(); - auto apm_command_failed_get_service_id = libmongoc::apm_command_failed_get_service_id.create_instance(); + auto apm_command_started_get_service_id = + libmongoc::apm_command_started_get_service_id.create_instance(); + auto apm_command_succeeded_get_service_id = + libmongoc::apm_command_succeeded_get_service_id.create_instance(); + auto apm_command_failed_get_service_id = + libmongoc::apm_command_failed_get_service_id.create_instance(); // Set up mocked functions that DO emit a service_id: - if(expect_service_id) - { - // Return a bson_oid_t with data where the service_id has some value: - struct { bson_oid_t *operator()(const void *) { - static bson_oid_t tmp = { 0x65 }; - return &tmp; - } - } make_service_id_bson_oid_t; - - // Add forever() so that the mock function also extends to endSession (thanks, kalbertson!): - apm_command_started_get_service_id->interpose(make_service_id_bson_oid_t).forever(); - apm_command_succeeded_get_service_id->interpose(make_service_id_bson_oid_t).forever(); - apm_command_failed_get_service_id->interpose(make_service_id_bson_oid_t).forever(); + if (expect_service_id) { + // Return a bson_oid_t with data where the service_id has some value: + struct { + bson_oid_t* operator()(const void*) { + static bson_oid_t tmp = {0x65}; + return &tmp; + } + } make_service_id_bson_oid_t; + + // Add forever() so that the mock function also extends to endSession (thanks, kalbertson!): + apm_command_started_get_service_id->interpose(make_service_id_bson_oid_t).forever(); + apm_command_succeeded_get_service_id->interpose(make_service_id_bson_oid_t).forever(); + apm_command_failed_get_service_id->interpose(make_service_id_bson_oid_t).forever(); } // Set up mocked functions that DO NOT emit a service_id: - if(!expect_service_id) - { - auto make_empty_bson_oid_t = [](const void *) -> bson_oid_t * { return nullptr; }; + if (!expect_service_id) { + auto make_empty_bson_oid_t = [](const void*) -> bson_oid_t* { return nullptr; }; - apm_command_started_get_service_id->interpose(make_empty_bson_oid_t).forever(); - apm_command_succeeded_get_service_id->interpose(make_empty_bson_oid_t).forever(); - apm_command_failed_get_service_id->interpose(make_empty_bson_oid_t).forever(); + apm_command_started_get_service_id->interpose(make_empty_bson_oid_t).forever(); + apm_command_succeeded_get_service_id->interpose(make_empty_bson_oid_t).forever(); + apm_command_failed_get_service_id->interpose(make_empty_bson_oid_t).forever(); } // Set up our connection: - client_opts.apm_opts (apm_opts); + client_opts.apm_opts(apm_opts); // Adding ?loadBalanced=true results in an error in the C driver because // the initial hello response from the server does not include serviceId. client mongo_client(uri("mongodb://localhost:27017/"), client_opts); @@ -575,12 +575,12 @@ TEST_CASE("serviceId presence depends on load-balancing mode") { database database = mongo_client[database_name]; // Run a command, triggering start and completion events: - auto cmd = make_document (kvp ("ping", 1)); - database.run_command (cmd.view()); + auto cmd = make_document(kvp("ping", 1)); + database.run_command(cmd.view()); // Attempt to trigger failure: - cmd = make_document (kvp ("some_sort_of_invalid_command_that_should_never_happen", 1)); - CHECK_THROWS(database.run_command (cmd.view())); + cmd = make_document(kvp("some_sort_of_invalid_command_that_should_never_happen", 1)); + CHECK_THROWS(database.run_command(cmd.view())); } } // namespace From 62a4563d05b0aedab4f4271d2a1214715bae0cf9 Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Wed, 15 Sep 2021 09:40:57 -0700 Subject: [PATCH 08/10] Cleanups in response to review comments. Signed-off-by: Jesse Williamson --- src/mongocxx/test/database.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mongocxx/test/database.cpp b/src/mongocxx/test/database.cpp index 6975129efb..394e918dc6 100644 --- a/src/mongocxx/test/database.cpp +++ b/src/mongocxx/test/database.cpp @@ -495,8 +495,8 @@ TEST_CASE("Database integration tests", "[database]") { // As C++11 lacks generic lambdas, and "ordinary" templates can't appear at block scope, // we'll have to define our helper for the serviceId tests here. This implementation would // be more straightforward in newer versions of C++ (C++14 and on allow generic lambdas), -// see CXX-2350 (migration to more recent C++ standards); our C++17 optional<> implmentation -// also has a few inconsistencies with the standard, which appear to vary across platforms: +// see CXX-2350 (migration to more recent C++ standards); our C++17 optional<> implementation +// also has a few inconsistencies with the standard, which appear to vary across platforms. template struct check_service_id { const bool expect_service_id; @@ -551,7 +551,7 @@ TEST_CASE("serviceId presence depends on load-balancing mode") { } } make_service_id_bson_oid_t; - // Add forever() so that the mock function also extends to endSession (thanks, kalbertson!): + // Add forever() so that the mock function also extends to endSession: apm_command_started_get_service_id->interpose(make_service_id_bson_oid_t).forever(); apm_command_succeeded_get_service_id->interpose(make_service_id_bson_oid_t).forever(); apm_command_failed_get_service_id->interpose(make_service_id_bson_oid_t).forever(); From 54de320e3b3989ff304e031f3827a1a3c08d6d17 Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Wed, 15 Sep 2021 13:14:11 -0700 Subject: [PATCH 09/10] Small fixups in response to review. Signed-off-by: Jesse Williamson --- src/bsoncxx/private/helpers.hh | 2 +- src/mongocxx/test/database.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bsoncxx/private/helpers.hh b/src/bsoncxx/private/helpers.hh index 9f2ee71aff..ec4e2d7abd 100644 --- a/src/bsoncxx/private/helpers.hh +++ b/src/bsoncxx/private/helpers.hh @@ -44,7 +44,7 @@ something that can be improved as part of CXX-2350 (migration to more recent C++ standards). */ inline bsoncxx::oid make_oid(const bson_oid_t* bson_oid) { - return bsoncxx::oid(reinterpret_cast(const_cast(bson_oid)), + return bsoncxx::oid(reinterpret_cast(bson_oid), bsoncxx::oid::size()); } diff --git a/src/mongocxx/test/database.cpp b/src/mongocxx/test/database.cpp index 394e918dc6..d7e1e19185 100644 --- a/src/mongocxx/test/database.cpp +++ b/src/mongocxx/test/database.cpp @@ -533,7 +533,7 @@ TEST_CASE("serviceId presence depends on load-balancing mode") { apm_opts.on_command_failed( check_service_id{expect_service_id}); - // Set up mocking for mongoc_apm_command_started_get_service_id: + // Set up mocking for get_service_id events: auto apm_command_started_get_service_id = libmongoc::apm_command_started_get_service_id.create_instance(); auto apm_command_succeeded_get_service_id = From 6256165e5eb3801ab5e675533cdad752ae4d0b6e Mon Sep 17 00:00:00 2001 From: Jesse Williamson Date: Wed, 15 Sep 2021 14:21:53 -0700 Subject: [PATCH 10/10] Update test, move function object to lambda. Signed-off-by: Jesse Williamson --- src/mongocxx/test/database.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/mongocxx/test/database.cpp b/src/mongocxx/test/database.cpp index d7e1e19185..e7aeb3341f 100644 --- a/src/mongocxx/test/database.cpp +++ b/src/mongocxx/test/database.cpp @@ -544,12 +544,10 @@ TEST_CASE("serviceId presence depends on load-balancing mode") { // Set up mocked functions that DO emit a service_id: if (expect_service_id) { // Return a bson_oid_t with data where the service_id has some value: - struct { - bson_oid_t* operator()(const void*) { - static bson_oid_t tmp = {0x65}; - return &tmp; - } - } make_service_id_bson_oid_t; + const auto make_service_id_bson_oid_t = [](const void *) -> const bson_oid_t* { + static bson_oid_t tmp = { 0x65 }; + return &tmp; + }; // Add forever() so that the mock function also extends to endSession: apm_command_started_get_service_id->interpose(make_service_id_bson_oid_t).forever();