From 3fb229e3e1f4502c60b70a77cf242b8dceca8dca Mon Sep 17 00:00:00 2001 From: Eiden Kim Date: Wed, 22 May 2024 00:14:05 -0700 Subject: [PATCH] Filter expected files out from the delete list. PiperOrigin-RevId: 636058470 --- sharing/fake_nearby_connections_manager.cc | 21 ++-- sharing/fake_nearby_connections_manager.h | 5 +- sharing/nearby_connections_manager.h | 7 -- sharing/nearby_connections_manager_impl.cc | 79 +++++++++---- sharing/nearby_connections_manager_impl.h | 15 ++- .../nearby_connections_manager_impl_test.cc | 108 ++++++++++++++++-- sharing/nearby_sharing_service_impl.cc | 5 + sharing/nearby_sharing_service_impl_test.cc | 8 +- 8 files changed, 187 insertions(+), 61 deletions(-) diff --git a/sharing/fake_nearby_connections_manager.cc b/sharing/fake_nearby_connections_manager.cc index ff5fc66a51..7c9e1ab3e9 100644 --- a/sharing/fake_nearby_connections_manager.cc +++ b/sharing/fake_nearby_connections_manager.cc @@ -306,27 +306,22 @@ void FakeNearbyConnectionsManager::SetCustomSavePath( } absl::flat_hash_set -FakeNearbyConnectionsManager::GetUnknownFilePathsToDelete() { - return file_paths_to_delete_; -} - -void FakeNearbyConnectionsManager::ClearUnknownFilePathsToDelete() { +FakeNearbyConnectionsManager::GetAndClearUnknownFilePathsToDelete() { + absl::flat_hash_set file_paths_to_delete = + file_paths_to_delete_; file_paths_to_delete_.clear(); + return file_paths_to_delete; } +absl::flat_hash_set +FakeNearbyConnectionsManager::GetUnknownFilePathsToDeleteForTesting() { + return file_paths_to_delete_; +} void FakeNearbyConnectionsManager::AddUnknownFilePathsToDeleteForTesting( std::filesystem::path file_path) { file_paths_to_delete_.insert(file_path); } -absl::flat_hash_set -FakeNearbyConnectionsManager::GetAndClearUnknownFilePathsToDelete() { - absl::flat_hash_set file_paths_to_delete = - file_paths_to_delete_; - file_paths_to_delete_.clear(); - return file_paths_to_delete; -} - std::string FakeNearbyConnectionsManager::Dump() const { return ""; } } // namespace sharing diff --git a/sharing/fake_nearby_connections_manager.h b/sharing/fake_nearby_connections_manager.h index 116d8e062c..f3bd77b996 100644 --- a/sharing/fake_nearby_connections_manager.h +++ b/sharing/fake_nearby_connections_manager.h @@ -72,11 +72,8 @@ class FakeNearbyConnectionsManager : public NearbyConnectionsManager { absl::string_view endpoint_id) override; void UpgradeBandwidth(absl::string_view endpoint_id) override; void SetCustomSavePath(absl::string_view custom_save_path) override; - absl::flat_hash_set GetUnknownFilePathsToDelete() - override; absl::flat_hash_set GetAndClearUnknownFilePathsToDelete() override; - void ClearUnknownFilePathsToDelete() override; // Testing methods void SetRawAuthenticationToken(absl::string_view endpoint_id, @@ -134,6 +131,8 @@ class FakeNearbyConnectionsManager : public NearbyConnectionsManager { return !incoming_payloads_.empty(); } + absl::flat_hash_set + GetUnknownFilePathsToDeleteForTesting(); void AddUnknownFilePathsToDeleteForTesting(std::filesystem::path file_path); private: diff --git a/sharing/nearby_connections_manager.h b/sharing/nearby_connections_manager.h index e22447f527..cac3ca94e1 100644 --- a/sharing/nearby_connections_manager.h +++ b/sharing/nearby_connections_manager.h @@ -158,13 +158,6 @@ class NearbyConnectionsManager { // Sets a custom save path. virtual void SetCustomSavePath(absl::string_view custom_save_path) = 0; - // Gets the file paths to delete. - virtual absl::flat_hash_set - GetUnknownFilePathsToDelete() = 0; - - // Deletes the file paths to delete. - virtual void ClearUnknownFilePathsToDelete() = 0; - // Gets the file paths to delete and clear the hash set. virtual absl::flat_hash_set GetAndClearUnknownFilePathsToDelete() = 0; diff --git a/sharing/nearby_connections_manager_impl.cc b/sharing/nearby_connections_manager_impl.cc index 001e6cdecf..d994da614f 100644 --- a/sharing/nearby_connections_manager_impl.cc +++ b/sharing/nearby_connections_manager_impl.cc @@ -134,12 +134,24 @@ std::string MediumSelectionToString(const MediumSelection& mediums) { return ss.str(); } +std::string PayloadStatusToString(PayloadStatus status) { + switch (status) { + case PayloadStatus::kSuccess: + return "Success"; + case PayloadStatus::kFailure: + return "Failure"; + case PayloadStatus::kInProgress: + return "In Progress"; + case PayloadStatus::kCanceled: + return "Canceled"; + } +} + } // namespace NearbyConnectionsManagerImpl::NearbyConnectionsManagerImpl( - TaskRunner* connections_callback_task_runner, - Context* context, ConnectivityManager& connectivity_manager, - nearby::DeviceInfo& device_info, + TaskRunner* connections_callback_task_runner, Context* context, + ConnectivityManager& connectivity_manager, nearby::DeviceInfo& device_info, std::unique_ptr nearby_connections_service) : connections_callback_task_runner_(connections_callback_task_runner), context_(context), @@ -797,13 +809,31 @@ void NearbyConnectionsManagerImpl::OnPayloadReceived( NL_DCHECK(result.second); } +void NearbyConnectionsManagerImpl::ProcessUnknownFilePathsToDelete( + PayloadStatus status, PayloadContent::Type type, + const std::filesystem::path& path) { + if (NearbyFlags::GetInstance().GetBoolFlag( + sharing::config_package_nearby::nearby_sharing_feature:: + kDeleteUnexpectedReceivedFile)) { + // Unknown payload comes with kCanceled and kFile type from + // NearbyConnections. Delete it. + if (status == PayloadStatus::kCanceled && + type == PayloadContent::Type::kFile) { + NL_LOG(WARNING) << __func__ + << ": Unknown payload has been canceled, removing."; + MutexLock lock(&mutex_); + file_paths_to_delete_.insert(path); + } + } +} + void NearbyConnectionsManagerImpl::OnPayloadTransferUpdate( absl::string_view endpoint_id, const PayloadTransferUpdate& update) { MutexLock lock(&mutex_); NL_LOG(INFO) << "Received payload transfer update id=" << update.payload_id - << ",status=" << update.status << ",total=" << update.total_bytes - << ",bytes_transferred=" << update.bytes_transferred - << std::endl; + << ",status=" << PayloadStatusToString(update.status) + << ",total=" << update.total_bytes + << ",bytes_transferred=" << update.bytes_transferred; // If this is a payload we've registered for, then forward its status to // the PayloadStatusListener if it still exists. We don't need to do @@ -838,19 +868,11 @@ void NearbyConnectionsManagerImpl::OnPayloadTransferUpdate( if (payload_it->second.content.type != PayloadContent::Type::kBytes) { NL_LOG(WARNING) << "Received unknown payload of file type. Cancelling."; - if (NearbyFlags::GetInstance().GetBoolFlag( - sharing::config_package_nearby::nearby_sharing_feature:: - kDeleteUnexpectedReceivedFile)) { - // if we get kFile and have file_path, delete the file path. - if (payload_it->second.content.type == PayloadContent::Type::kFile) { - auto file_path = payload_it->second.content.file_payload.file.path; - NL_LOG(WARNING) << __func__ - << ": Payload is Type::kFile type. Removing."; - file_paths_to_delete_.insert(file_path); - } - } nearby_connections_service_->CancelPayload(kServiceId, payload_it->first, [](Status status) {}); + ProcessUnknownFilePathsToDelete( + update.status, payload_it->second.content.type, + payload_it->second.content.file_payload.file.path); return; } @@ -922,25 +944,36 @@ NearbyConnectionsManagerImpl::GetUnknownFilePathsToDelete() { return file_paths_to_delete_; } -void NearbyConnectionsManagerImpl::ClearUnknownFilePathsToDelete() { - MutexLock lock(&mutex_); - file_paths_to_delete_.clear(); -} - absl::flat_hash_set NearbyConnectionsManagerImpl::GetAndClearUnknownFilePathsToDelete() { MutexLock lock(&mutex_); - auto file_paths_to_delete = file_paths_to_delete_; + auto file_paths_to_delete = std::move(file_paths_to_delete_); file_paths_to_delete_.clear(); return file_paths_to_delete; } +absl::flat_hash_set +NearbyConnectionsManagerImpl::GetUnknownFilePathsToDeleteForTesting() { + return GetUnknownFilePathsToDelete(); +} + void NearbyConnectionsManagerImpl::AddUnknownFilePathsToDeleteForTesting( std::filesystem::path file_path) { MutexLock lock(&mutex_); file_paths_to_delete_.insert(file_path); } +void NearbyConnectionsManagerImpl::ProcessUnknownFilePathsToDeleteForTesting( + PayloadStatus status, PayloadContent::Type type, + const std::filesystem::path& path) { + ProcessUnknownFilePathsToDelete(status, type, path); +} + +void NearbyConnectionsManagerImpl::OnPayloadTransferUpdateForTesting( + absl::string_view endpoint_id, const PayloadTransferUpdate& update) { + OnPayloadTransferUpdate(endpoint_id, update); +} + std::string NearbyConnectionsManagerImpl::Dump() const { return nearby_connections_service_->Dump(); } diff --git a/sharing/nearby_connections_manager_impl.h b/sharing/nearby_connections_manager_impl.h index 3944870b7a..c2d25d9cd3 100644 --- a/sharing/nearby_connections_manager_impl.h +++ b/sharing/nearby_connections_manager_impl.h @@ -84,19 +84,22 @@ class NearbyConnectionsManagerImpl : public NearbyConnectionsManager { absl::string_view endpoint_id) override; void UpgradeBandwidth(absl::string_view endpoint_id) override; void SetCustomSavePath(absl::string_view custom_save_path) override; - absl::flat_hash_set GetUnknownFilePathsToDelete() - override; absl::flat_hash_set GetAndClearUnknownFilePathsToDelete() override; - void ClearUnknownFilePathsToDelete() override; - std::string Dump() const override; NearbyConnectionsService* GetNearbyConnectionsService() const { return nearby_connections_service_.get(); } + absl::flat_hash_set + GetUnknownFilePathsToDeleteForTesting(); void AddUnknownFilePathsToDeleteForTesting(std::filesystem::path file_path); + void ProcessUnknownFilePathsToDeleteForTesting( + PayloadStatus status, PayloadContent::Type type, + const std::filesystem::path& path); + void OnPayloadTransferUpdateForTesting(absl::string_view endpoint_id, + const PayloadTransferUpdate& update); private: // EndpointDiscoveryListener: @@ -119,6 +122,10 @@ class NearbyConnectionsManagerImpl : public NearbyConnectionsManager { void OnConnectionTimedOut(absl::string_view endpoint_id); void OnConnectionRequested(absl::string_view endpoint_id, ConnectionsStatus status); + void ProcessUnknownFilePathsToDelete(PayloadStatus status, + PayloadContent::Type type, + const std::filesystem::path& path); + absl::flat_hash_set GetUnknownFilePathsToDelete(); void Reset(); diff --git a/sharing/nearby_connections_manager_impl_test.cc b/sharing/nearby_connections_manager_impl_test.cc index 75ceac5af2..257b1a56cd 100644 --- a/sharing/nearby_connections_manager_impl_test.cc +++ b/sharing/nearby_connections_manager_impl_test.cc @@ -1739,21 +1739,23 @@ TEST_F(NearbyConnectionsManagerImplTest, UnknownFilePathsToDelete) { nearby_connections_manager_->AddUnknownFilePathsToDeleteForTesting( "test2.txt"); auto unknown_file_paths = - nearby_connections_manager_->GetUnknownFilePathsToDelete(); + nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); nearby_connections_manager_->AddUnknownFilePathsToDeleteForTesting( "test3.txt"); // Test if we get copy of container. EXPECT_NE(unknown_file_paths.size(), 3); EXPECT_EQ(unknown_file_paths.size(), 2); - EXPECT_EQ(nearby_connections_manager_->GetUnknownFilePathsToDelete().size(), + EXPECT_EQ(nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting() + .size(), 3); unknown_file_paths = - nearby_connections_manager_->GetUnknownFilePathsToDelete(); + nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); EXPECT_THAT(unknown_file_paths, UnorderedElementsAre("test1.txt", "test2.txt", "test3.txt")); - nearby_connections_manager_->ClearUnknownFilePathsToDelete(); - EXPECT_EQ(nearby_connections_manager_->GetUnknownFilePathsToDelete().size(), + nearby_connections_manager_->GetAndClearUnknownFilePathsToDelete(); + EXPECT_EQ(nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting() + .size(), 0); // Test GetAndClearUnknownFilePathsToDelete @@ -1761,13 +1763,103 @@ TEST_F(NearbyConnectionsManagerImplTest, UnknownFilePathsToDelete) { "test1.txt"); nearby_connections_manager_->AddUnknownFilePathsToDeleteForTesting( "test2.txt"); - unknown_file_paths = - nearby_connections_manager_->GetAndClearUnknownFilePathsToDelete(); + unknown_file_paths = nearby_connections_manager_ + ->GetAndClearUnknownFilePathsToDelete(); EXPECT_EQ(unknown_file_paths.size(), 2); - EXPECT_EQ(nearby_connections_manager_->GetUnknownFilePathsToDelete().size(), + EXPECT_EQ(nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting() + .size(), 0); } +TEST_F(NearbyConnectionsManagerImplTest, + OnPayloadTransferUpdateForUnknownFile) { + NearbyConnectionsService::ConnectionListener connection_listener_remote; + testing::NiceMock + incoming_connection_listener; + StartAdvertising(connection_listener_remote, incoming_connection_listener); + + NearbyConnectionsService::PayloadListener payload_listener_remote; + NearbyConnection* connection = OnIncomingConnection( + connection_listener_remote, incoming_connection_listener, + payload_listener_remote); + EXPECT_TRUE(connection); + std::filesystem::path file(std::filesystem::temp_directory_path() / + "file.jpg"); + payload_listener_remote.payload_cb(kRemoteEndpointId, + Payload(kPayloadId, InputFile(file))); + + // Flag is off. Don't add unknown file paths to the list. + NearbyFlags::GetInstance().OverrideBoolFlagValue( + config_package_nearby::nearby_sharing_feature:: + kDeleteUnexpectedReceivedFile, + false); + auto unknown_file_paths = + nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); + EXPECT_EQ(unknown_file_paths.size(), 0); + nearby_connections_manager_->OnPayloadTransferUpdateForTesting( + kRemoteEndpointId, + PayloadTransferUpdate(kPayloadId, PayloadStatus::kCanceled, kTotalSize, + /*bytes_transferred=*/kTotalSize)); + + // Flag is on. Add unknown file paths with kCanceled to the list. + NearbyFlags::GetInstance().OverrideBoolFlagValue( + config_package_nearby::nearby_sharing_feature:: + kDeleteUnexpectedReceivedFile, + true); + nearby_connections_manager_->OnPayloadTransferUpdateForTesting( + kRemoteEndpointId, + PayloadTransferUpdate(kPayloadId, PayloadStatus::kCanceled, kTotalSize, + /*bytes_transferred=*/kTotalSize)); + unknown_file_paths = + nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); + EXPECT_EQ(unknown_file_paths.size(), 1); + nearby_connections_manager_->GetAndClearUnknownFilePathsToDelete(); + + // Flag is on. Don't add unknown file paths w/o kCanceled to the list. + nearby_connections_manager_->OnPayloadTransferUpdateForTesting( + kRemoteEndpointId, + PayloadTransferUpdate(kPayloadId, PayloadStatus::kFailure, kTotalSize, + /*bytes_transferred=*/kTotalSize)); + unknown_file_paths = + nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); + EXPECT_EQ(unknown_file_paths.size(), 0); +} + +TEST_F(NearbyConnectionsManagerImplTest, ProcessUnknownFilePathsToDelete) { + std::filesystem::path file(std::filesystem::temp_directory_path() / + "file.jpg"); + // Flag is off. Don't add unknown file paths to the list. + NearbyFlags::GetInstance().OverrideBoolFlagValue( + config_package_nearby::nearby_sharing_feature:: + kDeleteUnexpectedReceivedFile, + false); + auto unknown_file_paths = + nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); + nearby_connections_manager_->ProcessUnknownFilePathsToDeleteForTesting( + PayloadStatus::kCanceled, PayloadContent::Type::kFile, file); + EXPECT_EQ(unknown_file_paths.size(), 0); + + // Flag is on. Add unknown file paths with kCanceled to the list. + NearbyFlags::GetInstance().OverrideBoolFlagValue( + config_package_nearby::nearby_sharing_feature:: + kDeleteUnexpectedReceivedFile, + true); + nearby_connections_manager_->ProcessUnknownFilePathsToDeleteForTesting( + PayloadStatus::kCanceled, PayloadContent::Type::kFile, file); + unknown_file_paths = + nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); + EXPECT_EQ(unknown_file_paths.size(), 1); + nearby_connections_manager_->GetAndClearUnknownFilePathsToDelete(); + + // Flag is on. Don't add unknown file paths w/o kCanceled to the list. + nearby_connections_manager_->ProcessUnknownFilePathsToDeleteForTesting( + PayloadStatus::kFailure, PayloadContent::Type::kFile, file); + unknown_file_paths = + nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); + EXPECT_EQ(unknown_file_paths.size(), 0); + nearby_connections_manager_->GetAndClearUnknownFilePathsToDelete(); +} + } // namespace NearbyConnectionsManagerUnitTests } // namespace sharing } // namespace nearby diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index f532ce97ed..fe1cfc27ab 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -3089,6 +3089,11 @@ void NearbySharingServiceImpl::OnIncomingTransferUpdate( // For any type of failure, lets make sure any pending files get cleaned // up. RemoveIncomingPayloads(share_target); + } else { + if (!nearby_connections_manager_->GetAndClearUnknownFilePathsToDelete() + .empty()) { + NL_LOG(WARNING) << __func__ << ": Unknown file paths are not empty."; + } } } else if (metadata.status() == TransferMetadata::Status::kAwaitingLocalConfirmation) { diff --git a/sharing/nearby_sharing_service_impl_test.cc b/sharing/nearby_sharing_service_impl_test.cc index d1460a5d2a..f1cf201289 100644 --- a/sharing/nearby_sharing_service_impl_test.cc +++ b/sharing/nearby_sharing_service_impl_test.cc @@ -4835,7 +4835,7 @@ TEST_F(NearbySharingServiceImplTest, RemoveIncomingPayloads) { fake_nearby_connections_manager_->AddUnknownFilePathsToDeleteForTesting( "test2.txt"); auto unknown_file_paths_to_delete = - fake_nearby_connections_manager_->GetUnknownFilePathsToDelete(); + fake_nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting(); EXPECT_EQ(unknown_file_paths_to_delete.size(), 2); EXPECT_THAT(unknown_file_paths_to_delete, UnorderedElementsAre("test1.txt", "test2.txt")); @@ -4843,7 +4843,8 @@ TEST_F(NearbySharingServiceImplTest, RemoveIncomingPayloads) { share_target.is_incoming = true; service_->RemoveIncomingPayloads(share_target); EXPECT_EQ( - fake_nearby_connections_manager_->GetUnknownFilePathsToDelete().size(), + fake_nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting() + .size(), 0); // Test GetAndClearUnknownFilePathsToDelete @@ -4855,7 +4856,8 @@ TEST_F(NearbySharingServiceImplTest, RemoveIncomingPayloads) { fake_nearby_connections_manager_->GetAndClearUnknownFilePathsToDelete(); EXPECT_EQ(unknown_file_paths_to_delete.size(), 2); EXPECT_EQ( - fake_nearby_connections_manager_->GetUnknownFilePathsToDelete().size(), + fake_nearby_connections_manager_->GetUnknownFilePathsToDeleteForTesting() + .size(), 0); }