From d350dc39e07540b041cbff7ca0872ba95db0f068 Mon Sep 17 00:00:00 2001 From: saleh-amazon Date: Wed, 16 Jul 2025 07:08:21 -0700 Subject: [PATCH] [addresses #2648] add Resource URL to HTTP response object in case of an error (#3427) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- platform/darwin/core/http_file_source.mm | 20 +++++++++++--------- test/storage/http_file_source.test.cpp | 14 ++++++++++---- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/platform/darwin/core/http_file_source.mm b/platform/darwin/core/http_file_source.mm index 62ced8e7d..86d1820af 100644 --- a/platform/darwin/core/http_file_source.mm +++ b/platform/darwin/core/http_file_source.mm @@ -305,6 +305,8 @@ std::unique_ptr HTTPFileSource::request(const Resource& resource, Response response; using Error = Response::Error; + std::string stringResourceURL = " - Resource URL: " + resource.url; + if (error) { [MLNNativeNetworkManager.sharedManager errorLog:@"Requesting: %@ failed with error: %@", req.URL, error.debugDescription]; @@ -316,7 +318,8 @@ std::unique_ptr HTTPFileSource::request(const Resource& resource, switch ([error code]) { case NSURLErrorBadServerResponse: // 5xx errors response.error = std::make_unique( - Error::Reason::Server, [[error localizedDescription] UTF8String]); + Error::Reason::Server, std::string{ [[error localizedDescription] UTF8String]} + stringResourceURL); + break; case NSURLErrorNetworkConnectionLost: @@ -329,12 +332,12 @@ std::unique_ptr HTTPFileSource::request(const Resource& resource, case NSURLErrorDataNotAllowed: case NSURLErrorTimedOut: response.error = std::make_unique( - Error::Reason::Connection, [[error localizedDescription] UTF8String]); + Error::Reason::Connection, [[error localizedDescription] UTF8String] + stringResourceURL); break; default: response.error = std::make_unique( - Error::Reason::Other, [[error localizedDescription] UTF8String]); + Error::Reason::Other, [[error localizedDescription] UTF8String] + stringResourceURL); break; } } else if ([res isKindOfClass:[NSHTTPURLResponse class]]) { @@ -372,7 +375,7 @@ std::unique_ptr HTTPFileSource::request(const Resource& resource, response.notModified = true; } else if (responseCode == 404) { response.error = - std::make_unique(Error::Reason::NotFound, "HTTP status code 404"); + std::make_unique(Error::Reason::NotFound, "HTTP status code 404" + stringResourceURL); } else if (responseCode == 429) { // Get the standard header std::optional retryAfter; @@ -388,22 +391,22 @@ std::unique_ptr HTTPFileSource::request(const Resource& resource, xRateLimitReset = std::string([xReset UTF8String]); } - response.error = std::make_unique(Error::Reason::RateLimit, "HTTP status code 429", http::parseRetryHeaders(retryAfter, xRateLimitReset)); + response.error = std::make_unique(Error::Reason::RateLimit, "HTTP status code 429" + stringResourceURL, http::parseRetryHeaders(retryAfter, xRateLimitReset)); } else if (responseCode >= 500 && responseCode < 600) { response.error = std::make_unique(Error::Reason::Server, std::string{ "HTTP status code " } + - std::to_string(responseCode)); + std::to_string(responseCode) + stringResourceURL); } else { response.error = std::make_unique(Error::Reason::Other, std::string{ "HTTP status code " } + - std::to_string(responseCode)); + std::to_string(responseCode) + stringResourceURL); } } else if ([url isFileURL]) { response.data = std::make_shared((const char *)[data bytes], [data length]); } else { // This should never happen. response.error = std::make_unique(Error::Reason::Other, - "Response class is not NSHTTPURLResponse"); + "Response class is not NSHTTPURLResponse" + stringResourceURL); } shared->notify(response); @@ -430,5 +433,4 @@ void HTTPFileSource::setClientOptions(ClientOptions options) { ClientOptions HTTPFileSource::getClientOptions() { return impl->getClientOptions(); } - } diff --git a/test/storage/http_file_source.test.cpp b/test/storage/http_file_source.test.cpp index e17a9489a..652609df8 100644 --- a/test/storage/http_file_source.test.cpp +++ b/test/storage/http_file_source.test.cpp @@ -62,10 +62,13 @@ TEST(HTTPFileSource, TEST_REQUIRES_SERVER(HTTP404)) { util::RunLoop loop; HTTPFileSource fs(ResourceOptions::Default(), ClientOptions()); - auto req = fs.request({Resource::Unknown, "http://127.0.0.1:3000/doesnotexist"}, [&](Response res) { + std::string stringResourceURL = "http://127.0.0.1:3000/doesnotexist"; + std::string resourceURLInterpolation = " - Resource URL: " + stringResourceURL; + + auto req = fs.request({Resource::Unknown, stringResourceURL}, [&](Response res) { ASSERT_NE(nullptr, res.error); EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason); - EXPECT_EQ("HTTP status code 404", res.error->message); + EXPECT_EQ("HTTP status code 404" + resourceURLInterpolation, res.error->message + resourceURLInterpolation); EXPECT_FALSE(bool(res.data)); EXPECT_FALSE(bool(res.expires)); EXPECT_FALSE(res.mustRevalidate); @@ -135,10 +138,13 @@ TEST(HTTPFileSource, TEST_REQUIRES_SERVER(HTTP500)) { util::RunLoop loop; HTTPFileSource fs(ResourceOptions::Default(), ClientOptions()); - auto req = fs.request({Resource::Unknown, "http://127.0.0.1:3000/permanent-error"}, [&](Response res) { + std::string stringResourceURL = "http://127.0.0.1:3000/permanent-error"; + std::string resourceURLInterpolation = " - Resource URL: " + stringResourceURL; + + auto req = fs.request({Resource::Unknown, stringResourceURL}, [&](Response res) { ASSERT_NE(nullptr, res.error); EXPECT_EQ(Response::Error::Reason::Server, res.error->reason); - EXPECT_EQ("HTTP status code 500", res.error->message); + EXPECT_EQ("HTTP status code 500" + resourceURLInterpolation, res.error->message + resourceURLInterpolation); EXPECT_FALSE(bool(res.data)); EXPECT_FALSE(bool(res.expires)); EXPECT_FALSE(res.mustRevalidate);