Skip to content

Commit

Permalink
[#3033] fix: sqlite3 drop/renameColumn() breaks with postProcessResponse
Browse files Browse the repository at this point in the history
* 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)
  • Loading branch information
whefter committed Feb 7, 2019
1 parent 15ac75c commit 52c4f68
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 7 deletions.
30 changes: 26 additions & 4 deletions src/dialects/sqlite3/schema/ddl.js
Original file line number Diff line number Diff line change
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 Down Expand Up @@ -217,6 +226,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 +243,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 +271,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
Original file line number Diff line number Diff line change
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 test/integration/schema/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1061,7 +1061,7 @@ module.exports = function(knex) {
.dropTable('rename_col_test');
});

it('renames the column', function() {
it.only('renames the column', function() {
return knex.schema
.table('rename_column_test', function(tbl) {
return tbl.renameColumn('id_test', 'id');
Expand Down

0 comments on commit 52c4f68

Please sign in to comment.