Fix editing of primary and foreign keys for existing tables

See issues #872 and #918.

src/EditTableDialog.cpp

@@ -206,7 +206,7 @@ void EditTableDialog::updateTypes()
 
         m_table.fields().at(index)->setType(type);
         if(!m_bNewTable)
-            pdb.renameColumn(curTable, column, m_table.fields().at(index));
+            pdb.renameColumn(m_table, column, m_table.fields().at(index));
         checkInput();
     }
 }
@@ -441,7 +441,7 @@ void EditTableDialog::itemChanged(QTreeWidgetItem *item, int column)
         }
 
         if(callRenameColumn)
-            pdb.renameColumn(curTable, oldFieldName, field);
+            pdb.renameColumn(m_table, oldFieldName, field);
     }
 
     checkInput();
@@ -526,7 +526,7 @@ void EditTableDialog::removeField()
         QString msg = tr("Are you sure you want to delete the field '%1'?\nAll data currently stored in this field will be lost.").arg(ui->treeWidget->currentItem()->text(0));
         if(QMessageBox::warning(this, QApplication::applicationName(), msg, QMessageBox::Yes | QMessageBox::No, QMessageBox::No) == QMessageBox::Yes)
         {
-            if(!pdb.renameColumn(curTable, ui->treeWidget->currentItem()->text(0), sqlb::FieldPtr()))
+            if(!pdb.renameColumn(m_table, ui->treeWidget->currentItem()->text(0), sqlb::FieldPtr()))
             {
                 QMessageBox::warning(0, QApplication::applicationName(), pdb.lastErrorMessage);
             } else {
@@ -605,7 +605,7 @@ void EditTableDialog::moveCurrentField(bool down)
 
         // Move the actual column
         if(!pdb.renameColumn(
-                    curTable,
+                    m_table,
                     ui->treeWidget->currentItem()->text(0),
                     m_table.fields().at(ui->treeWidget->indexOfTopLevelItem(ui->treeWidget->currentItem())),
                     (down ? 1 : -1)

src/sqlitedb.cpp

@@ -967,7 +967,7 @@ bool DBBrowserDB::addColumn(const QString& tablename, const sqlb::FieldPtr& fiel
     return executeSQL(sql);
 }
 
-bool DBBrowserDB::renameColumn(const QString& tablename, const QString& name, sqlb::FieldPtr to, int move)
+bool DBBrowserDB::renameColumn(const sqlb::Table& table, const QString& name, sqlb::FieldPtr to, int move)
 {
     // NOTE: This function is working around the incomplete ALTER TABLE command in SQLite.
     // If SQLite should fully support this command one day, this entire
@@ -979,11 +979,17 @@ bool DBBrowserDB::renameColumn(const QString& tablename, const QString& name, sq
     //    sql = QString("ALTER TABLE %1 MODIFY %2 %3").arg(sqlb::escapeIdentifier(tablename)).arg(sqlb::escapeIdentifier(to)).arg(type);    // This is wrong...
     //return executeSQL(sql);
 
+    // TODO: This function needs to be cleaned up. It might make sense to split it up in several parts than can be reused
+    // more easily. Besides that, it might make sense to support some potential use cases in a more sophisticated way. These include:
+    // 1) Skip the entire column editing part when only the table constraints are changed.
+    // 2) Allow modifying multiple columns at once in order to only have to call this function (including all its overhead) once instead of once per change.
+    // 3) Include the addColumn() use case in here, so the calling side doesn't need to know anything about how this class handles table modifications.
+
     // Collect information on the current DB layout
-    QString tableSql = getObjectByName(tablename).getsql();
+    QString tableSql = getObjectByName(table.name()).getsql();
     if(tableSql.isEmpty())
     {
-        lastErrorMessage = tr("renameColumn: cannot find table %1.").arg(tablename);
+        lastErrorMessage = tr("renameColumn: cannot find table %1.").arg(table.name());
         qWarning() << lastErrorMessage;
         return false;
     }
@@ -1009,9 +1015,10 @@ bool DBBrowserDB::renameColumn(const QString& tablename, const QString& name, sq
 
     // Create a new table with a name that hopefully doesn't exist yet.
     // Its layout is exactly the same as the one of the table to change - except for the column to change
-    // of course
+    // of course, and the table constraints which are copied from the table parameter.
     sqlb::Table newSchema = oldSchema;
     newSchema.setName("sqlitebrowser_rename_column_new_table");
+    newSchema.setConstraints(table.allConstraints());
     QString select_cols;
     if(to.isNull())
     {
@@ -1055,7 +1062,7 @@ bool DBBrowserDB::renameColumn(const QString& tablename, const QString& name, sq
     }
 
     // Copy the data from the old table to the new one
-    if(!executeSQL(QString("INSERT INTO sqlitebrowser_rename_column_new_table SELECT %1 FROM %2;").arg(select_cols).arg(sqlb::escapeIdentifier(tablename))))
+    if(!executeSQL(QString("INSERT INTO sqlitebrowser_rename_column_new_table SELECT %1 FROM %2;").arg(select_cols).arg(sqlb::escapeIdentifier(table.name()))))
     {
         QString error(tr("renameColumn: copying data to new table failed. DB says:\n%1").arg(lastErrorMessage));
         qWarning() << error;
@@ -1069,7 +1076,7 @@ bool DBBrowserDB::renameColumn(const QString& tablename, const QString& name, sq
     for(auto it=objMap.constBegin();it!=objMap.constEnd();++it)
     {
         // If this object references the table and it's not the table itself save it's SQL string
-        if((*it).getTableName() == tablename && (*it).gettype() != "table")
+        if((*it).getTableName() == table.name() && (*it).gettype() != "table")
             otherObjectsSql += (*it).getsql().trimmed() + ";\n";
     }
 
@@ -1082,7 +1089,7 @@ bool DBBrowserDB::renameColumn(const QString& tablename, const QString& name, sq
     setPragma("defer_foreign_keys", "1");
 
     // Delete the old table
-    if(!executeSQL(QString("DROP TABLE %1;").arg(sqlb::escapeIdentifier(tablename)), true, true))
+    if(!executeSQL(QString("DROP TABLE %1;").arg(sqlb::escapeIdentifier(table.name())), true, true))
     {
         QString error(tr("renameColumn: deleting old table failed. DB says: %1").arg(lastErrorMessage));
         qWarning() << error;
@@ -1092,7 +1099,7 @@ bool DBBrowserDB::renameColumn(const QString& tablename, const QString& name, sq
     }
 
     // Rename the temporary table
-    if(!renameTable("sqlitebrowser_rename_column_new_table", tablename))
+    if(!renameTable("sqlitebrowser_rename_column_new_table", table.name()))
     {
         revertToSavepoint("sqlitebrowser_rename_column");
         return false;

src/sqlitedb.h

@@ -94,13 +94,13 @@ public:
 
     /**
      * @brief renameColumn Can be used to rename, modify or drop an existing column of a given table
-     * @param tablename Specifies the table name
+     * @param table Specifies the table to edit. The table name and the table constraints are used from this but not the columns
      * @param name Name of the column to edit
      * @param to The new field definition with changed name, type or the like. If Null-Pointer is given the column is dropped.
      * @param move Set this to a value != 0 to move the new column to a different position
      * @return true if renaming was successful, false if not. In the latter case also lastErrorMessage is set
      */
-    bool renameColumn(const QString& tablename, const QString& name, sqlb::FieldPtr to, int move = 0);
+    bool renameColumn(const sqlb::Table& table, const QString& name, sqlb::FieldPtr to, int move = 0);
 
     objectMap getBrowsableObjects() const;
     DBBrowserObject getObjectByName(const QString& name) const;

src/sqlitetypes.cpp

@@ -357,6 +357,11 @@ QList<ConstraintPtr> Table::constraints(FieldVector fields, Constraint::Constrai
     }
 }
 
+void Table::setConstraints(const ConstraintMap& constraints)
+{
+    m_constraints = constraints;
+}
+
 FieldVector& Table::primaryKeyRef()
 {
     return const_cast<FieldVector&>(static_cast<const Table*>(this)->primaryKey());

src/sqlitetypes.h

@@ -180,15 +180,19 @@ public:
     void setField(int index, FieldPtr f);
     const FieldPtr& field(int index) { return m_fields[index]; }
     QStringList fieldNames() const;
+
     void setRowidColumn(const QString& rowid) {  m_rowidColumn = rowid; }
     const QString& rowidColumn() const { return m_rowidColumn; }
     bool isWithoutRowidTable() const { return m_rowidColumn != "_rowid_"; }
+
     void clear();
 
     void addConstraint(FieldVector fields, ConstraintPtr constraint);
     void removeConstraints(FieldVector fields = FieldVector(), Constraint::ConstraintTypes type = Constraint::NoType); //! Only removes the first constraint, if any
     ConstraintPtr constraint(FieldVector fields = FieldVector(), Constraint::ConstraintTypes type = Constraint::NoType) const;   //! Only returns the first constraint, if any
     QList<ConstraintPtr> constraints(FieldVector fields = FieldVector(), Constraint::ConstraintTypes type = Constraint::NoType) const;
+    ConstraintMap allConstraints() const { return m_constraints; }
+    void setConstraints(const ConstraintMap& constraints);
     FieldVector& primaryKeyRef();
     const FieldVector& primaryKey() const;