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

SQLite dropColumn doesn't work for last column #544

Closed
ismyrnow opened this Issue Oct 29, 2014 · 5 comments

Comments

Projects
None yet
4 participants
@ismyrnow

ismyrnow commented Oct 29, 2014

The logic for dropping a column prevents the last column in a table from being dropped.

var a = this.formatter.wrap(column) + ' ' + currentCol.type + ', ';

// createTable.sql: CREATE TABLE... "some_col" varchar(255), "last_col" varchar(255))
// a: "last_col" varchar(255) ,

if (createTable.sql.indexOf(a) === -1) {
  throw new Error('Unable to find the column to change');
}

The comma after the column definition prevents it from matching the last column.

Here is a link to the code with the above snippet: https://github.com/tgriesser/knex/blob/master/lib/dialects/sqlite3/schema/ddl.js#L150

@bendrucker bendrucker added the bug label Oct 29, 2014

@ismyrnow

This comment has been minimized.

ismyrnow commented Oct 29, 2014

I looked into adding a test (there isn't one for dropColumn or renameColumn), but all of the sqlite3 tests just verify the SchemaBuilder sql output. The dropColumn function opens a transaction and copies data to a different table, which explains why there isn't currently a test for it.

I'm unsure how to write a test for a function like that.

@mabuzer

This comment has been minimized.

mabuzer commented Oct 30, 2014

Replacing

if (createTable.sql.indexOf(a) === -1) {
  throw new Error('Unable to find the column to change');
}

With the following snippt fix it.

 if (createTable.sql.indexOf(a) === -1) {
          var a1 = a.substring(0, a.length - 2);
          if(createTable.sql.indexOf(a1) === -1) {
              throw new Error('Unable to find the column to change');
          } else {
              a = ', ' + a1;
         }
  }
@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Oct 30, 2014

Thanks @mabuzer. I'll look at getting tests against this behavior.

@morungos

This comment has been minimized.

morungos commented Nov 6, 2014

I just stumbled on this one too. Fortunately, it only affects rolling back my migrations, which hopefully I won't have to do for a while. Can I help?

@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Nov 6, 2014

A tested PR is very welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment