CXX-801 Use .terminated().data() to extract safe c-strings

Author: samantharitter

src/bsoncxx/string/view_or_value.cpp

@@ -20,15 +20,19 @@ namespace bsoncxx {
 BSONCXX_INLINE_NAMESPACE_BEGIN
 namespace string {
 
-const char* view_or_value::c_str() {
+view_or_value view_or_value::terminated() const {
     // If we do not own our string, we cannot guarantee that it is null-terminated,
     // so make an owned copy.
-    if (!_value) {
-        _value = std::move(_view.to_string());
-        _view = *_value;
+    if (!is_owning()) {
+        return view_or_value(std::move(view().to_string()));
     }
 
-    return _value->c_str();
+    // If we are owning, return a view_or_value viewing our string
+    return {view()};
+}
+
+const char* view_or_value::data() const {
+    return view().data();
 }
 
 }  // namespace string

src/bsoncxx/string/view_or_value.hpp

@@ -66,15 +66,25 @@ class BSONCXX_API view_or_value : public bsoncxx::view_or_value<stdx::string_vie
     }
 
     ///
-    /// Return a null-terminated string from this string::view_or_value.
+    /// Return a string_view_or_value that is guaranteed to hold a null-terminated
+    /// string. The lifetime of the returned object must be a subset of this object's
+    /// lifetime, because the new view_or_value might hold a view into this one.
     ///
-    /// @note if we are non-owning, we cannot assume our string_view is null-terminated.
-    /// In this case we must make a new, owning view_or_value with a null-terminated
-    /// copy of our view.
+    /// It is recommended that this method be used before calling .data() on a
+    /// view_or_value, as that method may return a non-null-terminated string.
     ///
-    /// @return A pointer to the original string, or a null-terminated copy.
+    /// @return A new view_or_value object.
     ///
-    const char* c_str();
+    view_or_value terminated() const;
+
+    ///
+    /// Call data() on this view_or_value's string_view. This method is not
+    /// guaranteed to return a null-terminated string unless it is used in
+    /// combination with terminated().
+    ///
+    /// @return A const char* of this string.
+    ///
+    const char* data() const;
 };
 
 ///

src/bsoncxx/test/view_or_value.cpp

@@ -226,4 +226,38 @@ TEST_CASE("string::document::view_or_value", "[bsoncxx::string::view_or_value]")
         REQUIRE(carlin != other_name);
         REQUIRE(other_name != carlin);
     }
+
+    SECTION("has a terminated() method that returns a copy") {
+        SECTION("when owning, copy is non-owning") {
+            std::string name{"Rob"};
+            string::view_or_value original{std::move(name)};
+
+            string::view_or_value copy{original.terminated()};
+            REQUIRE(copy == "Rob");
+
+            char* original_name = const_cast<char*>(original.data());
+            original_name[0] = 'B';
+
+            // copy should also reflect the change
+            REQUIRE(copy == "Bob");
+        }
+
+        SECTION("when non-owning, copy is owning") {
+            std::string name{"Sam"};
+            string::view_or_value original{name};
+
+            string::view_or_value copy{original.terminated()};
+            REQUIRE(copy == "Sam");
+
+            char* original_name = const_cast<char*>(original.data());
+            original_name[0] = 'P';
+
+            // copy should not reflect the change
+            REQUIRE(copy == "Sam");
+
+            SECTION("and null-terminated") {
+                REQUIRE(copy.data()[3] == '\0');
+            }
+        }
+    }
 }

src/bsoncxx/view_or_value.hpp

@@ -91,7 +91,8 @@ class view_or_value {
 
     /// TODO CXX-800: Create a noexcept expression to check the conditions that must be met.
     BSONCXX_INLINE view_or_value(view_or_value&& other) noexcept
-        : _value{std::move(other._value)}, _view(_value ? *_value : std::move(other._view)) {
+        : _value{std::move(other._value)},
+          _view(_value ? *_value : std::move(other._view)) {
         other._view = View();
         other._value = stdx::nullopt;
     }
@@ -108,6 +109,15 @@ class view_or_value {
         return *this;
     }
 
+    ///
+    /// Return whether or not this view_or_value owns an underlying Value.
+    ///
+    /// @return bool Whether we are owning.
+    ///
+    BSONCXX_INLINE bool is_owning() const noexcept {
+        return static_cast<bool>(_value);
+    }
+
     ///
     /// This type may be used as a View.
     ///
@@ -121,8 +131,7 @@ class view_or_value {
         return _view;
     }
 
-    // TODO: make these private again (CXX-801)
-   protected:
+   private:
     stdx::optional<Value> _value;
     View _view;
 };

src/mongocxx/collection.cpp

@@ -131,19 +131,20 @@ stdx::string_view collection::name() const {
 void collection::rename(bsoncxx::string::view_or_value new_name, bool drop_target_before_rename) {
     bson_error_t error;
 
-    auto result =
-        libmongoc::collection_rename(_get_impl().collection_t, _get_impl().database_name.c_str(),
-                                     new_name.c_str(), drop_target_before_rename, &error);
+    auto result = libmongoc::collection_rename(
+        _get_impl().collection_t, _get_impl().database_name.c_str(), new_name.terminated().data(),
+        drop_target_before_rename, &error);
 
     if (!result) {
         throw_exception<operation_exception>(error);
     }
 }
 
 collection::collection(const database& database, bsoncxx::string::view_or_value collection_name)
-    : _impl(stdx::make_unique<impl>(libmongoc::database_get_collection(
-                                        database._get_impl().database_t, collection_name.c_str()),
-                                    database.name(), database._get_impl().client_impl)) {
+    : _impl(stdx::make_unique<impl>(
+          libmongoc::database_get_collection(database._get_impl().database_t,
+                                             collection_name.terminated().data()),
+          database.name(), database._get_impl().client_impl)) {
 }
 
 collection::collection(const database& database, void* collection)
@@ -290,7 +291,6 @@ cursor collection::aggregate(const pipeline& pipeline, const options::aggregate&
 
 stdx::optional<result::insert_one> collection::insert_one(view_or_value document,
                                                           const options::insert& options) {
-
     // TODO: We should consider making it possible to convert from an options::insert into
     // an options::bulk_write at the type level, removing the need to re-iterate this code
     // many times here and below.
@@ -382,7 +382,6 @@ stdx::optional<result::update> collection::update_many(view_or_value filter, vie
 
 stdx::optional<result::delete_result> collection::delete_many(
     view_or_value filter, const options::delete_options& options) {
-
     options::bulk_write bulk_opts;
     bulk_opts.ordered(false);
 
@@ -403,7 +402,6 @@ stdx::optional<result::delete_result> collection::delete_many(
 
 stdx::optional<result::update> collection::update_one(view_or_value filter, view_or_value update,
                                                       const options::update& options) {
-
     options::bulk_write bulk_opts;
     bulk_opts.ordered(false);
 
@@ -428,7 +426,6 @@ stdx::optional<result::update> collection::update_one(view_or_value filter, view
 
 stdx::optional<result::delete_result> collection::delete_one(
     view_or_value filter, const options::delete_options& options) {
-
     options::bulk_write bulk_opts;
     bulk_opts.ordered(false);
 

src/mongocxx/database.cpp

@@ -48,8 +48,8 @@ database::~database() = default;
 
 database::database(const class client& client, bsoncxx::string::view_or_value name)
     : _impl(stdx::make_unique<impl>(
-          libmongoc::client_get_database(client._get_impl().client_t, name.c_str()),
-          &client._get_impl(), name.c_str())) {
+          libmongoc::client_get_database(client._get_impl().client_t, name.terminated().data()),
+          &client._get_impl(), name.terminated().data())) {
 }
 
 database::database(const database& d) {
@@ -115,8 +115,8 @@ class collection database::create_collection(bsoncxx::string::view_or_value name
     bson_error_t error;
 
     libbson::scoped_bson_t opts_bson{options.to_document()};
-    auto result = libmongoc::database_create_collection(_get_impl().database_t, name.c_str(),
-                                                        opts_bson.bson(), &error);
+    auto result = libmongoc::database_create_collection(
+        _get_impl().database_t, name.terminated().data(), opts_bson.bson(), &error);
 
     if (!result) {
         throw_exception<operation_exception>(error);
@@ -151,7 +151,8 @@ void database::read_preference(class read_preference rp) {
 
 bool database::has_collection(bsoncxx::string::view_or_value name) const {
     bson_error_t error;
-    return libmongoc::database_has_collection(_get_impl().database_t, name.c_str(), &error);
+    return libmongoc::database_has_collection(_get_impl().database_t, name.terminated().data(),
+                                              &error);
 }
 
 class read_preference database::read_preference() const {