From eeeb93bf537caf8eec2ed36aa20dd0b0f30e5f6b Mon Sep 17 00:00:00 2001 From: Edwin Wu Date: Mon, 9 Dec 2024 06:40:53 -0800 Subject: [PATCH] analytics: Add operation result code for Payload analytics, IV PiperOrigin-RevId: 704265442 --- .../internal_payload_factory.cc | 40 +++++++++------- .../implementation/internal_payload_factory.h | 9 ++-- .../internal_payload_factory_test.cc | 48 ++++++++++++++----- connections/implementation/payload_manager.cc | 18 ++++--- 4 files changed, 75 insertions(+), 40 deletions(-) diff --git a/connections/implementation/internal_payload_factory.cc b/connections/implementation/internal_payload_factory.cc index 38268fc9..382740c0 100644 --- a/connections/implementation/internal_payload_factory.cc +++ b/connections/implementation/internal_payload_factory.cc @@ -27,6 +27,7 @@ #include "connections/payload_type.h" #include "internal/platform/byte_array.h" #include "internal/platform/exception.h" +#include "internal/platform/expected.h" #include "internal/platform/file.h" #include "internal/platform/implementation/platform.h" #include "internal/platform/input_stream.h" @@ -40,6 +41,7 @@ namespace connections { namespace { using ::location::nearby::connections::PayloadTransferFrame; +using ::location::nearby::proto::connections::OperationResultCode; class BytesInternalPayload : public InternalPayload { public: @@ -309,23 +311,24 @@ class IncomingFileInternalPayload : public InternalPayload { using ::nearby::api::ImplementationPlatform; using ::nearby::api::OSName; -std::unique_ptr CreateOutgoingInternalPayload( +ErrorOr> CreateOutgoingInternalPayload( Payload payload) { switch (payload.GetType()) { case PayloadType::kBytes: - return std::make_unique(std::move(payload)); + return {std::make_unique(std::move(payload))}; case PayloadType::kFile: { - return std::make_unique(std::move(payload)); + return { + std::make_unique(std::move(payload))}; } case PayloadType::kStream: - return std::make_unique( - std::move(payload)); + return { + std::make_unique(std::move(payload))}; default: DCHECK(false); // This should never happen. - return {}; + return {Error(OperationResultCode::DETAIL_UNKNOWN)}; } } @@ -350,26 +353,27 @@ std::string make_path(const std::string& custom_save_path, return api::ImplementationPlatform::GetDownloadPath(parent_folder, file_name); } -std::unique_ptr CreateIncomingInternalPayload( +ErrorOr> CreateIncomingInternalPayload( const location::nearby::connections::PayloadTransferFrame& frame, const std::string& custom_save_path) { if (frame.packet_type() != location::nearby::connections::PayloadTransferFrame::DATA) { - return {}; + return {Error( + OperationResultCode::NEARBY_GENERIC_INCOMING_PAYLOAD_NOT_DATA_TYPE)}; } const Payload::Id payload_id = frame.payload_header().id(); switch (frame.payload_header().type()) { case PayloadTransferFrame::PayloadHeader::BYTES: { - return std::make_unique( - Payload(payload_id, ByteArray(frame.payload_chunk().body()))); + return {std::make_unique( + Payload(payload_id, ByteArray(frame.payload_chunk().body())))}; } case PayloadTransferFrame::PayloadHeader::STREAM: { auto [input, output] = CreatePipe(); - return std::make_unique( - Payload(payload_id, std::move(input)), std::move(output)); + return {std::make_unique( + Payload(payload_id, std::move(input)), std::move(output))}; } case PayloadTransferFrame::PayloadHeader::FILE: { @@ -397,7 +401,7 @@ std::unique_ptr CreateIncomingInternalPayload( // file name for the output file. NEARBY_LOGS(ERROR) << "File name not found in incoming file Payload, " "and the Id wasn't found."; - return {}; + return {Error(OperationResultCode::IO_FILE_OPENING_ERROR)}; } } @@ -409,19 +413,19 @@ std::unique_ptr CreateIncomingInternalPayload( // there will be no input file to open. // On Chrome the file path should be empty, so use the payload id. if (ImplementationPlatform::GetCurrentOS() == OSName::kChromeOS) { - return std::make_unique( + return {std::make_unique( Payload(payload_id, InputFile(payload_id, total_size)), - OutputFile(payload_id), total_size); + OutputFile(payload_id), total_size)}; } else { - return std::make_unique( + return {std::make_unique( Payload(payload_id, parent_folder, file_name, InputFile(file_path, total_size)), - OutputFile(file_path), total_size); + OutputFile(file_path), total_size)}; } } default: DCHECK(false); // This should never happen. - return {}; + return {Error(OperationResultCode::DETAIL_UNKNOWN)}; } } diff --git a/connections/implementation/internal_payload_factory.h b/connections/implementation/internal_payload_factory.h index 6e510ade..26a41b80 100644 --- a/connections/implementation/internal_payload_factory.h +++ b/connections/implementation/internal_payload_factory.h @@ -15,22 +15,23 @@ #ifndef CORE_INTERNAL_INTERNAL_PAYLOAD_FACTORY_H_ #define CORE_INTERNAL_INTERNAL_PAYLOAD_FACTORY_H_ -#include - #include +#include #include "connections/implementation/internal_payload.h" #include "connections/payload.h" +#include "internal/platform/expected.h" namespace nearby { namespace connections { // Creates an InternalPayload representing an outgoing Payload. -std::unique_ptr CreateOutgoingInternalPayload(Payload payload); +ErrorOr> CreateOutgoingInternalPayload( + Payload payload); // Creates an InternalPayload representing an incoming Payload from a remote // endpoint. -std::unique_ptr CreateIncomingInternalPayload( +ErrorOr> CreateIncomingInternalPayload( const location::nearby::connections::PayloadTransferFrame& frame, const std::string& custom_save_path); diff --git a/connections/implementation/internal_payload_factory_test.cc b/connections/implementation/internal_payload_factory_test.cc index 9f6979ae..d69e6d7b 100644 --- a/connections/implementation/internal_payload_factory_test.cc +++ b/connections/implementation/internal_payload_factory_test.cc @@ -14,6 +14,7 @@ #include "connections/implementation/internal_payload_factory.h" +#include #include #include #include @@ -26,6 +27,7 @@ #include "connections/payload_type.h" #include "internal/platform/byte_array.h" #include "internal/platform/exception.h" +#include "internal/platform/expected.h" #include "internal/platform/file.h" #include "internal/platform/pipe.h" @@ -38,8 +40,10 @@ constexpr char kText[] = "data chunk"; TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromBytePayload) { ByteArray data(kText); - std::unique_ptr internal_payload = + ErrorOr> result = CreateOutgoingInternalPayload(Payload{data}); + ASSERT_FALSE(result.has_error()); + std::unique_ptr internal_payload = std::move(result.value()); EXPECT_NE(internal_payload, nullptr); Payload payload = internal_payload->ReleasePayload(); EXPECT_EQ(payload.AsFile(), nullptr); @@ -49,8 +53,10 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromBytePayload) { TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromStreamPayload) { auto [input, output] = CreatePipe(); - std::unique_ptr internal_payload = + ErrorOr> result = CreateOutgoingInternalPayload(Payload(std::move(input))); + ASSERT_FALSE(result.has_error()); + std::unique_ptr internal_payload = std::move(result.value()); EXPECT_NE(internal_payload, nullptr); Payload payload = internal_payload->ReleasePayload(); EXPECT_EQ(payload.AsFile(), nullptr); @@ -61,8 +67,10 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromStreamPayload) { TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFilePayload) { Payload::Id payload_id = Payload::GenerateId(); InputFile inputFile(payload_id, 512); - std::unique_ptr internal_payload = + ErrorOr> result = CreateOutgoingInternalPayload(Payload{payload_id, std::move(inputFile)}); + ASSERT_FALSE(result.has_error()); + std::unique_ptr internal_payload = std::move(result.value()); EXPECT_NE(internal_payload, nullptr); Payload payload = internal_payload->ReleasePayload(); EXPECT_NE(payload.AsFile(), nullptr); @@ -86,8 +94,10 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromByteMessage) { header.set_id(12345); header.set_total_size(512); *frame.mutable_payload_chunk() = std::move(payload_chunk); - std::unique_ptr internal_payload = + ErrorOr> result = CreateIncomingInternalPayload(frame, path); + ASSERT_FALSE(result.has_error()); + std::unique_ptr internal_payload = std::move(result.value()); EXPECT_NE(internal_payload, nullptr); Payload payload = internal_payload->ReleasePayload(); EXPECT_EQ(payload.AsFile(), nullptr); @@ -103,8 +113,10 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromStreamMessage) { header.set_type(PayloadTransferFrame::PayloadHeader::STREAM); header.set_id(12345); header.set_total_size(0); - std::unique_ptr internal_payload = + ErrorOr> result = CreateIncomingInternalPayload(frame, path); + ASSERT_FALSE(result.has_error()); + std::unique_ptr internal_payload = std::move(result.value()); EXPECT_NE(internal_payload, nullptr); { Payload payload = internal_payload->ReleasePayload(); @@ -126,8 +138,10 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFileMessage) { header.set_type(PayloadTransferFrame::PayloadHeader::FILE); header.set_id(12345); header.set_total_size(512); - std::unique_ptr internal_payload = + ErrorOr> result = CreateIncomingInternalPayload(frame, path); + ASSERT_FALSE(result.has_error()); + std::unique_ptr internal_payload = std::move(result.value()); EXPECT_NE(internal_payload, nullptr); Payload payload = internal_payload->ReleasePayload(); EXPECT_NE(payload.AsFile(), nullptr); @@ -144,9 +158,9 @@ TEST(InternalPayloadFactoryTest, auto& header = *frame.mutable_payload_header(); header.set_type(PayloadTransferFrame::PayloadHeader::FILE); header.set_total_size(512); - std::unique_ptr internal_payload = + ErrorOr> result = CreateIncomingInternalPayload(frame, path); - EXPECT_EQ(internal_payload, nullptr); + EXPECT_TRUE(result.has_error()); } TEST(InternalPayloadFactoryTest, @@ -158,8 +172,10 @@ TEST(InternalPayloadFactoryTest, header.set_type(PayloadTransferFrame::PayloadHeader::FILE); header.set_id(12345); header.set_total_size(512); - std::unique_ptr internal_payload = + ErrorOr> result = CreateIncomingInternalPayload(frame, path); + ASSERT_FALSE(result.has_error()); + std::unique_ptr internal_payload = std::move(result.value()); EXPECT_NE(internal_payload, nullptr); Payload payload = internal_payload->ReleasePayload(); EXPECT_EQ(payload.GetFileName(), "12345"); @@ -174,8 +190,10 @@ TEST(InternalPayloadFactoryTest, header.set_id(12345); header.set_total_size(512); header.set_file_name("test.file.name"); - std::unique_ptr internal_payload = + ErrorOr> result = CreateIncomingInternalPayload(frame, path); + ASSERT_FALSE(result.has_error()); + std::unique_ptr internal_payload = std::move(result.value()); EXPECT_NE(internal_payload, nullptr); auto test = internal_payload->GetFileName(); Payload payload = internal_payload->ReleasePayload(); @@ -196,8 +214,11 @@ TEST(InternalPayloadFactoryTest, Payload::Id payload_id = Payload::GenerateId(); CreateFileWithContents(payload_id, contents); InputFile inputFile(payload_id, contents.size()); - std::unique_ptr internal_payload = + ErrorOr> interal_payload_result = CreateOutgoingInternalPayload(Payload{payload_id, std::move(inputFile)}); + ASSERT_FALSE(interal_payload_result.has_error()); + std::unique_ptr internal_payload = + std::move(interal_payload_result.value()); EXPECT_NE(internal_payload, nullptr); ExceptionOr result = internal_payload->SkipToOffset(kOffset); @@ -215,8 +236,11 @@ TEST(InternalPayloadFactoryTest, ByteArray contents("0123456789"); constexpr size_t kOffset = 6; auto [input, output] = CreatePipe(); - std::unique_ptr internal_payload = + ErrorOr> interal_payload_result = CreateOutgoingInternalPayload(Payload(std::move(input))); + ASSERT_FALSE(interal_payload_result.has_error()); + std::unique_ptr internal_payload = + std::move(interal_payload_result.value()); EXPECT_NE(internal_payload, nullptr); output->Write(contents); diff --git a/connections/implementation/payload_manager.cc b/connections/implementation/payload_manager.cc index 5a07c40b..81abd312 100644 --- a/connections/implementation/payload_manager.cc +++ b/connections/implementation/payload_manager.cc @@ -305,7 +305,14 @@ std::string PayloadManager::ToString(EndpointInfo::Status status) { // Creates and starts tracking a PendingPayload for this Payload. Payload::Id PayloadManager::CreateOutgoingPayload( Payload payload, const EndpointIds& endpoint_ids) { - auto internal_payload{CreateOutgoingInternalPayload(std::move(payload))}; + ErrorOr> result = + CreateOutgoingInternalPayload(std::move(payload)); + if (result.has_error()) { + LOG(ERROR) << "Failed to create outgoing internal payload: " + << result.error().operation_result_code().value(); + return Payload::Id(); + } + std::unique_ptr internal_payload = std::move(result.value()); Payload::Id payload_id = internal_payload->GetId(); LOG(INFO) << "CreateOutgoingPayload: payload_id=" << payload_id; MutexLock lock(&mutex_); @@ -789,13 +796,12 @@ PayloadTransferFrame::PayloadChunk PayloadManager::CreatePayloadChunk( ErrorOr PayloadManager::CreateIncomingPayload(const PayloadTransferFrame& frame, const std::string& endpoint_id) { - // TODO(edwinwu): Add for return result code in CreateIncomingInternalPayload. - std::unique_ptr internal_payload = + ErrorOr> result = CreateIncomingInternalPayload(frame, custom_save_path_); - if (!internal_payload) { - return {Error(OperationResultCode::DETAIL_UNKNOWN)}; + if (result.has_error()) { + return {result.error()}; } - + std::unique_ptr internal_payload = std::move(result.value()); Payload::Id payload_id = internal_payload->GetId(); LOG(INFO) << "CreateIncomingPayload: payload_id=" << payload_id; pending_payloads_.StartTrackingPayload(