From c89a3e8a2f056217c374366eec2914a75973621e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hugo=20Beauz=C3=A9e-Luyssen?= <hugo@beauzee.fr>
Date: Tue, 20 May 2014 23:27:17 +0300
Subject: [PATCH] Rework object creation & insertion to cache to avoid invalid
 states.

Objects were always created and added to cache, regardless of their
status in DB
---
 src/Cache.h          | 37 +++++++++++++++++++++----------------
 src/File.cpp         | 18 +++++++++---------
 src/File.h           |  4 +++-
 src/Label.cpp        | 16 ++++++++--------
 src/Label.h          |  5 +++--
 src/MediaLibrary.cpp | 14 ++------------
 src/SqliteTools.h    | 12 ++++++++++++
 7 files changed, 58 insertions(+), 48 deletions(-)

diff --git a/src/Cache.h b/src/Cache.h
index 31822d97e..831f21378 100644
--- a/src/Cache.h
+++ b/src/Cache.h
@@ -1,6 +1,7 @@
 #ifndef CACHE_H
 #define CACHE_H
 
+#include <cassert>
 #include <memory>
 #include <mutex>
 #include <unordered_map>
@@ -86,22 +87,6 @@ class Cache
             return inst;
         }
 
-        /*
-         * Create a new instance of the cache class.
-         * This doesn't check for the existence of a similar entity already cached.
-         */
-        template <typename... Args>
-        static std::shared_ptr<IMPL> create( Args&&... args )
-        {
-            auto res = std::make_shared<IMPL>( std::forward<Args>( args )... );
-
-            typename CACHEPOLICY::KeyType cacheKey = CACHEPOLICY::key( res );
-
-            std::lock_guard<std::mutex> lock( Mutex );
-            Store[cacheKey] = res;
-            return res;
-        }
-
         static bool destroy( sqlite3* dbConnection, const typename CACHEPOLICY::KeyType& key )
         {
             std::lock_guard<std::mutex> lock( Mutex );
@@ -125,6 +110,26 @@ class Cache
             Store.clear();
         }
 
+    protected:
+        /*
+         * Create a new instance of the cache class.
+         */
+        template <typename... Args>
+        static unsigned int insert( sqlite3* dbConnection, std::shared_ptr<IMPL> self, const char* req, const Args&... args )
+        {
+            unsigned int pKey = SqliteTools::insert( dbConnection, req, args... );
+            if ( pKey == 0 )
+                return 0;
+            auto cacheKey = CACHEPOLICY::key( self );
+
+            std::lock_guard<std::mutex> lock( Mutex );
+            // We expect the cache column to be PRIMARY KEY / UNIQUE, so an insertion with
+            // a duplicated key should have been rejected by sqlite. This indicates an invalid state
+            assert( Store.find( cacheKey ) == Store.end() );
+            Store[cacheKey] = self;
+            return pKey;
+        }
+
     private:
         static std::unordered_map<typename CACHEPOLICY::KeyType, std::shared_ptr<IMPL> > Store;
         static std::mutex Mutex;
diff --git a/src/File.cpp b/src/File.cpp
index 915f7ac0c..0183b3a06 100644
--- a/src/File.cpp
+++ b/src/File.cpp
@@ -36,18 +36,18 @@ File::File( const std::string& mrl )
 {
 }
 
-bool File::insert( sqlite3* dbConnection )
+FilePtr File::create( sqlite3* dbConnection, const std::string& mrl )
 {
-    assert( m_dbConnection == NULL );
-    assert( m_id == 0 );
+    auto self = std::make_shared<File>( mrl );
     static const std::string req = "INSERT INTO " + policy::FileTable::Name +
             " VALUES(NULL, ?, ?, ?, ?, ?, ?)";
-    if ( SqliteTools::executeRequest( dbConnection, req.c_str(), (int)m_type, m_duration,
-        m_albumTrackId, m_playCount, m_showEpisodeId, m_mrl ) == false )
-        return false;
-    m_id = sqlite3_last_insert_rowid( dbConnection );
-    m_dbConnection = dbConnection;
-    return true;
+    auto pKey = _Cache::insert( dbConnection, self, req.c_str(), (int)self->m_type, self->m_duration,
+        self->m_albumTrackId, self->m_playCount, self->m_showEpisodeId, self->m_mrl );
+    if ( pKey == 0 )
+        return nullptr;
+    self->m_id = pKey;
+    self->m_dbConnection = dbConnection;
+    return self;
 }
 
 std::shared_ptr<IAlbumTrack> File::albumTrack()
diff --git a/src/File.h b/src/File.h
index fc50fc0ee..e0c7a97ff 100644
--- a/src/File.h
+++ b/src/File.h
@@ -30,6 +30,8 @@ struct FileCache
 
 class File : public IFile, public Cache<File, IFile, policy::FileTable, policy::FileCache>
 {
+    private:
+        typedef Cache<File, IFile, policy::FileTable, policy::FileCache> _Cache;
     public:
         enum Type
         {
@@ -47,7 +49,7 @@ class File : public IFile, public Cache<File, IFile, policy::FileTable, policy::
         File(sqlite3* dbConnection , sqlite3_stmt* stmt);
         File( const std::string& mrl );
 
-        bool insert(sqlite3* dbConnection);
+        static FilePtr create(sqlite3* dbConnection, const std::string& mrl );
         static bool createTable(sqlite3* connection);
 
         virtual unsigned int id() const;
diff --git a/src/Label.cpp b/src/Label.cpp
index 498eb13f0..4098045a2 100644
--- a/src/Label.cpp
+++ b/src/Label.cpp
@@ -49,16 +49,16 @@ std::vector<FilePtr>& Label::files()
     return *m_files;
 }
 
-bool Label::insert( sqlite3* dbConnection )
+LabelPtr Label::create( sqlite3* dbConnection, const std::string& name )
 {
-    assert( m_dbConnection == nullptr );
-    assert( m_id == 0 );
+    auto self = std::make_shared<Label>( name );
     const char* req = "INSERT INTO Label VALUES(NULL, ?)";
-    if ( SqliteTools::executeRequest( dbConnection, req, m_name ) == false )
-        return false;
-    m_dbConnection = dbConnection;
-    m_id = sqlite3_last_insert_rowid( dbConnection );
-    return true;
+    auto pKey = _Cache::insert( dbConnection, self, req, self->m_name );
+    if ( pKey == 0 )
+        return nullptr;
+    self->m_dbConnection = dbConnection;
+    self->m_id = pKey;
+    return self;
 }
 
 bool Label::createTable(sqlite3* dbConnection)
diff --git a/src/Label.h b/src/Label.h
index 7c60460f0..5e6519ffa 100644
--- a/src/Label.h
+++ b/src/Label.h
@@ -29,8 +29,9 @@ struct LabelCachePolicy
 
 class Label : public ILabel, public Cache<Label, ILabel, policy::LabelTable, policy::LabelCachePolicy>
 {
+    private:
+        typedef Cache<Label, ILabel, policy::LabelTable, policy::LabelCachePolicy> _Cache;
     public:
-
         Label(sqlite3* dbConnection, sqlite3_stmt* stmt);
         Label( const std::string& name );
 
@@ -38,8 +39,8 @@ class Label : public ILabel, public Cache<Label, ILabel, policy::LabelTable, pol
         virtual unsigned int id() const;
         virtual const std::string& name();
         virtual std::vector<FilePtr>& files();
-        bool insert( sqlite3* dbConnection );
 
+        static LabelPtr create(sqlite3* dbConnection, const std::string& name );
         static bool createTable( sqlite3* dbConnection );
     private:
         sqlite3* m_dbConnection;
diff --git a/src/MediaLibrary.cpp b/src/MediaLibrary.cpp
index dcfa9993c..fd549ca01 100644
--- a/src/MediaLibrary.cpp
+++ b/src/MediaLibrary.cpp
@@ -50,12 +50,7 @@ FilePtr MediaLibrary::file( const std::string& path )
 
 FilePtr MediaLibrary::addFile( const std::string& path )
 {
-    auto f = File::create( path );
-    if ( f->insert( m_dbConnection ) == false )
-    {
-        return nullptr;
-    }
-    return f;
+    return File::create( m_dbConnection, path );
 }
 
 bool MediaLibrary::deleteFile( const std::string& mrl )
@@ -70,12 +65,7 @@ bool MediaLibrary::deleteFile( FilePtr file )
 
 LabelPtr MediaLibrary::createLabel( const std::string& label )
 {
-    auto l = Label::create( label );
-    if ( l->insert( m_dbConnection ) == false )
-    {
-        return nullptr;
-    }
-    return l;
+    return Label::create( m_dbConnection, label );
 }
 
 bool MediaLibrary::deleteLabel( const std::string& text )
diff --git a/src/SqliteTools.h b/src/SqliteTools.h
index 1beccb96e..fdd7d1e0b 100644
--- a/src/SqliteTools.h
+++ b/src/SqliteTools.h
@@ -105,6 +105,18 @@ class SqliteTools
             return res == SQLITE_DONE;
         }
 
+        /**
+         * Inserts a record to the DB and return the newly created primary key.
+         * Returns 0 (which is an invalid sqlite primary key) when insertion fails.
+         */
+        template <typename... Args>
+        static unsigned int insert( sqlite3* dbConnection, const char* req, const Args&... args )
+        {
+            if ( executeRequest( dbConnection, req, args... ) == false )
+                return 0;
+            return sqlite3_last_insert_rowid( dbConnection );
+        }
+
     private:
         template <typename... Args>
         static StmtPtr prepareRequest( sqlite3* dbConnection, const char* req, const Args&... args )
-- 
GitLab