From 14c82ea8179afb38a867fc1dd22c54a56071abdd Mon Sep 17 00:00:00 2001
From: Martin Kleusberg <mkleusberg@gmail.com>
Date: Fri, 15 Feb 2019 17:05:53 +0100
Subject: [PATCH] Fix alterTable() function

Fix a few issues in the alterTable() code. One type of issue would
happen if there are any keys or constraints in the table. Because the
check whether more changes are needed did not work as expected, we would
try to edit the table again, even though it is already correct. The
second type of issue (which can be triggered independently but which can
also be a follow-up issue to the first one) tries to access the table by
its old name even though it might already have been renamed.

See issue #1725.
---
 src/sql/sqlitetypes.cpp | 27 +++++++++++++++++++++++++--
 src/sqlitedb.cpp        | 11 ++++++-----
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/src/sql/sqlitetypes.cpp b/src/sql/sqlitetypes.cpp
index 78ecbfee..af952418 100644
--- a/src/sql/sqlitetypes.cpp
+++ b/src/sql/sqlitetypes.cpp
@@ -360,13 +360,36 @@ bool Table::operator==(const Table& rhs) const
 
     if(m_rowidColumn != rhs.m_rowidColumn)
         return false;
-    if(m_constraints != rhs.m_constraints)
-        return false;
     if(m_virtual != rhs.m_virtual)
         return false;
     if(fields != rhs.fields)
         return false;
 
+    // We need to compare the constraint maps manually here. The reason is that the values are pointers and the default implementation
+    // would compare the pointers not the actual objects.
+    if(m_constraints.size() != rhs.m_constraints.size())
+        return false;
+    for(auto it=m_constraints.cbegin();it!=m_constraints.end();++it)
+    {
+        // For each element in this map we get the list of all elements with the same key from the other map.
+        // Then we loop through that list and check if we find an element of the same type which produces the same SQL substring. We use this
+        // approach to avoid casting both objects to their actual type, then dereferencing it etc.
+        auto range = rhs.m_constraints.equal_range(it->first);
+        bool found_something = false;
+        for(auto jt=range.first;jt!=range.second;++jt)
+        {
+            if(it->second->type() == jt->second->type() && it->second->toSql(it->first) == jt->second->toSql(jt->first))
+            {
+                found_something = true;
+                break;
+            }
+        }
+
+        // If no match was found, the constraint maps aren't equal
+        if(!found_something)
+            return false;
+    }
+
     return true;
 }
 
diff --git a/src/sqlitedb.cpp b/src/sqlitedb.cpp
index 398973bf..75fd396e 100644
--- a/src/sqlitedb.cpp
+++ b/src/sqlitedb.cpp
@@ -1592,12 +1592,13 @@ bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb
     }
 
     // Copy the data from the old table to the new one
-    if(!executeSQL(QString("INSERT INTO %1.%2 (%3) SELECT %4 FROM %5;")
+    if(!executeSQL(QString("INSERT INTO %1.%2 (%3) SELECT %4 FROM %5.%6;")
                    .arg(sqlb::escapeIdentifier(newSchemaName))
                    .arg(sqlb::escapeIdentifier(new_table_with_random_name.name()))
                    .arg(sqlb::escapeIdentifier(copy_values_to).join(","))
                    .arg(sqlb::escapeIdentifier(copy_values_from).join(","))
-                   .arg(tablename.toString())))
+                   .arg(sqlb::escapeIdentifier(tablename.schema()))
+                   .arg(sqlb::escapeIdentifier(old_table.name()))))
     {
         QString error(tr("Copying data to new table failed. DB says:\n%1").arg(lastErrorMessage));
         revertToSavepoint(savepointName);
@@ -1610,7 +1611,7 @@ bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb
     for(auto it : schemata[tablename.schema()])
     {
         // If this object references the table and it's not the table itself save it's SQL string
-        if(it->baseTable() == tablename.name() && it->type() != sqlb::Object::Types::Table)
+        if(it->baseTable() == old_table.name() && it->type() != sqlb::Object::Types::Table)
         {
             // If this is an index, update the fields first. This highly increases the chance that the SQL statement won't throw an
             // error later on when we try to recreate it.
@@ -1661,7 +1662,7 @@ bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb
     setPragma("defer_foreign_keys", "1");
 
     // Delete the old table
-    if(!executeSQL(QString("DROP TABLE %1;").arg(tablename.toString()), true, true))
+    if(!executeSQL(QString("DROP TABLE %1.%2;").arg(sqlb::escapeIdentifier(tablename.schema())).arg(sqlb::escapeIdentifier(old_table.name())), true, true))
     {
         QString error(tr("Deleting old table failed. DB says: %1").arg(lastErrorMessage));
         revertToSavepoint(savepointName);
@@ -1670,7 +1671,7 @@ bool DBBrowserDB::alterTable(const sqlb::ObjectIdentifier& tablename, const sqlb
     }
 
     // Rename the temporary table
-    if(!renameTable(newSchemaName, new_table_with_random_name.name(), tablename.name()))
+    if(!renameTable(newSchemaName, new_table_with_random_name.name(), new_table.name()))
     {
         revertToSavepoint(savepointName);
         return false;