Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#3033] fix: sqlite3 drop/renameColumn() breaks with postProcessResponse #3040

Merged
merged 6 commits into from Mar 13, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/dialects/mysql/schema/compiler.js
Expand Up @@ -3,7 +3,7 @@
import inherits from 'inherits';
import SchemaCompiler from '../../../schema/compiler';

import { assign } from 'lodash';
import { assign, some } from 'lodash';

function SchemaCompiler_MySQL(client, builder) {
SchemaCompiler.call(this, client, builder);
Expand Down Expand Up @@ -44,12 +44,14 @@ assign(SchemaCompiler_MySQL.prototype, {
// Check whether a column exists on the schema.
hasColumn(tableName, column) {
this.pushQuery({
sql:
`show columns from ${this.formatter.wrap(tableName)}` +
' like ' +
this.formatter.parameter(column),
sql: `show columns from ${this.formatter.wrap(tableName)}`,
output(resp) {
return resp.length > 0;
return some(resp, (row) => {
return (
this.client.wrapIdentifier(row.Field) ===
this.client.wrapIdentifier(column)
);
});
},
});
},
Expand Down
7 changes: 6 additions & 1 deletion src/dialects/sqlite3/schema/compiler.js
Expand Up @@ -26,7 +26,12 @@ SchemaCompiler_SQLite3.prototype.hasColumn = function(tableName, column) {
this.pushQuery({
sql: `PRAGMA table_info(${this.formatter.wrap(tableName)})`,
output(resp) {
return some(resp, { name: column });
return some(resp, (col) => {
return (
this.client.wrapIdentifier(col.name) ===
this.client.wrapIdentifier(column)
);
});
},
});
};
Expand Down
73 changes: 56 additions & 17 deletions src/dialects/sqlite3/schema/ddl.js
Expand Up @@ -5,7 +5,16 @@
// -------

import Promise from 'bluebird';
import { assign, uniqueId, find, identity, map, omit } from 'lodash';
import {
assign,
uniqueId,
find,
identity,
map,
omit,
invert,
fromPairs,
} from 'lodash';

// So altering the schema in SQLite3 is a major pain.
// We have our own object to deal with the renaming and altering the types
Expand All @@ -14,37 +23,54 @@ function SQLite3_DDL(client, tableCompiler, pragma, connection) {
this.client = client;
this.tableCompiler = tableCompiler;
this.pragma = pragma;
this.tableName = this.tableCompiler.tableNameRaw;
this.tableNameRaw = this.tableCompiler.tableNameRaw;
this.alteredName = uniqueId('_knex_temp_alter');
this.connection = connection;
this.formatter =
client && client.config && client.config.wrapIdentifier
? client.config.wrapIdentifier
: (value) => value;
}

assign(SQLite3_DDL.prototype, {
tableName() {
return this.formatter(this.tableNameRaw, (value) => value);
},

getColumn: Promise.method(function(column) {
const currentCol = find(this.pragma, { name: column });
const currentCol = find(this.pragma, (col) => {
return (
this.client.wrapIdentifier(col.name) ===
this.client.wrapIdentifier(column)
);
});
if (!currentCol)
throw new Error(
`The column ${column} is not in the ${this.tableName} table`
`The column ${column} is not in the ${this.tableName()} table`
);
return currentCol;
}),

getTableSql() {
return this.trx.raw(
`SELECT name, sql FROM sqlite_master WHERE type="table" AND name="${
this.tableName
}"`
);
this.trx.disableProcessing();
Copy link
Collaborator

@kibertoad kibertoad Mar 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an internal query which breaks if we e. g. uppercase fields name and sql.

return this.trx
.raw(
`SELECT name, sql FROM sqlite_master WHERE type="table" AND name="${this.tableName()}"`
)
.then((result) => {
this.trx.enableProcessing();
return result;
});
},

renameTable: Promise.method(function() {
return this.trx.raw(
`ALTER TABLE "${this.tableName}" RENAME TO "${this.alteredName}"`
`ALTER TABLE "${this.tableName()}" RENAME TO "${this.alteredName}"`
);
}),

dropOriginal() {
return this.trx.raw(`DROP TABLE "${this.tableName}"`);
return this.trx.raw(`DROP TABLE "${this.tableName()}"`);
},

dropTempTable() {
Expand All @@ -53,7 +79,7 @@ assign(SQLite3_DDL.prototype, {

copyData() {
return this.trx
.raw(`SELECT * FROM "${this.tableName}"`)
.raw(`SELECT * FROM "${this.tableName()}"`)
.bind(this)
.then(this.insertChunked(20, this.alteredName));
},
Expand All @@ -63,7 +89,7 @@ assign(SQLite3_DDL.prototype, {
return this.trx
.raw(`SELECT * FROM "${this.alteredName}"`)
.bind(this)
.then(this.insertChunked(20, this.tableName, iterator));
.then(this.insertChunked(20, this.tableName(), iterator));
};
},

Expand Down Expand Up @@ -97,7 +123,7 @@ assign(SQLite3_DDL.prototype, {
createTempTable(createTable) {
return function() {
return this.trx.raw(
createTable.sql.replace(this.tableName, this.alteredName)
createTable.sql.replace(this.tableName(), this.alteredName)
);
};
},
Expand Down Expand Up @@ -217,6 +243,14 @@ assign(SQLite3_DDL.prototype, {
if (sql === newSql) {
throw new Error('Unable to find the column to change');
}
const { from: mappedFrom, to: mappedTo } = invert(
this.client.postProcessResponse(
invert({
from,
to,
})
)
);
return Promise.bind(this)
.then(this.createTempTable(createTable))
.then(this.copyData)
Expand All @@ -226,8 +260,8 @@ assign(SQLite3_DDL.prototype, {
})
.then(
this.reinsertData(function(row) {
row[to] = row[from];
return omit(row, from);
row[mappedTo] = row[mappedFrom];
return omit(row, mappedFrom);
})
)
.then(this.dropTempTable);
Expand All @@ -254,14 +288,19 @@ assign(SQLite3_DDL.prototype, {
if (sql === newSql) {
throw new Error('Unable to find the column to change');
}
const mappedColumns = Object.keys(
this.client.postProcessResponse(
fromPairs(columns.map((column) => [column, column]))
)
);
return Promise.bind(this)
.then(this.createTempTable(createTable))
.then(this.copyData)
.then(this.dropOriginal)
.then(function() {
return this.trx.raw(newSql);
})
.then(this.reinsertData((row) => omit(row, ...columns)))
.then(this.reinsertData((row) => omit(row, ...mappedColumns)))
.then(this.dropTempTable);
});
},
Expand Down
7 changes: 5 additions & 2 deletions src/runner.js
Expand Up @@ -138,9 +138,12 @@ assign(Runner.prototype, {
queryPromise = queryPromise.timeout(obj.timeout);
}

// Await the return value of client.processResponse; in the case of sqlite3's
// dropColumn()/renameColumn(), it will be a Promise for the transaction
// containing the complete rename procedure.
return queryPromise
.then((resp) => {
const processedResponse = this.client.processResponse(resp, runner);
.then((resp) => this.client.processResponse(resp, runner))
.then((processedResponse) => {
const queryContext = this.builder.queryContext();
const postProcessedResponse = this.client.postProcessResponse(
processedResponse,
Expand Down
2 changes: 1 addition & 1 deletion src/transaction.js
Expand Up @@ -193,7 +193,7 @@ function makeTransactor(trx, connection, trxClient) {
);
};

transactor.userParams = trx.userParams;
transactor.userParams = trx.userParams || {};

transactor.transaction = function(container, options) {
return trxClient.transaction(container, options, trx);
Expand Down