From dfe57c9d8f7a66c127ddcc511f831ed3eca5e47e Mon Sep 17 00:00:00 2001 From: Vincent Liu <5900509+onecent1101@users.noreply.github.com> Date: Mon, 3 May 2021 16:16:19 -0700 Subject: [PATCH] [LYN-2964][LYN-2965] Improve user experience of using AWSScriptBehaviorS3 (#495) As AWS S3 GetObject doesn't provide proper file handling, move logic into custom request validation step --- .../Public/ScriptCanvas/AWSScriptBehaviorS3.h | 12 +++- .../ScriptCanvas/AWSScriptBehaviorS3.cpp | 58 +++++++++++----- .../ScriptCanvas/AWSScriptBehaviorS3Test.cpp | 67 ++++++++++++++++++- 3 files changed, 119 insertions(+), 18 deletions(-) diff --git a/Gems/AWSCore/Code/Include/Public/ScriptCanvas/AWSScriptBehaviorS3.h b/Gems/AWSCore/Code/Include/Public/ScriptCanvas/AWSScriptBehaviorS3.h index 5d7baf19a1..9d779e3629 100644 --- a/Gems/AWSCore/Code/Include/Public/ScriptCanvas/AWSScriptBehaviorS3.h +++ b/Gems/AWSCore/Code/Include/Public/ScriptCanvas/AWSScriptBehaviorS3.h @@ -75,6 +75,16 @@ namespace AWSCore class AWSScriptBehaviorS3 : public AWSScriptBehaviorBase { + static constexpr const char AWSScriptBehaviorS3Name[] = "AWSScriptBehaviorS3"; + static constexpr const char OutputFileIsEmptyErrorMessage[] = "Request validation failed, output file is empty."; + static constexpr const char OutputFileMissFullPathErrorMessage[] = "Request validation failed, output file miss full path."; + static constexpr const char OutputFileIsDirectoryErrorMessage[] = "Request validation failed, output file is a directory."; + static constexpr const char OutputFileDirectoryNotExistErrorMessage[] = "Request validation failed, output file directory doesn't exist."; + static constexpr const char OutputFileIsReadOnlyErrorMessage[] = "Request validation failed, output file is read-only."; + static constexpr const char BucketNameIsEmptyErrorMessage[] = "Request validation failed, bucket name is empty"; + static constexpr const char ObjectKeyNameIsEmptyErrorMessage[] = "Request validation failed, object key name is empty."; + static constexpr const char RegionNameIsEmptyErrorMessage[] = "Request validation failed, region name is empty."; + public: AWS_SCRIPT_BEHAVIOR_DEFINITION(AWSScriptBehaviorS3, "{7F4E956C-7463-4236-B320-C992D36A9C6E}"); @@ -87,7 +97,7 @@ namespace AWSCore private: using S3NotificationFunctionType = void(AWSScriptBehaviorS3Notifications::*)(const AZStd::string&); static bool ValidateGetObjectRequest(S3NotificationFunctionType notificationFunc, - const AZStd::string& bucket, const AZStd::string& objectKey, const AZStd::string& region, const AZStd::string& outFile); + const AZStd::string& bucket, const AZStd::string& objectKey, const AZStd::string& region, AZStd::string& outFile); static bool ValidateHeadObjectRequest(S3NotificationFunctionType notificationFunc, const AZStd::string& bucket, const AZStd::string& key, const AZStd::string& region); diff --git a/Gems/AWSCore/Code/Source/ScriptCanvas/AWSScriptBehaviorS3.cpp b/Gems/AWSCore/Code/Source/ScriptCanvas/AWSScriptBehaviorS3.cpp index 742a6826af..1f529572f1 100644 --- a/Gems/AWSCore/Code/Source/ScriptCanvas/AWSScriptBehaviorS3.cpp +++ b/Gems/AWSCore/Code/Source/ScriptCanvas/AWSScriptBehaviorS3.cpp @@ -43,7 +43,7 @@ namespace AWSCore void AWSScriptBehaviorS3::ReflectBehaviors(AZ::BehaviorContext* behaviorContext) { - behaviorContext->Class("AWSScriptBehaviorS3") + behaviorContext->Class(AWSScriptBehaviorS3Name) ->Attribute(AZ::Script::Attributes::Category, "AWSCore") ->Method("GetObject", &AWSScriptBehaviorS3::GetObject, {{{"Bucket Resource KeyName", "The resource key name of the bucket in resource mapping config file."}, @@ -86,7 +86,9 @@ namespace AWSCore void AWSScriptBehaviorS3::GetObjectRaw( const AZStd::string& bucket, const AZStd::string& objectKey, const AZStd::string& region, const AZStd::string& outFile) { - if (!ValidateGetObjectRequest(&AWSScriptBehaviorS3NotificationBus::Events::OnGetObjectError, bucket, objectKey, region, outFile)) + AZStd::string normalizedOutFile = outFile; + if (!ValidateGetObjectRequest( + &AWSScriptBehaviorS3NotificationBus::Events::OnGetObjectError, bucket, objectKey, region, normalizedOutFile)) { return; } @@ -112,10 +114,10 @@ namespace AWSCore job->request.SetBucket(Aws::String(bucket.c_str())); job->request.SetKey(Aws::String(objectKey.c_str())); - Aws::String outFileName(outFile.c_str()); + Aws::String outFileName(normalizedOutFile.c_str()); job->request.SetResponseStreamFactory([outFileName]() { return Aws::New( - "AWSScriptBehaviorS3", outFileName.c_str(), + AWSScriptBehaviorS3Name, outFileName.c_str(), std::ios_base::out | std::ios_base::in | std::ios_base::binary | std::ios_base::trunc); }); job->Start(); @@ -163,20 +165,44 @@ namespace AWSCore } bool AWSScriptBehaviorS3::ValidateGetObjectRequest(S3NotificationFunctionType notificationFunc, - const AZStd::string& bucket, const AZStd::string& objectKey, const AZStd::string& region, const AZStd::string& outFile) + const AZStd::string& bucket, const AZStd::string& objectKey, const AZStd::string& region, AZStd::string& outFile) { if (ValidateHeadObjectRequest(notificationFunc, bucket, objectKey, region)) { - if (!AzFramework::StringFunc::Path::IsValid(outFile.c_str())) + AzFramework::StringFunc::Path::Normalize(outFile); + if (outFile.empty()) { - AZ_Warning("AWSScriptBehaviorS3", false, "Request validation failed, outfile is not valid."); - AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, "Request validation failed, outfile is not valid."); + AZ_Warning(AWSScriptBehaviorS3Name, false, OutputFileIsEmptyErrorMessage); + AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, OutputFileIsEmptyErrorMessage); return false; } + if (!AzFramework::StringFunc::Path::HasDrive(outFile.c_str())) + { + AZ_Warning(AWSScriptBehaviorS3Name, false, OutputFileMissFullPathErrorMessage); + AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, OutputFileMissFullPathErrorMessage); + return false; + } + if (AZ::IO::FileIOBase::GetInstance()->IsDirectory(outFile.c_str())) + { + AZ_Warning(AWSScriptBehaviorS3Name, false, OutputFileIsDirectoryErrorMessage); + AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, OutputFileIsDirectoryErrorMessage); + return false; + } + auto lastSeparator = outFile.find_last_of(AZ_CORRECT_FILESYSTEM_SEPARATOR); + if (lastSeparator != AZStd::string::npos) + { + auto parentPath = outFile.substr(0, lastSeparator); + if (!AZ::IO::FileIOBase::GetInstance()->Exists(parentPath.c_str())) + { + AZ_Warning(AWSScriptBehaviorS3Name, false, OutputFileDirectoryNotExistErrorMessage); + AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, OutputFileDirectoryNotExistErrorMessage); + return false; + } + } if (AZ::IO::FileIOBase::GetInstance()->IsReadOnly(outFile.c_str())) { - AZ_Warning("AWSScriptBehaviorS3", false, "Request validation failed, outfile is read-only."); - AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, "Request validation failed, outfile is read-only."); + AZ_Warning(AWSScriptBehaviorS3Name, false, OutputFileIsReadOnlyErrorMessage); + AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, OutputFileIsReadOnlyErrorMessage); return false; } return true; @@ -189,20 +215,20 @@ namespace AWSCore { if (bucket.empty()) { - AZ_Warning("AWSScriptBehaviorS3", false, "Request validation failed, bucket name is required."); - AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, "Request validation failed, bucket name is required."); + AZ_Warning(AWSScriptBehaviorS3Name, false, BucketNameIsEmptyErrorMessage); + AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, BucketNameIsEmptyErrorMessage); return false; } if (objectKey.empty()) { - AZ_Warning("AWSScriptBehaviorS3", false, "Request validation failed, object key name is required."); - AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, "Request validation failed, object key name is required."); + AZ_Warning(AWSScriptBehaviorS3Name, false, ObjectKeyNameIsEmptyErrorMessage); + AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, ObjectKeyNameIsEmptyErrorMessage); return false; } if (region.empty()) { - AZ_Warning("AWSScriptBehaviorS3", false, "Request validation failed, region name is required."); - AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, "Request validation failed, region name is required."); + AZ_Warning(AWSScriptBehaviorS3Name, false, RegionNameIsEmptyErrorMessage); + AWSScriptBehaviorS3NotificationBus::Broadcast(notificationFunc, RegionNameIsEmptyErrorMessage); return false; } return true; diff --git a/Gems/AWSCore/Code/Tests/ScriptCanvas/AWSScriptBehaviorS3Test.cpp b/Gems/AWSCore/Code/Tests/ScriptCanvas/AWSScriptBehaviorS3Test.cpp index fe258580e0..3489f09ac3 100644 --- a/Gems/AWSCore/Code/Tests/ScriptCanvas/AWSScriptBehaviorS3Test.cpp +++ b/Gems/AWSCore/Code/Tests/ScriptCanvas/AWSScriptBehaviorS3Test.cpp @@ -11,6 +11,7 @@ */ #include +#include #include #include @@ -38,7 +39,37 @@ public: MOCK_METHOD1(OnGetObjectError, void(const AZStd::string&)); }; -using AWSScriptBehaviorS3Test = UnitTest::ScopedAllocatorSetupFixture; +class AWSScriptBehaviorS3Test + : public AWSCoreFixture +{ +public: + void CreateReadOnlyTestFile(const AZStd::string& filePath) + { + AZ::IO::SystemFile file; + if (!file.Open( + filePath.c_str(), + AZ::IO::SystemFile::OpenMode::SF_OPEN_CREATE | AZ::IO::SystemFile::SF_OPEN_CREATE_PATH | AZ::IO::SystemFile::SF_OPEN_WRITE_ONLY)) + { + AZ_Assert(false, "Failed to open test file at %s", filePath.c_str()); + } + AZStd::string testContent = "It is a test file"; + if (file.Write(testContent.c_str(), testContent.size()) != testContent.size()) + { + AZ_Assert(false, "Failed to write test file with content %s", testContent.c_str()); + } + file.Close(); + AZ_Assert(AZ::IO::SystemFile::SetWritable(filePath.c_str(), false), "Failed to mark test file as read-only"); + } + + void RemoveReadOnlyTestFile(const AZStd::string& filePath) + { + if (!filePath.empty()) + { + AZ_Assert(AZ::IO::SystemFile::SetWritable(filePath.c_str(), true), "Failed to mark test file as writeable"); + AZ_Assert(AZ::IO::SystemFile::Delete(filePath.c_str()), "Failed to delete test config file at %s", filePath.c_str()); + } + } +}; TEST_F(AWSScriptBehaviorS3Test, HeadObjectRaw_CallWithEmptyBucketName_InvokeOnError) { @@ -96,6 +127,40 @@ TEST_F(AWSScriptBehaviorS3Test, GetObjectRaw_CallWithEmptyOutfileName_InvokeOnEr AWSScriptBehaviorS3::GetObjectRaw("dummyBucket", "dummyObject", "dummyRegion", ""); } +TEST_F(AWSScriptBehaviorS3Test, GetObjectRaw_CallWithOutfileNameMissFullPath_InvokeOnError) +{ + AWSScriptBehaviorS3NotificationBusHandlerMock s3HandlerMock; + EXPECT_CALL(s3HandlerMock, OnGetObjectError(::testing::_)).Times(1); + AWSScriptBehaviorS3::GetObjectRaw("dummyBucket", "dummyObject", "dummyRegion", "dummyOut.txt"); +} + +TEST_F(AWSScriptBehaviorS3Test, GetObjectRaw_CallWithOutfileNameIsDirectory_InvokeOnError) +{ + AWSScriptBehaviorS3NotificationBusHandlerMock s3HandlerMock; + EXPECT_CALL(s3HandlerMock, OnGetObjectError(::testing::_)).Times(1); + AWSScriptBehaviorS3::GetObjectRaw("dummyBucket", "dummyObject", "dummyRegion", AZ::Test::GetCurrentExecutablePath()); +} + +TEST_F(AWSScriptBehaviorS3Test, GetObjectRaw_CallWithOutfileDirectoryNoExist_InvokeOnError) +{ + AWSScriptBehaviorS3NotificationBusHandlerMock s3HandlerMock; + EXPECT_CALL(s3HandlerMock, OnGetObjectError(::testing::_)).Times(1); + AZStd::string dummyDirectory = AZStd::string::format("%s/dummyDirectory/dummyOut.txt", AZ::Test::GetCurrentExecutablePath().c_str()); + AWSScriptBehaviorS3::GetObjectRaw("dummyBucket", "dummyObject", "dummyRegion", dummyDirectory); +} + +TEST_F(AWSScriptBehaviorS3Test, GetObjectRaw_CallWithOutfileIsReadOnly_InvokeOnError) +{ + AWSScriptBehaviorS3NotificationBusHandlerMock s3HandlerMock; + EXPECT_CALL(s3HandlerMock, OnGetObjectError(::testing::_)).Times(1); + AZStd::string randomTestFile = AZStd::string::format("%s/test%s.txt", + AZ::Test::GetCurrentExecutablePath().c_str(), AZ::Uuid::CreateRandom().ToString(false, false).c_str()); + AzFramework::StringFunc::Path::Normalize(randomTestFile); + CreateReadOnlyTestFile(randomTestFile); + AWSScriptBehaviorS3::GetObjectRaw("dummyBucket", "dummyObject", "dummyRegion", randomTestFile); + RemoveReadOnlyTestFile(randomTestFile); +} + TEST_F(AWSScriptBehaviorS3Test, GetObject_NoBucketNameInResourceMappingFound_InvokeOnError) { AWSScriptBehaviorS3NotificationBusHandlerMock s3HandlerMock;