Permalink
Browse files

Improve movement of tables between different schemata

This improves the Edit Table dialog to better handle moving tables from
one schema to another, though the UI currently only knows about the main
and the temp schema.

This also simplifies the grammar code by removing the temporary flag
from all classes because it's redundant now that we support multiple
schemata.
  • Loading branch information...
MKleusberg committed Sep 4, 2017
1 parent a5ca756 commit 7db96cdf13ff9131b87a71ebb48517080869b061
Showing with 17 additions and 23 deletions.
  1. +2 −5 src/EditTableDialog.cpp
  2. +11 −9 src/sqlitedb.cpp
  3. +2 −1 src/sqlitedb.h
  4. +1 −3 src/sqlitetypes.cpp
  5. +1 −5 src/sqlitetypes.h
@@ -40,7 +40,7 @@ EditTableDialog::EditTableDialog(DBBrowserDB& db, const sqlb::ObjectIdentifier&
// for error checking etc.
ui->checkWithoutRowid->blockSignals(true);
ui->checkWithoutRowid->setChecked(m_table.isWithoutRowidTable());
ui->checkTemporary->setChecked(m_table.isTemporary());
ui->checkTemporary->setChecked(curTable.schema() == "temp");
ui->checkWithoutRowid->blockSignals(false);

populateFields();
@@ -720,16 +720,13 @@ void EditTableDialog::setWithoutRowid(bool without_rowid)

void EditTableDialog::setTemporary(bool is_temp)
{
// Set the temporary flag in the table definition
m_table.setTemporary(is_temp);

// Update the SQL preview
updateSqlText();

// Update table if we're editing an existing table
if(!m_bNewTable)
{
if(!pdb.renameColumn(curTable, m_table, QString(), sqlb::FieldPtr(), 0))
if(!pdb.renameColumn(curTable, m_table, QString(), sqlb::FieldPtr(), 0, is_temp ? "temp" : "main"))
{
QMessageBox::warning(this, QApplication::applicationName(),
tr("Setting the temporary flag for the table failed. Error message:\n%1").arg(pdb.lastError()));
@@ -1038,7 +1038,7 @@ bool DBBrowserDB::addColumn(const sqlb::ObjectIdentifier& tablename, const sqlb:
return executeSQL(sql);
}

bool DBBrowserDB::renameColumn(const sqlb::ObjectIdentifier& tablename, const sqlb::Table& table, const QString& name, sqlb::FieldPtr to, int move)
bool DBBrowserDB::renameColumn(const sqlb::ObjectIdentifier& tablename, const sqlb::Table& table, const QString& name, sqlb::FieldPtr to, int move, QString newSchemaName)
{
/*
* USE CASES:
@@ -1089,7 +1089,6 @@ bool DBBrowserDB::renameColumn(const sqlb::ObjectIdentifier& tablename, const sq
newSchema.setName("sqlitebrowser_rename_column_new_table");
newSchema.setConstraints(table.allConstraints());
newSchema.setRowidColumn(table.rowidColumn());
newSchema.setTemporary(table.isTemporary());
QString select_cols;
if(to.isNull())
{
@@ -1123,9 +1122,13 @@ bool DBBrowserDB::renameColumn(const sqlb::ObjectIdentifier& tablename, const sq
newSchema.setField(index + move, to);
}

// If no new schema name has been set, we just use the old schema name
if(newSchemaName.isNull())
newSchemaName = tablename.schema();

// Create the new table
NoStructureUpdateChecks nup(*this);
if(!executeSQL(newSchema.sql(tablename.schema()), true, true))
if(!executeSQL(newSchema.sql(newSchemaName), true, true))
{
QString error(tr("renameColumn: creating new table failed. DB says: %1").arg(lastErrorMessage));
revertToSavepoint(savepointName);
@@ -1135,7 +1138,7 @@ bool DBBrowserDB::renameColumn(const sqlb::ObjectIdentifier& tablename, const sq

// Copy the data from the old table to the new one
if(!executeSQL(QString("INSERT INTO %1.sqlitebrowser_rename_column_new_table SELECT %2 FROM %3;")
.arg(tablename.schema())
.arg(newSchemaName)
.arg(select_cols)
.arg(tablename.toString())))
{
@@ -1173,9 +1176,10 @@ bool DBBrowserDB::renameColumn(const sqlb::ObjectIdentifier& tablename, const sq
;
}

// Only try to add the index later if it has any columns remaining
// Only try to add the index later if it has any columns remaining. Also use the new schema name here, too, to basically move
// any index that references the table to the same new schema as the table.
if(idx->columns().size())
otherObjectsSql << idx->sql();
otherObjectsSql << idx->sql(newSchemaName);
} else {
// If it's a view or a trigger we don't have any chance to corrections yet. Just store the statement as is and
// hope for the best.
@@ -1202,7 +1206,7 @@ bool DBBrowserDB::renameColumn(const sqlb::ObjectIdentifier& tablename, const sq
}

// Rename the temporary table
if(!renameTable(tablename.schema(), "sqlitebrowser_rename_column_new_table", tablename.name()))
if(!renameTable(newSchemaName, "sqlitebrowser_rename_column_new_table", tablename.name()))
{
revertToSavepoint(savepointName);
return false;
@@ -1362,8 +1366,6 @@ void DBBrowserDB::updateSchema()
if(!val_sql.isEmpty())
{
sqlb::ObjectPtr object = sqlb::Object::parseSQL(type, val_sql);
if(schema_name == "temp")
object->setTemporary(true);

// If parsing wasn't successful set the object name manually, so that at least the name is going to be correct
if(!object->fullyParsed())
@@ -84,9 +84,10 @@ class DBBrowserDB : public QObject
* @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
* @param newSchema Set this to a non-empty string to move the table to a new schema
* @return true if renaming was successful, false if not. In the latter case also lastErrorMessage is set
*/
bool renameColumn(const sqlb::ObjectIdentifier& tablename, const sqlb::Table& table, const QString& name, sqlb::FieldPtr to, int move = 0);
bool renameColumn(const sqlb::ObjectIdentifier& tablename, const sqlb::Table& table, const QString& name, sqlb::FieldPtr to, int move = 0, QString newSchemaName = QString());

objectMap getBrowsableObjects(const QString& schema) const;
const sqlb::ObjectPtr getObjectByName(const sqlb::ObjectIdentifier& name) const;
@@ -253,7 +253,6 @@ void Table::clear()
m_fields.clear();
m_constraints.clear();
m_virtual = QString();
m_temporary = false;
}
Table::~Table()
{
@@ -456,8 +455,7 @@ QString Table::sql(const QString& schema, bool ifNotExists) const
return QString("CREATE VIRTUAL TABLE %1 USING %2;").arg(ObjectIdentifier(schema, m_name).toString(true)).arg(m_virtual);

// This is a normal table, not a virtual one
QString sql = QString("CREATE %1TABLE%2 %3 (\n")
.arg(m_temporary ? QString("TEMPORARY ") : QString(""))
QString sql = QString("CREATE TABLE%1 %2 (\n")
.arg(ifNotExists ? QString(" IF NOT EXISTS") : QString(""))
.arg(ObjectIdentifier(schema, m_name).toString(true));

@@ -142,7 +142,7 @@ class Object
Trigger
};

explicit Object(const QString& name): m_name(name), m_temporary(false), m_fullyParsed(false) {}
explicit Object(const QString& name): m_name(name), m_fullyParsed(false) {}
virtual ~Object() {}

virtual Types type() const = 0;
@@ -154,9 +154,6 @@ class Object
void setOriginalSql(const QString& original_sql) { m_originalSql = original_sql; }
QString originalSql() const { return m_originalSql; }

void setTemporary(bool temp) { m_temporary = temp; }
bool isTemporary() const { return m_temporary; }

virtual QString baseTable() const { return QString(); }

void setFullyParsed(bool fully_parsed) { m_fullyParsed = fully_parsed; }
@@ -185,7 +182,6 @@ class Object
protected:
QString m_name;
QString m_originalSql;
bool m_temporary;
bool m_fullyParsed;
};

6 comments on commit 7db96cd

@justinclift

This comment has been minimized.

Copy link
Member

justinclift replied Sep 4, 2017

This is working decently so far, testing with simple tables. The automatic movement of associated indexes is good too. 😄

Any idea how we'll it'll handle tables with foreign keys between them? I haven't tried it yet, and I'm way brain faded atm. Probably try it out later tonight. 😄

@MKleusberg

This comment has been minimized.

Copy link
Member Author

MKleusberg replied Sep 4, 2017

Good question 😉 I'm not entirely sure what would happen but it probably won't really work. But then there isn't much we can do about it. Foreign keys can reference tables in another schema and moving or copying other tables would be too much. So the question is just how helpful the error message is, that you'll get 😉

@justinclift

This comment has been minimized.

Copy link
Member

justinclift replied Sep 4, 2017

Ahhh, didn't realise foreign key references in SQLite could refer to attached databases.

Sounds like that could be really powerful. But yeah... painful for stuff like this. 😉

@MKleusberg

This comment has been minimized.

Copy link
Member Author

MKleusberg replied Sep 5, 2017

Oops, I meant to write can't 😉 Sorry for the confusion! If they could, we could just move the table and change all references in the foreign keys. That would keep the databases in a working state as long as they are still attached, allowing the user to take further manual actions. But as I said (or intended to say) we can't do that. So it's definitely going to create some sort of error...

@chrisjlocke

This comment has been minimized.

Copy link
Contributor

chrisjlocke replied Sep 5, 2017

if(!pdb.renameColumn(curTable, m_table, QString(), sqlb::FieldPtr(), 0, is_temp ? "temp" : "main"))

Is this assuming you're only going to have the 'main' schema ... ie, what about attached databases?

@MKleusberg

This comment has been minimized.

Copy link
Member Author

MKleusberg replied Sep 5, 2017

Yes, because at this point the Edit Table dialog only had a checkbox which would allow you to toggle between temporary and non-temporary schema. But that has only been an intermediate step to split up the commits into smaller pieces. Here's the next one where support for attached databases was added 😄

Please sign in to comment.