From 9d324f79d8bfd4cee8605632d667aa39fa9f9aef Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Tue, 6 Aug 2024 17:25:08 -0700 Subject: [PATCH] Move payload update processing into IncomingShareSession. PiperOrigin-RevId: 660159962 --- sharing/incoming_share_session.cc | 35 ++ sharing/incoming_share_session.h | 26 +- sharing/incoming_share_session_test.cc | 534 +++++++++++++++++-------- sharing/nearby_sharing_service_impl.cc | 47 +-- 4 files changed, 435 insertions(+), 207 deletions(-) diff --git a/sharing/incoming_share_session.cc b/sharing/incoming_share_session.cc index 63adf9ca..6176f539 100644 --- a/sharing/incoming_share_session.cc +++ b/sharing/incoming_share_session.cc @@ -478,4 +478,39 @@ void IncomingShareSession::SendFailureResponse( TransferMetadataBuilder().set_status(status).build()); } +std::pair IncomingShareSession::PayloadTransferUpdate( + bool update_file_paths_in_progress, TransferMetadata metadata) { + if (metadata.status() == TransferMetadata::Status::kComplete) { + bool success = FinalizePayloads(); + return std::make_pair(/*completed=*/true, success); + } + + // Update file paths during progress. It may impact transfer speed. + // TODO: b/289290115 - Revisit UpdateFilePath to enhance transfer speed for + // MacOS. + if (update_file_paths_in_progress) { + UpdateFilePayloadPaths(); + } else { + if (metadata.status() == TransferMetadata::Status::kCancelled) { + NL_VLOG(1) << __func__ << ": Update file paths for cancelled transfer"; + UpdateFilePayloadPaths(); + } + } + + // Make sure to call this before calling Disconnect, or we risk losing some + // transfer updates in the receive case due to the Disconnect call cleaning up + // share targets. + UpdateTransferMetadata(metadata); + + if (TransferMetadata::IsFinalStatus(metadata.status())) { + // Cancellation has its own disconnection strategy, possibly adding a + // delay before disconnection to provide the other party time to process + // the cancellation. + if (metadata.status() != TransferMetadata::Status::kCancelled) { + Disconnect(); + } + } + return std::make_pair(/*completed=*/false, /*success=*/false); +} + } // namespace nearby::sharing diff --git a/sharing/incoming_share_session.h b/sharing/incoming_share_session.h index e55153a6..b38123d1 100644 --- a/sharing/incoming_share_session.h +++ b/sharing/incoming_share_session.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include "internal/platform/clock.h" @@ -69,9 +70,6 @@ class IncomingShareSession : public ShareSession { std::optional)> introduction_callback); - // Update file attachment paths with payload paths. - bool UpdateFilePayloadPaths(); - // Returns true if the transfer can begin and AcceptTransfer should be called // immediately. // Returns false if user needs to accept the transfer. @@ -91,11 +89,6 @@ class IncomingShareSession : public ShareSession { const nearby::sharing::service::proto::ProgressUpdateFrame& progress_update); - // Once transfer has completed, make payload content available in the - // corresponding Attachment. - // Returns true if all payloads were successfully finalized. - bool FinalizePayloads(); - // Returns the file paths of all file payloads. std::vector GetPayloadFilePaths() const; @@ -108,14 +101,31 @@ class IncomingShareSession : public ShareSession { // response to remote device. void SendFailureResponse(TransferMetadata::Status status); + // Process payload transfer updates. + // The `update_file_paths_in_progress` flag determines if the payload paths + // should be updated in the attachments on each update notification. + // Returns a pair where the first param indicates whether the transfer has + // completed. And if it has completed, the second param indicates whether + // the transfer was successful. + std::pair PayloadTransferUpdate( + bool update_file_paths_in_progress, TransferMetadata metadata); + protected: void InvokeTransferUpdateCallback(const TransferMetadata& metadata) override; bool OnNewConnection(NearbyConnection* connection) override; private: + // Update file attachment paths with payload paths. + bool UpdateFilePayloadPaths(); + // Copy payload contents from the NearbyConnection to the Attachment. bool UpdatePayloadContents(); + // Once transfer has completed, make payload content available in the + // corresponding Attachment. + // Returns true if all payloads were successfully finalized. + bool FinalizePayloads(); + std::function transfer_update_callback_; diff --git a/sharing/incoming_share_session_test.cc b/sharing/incoming_share_session_test.cc index 25420004..e07b06dd 100644 --- a/sharing/incoming_share_session_test.cc +++ b/sharing/incoming_share_session_test.cc @@ -48,6 +48,7 @@ #include "sharing/share_target.h" #include "sharing/text_attachment.h" #include "sharing/transfer_metadata.h" +#include "sharing/transfer_metadata_builder.h" #include "sharing/transfer_metadata_matchers.h" #include "sharing/wifi_credentials_attachment.h" #include "google/protobuf/text_format.h" @@ -63,6 +64,7 @@ using ::nearby::analytics::HasAction; using ::nearby::analytics::HasCategory; using ::nearby::analytics::HasEventType; using ::nearby::analytics::HasSessionId; +using ::nearby::sharing::TransferMetadataBuilder; using ::nearby::sharing::analytics::proto::SharingLog; using ::nearby::sharing::service::proto::ConnectionResponseFrame; using ::nearby::sharing::service::proto::FileMetadata; @@ -280,31 +282,8 @@ TEST_F(IncomingShareSessionTest, ProcessIntroductionSuccess) { Eq(wifimeta2.payload_id())); } -TEST_F(IncomingShareSessionTest, UpdateFilePayloadPathsSuccess) { - EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), - &connections_manager_, &connection_)); - EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), - Eq(std::nullopt)); - std::filesystem::path file1_path = "/usr/tmp/file1"; - int64_t payload_id1 = introduction_frame_.file_metadata(0).payload_id(); - connections_manager_.SetIncomingPayload( - payload_id1, CreateFilePayload(payload_id1, file1_path)); - - std::filesystem::path file2_path = "/usr/tmp/file2"; - int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); - connections_manager_.SetIncomingPayload( - payload_id2, CreateFilePayload(payload_id2, file2_path)); - - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsTrue()); - EXPECT_THAT( - session_.attachment_container().GetFileAttachments()[0].file_path(), - Eq(file1_path)); - EXPECT_THAT( - session_.attachment_container().GetFileAttachments()[1].file_path(), - Eq(file2_path)); -} - -TEST_F(IncomingShareSessionTest, UpdateFilePayloadPathsWrongType) { +TEST_F(IncomingShareSessionTest, + PayloadTransferUpdateCompleteWithWrongPayloadType) { EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), &connections_manager_, &connection_)); EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), @@ -317,11 +296,51 @@ TEST_F(IncomingShareSessionTest, UpdateFilePayloadPathsWrongType) { int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); connections_manager_.SetIncomingPayload( payload_id2, CreateFilePayload(payload_id2, file2_path)); + TransferMetadata metadata = + TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kComplete) + .build(); + EXPECT_CALL(transfer_metadata_callback_, Call(_, _)).Times(0); - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsFalse()); + std::pair result = + session_.PayloadTransferUpdate(false, metadata); + + EXPECT_THAT(result.first, IsTrue()); + EXPECT_THAT(result.second, IsFalse()); + // Verify that attachments are cleared out + EXPECT_THAT( + session_.attachment_container().GetFileAttachments()[0].file_path(), + Eq(std::nullopt)); + EXPECT_THAT( + session_.attachment_container().GetFileAttachments()[1].file_path(), + Eq(std::nullopt)); + EXPECT_THAT( + session_.attachment_container().GetTextAttachments()[0].text_body(), + IsEmpty()); + EXPECT_THAT( + session_.attachment_container().GetTextAttachments()[1].text_body(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[0] + .password(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[0] + .is_hidden(), + IsFalse()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[1] + .password(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[1] + .is_hidden(), + IsFalse()); + EXPECT_THAT(connection_.IsClosed(), IsFalse()); } -TEST_F(IncomingShareSessionTest, GetPayloadFilePaths) { +TEST_F(IncomingShareSessionTest, + PayloadTransferUpdateCompleteWithMissingFilePayloads) { EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), &connections_manager_, &connection_)); EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), @@ -330,12 +349,244 @@ TEST_F(IncomingShareSessionTest, GetPayloadFilePaths) { int64_t payload_id1 = introduction_frame_.file_metadata(0).payload_id(); connections_manager_.SetIncomingPayload( payload_id1, CreateFilePayload(payload_id1, file1_path)); + std::string text_content1 = "text1"; + int64_t text_payload_id1 = introduction_frame_.text_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + text_payload_id1, CreateTextPayload(text_payload_id1, text_content1)); + std::string text_content2 = "text2"; + int64_t text_payload_id2 = introduction_frame_.text_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + text_payload_id2, CreateTextPayload(text_payload_id2, text_content2)); + + int64_t wifi_payload_id1 = + introduction_frame_.wifi_credentials_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + wifi_payload_id1, + CreateWifiCredentialsPayload(wifi_payload_id1, "password1", false)); + + int64_t wifi_payload_id2 = + introduction_frame_.wifi_credentials_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + wifi_payload_id2, + CreateWifiCredentialsPayload(wifi_payload_id2, "password2", true)); + TransferMetadata metadata = + TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kComplete) + .build(); + EXPECT_CALL(transfer_metadata_callback_, Call(_, _)).Times(0); + + std::pair result = + session_.PayloadTransferUpdate(false, metadata); + + EXPECT_THAT(result.first, IsTrue()); + EXPECT_THAT(result.second, IsFalse()); + // Verify that attachments are cleared out + EXPECT_THAT( + session_.attachment_container().GetFileAttachments()[0].file_path(), + Eq(std::nullopt)); + EXPECT_THAT( + session_.attachment_container().GetFileAttachments()[1].file_path(), + Eq(std::nullopt)); + EXPECT_THAT( + session_.attachment_container().GetTextAttachments()[0].text_body(), + IsEmpty()); + EXPECT_THAT( + session_.attachment_container().GetTextAttachments()[1].text_body(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[0] + .password(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[0] + .is_hidden(), + IsFalse()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[1] + .password(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[1] + .is_hidden(), + IsFalse()); + EXPECT_THAT(connection_.IsClosed(), IsFalse()); +} + +TEST_F(IncomingShareSessionTest, + PayloadTransferUpdateCompleteWithMissingTextPayloads) { + EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), + &connections_manager_, &connection_)); + EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), + Eq(std::nullopt)); + std::filesystem::path file1_path = "/usr/tmp/file1"; + int64_t payload_id1 = introduction_frame_.file_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + payload_id1, CreateFilePayload(payload_id1, file1_path)); std::filesystem::path file2_path = "/usr/tmp/file2"; int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); connections_manager_.SetIncomingPayload( payload_id2, CreateFilePayload(payload_id2, file2_path)); - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsTrue()); + std::string text_content1 = "text1"; + int64_t text_payload_id1 = introduction_frame_.text_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + text_payload_id1, CreateTextPayload(text_payload_id1, text_content1)); + int64_t wifi_payload_id1 = + introduction_frame_.wifi_credentials_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + wifi_payload_id1, + CreateWifiCredentialsPayload(wifi_payload_id1, "password1", false)); + int64_t wifi_payload_id2 = + introduction_frame_.wifi_credentials_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + wifi_payload_id2, + CreateWifiCredentialsPayload(wifi_payload_id2, "password2", true)); + TransferMetadata metadata = + TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kComplete) + .build(); + EXPECT_CALL(transfer_metadata_callback_, Call(_, _)).Times(0); + + std::pair result = + session_.PayloadTransferUpdate(false, metadata); + + EXPECT_THAT(result.first, IsTrue()); + EXPECT_THAT(result.second, IsFalse()); + // Verify that attachments are cleared out + EXPECT_THAT( + session_.attachment_container().GetFileAttachments()[0].file_path(), + Eq(std::nullopt)); + EXPECT_THAT( + session_.attachment_container().GetFileAttachments()[1].file_path(), + Eq(std::nullopt)); + EXPECT_THAT( + session_.attachment_container().GetTextAttachments()[0].text_body(), + IsEmpty()); + EXPECT_THAT( + session_.attachment_container().GetTextAttachments()[1].text_body(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[0] + .password(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[0] + .is_hidden(), + IsFalse()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[1] + .password(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[1] + .is_hidden(), + IsFalse()); + EXPECT_THAT(connection_.IsClosed(), IsFalse()); +} + +TEST_F(IncomingShareSessionTest, + PayloadTransferUpdateCompleteWithMissingWifiPayloads) { + EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), + &connections_manager_, &connection_)); + EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), + Eq(std::nullopt)); + std::filesystem::path file1_path = "/usr/tmp/file1"; + int64_t payload_id1 = introduction_frame_.file_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + payload_id1, CreateFilePayload(payload_id1, file1_path)); + std::filesystem::path file2_path = "/usr/tmp/file2"; + int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + payload_id2, CreateFilePayload(payload_id2, file2_path)); + std::string text_content1 = "text1"; + int64_t text_payload_id1 = introduction_frame_.text_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + text_payload_id1, CreateTextPayload(text_payload_id1, text_content1)); + std::string text_content2 = "text2"; + int64_t text_payload_id2 = introduction_frame_.text_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + text_payload_id2, CreateTextPayload(text_payload_id2, text_content2)); + int64_t wifi_payload_id1 = + introduction_frame_.wifi_credentials_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + wifi_payload_id1, + CreateWifiCredentialsPayload(wifi_payload_id1, "password1", false)); + TransferMetadata metadata = + TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kComplete) + .build(); + EXPECT_CALL(transfer_metadata_callback_, Call(_, _)).Times(0); + + std::pair result = + session_.PayloadTransferUpdate(false, metadata); + + EXPECT_THAT(result.first, IsTrue()); + EXPECT_THAT(result.second, IsFalse()); + // Verify that attachments are cleared out + EXPECT_THAT( + session_.attachment_container().GetFileAttachments()[0].file_path(), + Eq(std::nullopt)); + EXPECT_THAT( + session_.attachment_container().GetFileAttachments()[1].file_path(), + Eq(std::nullopt)); + EXPECT_THAT( + session_.attachment_container().GetTextAttachments()[0].text_body(), + IsEmpty()); + EXPECT_THAT( + session_.attachment_container().GetTextAttachments()[1].text_body(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[0] + .password(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[0] + .is_hidden(), + IsFalse()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[1] + .password(), + IsEmpty()); + EXPECT_THAT(session_.attachment_container() + .GetWifiCredentialsAttachments()[1] + .is_hidden(), + IsFalse()); + EXPECT_THAT(connection_.IsClosed(), IsFalse()); +} + +TEST_F(IncomingShareSessionTest, GetPayloadFilePaths) { + EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), + &connections_manager_, &connection_)); + IntroductionFrame introduction_frame; + FileMetadata file1; + FileMetadata file2; + file1.set_id(23432); + file1.set_payload_id(123); + file1.set_size(1); + file2.set_id(42377); + file2.set_payload_id(456); + file2.set_size(1); + introduction_frame.mutable_file_metadata()->Add(std::move(file1)); + introduction_frame.mutable_file_metadata()->Add(std::move(file2)); + EXPECT_THAT(session_.ProcessIntroduction(introduction_frame), + Eq(std::nullopt)); + std::filesystem::path file1_path = "/usr/tmp/file1"; + int64_t payload_id1 = introduction_frame.file_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + payload_id1, CreateFilePayload(payload_id1, file1_path)); + + std::filesystem::path file2_path = "/usr/tmp/file2"; + int64_t payload_id2 = introduction_frame.file_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + payload_id2, CreateFilePayload(payload_id2, file2_path)); + TransferMetadata metadata = + TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kComplete) + .build(); + std::pair result = + session_.PayloadTransferUpdate(false, metadata); + EXPECT_THAT(result.first, IsTrue()); + EXPECT_THAT(result.second, IsTrue()); std::vector file_paths = session_.GetPayloadFilePaths(); @@ -343,7 +594,7 @@ TEST_F(IncomingShareSessionTest, GetPayloadFilePaths) { EXPECT_THAT(file_paths, UnorderedElementsAre(file1_path, file2_path)); } -TEST_F(IncomingShareSessionTest, FinalizePayloadsSuccess) { +TEST_F(IncomingShareSessionTest, PayloadTransferUpdateCompleteWithSuccess) { EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), &connections_manager_, &connection_)); EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), @@ -357,7 +608,6 @@ TEST_F(IncomingShareSessionTest, FinalizePayloadsSuccess) { int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); connections_manager_.SetIncomingPayload( payload_id2, CreateFilePayload(payload_id2, file2_path)); - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsTrue()); std::string text_content1 = "text1"; int64_t text_payload_id1 = introduction_frame_.text_metadata(0).payload_id(); @@ -380,8 +630,17 @@ TEST_F(IncomingShareSessionTest, FinalizePayloadsSuccess) { connections_manager_.SetIncomingPayload( wifi_payload_id2, CreateWifiCredentialsPayload(wifi_payload_id2, "password2", true)); + TransferMetadata metadata = + TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kComplete) + .build(); + EXPECT_CALL(transfer_metadata_callback_, Call(_, _)).Times(0); - EXPECT_THAT(session_.FinalizePayloads(), IsTrue()); + std::pair result = + session_.PayloadTransferUpdate(false, metadata); + + EXPECT_THAT(result.first, IsTrue()); + EXPECT_THAT(result.second, IsTrue()); EXPECT_THAT( session_.attachment_container().GetFileAttachments()[0].file_path(), Eq(file1_path)); @@ -410,9 +669,10 @@ TEST_F(IncomingShareSessionTest, FinalizePayloadsSuccess) { .GetWifiCredentialsAttachments()[1] .is_hidden(), IsTrue()); + EXPECT_THAT(connection_.IsClosed(), IsFalse()); } -TEST_F(IncomingShareSessionTest, FinalizePayloadsMissingFilePayloads) { +TEST_F(IncomingShareSessionTest, PayloadTransferUpdateCancelled) { EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), &connections_manager_, &connection_)); EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), @@ -421,7 +681,11 @@ TEST_F(IncomingShareSessionTest, FinalizePayloadsMissingFilePayloads) { int64_t payload_id1 = introduction_frame_.file_metadata(0).payload_id(); connections_manager_.SetIncomingPayload( payload_id1, CreateFilePayload(payload_id1, file1_path)); - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsFalse()); + + std::filesystem::path file2_path = "/usr/tmp/file2"; + int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + payload_id2, CreateFilePayload(payload_id2, file2_path)); std::string text_content1 = "text1"; int64_t text_payload_id1 = introduction_frame_.text_metadata(0).payload_id(); @@ -444,41 +708,27 @@ TEST_F(IncomingShareSessionTest, FinalizePayloadsMissingFilePayloads) { connections_manager_.SetIncomingPayload( wifi_payload_id2, CreateWifiCredentialsPayload(wifi_payload_id2, "password2", true)); + TransferMetadata metadata = + TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kCancelled) + .build(); + EXPECT_CALL(transfer_metadata_callback_, + Call(_, HasStatus(TransferMetadata::Status::kCancelled))); - EXPECT_THAT(session_.FinalizePayloads(), IsFalse()); + std::pair result = + session_.PayloadTransferUpdate(false, metadata); - // Verify that attachments are cleared out + EXPECT_THAT(result.first, IsFalse()); EXPECT_THAT( session_.attachment_container().GetFileAttachments()[0].file_path(), - Eq(std::nullopt)); + Eq(file1_path)); EXPECT_THAT( session_.attachment_container().GetFileAttachments()[1].file_path(), - Eq(std::nullopt)); - EXPECT_THAT( - session_.attachment_container().GetTextAttachments()[0].text_body(), - IsEmpty()); - EXPECT_THAT( - session_.attachment_container().GetTextAttachments()[1].text_body(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[0] - .password(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[0] - .is_hidden(), - IsFalse()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[1] - .password(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[1] - .is_hidden(), - IsFalse()); + Eq(file2_path)); + EXPECT_THAT(connection_.IsClosed(), IsFalse()); } -TEST_F(IncomingShareSessionTest, FinalizePayloadsMissingTextPayloads) { +TEST_F(IncomingShareSessionTest, PayloadTransferUpdateFailed) { EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), &connections_manager_, &connection_)); EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), @@ -487,80 +737,11 @@ TEST_F(IncomingShareSessionTest, FinalizePayloadsMissingTextPayloads) { int64_t payload_id1 = introduction_frame_.file_metadata(0).payload_id(); connections_manager_.SetIncomingPayload( payload_id1, CreateFilePayload(payload_id1, file1_path)); - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsFalse()); std::filesystem::path file2_path = "/usr/tmp/file2"; int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); connections_manager_.SetIncomingPayload( payload_id2, CreateFilePayload(payload_id2, file2_path)); - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsTrue()); - - std::string text_content1 = "text1"; - int64_t text_payload_id1 = introduction_frame_.text_metadata(0).payload_id(); - connections_manager_.SetIncomingPayload( - text_payload_id1, CreateTextPayload(text_payload_id1, text_content1)); - - int64_t wifi_payload_id1 = - introduction_frame_.wifi_credentials_metadata(0).payload_id(); - connections_manager_.SetIncomingPayload( - wifi_payload_id1, - CreateWifiCredentialsPayload(wifi_payload_id1, "password1", false)); - - int64_t wifi_payload_id2 = - introduction_frame_.wifi_credentials_metadata(1).payload_id(); - connections_manager_.SetIncomingPayload( - wifi_payload_id2, - CreateWifiCredentialsPayload(wifi_payload_id2, "password2", true)); - - EXPECT_THAT(session_.FinalizePayloads(), IsFalse()); - - // Verify that attachments are cleared out - EXPECT_THAT( - session_.attachment_container().GetFileAttachments()[0].file_path(), - Eq(std::nullopt)); - EXPECT_THAT( - session_.attachment_container().GetFileAttachments()[1].file_path(), - Eq(std::nullopt)); - EXPECT_THAT( - session_.attachment_container().GetTextAttachments()[0].text_body(), - IsEmpty()); - EXPECT_THAT( - session_.attachment_container().GetTextAttachments()[1].text_body(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[0] - .password(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[0] - .is_hidden(), - IsFalse()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[1] - .password(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[1] - .is_hidden(), - IsFalse()); -} - -TEST_F(IncomingShareSessionTest, FinalizePayloadsMissingWifiPayloads) { - EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), - &connections_manager_, &connection_)); - EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), - Eq(std::nullopt)); - std::filesystem::path file1_path = "/usr/tmp/file1"; - int64_t payload_id1 = introduction_frame_.file_metadata(0).payload_id(); - connections_manager_.SetIncomingPayload( - payload_id1, CreateFilePayload(payload_id1, file1_path)); - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsFalse()); - - std::filesystem::path file2_path = "/usr/tmp/file2"; - int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); - connections_manager_.SetIncomingPayload( - payload_id2, CreateFilePayload(payload_id2, file2_path)); - EXPECT_THAT(session_.UpdateFilePayloadPaths(), IsTrue()); std::string text_content1 = "text1"; int64_t text_payload_id1 = introduction_frame_.text_metadata(0).payload_id(); @@ -578,37 +759,72 @@ TEST_F(IncomingShareSessionTest, FinalizePayloadsMissingWifiPayloads) { wifi_payload_id1, CreateWifiCredentialsPayload(wifi_payload_id1, "password1", false)); - EXPECT_THAT(session_.FinalizePayloads(), IsFalse()); + int64_t wifi_payload_id2 = + introduction_frame_.wifi_credentials_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + wifi_payload_id2, + CreateWifiCredentialsPayload(wifi_payload_id2, "password2", true)); + TransferMetadata metadata = TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kFailed) + .build(); + EXPECT_CALL(transfer_metadata_callback_, + Call(_, HasStatus(TransferMetadata::Status::kFailed))); - // Verify that attachments are cleared out - EXPECT_THAT( - session_.attachment_container().GetFileAttachments()[0].file_path(), - Eq(std::nullopt)); - EXPECT_THAT( - session_.attachment_container().GetFileAttachments()[1].file_path(), - Eq(std::nullopt)); - EXPECT_THAT( - session_.attachment_container().GetTextAttachments()[0].text_body(), - IsEmpty()); - EXPECT_THAT( - session_.attachment_container().GetTextAttachments()[1].text_body(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[0] - .password(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[0] - .is_hidden(), - IsFalse()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[1] - .password(), - IsEmpty()); - EXPECT_THAT(session_.attachment_container() - .GetWifiCredentialsAttachments()[1] - .is_hidden(), - IsFalse()); + std::pair result = + session_.PayloadTransferUpdate(false, metadata); + + EXPECT_THAT(result.first, IsFalse()); + EXPECT_THAT(connection_.IsClosed(), IsTrue()); +} + +TEST_F(IncomingShareSessionTest, PayloadTransferUpdateInProgress) { + EXPECT_TRUE(session_.OnConnected(nearby_sharing_decoder_, absl::Now(), + &connections_manager_, &connection_)); + EXPECT_THAT(session_.ProcessIntroduction(introduction_frame_), + Eq(std::nullopt)); + std::filesystem::path file1_path = "/usr/tmp/file1"; + int64_t payload_id1 = introduction_frame_.file_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + payload_id1, CreateFilePayload(payload_id1, file1_path)); + + std::filesystem::path file2_path = "/usr/tmp/file2"; + int64_t payload_id2 = introduction_frame_.file_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + payload_id2, CreateFilePayload(payload_id2, file2_path)); + + std::string text_content1 = "text1"; + int64_t text_payload_id1 = introduction_frame_.text_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + text_payload_id1, CreateTextPayload(text_payload_id1, text_content1)); + + std::string text_content2 = "text2"; + int64_t text_payload_id2 = introduction_frame_.text_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + text_payload_id2, CreateTextPayload(text_payload_id2, text_content2)); + + int64_t wifi_payload_id1 = + introduction_frame_.wifi_credentials_metadata(0).payload_id(); + connections_manager_.SetIncomingPayload( + wifi_payload_id1, + CreateWifiCredentialsPayload(wifi_payload_id1, "password1", false)); + + int64_t wifi_payload_id2 = + introduction_frame_.wifi_credentials_metadata(1).payload_id(); + connections_manager_.SetIncomingPayload( + wifi_payload_id2, + CreateWifiCredentialsPayload(wifi_payload_id2, "password2", true)); + TransferMetadata metadata = + TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kInProgress) + .build(); + EXPECT_CALL(transfer_metadata_callback_, + Call(_, HasStatus(TransferMetadata::Status::kInProgress))); + + std::pair result = + session_.PayloadTransferUpdate(false, metadata); + + EXPECT_THAT(result.first, IsFalse()); + EXPECT_THAT(connection_.IsClosed(), IsFalse()); } TEST_F(IncomingShareSessionTest, ReadyForTransferNotConnected) { diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index 63a96d24..06235882 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -3124,52 +3124,19 @@ void NearbySharingServiceImpl::IncomingPayloadTransferUpdate( << share_target_id; return; } - // kInProgress status is logged extensively elsewhere so avoid the spam. - if (metadata.status() != TransferMetadata::Status::kInProgress) { - NL_VLOG(1) << __func__ << ": Nearby Share service: " - << "Payload transfer update for share target with ID " - << share_target_id << ": " - << TransferMetadata::StatusToString(metadata.status()); - } - - // Update file paths during progress. It may impact transfer speed. - // TODO: b/289290115 - Revisit UpdateFilePath to enhance transfer speed for - // MacOS. - if (update_file_paths_in_progress_) { - session->UpdateFilePayloadPaths(); - } - - if (metadata.status() == TransferMetadata::Status::kComplete) { - if (!session->FinalizePayloads()) { - metadata = TransferMetadataBuilder() - .set_status(TransferMetadata::Status::kIncompletePayloads) - .build(); + std::pair result = + session->PayloadTransferUpdate(update_file_paths_in_progress_, metadata); + if (result.first) { + if (!result.second) { + OnIncomingFilesMetadataUpdated(share_target_id, std::move(metadata), + /*success=*/false); + return; } file_handler_.UpdateFilesOriginMetadata( session->GetPayloadFilePaths(), absl::bind_front( &NearbySharingServiceImpl::OnIncomingFilesMetadataUpdated, this, share_target_id, std::move(metadata))); - return; - } else if (metadata.status() == TransferMetadata::Status::kCancelled) { - NL_VLOG(1) << __func__ << ": Update file paths for cancelled transfer"; - if (!update_file_paths_in_progress_) { - session->UpdateFilePayloadPaths(); - } - } - - // Make sure to call this before calling Disconnect, or we risk losing some - // transfer updates in the receive case due to the Disconnect call cleaning up - // share targets. - session->UpdateTransferMetadata(metadata); - - if (TransferMetadata::IsFinalStatus(metadata.status())) { - // Cancellation has its own disconnection strategy, possibly adding a - // delay before disconnection to provide the other party time to process - // the cancellation. - if (metadata.status() != TransferMetadata::Status::kCancelled) { - session->Disconnect(); - } } }