Skip to content

Commit

Permalink
[#3033] fix: sqlite3 drop/renameColumn() breaks with postProcessRespo…
Browse files Browse the repository at this point in the history 
…nse (#3040)

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

* When postProcessResponse is configured, and client.processResponse()
returns a Promise (e.g. for custom cases such as sqlite3 dropColumn()),
then that Promise is not awaited, but handed to postProcessResponse,
which might break is (e.g. with Objection's knexSnakeCaseMappers()).

* when reinserting data in the modified table, the rows are now being
handled with the "mapped" identifiers (instead of the unmapped)

* add tests, fix hasColumn

* add hasColumn tests for add mysql + snakeCaseMappers

* Improve tests and fix more cases related to processing

* Fix SQLite-specific tests

* MySQL is not case-sensitive, pg is
  • Loading branch information
@elunic @kibertoad
elunic authored and kibertoad committed Mar 13, 2019
1 parent 24fcf27 commit 19926d8
Show file tree
Hide file tree
Showing 7 changed files with 391 additions and 175 deletions.
14 changes: 8 additions & 6 deletions src/dialects/mysql/schema/compiler.js
View file
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
View file
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
View file
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();
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
View file
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

0 comments on commit 19926d8

Please sign in to comment.