Fix Laurents comments

Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>

actions.md

@@ -223,6 +223,7 @@ These are internal requirements that are relatively opaque to the user and/or co
 - [x] Store state in an SQL database
   - [x] Migrate forward through SQL schemas (sqlstorage_test.cc)
     - [x] Automatically use latest SQL schema version when initializing database (sqlstorage_test.cc)
+  - [x] Migrate backward through SQL schemas (sqlstorage_test.cc)
   - [x] Reject invalid SQL databases (sqlstorage_test.cc)
   - [x] Migrate from the legacy filesystem storage (sqlstorage_test.cc, uptane_test.cc)
   - [x] Load and store primary keys in an SQL database (storage_common_test.cc)

config/sql/migration/migrate.15.sql

@@ -1,7 +1,7 @@
 -- Don't modify this! Create a new migration instead--see docs/schema-migrations.adoc
 BEGIN TRANSACTION;
 
-CREATE TABLE migrations(version_from INT PRIMARY KEY, migration TEXT NOT NULL);
+CREATE TABLE rollback_migrations(version_from INT PRIMARY KEY, migration TEXT NOT NULL);
 
 DELETE FROM version;
 INSERT INTO version VALUES(15);

config/sql/rollback/migrate.15.sql → config/sql/rollback/rollback.15.sql

@@ -1,7 +1,7 @@
 -- Don't modify this! Create a new migration instead--see docs/schema-migrations.adoc
 BEGIN TRANSACTION;
 
-DROP TABLE migrations;
+DROP TABLE rollback_migrations;
 
 DELETE FROM version;
 INSERT INTO version VALUES(14);

config/sql/schema.sql

@@ -21,4 +21,4 @@ INSERT INTO repo_types(rowid,repo,repo_string) VALUES(1,0,'images');
 INSERT INTO repo_types(rowid,repo,repo_string) VALUES(2,1,'director');
 CREATE TABLE installation_result(unique_mark INTEGER PRIMARY KEY CHECK (unique_mark = 0), id TEXT, result_code INTEGER NOT NULL DEFAULT 0, result_text TEXT);
 CREATE TABLE need_reboot(unique_mark INTEGER PRIMARY KEY CHECK (unique_mark = 0), flag INTEGER NOT NULL DEFAULT 0);
-CREATE TABLE migrations(version_from INT PRIMARY KEY, migration TEXT NOT NULL);
+CREATE TABLE rollback_migrations(version_from INT PRIMARY KEY, migration TEXT NOT NULL);

src/libaktualizr/storage/embed_schemas.py

@@ -58,16 +58,16 @@ def apend_migration(migration_path, header_file):
         apend_migration(os.path.join(migration_folder, migration_list[-1]), header_file)
         header_file.write("\"\n};\n")
 
-        header_file.write("extern const std::map<int, std::string> {}_schema_rollback_migrations = {{".format(prefix))
+        header_file.write("extern const std::vector<std::string> {}_schema_rollback_migrations = {{".format(prefix))
+        ver = int(rollback_migrations_list[0].split(".")[1])
+        for i in range(ver):
+            header_file.write("\"\",\n")
         for migration in rollback_migrations_list[:-1]:
-            version = int(migration.split(".")[1])
-            header_file.write("{%d, "%version)
             apend_migration(os.path.join(rollback_folder, migration), header_file)
-            header_file.write("\"},\n")
-        version = int( migration_list[-1].split(".")[1])
-        header_file.write("{%d, "%version)
-        apend_migration(os.path.join(rollback_folder, migration_list[-1]), header_file)
-        header_file.write("\"}\n};\n")
+            header_file.write("\",\n")
+        version = int(rollback_migrations_list[-1].split(".")[1])
+        apend_migration(os.path.join(rollback_folder, rollback_migrations_list[-1]), header_file)
+        header_file.write("\"\n};\n")
 
         current_schema = open(os.path.join(sql_folder, "schema.sql"), 'r').read()
         current_schema_escaped = escape_string(current_schema)

src/libaktualizr/storage/sqlstorage.h

@@ -10,7 +10,7 @@
 #include "sqlstorage_base.h"
 
 extern const std::vector<std::string> libaktualizr_schema_migrations;
-extern const std::map<int, std::string> libaktualizr_schema_rollback_migrations;
+extern const std::vector<std::string> libaktualizr_schema_rollback_migrations;
 extern const std::string libaktualizr_current_schema;
 extern const int libaktualizr_current_schema_version;
 

src/libaktualizr/storage/sqlstorage_base.cc

@@ -7,7 +7,7 @@ boost::filesystem::path SQLStorageBase::dbPath() const { return sqldb_path_; }
 
 SQLStorageBase::SQLStorageBase(boost::filesystem::path sqldb_path, bool readonly,
                                std::vector<std::string> schema_migrations,
-                               std::map<int, std::string> schema_rollback_migrations, std::string current_schema,
+                               std::vector<std::string> schema_rollback_migrations, std::string current_schema,
                                int current_schema_version)
     : sqldb_path_(std::move(sqldb_path)),
       readonly_(readonly),
@@ -66,17 +66,19 @@ std::string SQLStorageBase::getTableSchemaFromDb(const std::string& tablename) {
 bool SQLStorageBase::dbMigrateForward(int version_from) {
   SQLite3Guard db = dbConnection();
   for (int32_t k = version_from + 1; k <= current_schema_version_; k++) {
-    if (db.exec(schema_migrations_.at(static_cast<size_t>(k)), nullptr, nullptr) != SQLITE_OK) {
+    auto result_code = db.exec(schema_migrations_.at(static_cast<size_t>(k)), nullptr, nullptr);
+    if (result_code != SQLITE_OK) {
       LOG_ERROR << "Can't migrate db from version " << (k - 1) << " to version " << k << ": " << db.errmsg();
       return false;
     }
-    if (schema_rollback_migrations_.count(k) != 0) {
-      auto statement =
-          db.prepareStatement("INSERT INTO migrations VALUES (?,?);", k, schema_rollback_migrations_.at(k));
-      if (statement.step() != SQLITE_DONE) {
-        LOG_ERROR << "Can't insert rollback migration script: " << db.errmsg();
-        return false;
-      }
+    if (schema_rollback_migrations_.at(k).empty()) {
+      continue;
+    }
+    auto statement = db.prepareStatement("INSERT OR REPLACE INTO rollback_migrations VALUES (?,?);", k,
+                                         schema_rollback_migrations_.at(k));
+    if (statement.step() != SQLITE_DONE) {
+      LOG_ERROR << "Can't insert rollback migration script: " << db.errmsg();
+      return false;
     }
   }
   return true;
@@ -85,7 +87,7 @@ bool SQLStorageBase::dbMigrateForward(int version_from) {
 bool SQLStorageBase::dbMigrateBackward(int version_from) {
   SQLite3Guard db = dbConnection();
   for (int ver = version_from; ver > current_schema_version_; --ver) {
-    auto statement = db.prepareStatement("SELECT migration FROM migrations WHERE version_from=?;", ver);
+    auto statement = db.prepareStatement("SELECT migration FROM rollback_migrations WHERE version_from=?;", ver);
     if (statement.step() != SQLITE_ROW) {
       LOG_ERROR << "Can't extract migration script: " << db.errmsg();
       return false;
@@ -96,7 +98,7 @@ bool SQLStorageBase::dbMigrateBackward(int version_from) {
       LOG_ERROR << "Can't migrate db from version " << (ver) << " to version " << ver - 1 << ": " << db.errmsg();
       return false;
     }
-    if (db.exec(std::string("DELETE FROM migrations WHERE version_from=") + std::to_string(ver) + ";", nullptr,
+    if (db.exec(std::string("DELETE FROM rollback_migrations WHERE version_from=") + std::to_string(ver) + ";", nullptr,
                 nullptr) != SQLITE_OK) {
       LOG_ERROR << "Can't clear old migration script: " << db.errmsg();
       return false;

src/libaktualizr/storage/sqlstorage_base.h

@@ -11,7 +11,7 @@ enum class DbVersion : int32_t { kEmpty = -1, kInvalid = -2 };
 class SQLStorageBase {
  public:
   explicit SQLStorageBase(boost::filesystem::path sqldb_path, bool readonly, std::vector<std::string> schema_migrations,
-                          std::map<int, std::string> schema_rollback_migrations, std::string current_schema,
+                          std::vector<std::string> schema_rollback_migrations, std::string current_schema,
                           int current_schema_version);
   ~SQLStorageBase() = default;
   std::string getTableSchemaFromDb(const std::string& tablename);
@@ -26,7 +26,7 @@ class SQLStorageBase {
   boost::filesystem::path sqldb_path_;
   bool readonly_{false};
   const std::vector<std::string> schema_migrations_;
-  std::map<int, std::string> schema_rollback_migrations_;
+  std::vector<std::string> schema_rollback_migrations_;
   const std::string current_schema_;
   const int current_schema_version_;
 };

src/libaktualizr/storage/sqlstorage_test.cc

@@ -173,12 +173,13 @@ TEST(sqlstorage, migrate_back) {
     INSERT INTO version VALUES(" +
       std::to_string(static_cast<int>(ver)) + ");";
 
-  auto statement =
-      db.prepareStatement("insert into migrations VALUES (?,?);", static_cast<int>(ver) + 1, back_migration_script);
+  auto statement = db.prepareStatement("insert into rollback_migrations VALUES (?,?);", static_cast<int>(ver) + 1,
+                                       back_migration_script);
   statement.step();
 
-  EXPECT_EQ(static_cast<int>(storage.getVersion()), 16);
+  EXPECT_EQ(static_cast<int>(storage.getVersion()), static_cast<int>(ver) + 1);
   EXPECT_TRUE(storage.dbMigrate());
+  EXPECT_LT(static_cast<int>(storage.getVersion()), static_cast<int>(ver) + 1);
   EXPECT_TRUE(dbSchemaCheck(storage));
 }