Harden OfflineDatabase

CMakeLists.txt

@@ -176,8 +176,10 @@ if(WITH_NODEJS AND COMMAND mbgl_platform_node)
 endif()
 
 if(CMAKE_GENERATOR STREQUAL "Xcode")
-    write_xcconfig_target_properties(
-        mbgl-core
-        mbgl-filesource
+    set_xcconfig_target_properties(mbgl-core)
+    set_xcconfig_target_properties(mbgl-filesource)
+    file(GENERATE
+        OUTPUT "${CMAKE_BINARY_DIR}/config.xcconfig"
+        INPUT "${CMAKE_SOURCE_DIR}/scripts/config.xcconfig.in"
     )
 endif()

bin/offline.cpp

@@ -136,9 +136,9 @@ int main(int argc, char *argv[]) {
 
     std::signal(SIGINT, [] (int) { stop(); });
 
-    fileSource.createOfflineRegion(definition, metadata, [&] (std::exception_ptr error, optional<OfflineRegion> region_) {
-        if (error) {
-            std::cerr << "Error creating region: " << util::toString(error) << std::endl;
+    fileSource.createOfflineRegion(definition, metadata, [&] (mbgl::expected<OfflineRegion, std::exception_ptr> region_) {
+        if (!region_) {
+            std::cerr << "Error creating region: " << util::toString(region_.error()) << std::endl;
             loop.stop();
             exit(1);
         } else {

cmake/core-files.cmake

@@ -664,6 +664,7 @@ set(MBGL_CORE_FILES
     include/mbgl/util/enum.hpp
     include/mbgl/util/event.hpp
     include/mbgl/util/exception.hpp
+    include/mbgl/util/expected.hpp
     include/mbgl/util/feature.hpp
     include/mbgl/util/font_stack.hpp
     include/mbgl/util/geo.hpp

cmake/filesource.cmake

@@ -1,3 +1,5 @@
+add_vendor_target(expected INTERFACE)
+
 add_library(mbgl-filesource STATIC
     # File source
     include/mbgl/storage/default_file_source.hpp
@@ -39,6 +41,7 @@ target_include_directories(mbgl-filesource
 
 target_link_libraries(mbgl-filesource
     PUBLIC mbgl-core
+    PUBLIC expected
 )
 
 mbgl_filesource()

cmake/mbgl.cmake

@@ -64,9 +64,14 @@ if(WITH_NODEJS)
     endfunction()
 
     # Run submodule update
+    set(MBGL_SUBMODULES mapbox-gl-js)
+    if (MBGL_PLATFORM STREQUAL "ios")
+        list(APPEND MBGL_SUBMODULES platform/ios/vendor/mapbox-events-ios)
+    endif()
+
     message(STATUS "Updating submodules...")
     execute_process(
-        COMMAND git submodule update --init mapbox-gl-js
+        COMMAND git submodule update --init ${MBGL_SUBMODULES}
         WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}")
 
     if(NOT EXISTS "${CMAKE_SOURCE_DIR}/mapbox-gl-js/node_modules")
@@ -80,7 +85,7 @@ if(WITH_NODEJS)
     # Add target for running submodule update during builds
     add_custom_target(
         update-submodules ALL
-        COMMAND git submodule update --init mapbox-gl-js
+        COMMAND git submodule update --init ${MBGL_SUBMODULES}
         WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}"
         COMMENT "Updating submodules..."
     )
@@ -115,13 +120,15 @@ endfunction()
 
 # Creates a library target for a vendored dependency
 function(add_vendor_target NAME TYPE)
-    add_library(${NAME} ${TYPE} "${CMAKE_CURRENT_SOURCE_DIR}/cmake/empty.cpp")
     set(INCLUDE_TYPE "INTERFACE")
     set(SOURCE_TYPE "INTERFACE")
     if (TYPE STREQUAL "STATIC" OR TYPE STREQUAL "SHARED")
+        add_library(${NAME} ${TYPE} "${CMAKE_CURRENT_SOURCE_DIR}/cmake/empty.cpp")
         set(INCLUDE_TYPE "PUBLIC")
         set(SOURCE_TYPE "PRIVATE")
         set_target_properties(${NAME} PROPERTIES SOURCES "")
+    else()
+        add_library(${NAME} ${TYPE})
     endif()
     set_target_properties(${NAME} PROPERTIES INTERFACE_SOURCES "")
     file(STRINGS "${CMAKE_CURRENT_SOURCE_DIR}/vendor/${NAME}/files.txt" FILES)
@@ -137,48 +144,25 @@ macro(set_xcode_property TARGET XCODE_PROPERTY XCODE_VALUE)
     set_property(TARGET ${TARGET} PROPERTY XCODE_ATTRIBUTE_${XCODE_PROPERTY} ${XCODE_VALUE})
 endmacro (set_xcode_property)
 
-function(_get_xcconfig_property target var)
-    get_property(result TARGET ${target} PROPERTY INTERFACE_${var} SET)
-    if (result)
-        get_property(result TARGET ${target} PROPERTY INTERFACE_${var})
-        if (var STREQUAL "LINK_LIBRARIES")
-            # Remove target names from the list of linker flags, since Xcode can't deal with them.
-            set(link_flags)
-            foreach(item IN LISTS result)
-                if (NOT TARGET ${item})
-                    list(APPEND link_flags ${item})
-                endif()
-            endforeach()
-            set(result "${link_flags}")
+function(set_xcconfig_target_properties target)
+    # Create a list of linked libraries for use in the xcconfig generation script.
+    get_property(result TARGET ${target} PROPERTY INTERFACE_LINK_LIBRARIES)
+    string(GENEX_STRIP "${result}" result)
+    # Remove target names from the list of linker flags, since Xcode can't deal with them.
+    set(link_flags)
+    foreach(item IN LISTS result)
+        if (NOT TARGET ${item})
+            list(APPEND link_flags ${item})
         endif()
-        string(REPLACE ";-framework " ";-framework;" result "${result}")
-        string(REPLACE ";" "\" \"" result "${result}")
-        string(REPLACE "-" "_" target "${target}")
-        set(${target}_${var} "${result}" PARENT_SCOPE)
-    endif()
-endfunction()
-
-if(MBGL_PLATFORM STREQUAL "ios")
-    execute_process(
-        COMMAND git submodule update --init platform/ios/vendor/mapbox-events-ios
-        WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}")
-endif()
-
-function(write_xcconfig_target_properties)
-    foreach(target ${ARGN})
-        _get_xcconfig_property(${target} INCLUDE_DIRECTORIES)
-        _get_xcconfig_property(${target} LINK_LIBRARIES)
     endforeach()
-    configure_file(
-        "${CMAKE_SOURCE_DIR}/scripts/config.xcconfig.in"
-        "${CMAKE_BINARY_DIR}/config.xcconfig"
-        @ONLY
-    )
+    string(REPLACE ";-framework " ";-framework;" link_flags "${link_flags}")
+    string(REPLACE ";" "\" \"" link_flags "${link_flags}")
+    set_xcode_property(${target} XCCONFIG_LINK_LIBRARIES "${link_flags}")
 endfunction()
 
 # Set Xcode project build settings to be consistent with the CXX flags we're
 # using. (Otherwise, Xcode's defaults may override some of these.)
-macro(initialize_xcode_cxx_build_settings target)
+function(initialize_xcode_cxx_build_settings target)
     # -Wall
     set_xcode_property(${target} GCC_WARN_SIGN_COMPARE YES)
     set_xcode_property(${target} GCC_WARN_UNINITIALIZED_AUTOS YES)
@@ -206,7 +190,7 @@ macro(initialize_xcode_cxx_build_settings target)
 
     # -flto
     set_xcode_property(${target} LLVM_LTO $<$<OR:$<CONFIG:Release>,$<CONFIG:RelWithDebugInfo>>:YES>)
-endmacro(initialize_xcode_cxx_build_settings)
+endfunction()
 
 # CMake 3.1 does not have this yet.
 set(CMAKE_CXX14_STANDARD_COMPILE_OPTION "-std=c++14")

cmake/test-files.cmake

@@ -95,6 +95,8 @@ set(MBGL_TEST_FILES
     test/src/mbgl/test/fixture_log_observer.hpp
     test/src/mbgl/test/getrss.cpp
     test/src/mbgl/test/getrss.hpp
+    test/src/mbgl/test/sqlite3_test_fs.cpp
+    test/src/mbgl/test/sqlite3_test_fs.hpp
     test/src/mbgl/test/stub_file_source.cpp
     test/src/mbgl/test/stub_file_source.hpp
     test/src/mbgl/test/stub_geometry_tile_feature.hpp

include/mbgl/storage/default_file_source.hpp

@@ -5,6 +5,7 @@
 #include <mbgl/storage/offline.hpp>
 #include <mbgl/util/constants.hpp>
 #include <mbgl/util/optional.hpp>
+#include <mbgl/util/expected.hpp>
 
 #include <vector>
 #include <mutex>
@@ -55,8 +56,7 @@ class DefaultFileSource : public FileSource {
      * callback, which will be executed on the database thread; it is the responsibility
      * of the SDK bindings to re-execute a user-provided callback on the main thread.
      */
-    void listOfflineRegions(std::function<void (std::exception_ptr,
-                                                optional<std::vector<OfflineRegion>>)>);
+    void listOfflineRegions(std::function<void (expected<OfflineRegions, std::exception_ptr>)>);
 
     /*
      * Create an offline region in the database.
@@ -71,16 +71,14 @@ class DefaultFileSource : public FileSource {
      */
     void createOfflineRegion(const OfflineRegionDefinition& definition,
                              const OfflineRegionMetadata& metadata,
-                             std::function<void (std::exception_ptr,
-                                                 optional<OfflineRegion>)>);
+                             std::function<void (expected<OfflineRegion, std::exception_ptr>)>);
 
     /*
      * Update an offline region metadata in the database.
      */
     void updateOfflineMetadata(const int64_t regionID,
                                const OfflineRegionMetadata& metadata,
-                               std::function<void (std::exception_ptr,
-                                                   optional<OfflineRegionMetadata>)>);
+                               std::function<void (expected<OfflineRegionMetadata, std::exception_ptr>)>);
     /*
      * Register an observer to be notified when the state of the region changes.
      */
@@ -97,8 +95,9 @@ class DefaultFileSource : public FileSource {
      * executed on the database thread; it is the responsibility of the SDK bindings
      * to re-execute a user-provided callback on the main thread.
      */
-    void getOfflineRegionStatus(OfflineRegion&, std::function<void (std::exception_ptr,
-                                                                    optional<OfflineRegionStatus>)>) const;
+    void getOfflineRegionStatus(
+        OfflineRegion&,
+        std::function<void (expected<OfflineRegionStatus, std::exception_ptr>)>) const;
 
     /*
      * Remove an offline region from the database and perform any resources evictions

include/mbgl/storage/offline.hpp

@@ -187,11 +187,11 @@ class OfflineRegion {
 public:
     // Move-only; not publicly constructible.
     OfflineRegion(OfflineRegion&&);
-    OfflineRegion& operator=(OfflineRegion&&);
     ~OfflineRegion();
 
     OfflineRegion() = delete;
     OfflineRegion(const OfflineRegion&) = delete;
+    OfflineRegion& operator=(OfflineRegion&&) = delete;
     OfflineRegion& operator=(const OfflineRegion&) = delete;
 
     int64_t getID() const;
@@ -210,4 +210,6 @@ class OfflineRegion {
     const OfflineRegionMetadata metadata;
 };
 
+using OfflineRegions = std::vector<OfflineRegion>;
+
 } // namespace mbgl

include/mbgl/util/expected.hpp

@@ -0,0 +1,16 @@
+#pragma once
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wshadow"
+#include <expected.hpp>
+#pragma GCC diagnostic pop
+
+namespace mbgl {
+
+template <class T, class E>
+using expected = nonstd::expected<T, E>;
+
+template <class E>
+using unexpected = nonstd::unexpected_type<E>;
+
+} // namespace mbgl

platform/android/config.cmake

@@ -97,6 +97,7 @@ macro(mbgl_platform_core)
 
     target_link_libraries(mbgl-core
         PRIVATE nunicode
+        PUBLIC expected
         PUBLIC -llog
         PUBLIC -landroid
         PUBLIC -ljnigraphics

platform/android/src/offline/offline_manager.cpp

@@ -26,15 +26,18 @@ void OfflineManager::listOfflineRegions(jni::JNIEnv& env_, jni::Object<FileSourc
         //Keep a shared ptr to a global reference of the callback and file source so they are not GC'd in the meanwhile
         callback = std::shared_ptr<jni::jobject>(callback_.NewGlobalRef(env_).release()->Get(), GenericGlobalRefDeleter()),
         jFileSource = std::shared_ptr<jni::jobject>(jFileSource_.NewGlobalRef(env_).release()->Get(), GenericGlobalRefDeleter())
-    ](std::exception_ptr error, mbgl::optional<std::vector<mbgl::OfflineRegion>> regions) mutable {
+    ](mbgl::expected<mbgl::OfflineRegions, std::exception_ptr> regions) mutable {
 
         // Reattach, the callback comes from a different thread
         android::UniqueEnv env = android::AttachEnv();
 
-        if (error) {
-            OfflineManager::ListOfflineRegionsCallback::onError(*env, jni::Object<ListOfflineRegionsCallback>(*callback), error);
-        } else if (regions) {
-            OfflineManager::ListOfflineRegionsCallback::onList(*env, jni::Object<FileSource>(*jFileSource), jni::Object<ListOfflineRegionsCallback>(*callback), std::move(regions));
+        if (regions) {
+            OfflineManager::ListOfflineRegionsCallback::onList(
+                *env, jni::Object<FileSource>(*jFileSource),
+                jni::Object<ListOfflineRegionsCallback>(*callback), std::move(*regions));
+        } else {
+            OfflineManager::ListOfflineRegionsCallback::onError(
+                *env, jni::Object<ListOfflineRegionsCallback>(*callback), regions.error());
         }
     });
 }
@@ -59,19 +62,19 @@ void OfflineManager::createOfflineRegion(jni::JNIEnv& env_,
         //Keep a shared ptr to a global reference of the callback and file source so they are not GC'd in the meanwhile
         callback = std::shared_ptr<jni::jobject>(callback_.NewGlobalRef(env_).release()->Get(), GenericGlobalRefDeleter()),
         jFileSource = std::shared_ptr<jni::jobject>(jFileSource_.NewGlobalRef(env_).release()->Get(), GenericGlobalRefDeleter())
-    ](std::exception_ptr error, mbgl::optional<mbgl::OfflineRegion> region) mutable {
+    ](mbgl::expected<mbgl::OfflineRegion, std::exception_ptr> region) mutable {
 
         // Reattach, the callback comes from a different thread
         android::UniqueEnv env = android::AttachEnv();
 
-        if (error) {
-            OfflineManager::CreateOfflineRegionCallback::onError(*env, jni::Object<CreateOfflineRegionCallback>(*callback), error);
-        } else if (region) {
+        if (region) {
             OfflineManager::CreateOfflineRegionCallback::onCreate(
                 *env,
                 jni::Object<FileSource>(*jFileSource),
-                jni::Object<CreateOfflineRegionCallback>(*callback), std::move(region)
+                jni::Object<CreateOfflineRegionCallback>(*callback), std::move(*region)
             );
+        } else {
+            OfflineManager::CreateOfflineRegionCallback::onError(*env, jni::Object<CreateOfflineRegionCallback>(*callback), region.error());
         }
     });
 }

platform/android/src/offline/offline_region.cpp

@@ -107,14 +107,14 @@ void OfflineRegion::getOfflineRegionStatus(jni::JNIEnv& env_, jni::Object<Offlin
     fileSource.getOfflineRegionStatus(*region, [
         //Ensure the object is not gc'd in the meanwhile
         callback = std::shared_ptr<jni::jobject>(callback_.NewGlobalRef(env_).release()->Get(), GenericGlobalRefDeleter())
-    ](std::exception_ptr error, mbgl::optional<mbgl::OfflineRegionStatus> status) mutable {
+    ](mbgl::expected<mbgl::OfflineRegionStatus, std::exception_ptr> status) mutable {
         // Reattach, the callback comes from a different thread
         android::UniqueEnv env = android::AttachEnv();
 
-        if (error) {
-            OfflineRegionStatusCallback::onError(*env, jni::Object<OfflineRegionStatusCallback>(*callback), error);
-        } else if (status) {
-            OfflineRegionStatusCallback::onStatus(*env, jni::Object<OfflineRegionStatusCallback>(*callback), std::move(status));
+        if (status) {
+            OfflineRegionStatusCallback::onStatus(*env, jni::Object<OfflineRegionStatusCallback>(*callback), std::move(*status));
+        } else {
+            OfflineRegionStatusCallback::onError(*env, jni::Object<OfflineRegionStatusCallback>(*callback), status.error());
         }
     });
 }
@@ -144,14 +144,14 @@ void OfflineRegion::updateOfflineRegionMetadata(jni::JNIEnv& env_, jni::Array<jn
     fileSource.updateOfflineMetadata(region->getID(), metadata, [
         //Ensure the object is not gc'd in the meanwhile
         callback = std::shared_ptr<jni::jobject>(callback_.NewGlobalRef(env_).release()->Get(), GenericGlobalRefDeleter())
-    ](std::exception_ptr error, mbgl::optional<mbgl::OfflineRegionMetadata> data) mutable {
+    ](mbgl::expected<mbgl::OfflineRegionMetadata, std::exception_ptr> data) mutable {
         // Reattach, the callback comes from a different thread
         android::UniqueEnv env = android::AttachEnv();
 
-        if (error) {
-            OfflineRegionUpdateMetadataCallback::onError(*env, jni::Object<OfflineRegionUpdateMetadataCallback>(*callback), error);
-        } else if (data) {
-            OfflineRegionUpdateMetadataCallback::onUpdate(*env, jni::Object<OfflineRegionUpdateMetadataCallback>(*callback), std::move(data));
+        if (data) {
+            OfflineRegionUpdateMetadataCallback::onUpdate(*env, jni::Object<OfflineRegionUpdateMetadataCallback>(*callback), std::move(*data));
+        } else {
+            OfflineRegionUpdateMetadataCallback::onError(*env, jni::Object<OfflineRegionUpdateMetadataCallback>(*callback), data.error());
         }
     });
 }

platform/darwin/src/MGLOfflinePack.mm

@@ -141,7 +141,7 @@ - (void)requestProgress {
     mbgl::DefaultFileSource *mbglFileSource = [[MGLOfflineStorage sharedOfflineStorage] mbglFileSource];
 
     __weak MGLOfflinePack *weakSelf = self;
-    mbglFileSource->getOfflineRegionStatus(*_mbglOfflineRegion, [&, weakSelf](__unused std::exception_ptr exception, mbgl::optional<mbgl::OfflineRegionStatus> status) {
+    mbglFileSource->getOfflineRegionStatus(*_mbglOfflineRegion, [&, weakSelf](mbgl::expected<mbgl::OfflineRegionStatus, std::exception_ptr> status) {
         if (status) {
             mbgl::OfflineRegionStatus checkedStatus = *status;
             dispatch_async(dispatch_get_main_queue(), ^{