Merge lp:~jamesh/storage-framework/convert-exception-ptr into lp:storage-framework/devel

[James Henstridge]

Fix the report_error() methods of DownloadJob and UploadJob to correctly preserve the type of StorageException subclasses.

Make the report_error() methods on DownloadJob and UploadJob correctly preserve the exception type for StorageExceptions.

Previously subclasses of StorageException were being squashed down to the base type, losing additional data members on e.g. ResourceException.

I've also made StorageException's destructor protected, as discussed on IRC.

[unity-api-1-bot (unity-api-1-bot)]

PASSED: Continuous integration, rev:124
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/271/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1880
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1887
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1669
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1669/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1669
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1669/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1669
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1669/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1669
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1669/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1669
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1669/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1669
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1669/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/271/rebuild

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

This looks good!

One request: can you please also replace the existing comment in provider/Exceptions.h with the following:

// Note: Adding new exception types also requires updating the marshaling and
// unmarshaling code for exceptions in provider/internal/Handler.cpp and
// qt/internal/unmarshal_error.cpp, as well as the exception re-throwing
// code in provider/internal/utils.cpp.

It's nice to have my memory jogged when there are internal dependencies such as these :-)

review: Needs Fixing

[lp:~jamesh/storage-framework/convert-exception-ptr]

125. By James Henstridge on 2017-03-30

Add a note about source files that need to know about all provider
exceptions.

[unity-api-1-bot (unity-api-1-bot)]

PASSED: Continuous integration, rev:125
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/274/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1885
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1892
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1674
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1674/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1674
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1674/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1674
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1674/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1674
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1674/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1674
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1674/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1674
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1674/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/274/rebuild

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

Sweet!

review: Approve

=== modified file 'include/unity/storage/provider/Exceptions.h'
--- include/unity/storage/provider/Exceptions.h	2017-03-09 07:02:46 +0000
+++ include/unity/storage/provider/Exceptions.h	2017-03-30 02:53:50 +0000
@@ -29,8 +29,12 @@
 namespace provider
 {
 
-// Note: Adding new exception types also requires updating the marshaling and
-//       unmarshaling code for exceptions in the client and server APIs.
+// Note: Adding new exception types also requires updating the
+//       marshaling and unmarshaling code for exceptions in the client
+//       and server APIs.  In particular:
+//         - src/provider/internal/Handler.cpp
+//         - src/provider/internal/utils.cpp
+//         - src/qt/internal/unmarshal_error.cpp
 
 /**
 \brief Base exception class for all server-side exceptions.
@@ -39,8 +43,11 @@
 {
 public:
     StorageException(std::string const& exception_type, std::string const& error_message);
+
+protected:
     ~StorageException();
 
+public:
     virtual char const* what() const noexcept override;
 
     std::string type() const;

=== added file 'include/unity/storage/provider/internal/utils.h'
--- include/unity/storage/provider/internal/utils.h	1970-01-01 00:00:00 +0000
+++ include/unity/storage/provider/internal/utils.h	2017-03-30 02:53:50 +0000
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2017 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors: James Henstridge <[e-mail]>
+ */
+
+#pragma once
+
+#include <boost/exception_ptr.hpp>
+#include <exception>
+
+namespace unity
+{
+namespace storage
+{
+namespace provider
+{
+namespace internal
+{
+
+boost::exception_ptr convert_exception_ptr(std::exception_ptr ep);
+
+}
+}
+}
+}

=== modified file 'src/provider/CMakeLists.txt'
--- src/provider/CMakeLists.txt	2017-03-09 08:13:34 +0000
+++ src/provider/CMakeLists.txt	2017-03-30 02:53:50 +0000
@@ -34,6 +34,7 @@
   internal/TestServerImpl.cpp
   internal/UploadJobImpl.cpp
   internal/dbusmarshal.cpp
+  internal/utils.cpp
   ${CMAKE_SOURCE_DIR}/include/unity/storage/provider/internal/AccountData.h
   ${CMAKE_SOURCE_DIR}/include/unity/storage/provider/internal/DownloadJobImpl.h
   ${CMAKE_SOURCE_DIR}/include/unity/storage/provider/internal/FixedAccountData.h

=== modified file 'src/provider/internal/DownloadJobImpl.cpp'
--- src/provider/internal/DownloadJobImpl.cpp	2016-12-20 03:00:07 +0000
+++ src/provider/internal/DownloadJobImpl.cpp	2017-03-30 02:53:50 +0000
@@ -20,6 +20,7 @@
 #include <unity/storage/internal/safe_strerror.h>
 #include <unity/storage/provider/DownloadJob.h>
 #include <unity/storage/provider/Exceptions.h>
+#include <unity/storage/provider/internal/utils.h>
 
 #include <sys/socket.h>
 #include <sys/types.h>
@@ -133,19 +134,7 @@
 
     lock_guard<mutex> guard(completion_lock_);
     completed_ = true;
-    // Convert std::exception_ptr to boost::exception_ptr
-    try
-    {
-        std::rethrow_exception(p);
-    }
-    catch (StorageException const& e)
-    {
-        completion_promise_.set_exception(e);
-    }
-    catch (...)
-    {
-        completion_promise_.set_exception(boost::current_exception());
-    }
+    completion_promise_.set_exception(convert_exception_ptr(p));
 }
 
 boost::future<void> DownloadJobImpl::finish(DownloadJob& job)

=== modified file 'src/provider/internal/UploadJobImpl.cpp'
--- src/provider/internal/UploadJobImpl.cpp	2016-12-20 03:00:07 +0000
+++ src/provider/internal/UploadJobImpl.cpp	2017-03-30 02:53:50 +0000
@@ -21,6 +21,7 @@
 #include <unity/storage/provider/Exceptions.h>
 #include <unity/storage/provider/UploadJob.h>
 #include <unity/storage/provider/internal/MainLoopExecutor.h>
+#include <unity/storage/provider/internal/utils.h>
 
 #include <sys/socket.h>
 #include <sys/types.h>
@@ -123,19 +124,7 @@
 
     lock_guard<mutex> guard(completion_lock_);
     completed_ = true;
-    // Convert std::exception_ptr to boost::exception_ptr
-    try
-    {
-        rethrow_exception(p);
-    }
-    catch (StorageException const& e)
-    {
-        completion_promise_.set_exception(e);
-    }
-    catch (...)
-    {
-        completion_promise_.set_exception(boost::current_exception());
-    }
+    completion_promise_.set_exception(convert_exception_ptr(p));
 }
 
 boost::future<Item> UploadJobImpl::finish(UploadJob& job)

=== added file 'src/provider/internal/utils.cpp'
--- src/provider/internal/utils.cpp	1970-01-01 00:00:00 +0000
+++ src/provider/internal/utils.cpp	2017-03-30 02:53:50 +0000
@@ -0,0 +1,95 @@
+/*
+ * Copyright (C) 2017 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors: James Henstridge <[e-mail]>
+ */
+
+#include <unity/storage/provider/internal/utils.h>
+#include <unity/storage/provider/Exceptions.h>
+
+namespace unity
+{
+namespace storage
+{
+namespace provider
+{
+namespace internal
+{
+
+boost::exception_ptr convert_exception_ptr(std::exception_ptr ep)
+{
+    // Convert std::exception_ptr to boost::exception_ptr
+    try
+    {
+        std::rethrow_exception(ep);
+    }
+    catch (RemoteCommsException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (NotExistsException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (ExistsException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (ConflictException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (UnauthorizedException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (PermissionException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (QuotaException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (CancelledException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (LogicException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (InvalidArgumentException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (ResourceException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (UnknownException const& e)
+    {
+        return boost::copy_exception(e);
+    }
+    catch (...)
+    {
+        return boost::current_exception();
+    }
+}
+
+}
+}
+}
+}

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2017-03-14 07:15:06 +0000
+++ tests/CMakeLists.txt	2017-03-30 02:53:50 +0000
@@ -15,6 +15,7 @@
     provider-DBusPeerCache
     provider-ProviderInterface
     provider-Server
+    provider-utils
 )
 
 set(slow_test_dirs

=== added directory 'tests/provider-utils'
=== added file 'tests/provider-utils/CMakeLists.txt'
--- tests/provider-utils/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ tests/provider-utils/CMakeLists.txt	2017-03-30 02:53:50 +0000
@@ -0,0 +1,8 @@
+add_executable(provider-utils_test
+  utils_test.cpp
+)
+target_link_libraries(provider-utils_test
+  storage-framework-provider-static
+  gtest
+)
+add_test(provider-utils provider-utils_test)

=== added file 'tests/provider-utils/utils_test.cpp'
--- tests/provider-utils/utils_test.cpp	1970-01-01 00:00:00 +0000
+++ tests/provider-utils/utils_test.cpp	2017-03-30 02:53:50 +0000
@@ -0,0 +1,151 @@
+/*
+ * Copyright (C) 2017 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors: James Henstridge <[e-mail]>
+ */
+
+#include <unity/storage/provider/Exceptions.h>
+#include <unity/storage/provider/internal/utils.h>
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wctor-dtor-privacy"
+#include <gtest/gtest.h>
+#pragma GCC diagnostic pop
+
+#include <stdexcept>
+#include <string>
+
+using namespace unity::storage::provider;
+
+// Helper function to create exceptions, with specialisations for
+// types with irregular constructors.
+template <typename Exception>
+Exception make_exception(std::string const& message)
+{
+    return Exception(message);
+}
+
+template<>
+NotExistsException make_exception<NotExistsException>(std::string const& message)
+{
+    return NotExistsException(message, "key");
+}
+
+template<>
+ExistsException make_exception<ExistsException>(std::string const& message)
+{
+    return ExistsException(message, "item_id", "name");
+}
+
+template<>
+ResourceException make_exception<ResourceException>(std::string const& message)
+{
+    return ResourceException(message, 0);
+}
+
+template <typename Ex>
+class ConvertExceptionTest : public ::testing::Test {
+};
+
+using ExceptionTypes = ::testing::Types<
+    RemoteCommsException,
+    NotExistsException,
+    ExistsException,
+    ConflictException,
+    UnauthorizedException,
+    PermissionException,
+    QuotaException,
+    CancelledException,
+    LogicException,
+    InvalidArgumentException,
+    ResourceException,
+    UnknownException>;
+
+TYPED_TEST_CASE(ConvertExceptionTest, ExceptionTypes);
+
+TYPED_TEST(ConvertExceptionTest, storage_exception)
+{
+    using Exception = TypeParam;
+    std::string const message = "exception message";
+
+    std::exception_ptr sep = std::make_exception_ptr(
+        make_exception<Exception>(message));
+
+    boost::exception_ptr bep = internal::convert_exception_ptr(sep);
+    try
+    {
+        boost::rethrow_exception(bep);
+        FAIL();
+    }
+    catch (Exception const& e)
+    {
+        EXPECT_EQ(message, e.error_message());
+    }
+}
+
+class CustomException : public std::exception
+{
+public:
+    virtual char const* what() const noexcept override
+    {
+        return "custom message";
+    }
+};
+
+// Exceptions raised with boost::enable_current_exception() are preserved
+TEST(ConvertExceptionTest, enable_current_exception)
+{
+    std::exception_ptr sep = std::make_exception_ptr(
+        boost::enable_current_exception(CustomException()));
+
+    boost::exception_ptr bep = internal::convert_exception_ptr(sep);
+    try
+    {
+        boost::rethrow_exception(bep);
+        FAIL();
+    }
+    catch (CustomException const& e)
+    {
+        EXPECT_EQ("custom message", e.what());
+    }
+}
+
+TEST(ConvertExceptionTest, unknown_exception)
+{
+    std::exception_ptr sep = std::make_exception_ptr(CustomException());
+
+    boost::exception_ptr bep = internal::convert_exception_ptr(sep);
+    try
+    {
+        boost::rethrow_exception(bep);
+        FAIL();
+    }
+    catch (CustomException const& e)
+    {
+        // Boost can't convert exceptions that haven't been annotated
+        // with enable_current_exception().
+        FAIL();
+    }
+    catch (boost::unknown_exception const& e)
+    {
+        EXPECT_EQ("std::exception", std::string(e.what()));
+    }
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}