Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/aws-cpp-sdk-core/include/aws/core/http/URI.h
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ namespace Aws
{
m_pathSegments.push_back(segment);
}
m_pathHasTrailingSlash = (m_pathSegments.empty() || !s_preservePathSeparators) && (!segments.empty() && segments.back() == '/');
m_pathHasTrailingSlash = !segments.empty() && segments.back() == '/';
}

/**
Expand Down
6 changes: 5 additions & 1 deletion src/aws-cpp-sdk-core/source/http/URI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,11 @@ Aws::String URI::GetPath() const
path.append(segment);
}

if (m_pathSegments.empty() || m_pathHasTrailingSlash)
if (m_pathSegments.empty())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you change this?

if (m_pathSegments.empty() || m_pathHasTrailingSlash)
{
  path.push_back('/');
}

it is functionally the same as

if (m_pathSegments.empty())
{
  path.push_back('/');
}
else if (m_pathHasTrailingSlash)
{
  path.push_back('/');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right - this was a mistake while trying to change the code back. Updating now.

{
path.push_back('/');
}
else if (m_pathHasTrailingSlash)
{
path.push_back('/');
}
Expand Down
61 changes: 42 additions & 19 deletions tests/aws-cpp-sdk-s3-unit-tests/S3UnitTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -224,30 +224,53 @@ TEST_F(S3UnitTest, S3UriPathPreservationOn) {
//Turn on path preservation
Aws::Http::SetPreservePathSeparators(true);

auto putObjectRequest = PutObjectRequest()
.WithBucket("velvetunderground")
.WithKey("////stephanie////says////////////that////////she//wants///////to/know.txt");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think we want to remove this test case. Its still a valuable test case, i think we just want to add more test cases. additionally can you add a test case for .txt// to make sure that it works for many slashes and not just one

struct TestCase {
const char* bucket;
const char* key;
const char* expectedUri;
};

std::shared_ptr<IOStream> body = Aws::MakeShared<StringStream>(ALLOCATION_TAG,
"What country shall I say is calling From across the world?",
std::ios_base::in | std::ios_base::binary);
TestCase testCases[] = {
{
"velvetunderground",
"////stephanie////says////////////that////////she//wants///////to/know.txt",
"https://velvetunderground.s3.us-east-1.amazonaws.com/////stephanie////says////////////that////////she//wants///////to/know.txt"
},
{
"velvetunderground",
"////stephanie////says////////////that////////she//wants///////to/know.txt/",
"https://velvetunderground.s3.us-east-1.amazonaws.com/////stephanie////says////////////that////////she//wants///////to/know.txt/"
},
{
"velvetunderground",
"////stephanie////says////////////that////////she//wants///////to/know.txt//",
"https://velvetunderground.s3.us-east-1.amazonaws.com/////stephanie////says////////////that////////she//wants///////to/know.txt//"
}
};

putObjectRequest.SetBody(body);
for (const auto& testCase : testCases) {
auto putObjectRequest = PutObjectRequest()
.WithBucket(testCase.bucket)
.WithKey(testCase.key);

//We have to mock requset because it is used to create the return body, it actually isnt used.
auto mockRequest = Aws::MakeShared<Standard::StandardHttpRequest>(ALLOCATION_TAG, "mockuri", HttpMethod::HTTP_GET);
mockRequest->SetResponseStreamFactory([]() -> IOStream* {
return Aws::New<StringStream>(ALLOCATION_TAG, "response-string", std::ios_base::in | std::ios_base::binary);
});
auto mockResponse = Aws::MakeShared<Standard::StandardHttpResponse>(ALLOCATION_TAG, mockRequest);
mockResponse->SetResponseCode(HttpResponseCode::OK);
_mockHttpClient->AddResponseToReturn(mockResponse);
std::shared_ptr<IOStream> body = Aws::MakeShared<StringStream>(ALLOCATION_TAG,
"test content", std::ios_base::in | std::ios_base::binary);
putObjectRequest.SetBody(body);

const auto response = _s3Client->PutObject(putObjectRequest);
AWS_EXPECT_SUCCESS(response);
auto mockRequest = Aws::MakeShared<Standard::StandardHttpRequest>(ALLOCATION_TAG, "mockuri", HttpMethod::HTTP_GET);
mockRequest->SetResponseStreamFactory([]() -> IOStream* {
return Aws::New<StringStream>(ALLOCATION_TAG, "response-string", std::ios_base::in | std::ios_base::binary);
});
auto mockResponse = Aws::MakeShared<Standard::StandardHttpResponse>(ALLOCATION_TAG, mockRequest);
mockResponse->SetResponseCode(HttpResponseCode::OK);
_mockHttpClient->AddResponseToReturn(mockResponse);

const auto seenRequest = _mockHttpClient->GetMostRecentHttpRequest();
EXPECT_EQ("https://velvetunderground.s3.us-east-1.amazonaws.com/////stephanie////says////////////that////////she//wants///////to/know.txt", seenRequest.GetUri().GetURIString());
const auto response = _s3Client->PutObject(putObjectRequest);
AWS_EXPECT_SUCCESS(response);

const auto seenRequest = _mockHttpClient->GetMostRecentHttpRequest();
EXPECT_EQ(testCase.expectedUri, seenRequest.GetUri().GetURIString());
}
}

TEST_F(S3UnitTest, S3EmbeddedErrorTest) {
Expand Down
Loading