From 5529d2fd1259ce18f7ed85c97756fa01b95da2e2 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Mon, 10 Jul 2023 12:48:52 -0700 Subject: [PATCH] Don't duplicate error messages for SyncError (#6774) SystemError is used in two places: as a base class for SyncError, and for filesystem errors. Filesystem errors don't include the actual error message and need it appended, but sync errors do and appending the message from the error code just results in it being duplicated. --- CHANGELOG.md | 2 +- src/realm/exceptions.cpp | 5 +++++ src/realm/exceptions.hpp | 13 +++++++++---- test/object-store/sync/app.cpp | 13 +++++-------- test/test_util_file.cpp | 8 ++++---- test/util/unit_test.cpp | 2 +- 6 files changed, 25 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bf2e3de571..aaf44b78a01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* Sync errors included the error message twice ([PR #6774](https://github.com/realm/realm-core/pull/6774), since v13.16.0). ### Breaking changes * None. diff --git a/src/realm/exceptions.cpp b/src/realm/exceptions.cpp index 1bfec8b815d..57528c79dac 100644 --- a/src/realm/exceptions.cpp +++ b/src/realm/exceptions.cpp @@ -103,6 +103,11 @@ RuntimeError::RuntimeError(ErrorCodes::Error code, std::string_view msg) { REALM_ASSERT(ErrorCodes::error_categories(code).test(ErrorCategory::runtime_error)); } +RuntimeError::RuntimeError(Status&& status) + : Exception(std::move(status)) +{ + REALM_ASSERT(ErrorCodes::error_categories(to_status().code()).test(ErrorCategory::runtime_error)); +} RuntimeError::~RuntimeError() noexcept = default; InvalidArgument::InvalidArgument(std::string_view msg) diff --git a/src/realm/exceptions.hpp b/src/realm/exceptions.hpp index 7c0fc4a4137..f5dbeffa361 100644 --- a/src/realm/exceptions.hpp +++ b/src/realm/exceptions.hpp @@ -109,6 +109,7 @@ struct LogicError : Exception { struct RuntimeError : Exception { RuntimeError(ErrorCodes::Error code, std::string_view msg); + RuntimeError(Status&& status); ~RuntimeError() noexcept override; }; @@ -355,14 +356,12 @@ class FileAccessError : public RuntimeError { struct SystemError : RuntimeError { SystemError(std::error_code err, std::string_view msg) - : RuntimeError(ErrorCodes::SystemError, - util::format("%1 (SystemError %2: %3)", msg, err.value(), err.message())) + : RuntimeError(make_status(err, msg, false)) { - const_cast(to_status()).set_std_error_code(err); } SystemError(int err_no, std::string_view msg) - : SystemError(std::error_code(err_no, std::generic_category()), msg) + : RuntimeError(make_status(std::error_code(err_no, std::generic_category()), msg, true)) { } @@ -377,6 +376,12 @@ struct SystemError : RuntimeError { { return get_system_error().category(); } + +private: + static Status make_status(std::error_code err, std::string_view msg, bool msg_is_prefix) + { + return Status(err, msg_is_prefix ? util::format("%1: %2 (%3)", msg, err.message(), err.value()) : msg); + } }; namespace query_parser { diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 51c0495c58c..21dc8dc07fe 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -2470,7 +2470,7 @@ TEST_CASE("app: sync integration", "[sync][app][baas]") { auto partition = random_string(100); auto user1 = test_session.app()->current_user(); SyncTestFile r_config(user1, partition, schema); - // Overrride the default + // Override the default r_config.sync_config->error_handler = [&](std::shared_ptr, SyncError error) { if (error.get_system_error() == sync::make_error_code(realm::sync::ProtocolError::bad_authentication)) { util::format(std::cerr, "Websocket redirect test: User logged out\n"); @@ -2771,8 +2771,7 @@ TEST_CASE("app: sync integration", "[sync][app][baas]") { sync_error_handler_called.store(true); REQUIRE(error.get_system_error() == sync::make_error_code(realm::sync::ProtocolError::bad_authentication)); - REQUIRE_THAT(std::string(error.reason()), - Catch::Matchers::ContainsSubstring("Unable to refresh the user access token.")); + REQUIRE(error.reason() == "Unable to refresh the user access token."); }; auto r = Realm::get_shared_realm(config); timed_wait_for([&] { @@ -2872,8 +2871,7 @@ TEST_CASE("app: sync integration", "[sync][app][baas]") { sync_error_handler_called.store(true); REQUIRE(error.get_system_error() == sync::make_error_code(realm::sync::ProtocolError::bad_authentication)); - REQUIRE_THAT(std::string(error.reason()), - Catch::Matchers::ContainsSubstring("Unable to refresh the user access token.")); + REQUIRE(error.reason() == "Unable to refresh the user access token."); }; auto transport = static_cast(session.transport()); @@ -3103,9 +3101,8 @@ TEST_CASE("app: sync integration", "[sync][app][baas]") { auto error = wait_for_future(std::move(pf.future), std::chrono::minutes(5)).get(); REQUIRE(error.get_system_error() == make_error_code(sync::ProtocolError::limits_exceeded)); - REQUIRE_THAT(std::string(error.reason()), - Catch::Matchers::ContainsSubstring("Sync websocket closed because the server received a message " - "that was too large: read limited at 16777217 bytes")); + REQUIRE(error.reason() == "Sync websocket closed because the server received a message that was too large: " + "read limited at 16777217 bytes"); REQUIRE(error.is_client_reset_requested()); REQUIRE(error.server_requests_action == sync::ProtocolErrorInfo::Action::ClientReset); } diff --git a/test/test_util_file.cpp b/test/test_util_file.cpp index 054b8e1ca55..55e0a4643b5 100644 --- a/test/test_util_file.cpp +++ b/test/test_util_file.cpp @@ -309,12 +309,12 @@ TEST(Utils_File_SystemErrorMessage) std::error_code err = std::make_error_code(std::errc::too_many_files_open); std::string_view message = "my message"; #ifdef _WIN32 - std::string expected = util::format("%1 (SystemError %2: %3)", message, err.value(), "too many files open"); + const char* expected = "my message: too many files open (%1)"; #else - std::string expected = util::format("%1 (SystemError %2: %3)", message, err.value(), "Too many open files"); + const char* expected = "my message: Too many open files (%1)"; #endif - CHECK_THROW_CONTAINING_MESSAGE(throw SystemError(err, message), expected); - CHECK_THROW_CONTAINING_MESSAGE(throw SystemError(err.value(), message), expected); + CHECK_THROW_CONTAINING_MESSAGE(throw SystemError(err, message), message); + CHECK_THROW_CONTAINING_MESSAGE(throw SystemError(err.value(), message), util::format(expected, err.value())); } #endif diff --git a/test/util/unit_test.cpp b/test/util/unit_test.cpp index 71185c7808c..c30d8f49b6e 100644 --- a/test/util/unit_test.cpp +++ b/test/util/unit_test.cpp @@ -1034,7 +1034,7 @@ void SimpleReporter::fail(const TestContext& context, const char* file, long lin if (m_report_progress) { m_error_messages.push_back(msg); } - context.thread_context.report_logger.info(msg.c_str()); + context.thread_context.report_logger.info("%1", msg.c_str()); } void SimpleReporter::thread_end(const ThreadContext& context)