Skip to content
This repository has been archived by the owner on Jan 15, 2025. It is now read-only.

Commit

Permalink
Merge pull request #255 from ekinanp/fixes
Browse files Browse the repository at this point in the history 
(maint) Rename http_exceptions and specify error in the error msg
  • Loading branch information
@MikaelSmith
MikaelSmith authored Aug 25, 2017
2 parents b0c6985 + ea77072 commit a3010fc
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 58 deletions.
7 changes: 3 additions & 4 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ This project adheres to [Semantic Versioning](http://semver.org/).

## [1.1.2]

### Added
- Separated the possible failure modes for Leatherman.curl into three categories
### Fixed
- Separated the possible failure modes for Leatherman.curl into three categories (LTH-142)
* Curl setup errors
* File operation errors (e.g. writing to disk during file download)
* Server side errors (e.g. bad host)

### Changed
- Simplified the logic of Leatherman.curl's download_file method by abstracting out actions associated with the temporary file (creating, writing, removing) to a class that follows the RAII pattern.
- Simplified the logic of Leatherman.curl's download_file method by abstracting out actions associated with the temporary file (creating, writing, removing) to a class that follows the RAII pattern. (LTH-142)

## [1.1.1]

Expand Down
26 changes: 13 additions & 13 deletions curl/inc/leatherman/curl/client.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,15 +148,15 @@ namespace leatherman { namespace curl {
/**
* The exception for curl_easy_setopt errors.
*/
struct LEATHERMAN_CURL_EXPORT http_setup_exception : http_request_exception
struct LEATHERMAN_CURL_EXPORT http_curl_setup_exception : http_request_exception
{
/**
* Constructs an http_setup_exception.
* Constructs an http_curl_setup_exception.
* @param req The HTTP request that caused the exception.
* @param message The exception message.
* @param curl_opt The CURL option that failed.
*/
http_setup_exception(request req, CURLoption curl_opt, std::string const &message) :
http_curl_setup_exception(request req, CURLoption curl_opt, std::string const &message) :
http_request_exception(req, message),
_curl_opt(std::move(curl_opt))
{
Expand All @@ -179,15 +179,15 @@ namespace leatherman { namespace curl {
/**
* The exception for HTTP file download server-side errors.
*/
struct LEATHERMAN_CURL_EXPORT http_download_exception : http_request_exception
struct LEATHERMAN_CURL_EXPORT http_file_download_exception : http_request_exception
{
/**
* Constructs an http_download_exception.
* Constructs an http_file_download_exception.
* @param request The request that caused the exception
* @param file_path The file that was meant to be downloaded
* @param message The exception message.
*/
http_download_exception(request req, std::string file_path, std::string const &message) :
http_file_download_exception(request req, std::string file_path, std::string const &message) :
http_request_exception(req, message),
_file_path(std::move(file_path))
{
Expand All @@ -209,27 +209,27 @@ namespace leatherman { namespace curl {
/**
* The exception for HTTP file download file operation errors.
*/
struct LEATHERMAN_CURL_EXPORT http_file_exception : http_download_exception
struct LEATHERMAN_CURL_EXPORT http_file_operation_exception : http_file_download_exception
{
/**
* Constructs an http_file_exception.
* Constructs an http_file_operation_exception.
* @param request The request that caused the exception
* @param file_path The file that was meant to be downloaded
* @param message The exception message.
*/
http_file_exception(request req, std::string file_path, std::string const &message) : http_file_exception(req, file_path, "", message)
http_file_operation_exception(request req, std::string file_path, std::string const &message) : http_file_operation_exception(req, file_path, "", message)
{
}

/**
* Constructs an http_file_exception.
* Constructs an http_file_operation_exception.
* @param request The request that caused the exception
* @param file_path The file that was meant to be downloaded
* @param temp_path The path to the temporary file that wasn't successfully cleaned up.
* @param message The exception message.
*/
http_file_exception(request req, std::string file_path, std::string temp_path, std::string const &message) :
http_download_exception(req, file_path, message),
http_file_operation_exception(request req, std::string file_path, std::string temp_path, std::string const &message) :
http_file_download_exception(req, file_path, message),
_temp_path(std::move(temp_path))
{
}
Expand Down Expand Up @@ -376,7 +376,7 @@ namespace leatherman { namespace curl {
) {
auto result = curl_easy_setopt(_handle, option, param);
if (result != CURLE_OK) {
throw http_setup_exception(ctx.req, option, leatherman::locale::_("Failed setting up libcurl. Reason: {1}", curl_easy_strerror(result)));
throw http_curl_setup_exception(ctx.req, option, leatherman::locale::_("Failed setting up libcurl. Reason: {1}", curl_easy_strerror(result)));
}
}

Expand Down
16 changes: 10 additions & 6 deletions curl/src/client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ namespace leatherman { namespace curl {
}
}

static std::string make_file_err_msg(std::string const& reason) {
return _("File operation error: {1}", reason);
}

download_temp_file::download_temp_file(request const& req, std::string const& file_path, boost::optional<boost::filesystem::perms> perms) :
_req(req),
_file_path(file_path)
Expand All @@ -99,7 +103,7 @@ namespace leatherman { namespace curl {
_temp_path = fs::path(file_path).parent_path() / fs::unique_path("temp_file_%%%%-%%%%-%%%%-%%%%");
_fp = boost::nowide::fopen(_temp_path.string().c_str(), "wb");
if (!_fp) {
throw http_file_exception(_req, _file_path, _("Failed to open temporary file for writing"));
throw http_file_operation_exception(_req, _file_path, make_file_err_msg(_("failed to open temporary file for writing")));
}
if (!perms) {
return;
Expand All @@ -109,10 +113,10 @@ namespace leatherman { namespace curl {
fs::permissions(_temp_path.string(), *perms, ec);
if (ec) {
cleanup();
throw http_file_exception(_req, _file_path, _("Failed to modify permissions of temporary file"));
throw http_file_operation_exception(_req, _file_path, make_file_err_msg(_("failed to modify permissions of temporary file")));
}
} catch (fs::filesystem_error& e) {
throw http_file_exception(_req, _file_path, e.what());
throw http_file_operation_exception(_req, _file_path, make_file_err_msg(e.what()));
}
}

Expand All @@ -132,7 +136,7 @@ namespace leatherman { namespace curl {
fs::rename(_temp_path, _file_path, ec);
if (ec) {
LOG_WARNING("Failed to write the results of the temporary file to the actual file {1}", _file_path);
throw http_file_exception(_req, _file_path, _("Failed to move over the temporary file's downloaded contents"));
throw http_file_operation_exception(_req, _file_path, make_file_err_msg(_("failed to move over the temporary file's downloaded contents")));
}
}

Expand Down Expand Up @@ -251,9 +255,9 @@ namespace leatherman { namespace curl {
// Perform the request
auto result = curl_easy_perform(_handle);
if (result == CURLE_WRITE_ERROR) {
throw http_file_exception(req, file_path, _("Failed to write to the temporary file during download"));
throw http_file_operation_exception(req, file_path, make_file_err_msg(_("failed to write to the temporary file during download")));
} else if (result != CURLE_OK) {
throw http_download_exception(req, file_path, errbuf);
throw http_file_download_exception(req, file_path, _("File download server side error: {1}", errbuf));
}

temp_file.write();
Expand Down
62 changes: 31 additions & 31 deletions curl/tests/client_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,90 +307,90 @@ namespace leatherman { namespace curl {

SECTION("client fails to set HTTP method to POST") {
test_impl->test_failure_mode = curl_impl::error_mode::http_post_error;
REQUIRE_THROWS_AS(test_client.post(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.post(test_request), http_curl_setup_exception);
}

SECTION("client fails to set HTTP method to PUT") {
test_impl->test_failure_mode = curl_impl::error_mode::http_put_error;
REQUIRE_THROWS_AS(test_client.put(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.put(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the request URL") {
test_impl->test_failure_mode = curl_impl::error_mode::set_url_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the request headers") {
test_impl->test_failure_mode = curl_impl::error_mode::set_header_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set cookies in the request") {
test_impl->test_failure_mode = curl_impl::error_mode::set_cookie_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the header callback function") {
test_impl->test_failure_mode = curl_impl::error_mode::header_function_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the header write location") {
test_impl->test_failure_mode = curl_impl::error_mode::header_context_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the body writing callback function") {
test_impl->test_failure_mode = curl_impl::error_mode::write_body_function_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the body write location") {
test_impl->test_failure_mode = curl_impl::error_mode::write_body_context_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}
SECTION("client fails to set the read_body callback function") {
test_impl->test_failure_mode = curl_impl::error_mode::read_body_function_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the read_body data source") {
test_impl->test_failure_mode = curl_impl::error_mode::read_body_context_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the connection timeout") {
test_impl->test_failure_mode = curl_impl::error_mode::connect_timeout_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set the request timeout") {
test_impl->test_failure_mode = curl_impl::error_mode::request_timeout_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set certificate authority info") {
test_client.set_ca_cert("certfile");
test_impl->test_failure_mode = curl_impl::error_mode::ca_bundle_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set SSL cert info") {
test_client.set_client_cert("cert", "key");
test_impl->test_failure_mode = curl_impl::error_mode::ssl_cert_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to set SSL key info") {
test_client.set_client_cert("cert", "key");
test_impl->test_failure_mode = curl_impl::error_mode::ssl_key_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}

SECTION("client fails to make http call with https protocol only enabled") {
test_client.set_supported_protocols(CURLPROTO_HTTPS);
test_impl->test_failure_mode = curl_impl::error_mode::protocol_error;
REQUIRE_THROWS_AS(test_client.get(test_request), http_setup_exception);
REQUIRE_THROWS_AS(test_client.get(test_request), http_curl_setup_exception);
}
}

Expand Down Expand Up @@ -460,56 +460,56 @@ namespace leatherman { namespace curl {
CURL* const& handle = test_client.get_handle();
auto test_impl = reinterpret_cast<curl_impl* const>(handle);

SECTION("when fopen fails, an http_file_exception is thrown") {
SECTION("when fopen fails, an http_file_operation_exception is thrown") {
fs::path parent_dir = temp_dir_path / "parent";
std::string file_path = (parent_dir / "child").string();
curl::request req("");
REQUIRE_THROWS_AS_WITH(
test_client.download_file(req, file_path),
curl::http_file_exception,
Catch::Equals("Failed to open temporary file for writing"));
curl::http_file_operation_exception,
Catch::Equals("File operation error: failed to open temporary file for writing"));
}

SECTION("when curl_easy_setopt fails, an http_setup_exception is thrown and the temporary file is removed") {
SECTION("when curl_easy_setopt fails, an http_curl_setup_exception is thrown and the temporary file is removed") {
curl::request req("");
std::string file_path = (temp_dir_path / "file").string();
test_impl->test_failure_mode = curl_impl::error_mode::set_url_error;
REQUIRE_THROWS_AS(test_client.download_file(req, file_path), curl::http_setup_exception);
REQUIRE_THROWS_AS(test_client.download_file(req, file_path), curl::http_curl_setup_exception);
// Ensure that the temp file was removed
REQUIRE(fs::is_empty(temp_dir_path));
}

SECTION("when curl_easy_perform fails due to a CURLE_WRITE_ERROR, but the temporary file is removed, an http_file_exception is thrown") {
SECTION("when curl_easy_perform fails due to a CURLE_WRITE_ERROR, but the temporary file is removed, an http_file_operation_exception is thrown") {
std::string file_path = (temp_dir_path / "file").string();
curl::request req("");
test_impl->test_failure_mode = curl_impl::error_mode::easy_perform_write_error;
REQUIRE_THROWS_AS_WITH(
test_client.download_file(req, file_path),
curl::http_file_exception,
Catch::StartsWith("Failed to write to the temporary file during download"));
curl::http_file_operation_exception,
Catch::StartsWith("File operation error: failed to write to the temporary file during download"));
}

SECTION("when curl_easy_perform fails for reasons other than a CURLE_WRITE_ERROR, but the temporary file is removed, only the errbuf message is contained in the thrown http_download_exception") {
SECTION("when curl_easy_perform fails for reasons other than a CURLE_WRITE_ERROR, but the temporary file is removed, only the errbuf message is contained in the thrown http_file_download_exception") {
std::string file_path = (temp_dir_path / "file").string();
curl::request req("");
test_impl->test_failure_mode = curl_impl::error_mode::easy_perform_error;
REQUIRE_THROWS_AS_WITH(
test_client.download_file(req, file_path),
curl::http_download_exception,
Catch::Equals("easy perform failed"));
curl::http_file_download_exception,
Catch::Equals("File download server side error: easy perform failed"));

// Ensure that the temp file was removed
REQUIRE(fs::is_empty(temp_dir_path));
}

SECTION("when renaming the temporary file to the user-provided file path fails, an http_file_exception is thrown") {
SECTION("when renaming the temporary file to the user-provided file path fails, an http_file_operation_exception is thrown") {
std::string file_path = (temp_dir_path / "file").string();
curl::request req("https://remove_temp_file.com");
test_impl->trigger_external_failure = remove_temp_file;
REQUIRE_THROWS_AS_WITH(
test_client.download_file(req, file_path),
curl::http_file_exception,
Catch::StartsWith("Failed to move over the temporary file's downloaded contents"));
curl::http_file_operation_exception,
Catch::StartsWith("File operation error: failed to move over the temporary file's downloaded contents"));
}
}
}}
16 changes: 12 additions & 4 deletions locales/leatherman.pot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,15 @@ msgid "curl_easy_escape failed to escape string."
msgstr ""

#: curl/src/client.cc
msgid "Failed to open temporary file for writing"
msgid "File operation error: {1}"
msgstr ""

#: curl/src/client.cc
msgid "Failed to modify permissions of temporary file"
msgid "failed to open temporary file for writing"
msgstr ""

#: curl/src/client.cc
msgid "failed to modify permissions of temporary file"
msgstr ""

#. debug
Expand All @@ -45,7 +49,7 @@ msgid ""
msgstr ""

#: curl/src/client.cc
msgid "Failed to move over the temporary file's downloaded contents"
msgid "failed to move over the temporary file's downloaded contents"
msgstr ""

#: curl/src/client.cc
Expand All @@ -62,7 +66,11 @@ msgid "request completed (status {1})."
msgstr ""

#: curl/src/client.cc
msgid "Failed to write to the temporary file during download"
msgid "failed to write to the temporary file during download"
msgstr ""

#: curl/src/client.cc
msgid "File download server side error: {1}"
msgstr ""

#: curl/src/client.cc
Expand Down

0 comments on commit a3010fc

Please sign in to comment.