From f15f7ecc2413ca70e242844f341c5b1fb2d249db Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Wed, 29 May 2024 10:01:13 -0700 Subject: [PATCH] Add kInvalidArgument status. PiperOrigin-RevId: 638322753 --- sharing/nearby_sharing_service.cc | 3 +- sharing/nearby_sharing_service.h | 6 +- sharing/nearby_sharing_service_impl.cc | 76 +++++++++++++-------- sharing/nearby_sharing_service_impl.h | 2 +- sharing/nearby_sharing_service_impl_test.cc | 14 ++-- sharing/nearby_sharing_service_test.cc | 3 +- 6 files changed, 63 insertions(+), 41 deletions(-) diff --git a/sharing/nearby_sharing_service.cc b/sharing/nearby_sharing_service.cc index 8b245ab3..12c4b1f7 100644 --- a/sharing/nearby_sharing_service.cc +++ b/sharing/nearby_sharing_service.cc @@ -14,7 +14,6 @@ #include "sharing/nearby_sharing_service.h" -#include #include #include "sharing/internal/public/logging.h" @@ -46,6 +45,8 @@ std::string NearbySharingService::StatusCodeToString(StatusCodes status_code) { return "kNoAvailableConnectionMedium"; case StatusCodes::kIrrecoverableHardwareError: return "kIrrecoverableHardwareError"; + case StatusCodes::kInvalidArgument: + return "kInvalidArgument"; } NL_LOG(ERROR) << "Unexpected value for StatusCodes: " << static_cast(status_code); diff --git a/sharing/nearby_sharing_service.h b/sharing/nearby_sharing_service.h index baf1df1d..2cc218ec 100644 --- a/sharing/nearby_sharing_service.h +++ b/sharing/nearby_sharing_service.h @@ -70,7 +70,11 @@ class NearbySharingService { // Bluetooth or WiFi hardware ran into an irrecoverable state. User PC needs // to be restarted. kIrrecoverableHardwareError = 6, - kMaxValue = kIrrecoverableHardwareError + // Method argument is invalid. + // TODO(b/341292610): update dart side to use kInvalidArgument. + // https://source.corp.google.com/piper///depot/google3/location/nearby/cpp/sharing/clients/dart/platform/lib/types/models.dart;rcl=637648200;l=21 + kInvalidArgument = 7, + kMaxValue = kInvalidArgument }; enum class ReceiveSurfaceState { diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index fe1cfc27..545527b6 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -376,7 +376,7 @@ void NearbySharingServiceImpl::RegisterSendSurface( << __func__ << ": RegisterSendSurface failed. Already registered for a " "different state."; - std::move(status_codes_callback)(StatusCodes::kError); + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); return; } @@ -534,7 +534,7 @@ void NearbySharingServiceImpl::RegisterReceiveSurface( NL_LOG(ERROR) << __func__ << ": transfer callback already registered but for a " "different state."; - std::move(status_codes_callback)(StatusCodes::kError); + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); return; } @@ -659,7 +659,7 @@ void NearbySharingServiceImpl::SendAttachments( if (attachments.empty()) { NL_LOG(WARNING) << __func__ << ": No attachments to send."; - std::move(status_codes_callback)(StatusCodes::kError); + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); return; } // Outgoing connections always announces with contacts visibility. @@ -673,20 +673,33 @@ void NearbySharingServiceImpl::SendAttachments( return; } - ShareTargetInfo* info = GetShareTargetInfo(share_target_id); + OutgoingShareTargetInfo* info = + GetOutgoingShareTargetInfo(share_target_id); if (!info) { NL_LOG(WARNING) << __func__ << ": Failed to send attachments. Unknown ShareTarget."; - std::move(status_codes_callback)(StatusCodes::kError); + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); return; } - app_info_->SetActiveFlag(); ShareTarget share_target = info->share_target(); for (std::unique_ptr& attachment : attachments) { attachment->MoveToShareTarget(share_target); } + if (!share_target.has_attachments()) { + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); + return; + } + for (const FileAttachment& attachment : share_target.file_attachments) { + if (!attachment.file_path()) { + NL_LOG(WARNING) << __func__ << ": Got file attachment without path"; + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); + return; + } + } + + app_info_->SetActiveFlag(); // Set session ID. info->set_session_id(analytics_recorder_->GenerateNextId()); info->set_share_target(share_target); @@ -710,7 +723,7 @@ void NearbySharingServiceImpl::SendAttachments( .set_status(TransferMetadata::Status::kConnecting) .build()); - CreatePayloads(std::move(share_target), + CreatePayloads(*info, [this, endpoint_info = std::move(*endpoint_info)]( ShareTarget share_target, bool success) { // Log analytics event of describing attachments. @@ -737,9 +750,15 @@ void NearbySharingServiceImpl::Accept( ResponseToIntroduction::ACCEPT_INTRODUCTION, receiving_session_id_); ShareTargetInfo* info = GetShareTargetInfo(share_target_id); - if (!info || !info->connection()) { + if (info == nullptr) { NL_LOG(WARNING) << __func__ << ": Accept invoked for unknown share target"; + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); + return; + } + if (!info->connection()) { + NL_LOG(WARNING) << __func__ + << ": Accept invoked for unconnected share target"; std::move(status_codes_callback)(StatusCodes::kOutOfOrderApiCall); return; } @@ -780,9 +799,15 @@ void NearbySharingServiceImpl::Reject( ResponseToIntroduction::REJECT_INTRODUCTION, receiving_session_id_); ShareTargetInfo* info = GetShareTargetInfo(share_target_id); - if (!info || !info->connection()) { + if (info == nullptr) { NL_LOG(WARNING) << __func__ << ": Reject invoked for unknown share target"; + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); + return; + } + if (!info->connection()) { + NL_LOG(WARNING) << __func__ + << ": Reject invoked for unconnected share target"; std::move(status_codes_callback)(StatusCodes::kOutOfOrderApiCall); return; } @@ -835,11 +860,10 @@ void NearbySharingServiceImpl::DoCancel( std::function status_codes_callback, bool is_initiator_of_cancellation) { ShareTargetInfo* info = GetShareTargetInfo(share_target_id); - if (!info) { - NL_LOG(ERROR) << __func__ - << ": Cancel invoked for unknown share target, returning " - "kOutOfOrderApiCall"; - std::move(status_codes_callback)(StatusCodes::kOutOfOrderApiCall); + if (info == nullptr) { + NL_LOG(WARNING) << __func__ + << ": Cancel invoked for unknown share target"; + std::move(status_codes_callback)(StatusCodes::kInvalidArgument); return; } @@ -2752,23 +2776,19 @@ void NearbySharingServiceImpl::SendIntroduction( } void NearbySharingServiceImpl::CreatePayloads( - ShareTarget share_target, std::function callback) { - OutgoingShareTargetInfo* info = GetOutgoingShareTargetInfo(share_target.id); - if (!info || !share_target.has_attachments()) { - std::move(callback)(std::move(share_target), /*success=*/false); - return; - } - - if (!info->file_payloads().empty() || !info->text_payloads().empty() || - !info->wifi_credentials_payloads().empty()) { + OutgoingShareTargetInfo& info, + std::function callback) { + ShareTarget share_target = info.share_target(); + if (!info.file_payloads().empty() || !info.text_payloads().empty() || + !info.wifi_credentials_payloads().empty()) { // We may have already created the payloads in the case of retry, so we can // skip this step. std::move(callback)(std::move(share_target), /*success=*/false); return; } - info->set_text_payloads(CreateTextPayloads(share_target.text_attachments)); - info->set_wifi_credentials_payloads( + info.set_text_payloads(CreateTextPayloads(share_target.text_attachments)); + info.set_wifi_credentials_payloads( CreateWifiCredentialsPayloads(share_target.wifi_credentials_attachments)); if (share_target.file_attachments.empty()) { std::move(callback)(std::move(share_target), /*success=*/true); @@ -2776,12 +2796,8 @@ void NearbySharingServiceImpl::CreatePayloads( } std::vector file_paths; + file_paths.reserve(share_target.file_attachments.size()); for (const FileAttachment& attachment : share_target.file_attachments) { - if (!attachment.file_path()) { - NL_LOG(WARNING) << __func__ << ": Got file attachment without path"; - std::move(callback)(std::move(share_target), /*success=*/false); - return; - } file_paths.push_back(*attachment.file_path()); } diff --git a/sharing/nearby_sharing_service_impl.h b/sharing/nearby_sharing_service_impl.h index ff331749..80e5e6f0 100644 --- a/sharing/nearby_sharing_service_impl.h +++ b/sharing/nearby_sharing_service_impl.h @@ -319,7 +319,7 @@ class NearbySharingServiceImpl void SendIntroduction(const ShareTarget& share_target, std::optional four_digit_token); - void CreatePayloads(ShareTarget share_target, + void CreatePayloads(OutgoingShareTargetInfo& info, std::function callback); void OnCreatePayloads(std::vector endpoint_info, ShareTarget share_target, bool success); diff --git a/sharing/nearby_sharing_service_impl_test.cc b/sharing/nearby_sharing_service_impl_test.cc index f1cf2012..63ca196b 100644 --- a/sharing/nearby_sharing_service_impl_test.cc +++ b/sharing/nearby_sharing_service_impl_test.cc @@ -1386,7 +1386,7 @@ TEST_F(NearbySharingServiceImplTest, StartFastInitiationAdvertising) { // not called again. EXPECT_EQ(RegisterSendSurface(&transfer_callback, &discovery_callback, SendSurfaceState::kForeground), - NearbySharingService::StatusCodes::kError); + NearbySharingService::StatusCodes::kInvalidArgument); EXPECT_EQ(fast_initiation->StartAdvertisingCount(), 1); } @@ -1627,7 +1627,7 @@ TEST_F(NearbySharingServiceImplTest, EXPECT_EQ(RegisterSendSurface(&transfer_callback, &discovery_callback, SendSurfaceState::kForeground), - NearbySharingService::StatusCodes::kError); + NearbySharingService::StatusCodes::kInvalidArgument); EXPECT_TRUE(fake_nearby_connections_manager_->IsDiscovering()); } @@ -1673,7 +1673,7 @@ TEST_F(NearbySharingServiceImplTest, EXPECT_EQ(RegisterSendSurface(&transfer_callback, &discovery_callback, SendSurfaceState::kBackground), - NearbySharingService::StatusCodes::kError); + NearbySharingService::StatusCodes::kInvalidArgument); EXPECT_TRUE(fake_nearby_connections_manager_->IsDiscovering()); } @@ -2779,7 +2779,7 @@ TEST_F(NearbySharingServiceImplTest, AcceptInvalidShareTarget) { service_->Accept( share_target.id, [&](NearbySharingServiceImpl::StatusCodes status_code) { EXPECT_EQ(status_code, - NearbySharingServiceImpl::StatusCodes::kOutOfOrderApiCall); + NearbySharingServiceImpl::StatusCodes::kInvalidArgument); notification.Notify(); }); @@ -3065,7 +3065,7 @@ TEST_F(NearbySharingServiceImplTest, RejectInvalidShareTarget) { service_->Reject( share_target.id, [&](NearbySharingServiceImpl::StatusCodes status_code) { EXPECT_EQ(status_code, - NearbySharingServiceImpl::StatusCodes::kOutOfOrderApiCall); + NearbySharingServiceImpl::StatusCodes::kInvalidArgument); notification.Notify(); }); @@ -3319,7 +3319,7 @@ TEST_F(NearbySharingServiceImplTest, SendAttachmentsWithoutAttachments) { DiscoverShareTarget(transfer_callback, discovery_callback); EXPECT_EQ(SendAttachments(target, /*attachments=*/{}), - NearbySharingServiceImpl::StatusCodes::kError); + NearbySharingServiceImpl::StatusCodes::kInvalidArgument); UnregisterSendSurface(&transfer_callback, &discovery_callback); } @@ -3386,7 +3386,7 @@ TEST_F(NearbySharingServiceImplTest, SendTextUnknownTarget) { ShareTarget target; EXPECT_EQ(SendAttachments(target, CreateTextAttachments({kTextPayload})), - NearbySharingServiceImpl::StatusCodes::kError); + NearbySharingServiceImpl::StatusCodes::kInvalidArgument); UnregisterSendSurface(&transfer_callback, &discovery_callback); } diff --git a/sharing/nearby_sharing_service_test.cc b/sharing/nearby_sharing_service_test.cc index a0cdea64..4ed44d53 100644 --- a/sharing/nearby_sharing_service_test.cc +++ b/sharing/nearby_sharing_service_test.cc @@ -44,9 +44,10 @@ std::vector GetTestData() { "kNoAvailableConnectionMedium"}, {StatusCodes::kIrrecoverableHardwareError, "kIrrecoverableHardwareError"}, + {StatusCodes::kInvalidArgument, "kInvalidArgument"}, // If entries are added, kMaxValue and // NearbySharingService::StatusCodeToString should be updated. - {StatusCodes::kMaxValue, "kIrrecoverableHardwareError"}, + {StatusCodes::kMaxValue, "kInvalidArgument"}, }); return *kStatusCodeToStringData;