From 4c13db0c2d3fc9a5ed6da292474e1760c5710200 Mon Sep 17 00:00:00 2001 From: jfcarroll Date: Fri, 8 Apr 2022 13:23:41 -0700 Subject: [PATCH] This is a fix for incoming payloads not having a file_name set. PiperOrigin-RevId: 440438936 --- .../internal_payload_factory.cc | 23 +++++-- .../internal_payload_factory_test.cc | 61 ++++++++++++++++--- connections/payload.cc | 8 +++ connections/payload.h | 2 + 4 files changed, 81 insertions(+), 13 deletions(-) diff --git a/connections/implementation/internal_payload_factory.cc b/connections/implementation/internal_payload_factory.cc index 9f962a78..d3d3116e 100644 --- a/connections/implementation/internal_payload_factory.cc +++ b/connections/implementation/internal_payload_factory.cc @@ -347,16 +347,30 @@ std::unique_ptr CreateIncomingInternalPayload( } case PayloadTransferFrame::PayloadHeader::FILE: { + std::string parent_folder(""); + std::string file_name(""); std::string file_path(""); + int64_t total_size = 0; if (frame.payload_header().has_parent_folder()) { - file_path = frame.payload_header().parent_folder(); + parent_folder = frame.payload_header().parent_folder(); } if (frame.payload_header().has_file_name()) { - std::string file_name(frame.payload_header().file_name()); - file_path = make_path(file_path, file_name); + file_name = frame.payload_header().file_name(); + file_path = make_path(parent_folder, file_name); + } else { + if (frame.payload_header().has_id()) { + file_name = std::to_string(frame.payload_header().id()); + file_path = make_path(parent_folder, file_name); + } else { + // This is an error condition, we don't have any way to generate a + // file name for the output file. + NEARBY_LOGS(ERROR) << "File name not found in incoming file Payload, " + "and the Id wasn't found."; + return {}; + } } if (frame.payload_header().has_total_size()) { @@ -372,7 +386,8 @@ std::unique_ptr CreateIncomingInternalPayload( OutputFile(payload_id), total_size); } else { return absl::make_unique( - Payload(payload_id, InputFile(file_path, total_size)), + Payload(payload_id, parent_folder, file_name, + InputFile(file_path, total_size)), OutputFile(file_path), total_size); } } diff --git a/connections/implementation/internal_payload_factory_test.cc b/connections/implementation/internal_payload_factory_test.cc index 98df8960..ff1b680f 100644 --- a/connections/implementation/internal_payload_factory_test.cc +++ b/connections/implementation/internal_payload_factory_test.cc @@ -1,4 +1,4 @@ -// Copyright 2020 Google LLC +// Copyright 2020-2022 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -32,7 +32,7 @@ namespace { constexpr char kText[] = "data chunk"; -TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromBytePayload) { +TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromBytePayload) { ByteArray data(kText); std::unique_ptr internal_payload = CreateOutgoingInternalPayload(Payload{data}); @@ -43,7 +43,7 @@ TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromBytePayload) { EXPECT_EQ(payload.AsBytes(), ByteArray(kText)); } -TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromStreamPayload) { +TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromStreamPayload) { auto pipe = std::make_shared(); std::unique_ptr internal_payload = CreateOutgoingInternalPayload(Payload{[pipe]() -> InputStream& { @@ -56,7 +56,7 @@ TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromStreamPayload) { EXPECT_EQ(payload.AsBytes(), ByteArray()); } -TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromFilePayload) { +TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFilePayload) { Payload::Id payload_id = Payload::GenerateId(); InputFile inputFile(payload_id, 512); std::unique_ptr internal_payload = @@ -69,7 +69,7 @@ TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromFilePayload) { EXPECT_EQ(payload.GetId(), payload_id); } -TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromByteMessage) { +TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromByteMessage) { PayloadTransferFrame frame; frame.set_packet_type(PayloadTransferFrame::DATA); std::int64_t payload_chunk_offset = 0; @@ -92,7 +92,7 @@ TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromByteMessage) { EXPECT_EQ(payload.AsBytes(), ByteArray(kText)); } -TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromStreamMessage) { +TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromStreamMessage) { PayloadTransferFrame frame; frame.set_packet_type(PayloadTransferFrame::DATA); auto& header = *frame.mutable_payload_header(); @@ -109,7 +109,7 @@ TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromStreamMessage) { EXPECT_EQ(payload.GetType(), PayloadType::kStream); } -TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromFileMessage) { +TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFileMessage) { PayloadTransferFrame frame; frame.set_packet_type(PayloadTransferFrame::DATA); auto& header = *frame.mutable_payload_header(); @@ -126,13 +126,56 @@ TEST(InternalPayloadFActoryTest, CanCreateIternalPayloadFromFileMessage) { EXPECT_EQ(payload.GetType(), PayloadType::kFile); } +TEST(InternalPayloadFactoryTest, + InternalPayloadFromFileMessageWithoutIdReturnsNullptr) { + PayloadTransferFrame frame; + frame.set_packet_type(PayloadTransferFrame::DATA); + auto& header = *frame.mutable_payload_header(); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + header.set_total_size(512); + std::unique_ptr internal_payload = + CreateIncomingInternalPayload(frame); + EXPECT_EQ(internal_payload, nullptr); +} + +TEST(InternalPayloadFactoryTest, + CanCreateInternalPayloadFromFileMessageWithFileNameNotSet) { + PayloadTransferFrame frame; + frame.set_packet_type(PayloadTransferFrame::DATA); + auto& header = *frame.mutable_payload_header(); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + header.set_id(12345); + header.set_total_size(512); + std::unique_ptr internal_payload = + CreateIncomingInternalPayload(frame); + EXPECT_NE(internal_payload, nullptr); + Payload payload = internal_payload->ReleasePayload(); + EXPECT_EQ(payload.GetFileName(), "12345"); +} +TEST(InternalPayloadFactoryTest, + CanCreateInternalPayloadFromFileMessageWithFileNameSet) { + PayloadTransferFrame frame; + frame.set_packet_type(PayloadTransferFrame::DATA); + auto& header = *frame.mutable_payload_header(); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + header.set_id(12345); + header.set_total_size(512); + header.set_file_name("test.file.name"); + std::unique_ptr internal_payload = + CreateIncomingInternalPayload(frame); + EXPECT_NE(internal_payload, nullptr); + auto test = internal_payload->GetFileName(); + Payload payload = internal_payload->ReleasePayload(); + EXPECT_EQ(payload.GetFileName(), "test.file.name"); +} + void CreateFileWithContents(Payload::Id payload_id, const ByteArray& contents) { OutputFile file(payload_id); EXPECT_TRUE(file.Write(contents).Ok()); EXPECT_TRUE(file.Close().Ok()); } -TEST(InternalPayloadFActoryTest, +TEST(InternalPayloadFactoryTest, SkipToOffset_FilePayloadValidOffset_SkipsOffset) { ByteArray contents("0123456789"); constexpr size_t kOffset = 4; @@ -154,7 +197,7 @@ TEST(InternalPayloadFActoryTest, EXPECT_EQ(contents_after_skip, ByteArray("456789")); } -TEST(InternalPayloadFActoryTest, +TEST(InternalPayloadFactoryTest, SkipToOffset_StreamPayloadValidOffset_SkipsOffset) { ByteArray contents("0123456789"); constexpr size_t kOffset = 6; diff --git a/connections/payload.cc b/connections/payload.cc index 41f5f90f..c930e37b 100644 --- a/connections/payload.cc +++ b/connections/payload.cc @@ -62,6 +62,14 @@ Payload::Payload(Id id, ByteArray&& bytes) Payload::Payload(Id id, const ByteArray& bytes) : id_(id), type_(PayloadType::kBytes), content_(bytes) {} +Payload::Payload(Id id, std::string parent_folder, std::string file_name, + InputFile input_file) + : id_(id), + parent_folder_(parent_folder), + file_name_(file_name), + type_(PayloadType::kFile), + content_(std::move(input_file)) {} + Payload::Payload(Id id, std::function stream) : id_(id), type_(PayloadType::kStream), content_(std::move(stream)) {} diff --git a/connections/payload.h b/connections/payload.h index 07eb304a..f930f3e1 100644 --- a/connections/payload.h +++ b/connections/payload.h @@ -79,6 +79,8 @@ class DLL_API Payload { Payload(Id id, ByteArray&& bytes); Payload(Id id, const ByteArray& bytes); Payload(Id id, InputFile file); + Payload(Id id, std::string parent_folder, std::string file_name, + InputFile input_file); Payload(Id id, std::function stream); // Returns ByteArray payload, if it has been defined, or empty ByteArray.