diff --git a/sharing/outgoing_share_session.cc b/sharing/outgoing_share_session.cc index af33e1c9..a596c015 100644 --- a/sharing/outgoing_share_session.cc +++ b/sharing/outgoing_share_session.cc @@ -25,6 +25,7 @@ #include #include +#include "absl/functional/any_invocable.h" #include "absl/strings/string_view.h" #include "absl/time/time.h" #include "internal/platform/clock.h" @@ -116,7 +117,7 @@ OutgoingShareSession::OutgoingShareSession( NearbyConnectionsManager* connections_manager, analytics::AnalyticsRecorder& analytics_recorder, std::string endpoint_id, const ShareTarget& share_target, - std::function + absl::AnyInvocable transfer_update_callback) : ShareSession(clock, service_thread, connections_manager, analytics_recorder, std::move(endpoint_id), share_target), @@ -489,18 +490,23 @@ void OutgoingShareSession::DelayComplete( }); } -void OutgoingShareSession::UpdateSessionForDedup( +bool OutgoingShareSession::UpdateSessionForDedup( const ShareTarget& share_target, std::optional certificate, absl::string_view endpoint_id) { - if (IsConnected()) return; + LOG_IF(DFATAL, share_target.id != this->share_target().id) + << "Share target id cannot be changed during deduplication."; set_share_target(share_target); + if (IsConnected()) { + return false; + } set_endpoint_id(endpoint_id); if (certificate.has_value()) { set_certificate(std::move(certificate.value())); } else { clear_certificate(); } + return true; } void OutgoingShareSession::Connect( diff --git a/sharing/outgoing_share_session.h b/sharing/outgoing_share_session.h index 59d890c9..e314ff1e 100644 --- a/sharing/outgoing_share_session.h +++ b/sharing/outgoing_share_session.h @@ -24,6 +24,7 @@ #include #include +#include "absl/functional/any_invocable.h" #include "absl/strings/string_view.h" #include "absl/time/time.h" #include "internal/platform/clock.h" @@ -53,7 +54,7 @@ class OutgoingShareSession : public ShareSession { NearbyConnectionsManager* connections_manager, analytics::AnalyticsRecorder& analytics_recorder, std::string endpoint_id, const ShareTarget& share_target, - std::function + absl::AnyInvocable transfer_update_callback); OutgoingShareSession(OutgoingShareSession&&); ~OutgoingShareSession() override; @@ -137,8 +138,11 @@ class OutgoingShareSession : public ShareSession { // A 1 min timer is setup so that if we do not receive disconnect from // receiver, we assume the transfer has failed. void DelayComplete(const TransferMetadata& complete_metadata); - // Used only for OutgoingShareSession De-duplication. - void UpdateSessionForDedup( + // Updates the share target in the session to `share_target`. + // If the session is not connected, also updates the `certificate` and + // `endpoint_id`. + // Returns true if the session is not connected and updated successfully. + bool UpdateSessionForDedup( const ShareTarget& share_target, std::optional certificate, absl::string_view endpoint_id); @@ -177,7 +181,7 @@ class OutgoingShareSession : public ShareSession { std::vector file_payloads_; std::vector wifi_credentials_payloads_; Status connection_layer_status_ = Status::kUnknown; - std::function + absl::AnyInvocable transfer_update_callback_; bool ready_for_accept_ = false; // This alarm is used to disconnect the sharing connection if both sides do diff --git a/sharing/outgoing_share_session_test.cc b/sharing/outgoing_share_session_test.cc index 07e2df56..f1ce6c0f 100644 --- a/sharing/outgoing_share_session_test.cc +++ b/sharing/outgoing_share_session_test.cc @@ -78,6 +78,7 @@ using ::testing::IsFalse; using ::testing::IsTrue; using ::testing::Matcher; using ::testing::MockFunction; +using ::testing::Not; using ::testing::Property; using ::testing::SizeIs; using ::testing::StrictMock; @@ -876,15 +877,19 @@ TEST_F(OutgoingShareSessionTest, DelayCompleteDisconnectTimeout) { TEST_F(OutgoingShareSessionTest, UpdateSessionForDedupWithCertificate) { EXPECT_FALSE(session_.certificate().has_value()); - EXPECT_FALSE(session_.self_share()); - ShareTarget share_target2{ - "test_update_name", ::nearby::network::Url(), ShareTargetType::kPhone, - /* is_incoming */ true, "test_update_full_name", - /* is_known */ true, "test_update_device_id", true}; + ShareTarget share_target2{"test_update_name", ::nearby::network::Url(), + ShareTargetType::kPhone, + /* is_incoming */ true, "test_update_full_name", + /* is_known */ true, "test_update_device_id", + /*for_self_share=*/true}; + share_target2.id = session_.share_target().id; + EXPECT_THAT(session_.share_target(), Not(Eq(share_target2))); + session_.UpdateSessionForDedup(share_target2, GetNearbyShareTestDecryptedPublicCertificate(), "test_update_endpoint_id"); - EXPECT_THAT(session_.share_target().ToString(), Eq(share_target2.ToString())); + + EXPECT_THAT(session_.share_target(), Eq(share_target2)); EXPECT_TRUE(session_.certificate().has_value()); EXPECT_THAT(session_.endpoint_id(), Eq("test_update_endpoint_id")); EXPECT_TRUE(session_.self_share()); @@ -892,29 +897,47 @@ TEST_F(OutgoingShareSessionTest, UpdateSessionForDedupWithCertificate) { TEST_F(OutgoingShareSessionTest, UpdateSessionForDedupWithoutCertificate) { session_.set_certificate(GetNearbyShareTestDecryptedPublicCertificate()); - ShareTarget share_target2{ - "test_update_name", ::nearby::network::Url(), ShareTargetType::kPhone, - /* is_incoming */ true, "test_update_full_name", - /* is_known */ true, "test_update_device_id", true}; + EXPECT_TRUE(session_.certificate().has_value()); + ShareTarget share_target2{"test_update_name", ::nearby::network::Url(), + ShareTargetType::kPhone, + /* is_incoming */ true, "test_update_full_name", + /* is_known */ true, "test_update_device_id", + /*for_self_share=*/true}; + share_target2.id = session_.share_target().id; + EXPECT_THAT(session_.share_target(), Not(Eq(share_target2))); + session_.UpdateSessionForDedup(share_target2, std::nullopt, "test_update_endpoint_id"); + + EXPECT_THAT(session_.share_target(), Eq(share_target2)); // Certificate is cleared. EXPECT_FALSE(session_.certificate().has_value()); + EXPECT_THAT(session_.endpoint_id(), Eq("test_update_endpoint_id")); + EXPECT_TRUE(session_.self_share()); } -TEST_F(OutgoingShareSessionTest, UpdateSessionForDedupConnectedIsNoOp) { - auto share_target_org = session_.share_target(); +TEST_F(OutgoingShareSessionTest, + UpdateSessionForDedupConnectedDoesNotUpdateCertAndEndpointId) { + auto endpoint_id_org = session_.endpoint_id(); NearbyConnectionImpl connection(device_info_); session_.set_session_id(1234); ConnectionSuccess(&connection); - ShareTarget share_target2{ - "test_update_name", ::nearby::network::Url(), ShareTargetType::kPhone, - /* is_incoming */ true, "test_update_full_name", - /* is_known */ true, "test_update_device_id", true}; + session_.set_certificate(GetNearbyShareTestDecryptedPublicCertificate()); + EXPECT_TRUE(session_.certificate().has_value()); + ShareTarget share_target2{"test_update_name", ::nearby::network::Url(), + ShareTargetType::kPhone, + /* is_incoming */ true, "test_update_full_name", + /* is_known */ true, "test_update_device_id", + /*for_self_share=*/true}; + share_target2.id = session_.share_target().id; + EXPECT_THAT(session_.share_target(), Not(Eq(share_target2))); + session_.UpdateSessionForDedup(share_target2, std::nullopt, "test_update_endpoint_id"); - EXPECT_THAT(session_.share_target().ToString(), - Eq(share_target_org.ToString())); + + EXPECT_THAT(session_.share_target(), Eq(share_target2)); + EXPECT_TRUE(session_.certificate().has_value()); + EXPECT_THAT(session_.endpoint_id(), Eq(endpoint_id_org)); } } // namespace } // namespace nearby::sharing diff --git a/sharing/share_target.cc b/sharing/share_target.cc index 4eaca7a6..66e5dfb1 100644 --- a/sharing/share_target.cc +++ b/sharing/share_target.cc @@ -87,5 +87,15 @@ std::string ShareTarget::ToString() const { return absl::StrCat("ShareTarget<", absl::StrJoin(fmt, ", "), ">"); } +bool ShareTarget::operator==(const ShareTarget& other) const { + return id == other.id && device_name == other.device_name && + image_url == other.image_url && type == other.type && + is_incoming == other.is_incoming && full_name == other.full_name && + is_known == other.is_known && device_id == other.device_id && + for_self_share == other.for_self_share && + vendor_id == other.vendor_id && + receive_disabled == other.receive_disabled; +} + } // namespace sharing } // namespace nearby diff --git a/sharing/share_target.h b/sharing/share_target.h index 71d7363e..33a17043 100644 --- a/sharing/share_target.h +++ b/sharing/share_target.h @@ -42,6 +42,8 @@ struct ShareTarget { std::string ToString() const; + bool operator==(const ShareTarget& other) const; + int64_t id; std::string device_name; // Uri that points to an image of the ShareTarget, if one exists. diff --git a/sharing/share_target_test.cc b/sharing/share_target_test.cc index 1b5fd243..bb78514c 100644 --- a/sharing/share_target_test.cc +++ b/sharing/share_target_test.cc @@ -64,10 +64,21 @@ using ShareTargetToStringTest = testing::TestWithParam; TEST_P(ShareTargetToStringTest, ToStringResultMatches) { - ShareTarget test_share_target = GetParam().share_target; + const ShareTarget& test_share_target = GetParam().share_target; EXPECT_EQ(GetParam().expected_string_result, test_share_target.ToString()); } +TEST_P(ShareTargetToStringTest, EqualsResultMatches) { + const ShareTarget& test_share_target = GetParam().share_target; + EXPECT_EQ(test_share_target, test_share_target); +} + +TEST_P(ShareTargetToStringTest, EqualsCopyConstructionResultMatches) { + const ShareTarget& test_share_target = GetParam().share_target; + ShareTarget copy_share_target = test_share_target; + EXPECT_EQ(test_share_target, copy_share_target); +} + INSTANTIATE_TEST_SUITE_P(ShareTargetToStringTest, ShareTargetToStringTest, testing::ValuesIn(GetTestData()));