Merge pull request #445 from ssams/develop

Fix copy/move constructor for connect/disconnect opts with properties

src/connect_options.cpp

@@ -62,6 +62,9 @@ connect_options::connect_options(const connect_options& opt) : opts_(opt.opts_),
 	if (opts_.ssl)
 		set_ssl(opt.ssl_);
 
+	if (opts_.connectProperties)
+		set_properties(opt.props_);
+
 	update_c_struct();
 }
 
@@ -85,12 +88,15 @@ connect_options::connect_options(connect_options&& opt) : opts_(opt.opts_),
 
 	if (opts_.ssl)
 		opts_.ssl = &ssl_.opts_;
+
+	if (opts_.connectProperties)
+		opts_.connectProperties = const_cast<MQTTProperties*>(&props_.c_struct());
 
 	update_c_struct();
 }
 
 // Unfortunately, with the existing implementation, there's no way to know
-// if the will and ssl options were set by looking at the C++ structs.
+// if the (connect) properties, will and ssl options were set by looking at the C++ structs.
 // In a major update, we can consider using a pointer or optional<> to
 // indicate that they were set.
 // But, for now, the copy and assignment operations must handle it manually
@@ -164,7 +170,8 @@ connect_options& connect_options::operator=(const connect_options& opt)
 
 	tok_ = opt.tok_;
 	serverURIs_ = opt.serverURIs_;
-	props_ = opt.props_;
+	if (opts_.connectProperties)
+		set_properties(opt.props_);
 
 	httpHeaders_ = opt.httpHeaders_;
 	httpProxy_ = opt.httpProxy_;
@@ -192,7 +199,8 @@ connect_options& connect_options::operator=(connect_options&& opt)
 
 	tok_ = std::move(opt.tok_);
 	serverURIs_ = std::move(opt.serverURIs_);
-	props_ = std::move(opt.props_);
+	if (opts_.connectProperties)
+		set_properties(std::move(opt.props_));
 
 	httpHeaders_ = std::move(opt.httpHeaders_);
 	httpProxy_ = std::move(opt.httpProxy_);

src/disconnect_options.cpp

@@ -15,26 +15,28 @@ disconnect_options::disconnect_options() : opts_(DFLT_C_STRUCT)
 }
 
 disconnect_options::disconnect_options(const disconnect_options& opt)
-			: opts_(opt.opts_), tok_(opt.tok_)
+			: opts_(opt.opts_), tok_(opt.tok_), props_(opt.props_)
 {
 }
 
 disconnect_options::disconnect_options(disconnect_options&& opt)
-			: opts_(opt.opts_), tok_(std::move(opt.tok_))
+			: opts_(opt.opts_), tok_(std::move(opt.tok_)), props_(std::move(opt.props_))
 {
 }
 
 disconnect_options& disconnect_options::operator=(const disconnect_options& opt)
 {
 	opts_ = opt.opts_;
 	tok_ = opt.tok_;
+	props_ = opt.props_;
 	return *this;
 }
 
 disconnect_options& disconnect_options::operator=(disconnect_options&& opt)
 {
 	opts_ = opt.opts_;
 	tok_ = std::move(opt.tok_);
+	props_ = std::move(opt.props_);
 	return *this;
 }
 

test/unit/test_connect_options.cpp

@@ -184,6 +184,15 @@ TEST_CASE("connect_options copy ctor", "[options]")
 		REQUIRE(HTTPS_PROXY == opts.get_https_proxy());
 		REQUIRE(opts.get_http_proxy().empty());
 	}
+
+	SECTION("properties") {
+		orgOpts.set_properties({{ mqtt::property::SESSION_EXPIRY_INTERVAL, 0 }});
+
+		mqtt::connect_options opts { orgOpts };
+
+		REQUIRE(opts.get_properties().contains(mqtt::property::SESSION_EXPIRY_INTERVAL));
+		REQUIRE(opts.c_struct().connectProperties == &opts.get_properties().c_struct());
+	}
 }
 
 // ----------------------------------------------------------------------
@@ -193,30 +202,45 @@ TEST_CASE("connect_options copy ctor", "[options]")
 TEST_CASE("connect_options move_constructor", "[options]")
 {
 	mqtt::connect_options orgOpts { USER, PASSWD };
-	mqtt::connect_options opts { std::move(orgOpts) };
 
-	REQUIRE(USER == opts.get_user_name());
-	REQUIRE(PASSWD == opts.get_password_str());
+	SECTION("simple options") {
+		mqtt::connect_options opts { std::move(orgOpts) };
 
-	const auto& c_struct = opts.c_struct();
+		REQUIRE(USER == opts.get_user_name());
+		REQUIRE(PASSWD == opts.get_password_str());
 
-	REQUIRE(0 == memcmp(&c_struct.struct_id, CSIG, CSIG_LEN));
+		const auto& c_struct = opts.c_struct();
 
-	REQUIRE(0 == strcmp(USER.c_str(), c_struct.username));
-	REQUIRE(c_struct.password == nullptr);
-	REQUIRE(PASSWD.size() == size_t(c_struct.binarypwd.len));
-	REQUIRE(0 == memcmp(PASSWD.data(), c_struct.binarypwd.data, PASSWD.size()));
+		REQUIRE(0 == memcmp(&c_struct.struct_id, CSIG, CSIG_LEN));
 
-	// Make sure it's a true copy, not linked to the original
-	orgOpts.set_user_name(EMPTY_STR);
-	orgOpts.set_password(EMPTY_STR);
+		REQUIRE(0 == strcmp(USER.c_str(), c_struct.username));
+		REQUIRE(c_struct.password == nullptr);
+		REQUIRE(PASSWD.size() == size_t(c_struct.binarypwd.len));
+		REQUIRE(0 == memcmp(PASSWD.data(), c_struct.binarypwd.data, PASSWD.size()));
 
-	REQUIRE(USER == opts.get_user_name());
-	REQUIRE(PASSWD == opts.get_password_str());
+		// Make sure it's a true copy, not linked to the original
+		orgOpts.set_user_name(EMPTY_STR);
+		orgOpts.set_password(EMPTY_STR);
 
-	// Check that the original was moved
-	REQUIRE(EMPTY_STR == orgOpts.get_user_name());
-	REQUIRE(EMPTY_STR == orgOpts.get_password_str());
+		REQUIRE(USER == opts.get_user_name());
+		REQUIRE(PASSWD == opts.get_password_str());
+
+		// Check that the original was moved
+		REQUIRE(EMPTY_STR == orgOpts.get_user_name());
+		REQUIRE(EMPTY_STR == orgOpts.get_password_str());
+		}
+
+	SECTION("properties") {
+		orgOpts.set_properties({{ mqtt::property::SESSION_EXPIRY_INTERVAL, 0 }});
+
+		mqtt::connect_options opts { std::move(orgOpts) };
+
+		REQUIRE(opts.get_properties().contains(mqtt::property::SESSION_EXPIRY_INTERVAL));
+		REQUIRE(opts.c_struct().connectProperties == &opts.get_properties().c_struct());
+
+		// Check that the original was moved
+		REQUIRE(orgOpts.get_properties().empty());
+	}
 }
 
 // ----------------------------------------------------------------------

test/unit/test_disconnect_options.cpp

@@ -76,6 +76,69 @@ TEST_CASE("disconnect_options user constructor", "[options]")
 	REQUIRE(tok == opts.get_token());
 }
 
+// ----------------------------------------------------------------------
+// Test the copy constructor
+// ----------------------------------------------------------------------
+
+TEST_CASE("disconnect_options copy ctor", "[options]")
+{
+	constexpr std::chrono::milliseconds TIMEOUT { 50 };
+
+	mqtt::disconnect_options orgOpts { TIMEOUT };
+
+	SECTION("simple options") {
+    	mqtt::disconnect_options opts { orgOpts };
+
+    	REQUIRE(TIMEOUT == opts.get_timeout());
+
+    	REQUIRE(opts.get_properties().empty());
+
+    	// Make sure it's a true copy, not linked to the original
+    	orgOpts.set_timeout(0);
+    	REQUIRE(TIMEOUT == opts.get_timeout());
+	}
+
+	SECTION("properties") {
+		orgOpts.set_properties({{ mqtt::property::SESSION_EXPIRY_INTERVAL, 0 }});
+
+		mqtt::disconnect_options opts { orgOpts };
+
+		REQUIRE(opts.get_properties().contains(mqtt::property::SESSION_EXPIRY_INTERVAL));
+		REQUIRE(1 == opts.c_struct().properties.count);
+	}
+}
+
+// ----------------------------------------------------------------------
+// Test the move constructor
+// ----------------------------------------------------------------------
+
+TEST_CASE("disconnect_options move_constructor", "[options]")
+{
+	constexpr std::chrono::milliseconds TIMEOUT { 50 };
+
+	mqtt::disconnect_options orgOpts { TIMEOUT };
+
+	SECTION("simple options") {
+		mqtt::disconnect_options opts { std::move(orgOpts) };
+
+		REQUIRE(TIMEOUT == opts.get_timeout());
+
+    	REQUIRE(opts.get_properties().empty());
+	}
+
+	SECTION("properties") {
+		orgOpts.set_properties({{ mqtt::property::SESSION_EXPIRY_INTERVAL, 0 }});
+
+		mqtt::disconnect_options opts { std::move(orgOpts) };
+
+		REQUIRE(opts.get_properties().contains(mqtt::property::SESSION_EXPIRY_INTERVAL));
+		REQUIRE(1 == opts.c_struct().properties.count);
+
+		// Check that the original was moved
+		REQUIRE(orgOpts.get_properties().empty());
+	}
+}
+
 // ----------------------------------------------------------------------
 // Test set timeout
 // ----------------------------------------------------------------------