From 58757e2689b670a3d4d28eb0cc97685c86a4661d Mon Sep 17 00:00:00 2001 From: Jon Simantov Date: Fri, 22 Apr 2022 10:50:07 -0700 Subject: [PATCH 1/7] Fix storage content-type on desktop, and add test. --- .../integration_test/src/integration_test.cc | 61 +++++++++++++++++++ .../src/desktop/storage_reference_desktop.cc | 27 +++++--- .../src/desktop/storage_reference_desktop.h | 8 ++- 3 files changed, 84 insertions(+), 12 deletions(-) diff --git a/storage/integration_test/src/integration_test.cc b/storage/integration_test/src/integration_test.cc index 962141c344..a5040ceddf 100644 --- a/storage/integration_test/src/integration_test.cc +++ b/storage/integration_test/src/integration_test.cc @@ -509,6 +509,67 @@ TEST_F(FirebaseStorageTest, TestWriteAndReadFileWithCustomMetadata) { } } +// 1x1 transparent PNG file +static const unsigned char kEmptyPngFileBytes[] = { + 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, + 0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, + 0x08, 0x06, 0x00, 0x00, 0x00, 0x1f, 0x15, 0xc4, 0x89, 0x00, 0x00, 0x00, + 0x0d, 0x49, 0x44, 0x41, 0x54, 0x78, 0xda, 0x63, 0xfc, 0xcf, 0xc0, 0x50, + 0x0f, 0x00, 0x04, 0x85, 0x01, 0x80, 0x84, 0xa9, 0x8c, 0x21, 0x00, 0x00, + 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82 +}; + +TEST_F(FirebaseStorageTest, TestWriteAndReadContentType) { + SignIn(); + + firebase::storage::StorageReference ref = + CreateFolder().Child("TestImage-CustomMetadata.png"); + LogDebug("Storage URL: gs://%s%s", ref.bucket().c_str(), + ref.full_path().c_str()); + cleanup_files_.push_back(ref); + std::string content_type = "image/png"; + // Write to a simple file. + { + LogDebug("Write a sample file with custom content-type from byte buffer."); + firebase::storage::Metadata metadata; + metadata.set_content_type(content_type.c_str()); + firebase::Future future = + ref.PutBytes(kEmptyPngFileBytes, sizeof(kEmptyPngFileBytes), metadata); + WaitForCompletion(future, "PutBytes"); + const firebase::storage::Metadata* metadata_written = future.result(); + ASSERT_NE(metadata_written, nullptr); + EXPECT_EQ(metadata_written->content_type(), content_type); + } + // Now read back the file. + { + LogDebug("Download sample file with custom content-type to memory."); + const size_t kBufferSize = 1024; + char buffer[kBufferSize]; + memset(buffer, 0, sizeof(buffer)); + + firebase::Future future = RunWithRetry( + [&]() { return ref.GetBytes(buffer, kBufferSize); }); + WaitForCompletion(future, "GetBytes"); + ASSERT_NE(future.result(), nullptr); + size_t file_size = *future.result(); + EXPECT_EQ(file_size, sizeof(kEmptyPngFileBytes)); + EXPECT_THAT(kEmptyPngFileBytes, ElementsAreArray(buffer, file_size)) + << "Download failed, file contents did not match."; + } + // And read the custom content type + { + LogDebug("Read custom content-type."); + firebase::Future future = + RunWithRetry( + [&]() { return ref.GetMetadata(); }); + WaitForCompletion(future, "GetFileMetadata"); + const firebase::storage::Metadata* metadata = future.result(); + ASSERT_NE(metadata, nullptr); + + EXPECT_EQ(metadata->content_type(), content_type); + } +} + const char kPutFileTestFile[] = "PutFileTest.txt"; const char kGetFileTestFile[] = "GetFileTest.txt"; const char kFileUriScheme[] = "file://"; diff --git a/storage/src/desktop/storage_reference_desktop.cc b/storage/src/desktop/storage_reference_desktop.cc index abc4c01f90..05110d0e06 100644 --- a/storage/src/desktop/storage_reference_desktop.cc +++ b/storage/src/desktop/storage_reference_desktop.cc @@ -230,7 +230,8 @@ Future StorageReferenceInternal::DeleteLastResult() { // Handy utility function, since REST calls have similar setup and teardown. void StorageReferenceInternal::PrepareRequest(rest::Request* request, const char* url, - const char* method) { + const char* method, + const char* content_type) { request->set_url(url); request->set_method(method); @@ -240,6 +241,10 @@ void StorageReferenceInternal::PrepareRequest(rest::Request* request, std::string auth_header = "Bearer " + token; request->add_header("Authorization", auth_header.c_str()); } + // if content_type was specified, add a header. + if (content_type != nullptr && *content_type != '\0') { + request->add_header("Content-Type", content_type); + } // Unfortunately the storage backend rejects requests with the complete // user agent specified by the x-goog-api-client header so we only use // the X-Firebase-Storage-Version header to attribute the client. @@ -404,7 +409,7 @@ Future StorageReferenceInternal::PutBytes( Future StorageReferenceInternal::PutBytesInternal( const void* buffer, size_t buffer_size, Listener* listener, - Controller* controller_out) { + Controller* controller_out, const char* content_type) { auto* future_api = future(); auto handle = future_api->SafeAlloc(kStorageReferenceFnPutBytes); auto send_request_funct{[&, buffer, buffer_size, listener, @@ -416,7 +421,8 @@ Future StorageReferenceInternal::PutBytesInternal( storage::internal::RequestBinary* request = new storage::internal::RequestBinary(static_cast(buffer), buffer_size); - PrepareRequest(request, storageUri_.AsHttpUrl().c_str(), rest::util::kPost); + PrepareRequest(request, storageUri_.AsHttpUrl().c_str(), rest::util::kPost, + content_type); ReturnedMetadataResponse* response = new ReturnedMetadataResponse(handle, future_api, AsStorageReference()); RestCall(request, request->notifier(), response, handle.get(), listener, @@ -442,8 +448,9 @@ Future StorageReferenceInternal::PutBytes( // different storage reference than the original, so the caller of this // function can't access it via PutFileLastResult. Future putbytes_internal = - data->storage_ref.internal_->PutBytesInternal(buffer, buffer_size, - listener, controller_out); + data->storage_ref.internal_->PutBytesInternal( + buffer, buffer_size, listener, controller_out, + metadata ? metadata->content_type() : nullptr); SetupMetadataChain(putbytes_internal, data); @@ -466,7 +473,8 @@ Future StorageReferenceInternal::PutFile(const char* path, } Future StorageReferenceInternal::PutFileInternal( - const char* path, Listener* listener, Controller* controller_out) { + const char* path, Listener* listener, Controller* controller_out, + const char* content_type) { auto* future_api = future(); auto handle = future_api->SafeAlloc(kStorageReferenceFnPutFile); @@ -490,7 +498,7 @@ Future StorageReferenceInternal::PutFileInternal( handle, future_api, AsStorageReference()); PrepareRequest(request, storageUri_.AsHttpUrl().c_str(), - rest::util::kPost); + rest::util::kPost, content_type); RestCall(request, request->notifier(), response, handle.get(), listener, controller_out); return response; @@ -516,8 +524,9 @@ Future StorageReferenceInternal::PutFile(const char* path, // different storage reference than the original, so the caller of this // function can't access it via PutFileLastResult. Future putfile_internal = - data->storage_ref.internal_->PutFileInternal(path, listener, - controller_out); + data->storage_ref.internal_->PutFileInternal( + path, listener, controller_out, + metadata ? metadata->content_type() : nullptr); SetupMetadataChain(putfile_internal, data); diff --git a/storage/src/desktop/storage_reference_desktop.h b/storage/src/desktop/storage_reference_desktop.h index 62c59e5d51..784ac9ab04 100644 --- a/storage/src/desktop/storage_reference_desktop.h +++ b/storage/src/desktop/storage_reference_desktop.h @@ -176,17 +176,19 @@ class StorageReferenceInternal { // Upload data without metadata. Future PutBytesInternal(const void* buffer, size_t buffer_size, Listener* listener, - Controller* controller_out); + Controller* controller_out, + const char* content_type = nullptr); // Upload file without metadata. Future PutFileInternal(const char* path, Listener* listener, - Controller* controller_out); + Controller* controller_out, + const char* content_type = nullptr); void RestCall(rest::Request* request, internal::Notifier* request_notifier, BlockingResponse* response, FutureHandle handle, Listener* listener, Controller* controller_out); void PrepareRequest(rest::Request* request, const char* url, - const char* method); + const char* method, const char* content_type = nullptr); void SetupMetadataChain(Future starting_future, MetadataChainData* data); From 8d6e1e78c6d593e4a2c761630953a2912f60ee14 Mon Sep 17 00:00:00 2001 From: Jon Simantov Date: Fri, 22 Apr 2022 11:26:31 -0700 Subject: [PATCH 2/7] Format code. --- storage/integration_test/src/integration_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/storage/integration_test/src/integration_test.cc b/storage/integration_test/src/integration_test.cc index a5040ceddf..ea5d5ef8ed 100644 --- a/storage/integration_test/src/integration_test.cc +++ b/storage/integration_test/src/integration_test.cc @@ -516,8 +516,7 @@ static const unsigned char kEmptyPngFileBytes[] = { 0x08, 0x06, 0x00, 0x00, 0x00, 0x1f, 0x15, 0xc4, 0x89, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x44, 0x41, 0x54, 0x78, 0xda, 0x63, 0xfc, 0xcf, 0xc0, 0x50, 0x0f, 0x00, 0x04, 0x85, 0x01, 0x80, 0x84, 0xa9, 0x8c, 0x21, 0x00, 0x00, - 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82 -}; + 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82}; TEST_F(FirebaseStorageTest, TestWriteAndReadContentType) { SignIn(); @@ -534,7 +533,7 @@ TEST_F(FirebaseStorageTest, TestWriteAndReadContentType) { firebase::storage::Metadata metadata; metadata.set_content_type(content_type.c_str()); firebase::Future future = - ref.PutBytes(kEmptyPngFileBytes, sizeof(kEmptyPngFileBytes), metadata); + ref.PutBytes(kEmptyPngFileBytes, sizeof(kEmptyPngFileBytes), metadata); WaitForCompletion(future, "PutBytes"); const firebase::storage::Metadata* metadata_written = future.result(); ASSERT_NE(metadata_written, nullptr); From 9b34f2174f3a24a726fe70e7d06bee11f93b4ac3 Mon Sep 17 00:00:00 2001 From: Jon Simantov Date: Fri, 22 Apr 2022 11:27:34 -0700 Subject: [PATCH 3/7] Add release note --- release_build_files/readme.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 0a1495d248..813544e418 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -567,6 +567,11 @@ workflow use only during the development of your app, not for publicly shipping code. ## Release Notes +### Upcoming release +- Changes + - Storage (Desktop): Fix Content-Type when uploading with custom + metadata. + ### 8.11.0 - Changes - Firestore/Database (Desktop): Upgrade LevelDb dependency to 1.23 From b7215490816a4df8384937974e4a3d6ef3d4fabe Mon Sep 17 00:00:00 2001 From: Jon Simantov Date: Fri, 22 Apr 2022 11:28:43 -0700 Subject: [PATCH 4/7] Rename test --- storage/integration_test/src/integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/integration_test/src/integration_test.cc b/storage/integration_test/src/integration_test.cc index ea5d5ef8ed..1f495a6bf6 100644 --- a/storage/integration_test/src/integration_test.cc +++ b/storage/integration_test/src/integration_test.cc @@ -518,11 +518,11 @@ static const unsigned char kEmptyPngFileBytes[] = { 0x0f, 0x00, 0x04, 0x85, 0x01, 0x80, 0x84, 0xa9, 0x8c, 0x21, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82}; -TEST_F(FirebaseStorageTest, TestWriteAndReadContentType) { +TEST_F(FirebaseStorageTest, TestWriteAndReadCustomContentType) { SignIn(); firebase::storage::StorageReference ref = - CreateFolder().Child("TestImage-CustomMetadata.png"); + CreateFolder().Child("TestFile-CustomContentType.png"); LogDebug("Storage URL: gs://%s%s", ref.bucket().c_str(), ref.full_path().c_str()); cleanup_files_.push_back(ref); From bd0a8014e6f2c8607b0459af866633009a2a5512 Mon Sep 17 00:00:00 2001 From: Jon Simantov Date: Fri, 22 Apr 2022 11:29:18 -0700 Subject: [PATCH 5/7] Reword --- release_build_files/readme.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 813544e418..f167b6c9e7 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -569,8 +569,8 @@ code. ## Release Notes ### Upcoming release - Changes - - Storage (Desktop): Fix Content-Type when uploading with custom - metadata. + - Storage (Desktop): Set Content-Type HTTP header when uploading with + custom metadata. ### 8.11.0 - Changes From b37a5c0507ba8cd0d901ccf6d96588bebdde13fb Mon Sep 17 00:00:00 2001 From: Jon Simantov Date: Fri, 22 Apr 2022 13:17:41 -0700 Subject: [PATCH 6/7] Fix data thread access. --- .../src/desktop/storage_reference_desktop.cc | 57 ++++++++++--------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/storage/src/desktop/storage_reference_desktop.cc b/storage/src/desktop/storage_reference_desktop.cc index 05110d0e06..8b33b331e9 100644 --- a/storage/src/desktop/storage_reference_desktop.cc +++ b/storage/src/desktop/storage_reference_desktop.cc @@ -412,7 +412,9 @@ Future StorageReferenceInternal::PutBytesInternal( Controller* controller_out, const char* content_type) { auto* future_api = future(); auto handle = future_api->SafeAlloc(kStorageReferenceFnPutBytes); - auto send_request_funct{[&, buffer, buffer_size, listener, + + std::string content_type_str = content_type ? content_type : ""; + auto send_request_funct{[&, content_type_str, buffer, buffer_size, listener, controller_out]() -> BlockingResponse* { auto* future_api = future(); auto handle = @@ -422,7 +424,7 @@ Future StorageReferenceInternal::PutBytesInternal( new storage::internal::RequestBinary(static_cast(buffer), buffer_size); PrepareRequest(request, storageUri_.AsHttpUrl().c_str(), rest::util::kPost, - content_type); + content_type_str.c_str()); ReturnedMetadataResponse* response = new ReturnedMetadataResponse(handle, future_api, AsStorageReference()); RestCall(request, request->notifier(), response, handle.get(), listener, @@ -479,31 +481,32 @@ Future StorageReferenceInternal::PutFileInternal( auto handle = future_api->SafeAlloc(kStorageReferenceFnPutFile); std::string final_path = StripProtocol(path); - auto send_request_funct{ - [&, final_path, listener, controller_out]() -> BlockingResponse* { - auto* future_api = future(); - auto handle = - future_api->SafeAlloc(kStorageReferenceFnPutFileInternal); - - // Open the file, calculate the length. - storage::internal::RequestFile* request( - new storage::internal::RequestFile(final_path.c_str(), 0)); - if (!request->IsFileOpen()) { - delete request; - future_api->Complete(handle, kErrorUnknown, "Could not read file."); - return nullptr; - } else { - // Everything is good. Fire off the request. - ReturnedMetadataResponse* response = new ReturnedMetadataResponse( - handle, future_api, AsStorageReference()); - - PrepareRequest(request, storageUri_.AsHttpUrl().c_str(), - rest::util::kPost, content_type); - RestCall(request, request->notifier(), response, handle.get(), - listener, controller_out); - return response; - } - }}; + std::string content_type_str = content_type ? content_type : ""; + auto send_request_funct{[&, final_path, content_type_str, listener, + controller_out]() -> BlockingResponse* { + auto* future_api = future(); + auto handle = + future_api->SafeAlloc(kStorageReferenceFnPutFileInternal); + + // Open the file, calculate the length. + storage::internal::RequestFile* request( + new storage::internal::RequestFile(final_path.c_str(), 0)); + if (!request->IsFileOpen()) { + delete request; + future_api->Complete(handle, kErrorUnknown, "Could not read file."); + return nullptr; + } else { + // Everything is good. Fire off the request. + ReturnedMetadataResponse* response = new ReturnedMetadataResponse( + handle, future_api, AsStorageReference()); + + PrepareRequest(request, storageUri_.AsHttpUrl().c_str(), + rest::util::kPost, content_type_str.c_str()); + RestCall(request, request->notifier(), response, handle.get(), listener, + controller_out); + return response; + } + }}; SendRequestWithRetry(kStorageReferenceFnPutFileInternal, send_request_funct, handle, storage_->max_upload_retry_time()); return PutFileLastResult(); From f9464999765dac69ac3efc6377dcb5109f0037a7 Mon Sep 17 00:00:00 2001 From: Jon Simantov Date: Mon, 25 Apr 2022 13:54:50 -0700 Subject: [PATCH 7/7] Set content_type in a consistent way. --- storage/src/desktop/storage_reference_desktop.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/src/desktop/storage_reference_desktop.cc b/storage/src/desktop/storage_reference_desktop.cc index 8b33b331e9..367e270dc5 100644 --- a/storage/src/desktop/storage_reference_desktop.cc +++ b/storage/src/desktop/storage_reference_desktop.cc @@ -591,11 +591,11 @@ Future StorageReferenceInternal::UpdateMetadata( new ReturnedMetadataResponse(handle, future_api, AsStorageReference()); storage::internal::Request* request = new storage::internal::Request(); - PrepareRequest(request, storageUri_.AsHttpUrl().c_str(), "PATCH"); + PrepareRequest(request, storageUri_.AsHttpUrl().c_str(), "PATCH", + "application/json"); std::string metadata_json = metadata->internal_->ExportAsJson(); request->set_post_fields(metadata_json.c_str(), metadata_json.length()); - request->add_header("Content-Type", "application/json"); RestCall(request, request->notifier(), response, handle.get(), nullptr, nullptr);