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

SQLite dropColumn doesn't work for last column #544

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

SQLite dropColumn doesn't work for last column #544

ismyrnow opened this issue Oct 29, 2014 · 5 comments
Labels
bug

Comments

@ismyrnow
Copy link

@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
Copy link
Author

@ismyrnow 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
Copy link

@mabuzer 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
Copy link
Member

@bendrucker bendrucker commented Oct 30, 2014

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

@morungos
Copy link

@morungos 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
Copy link
Member

@bendrucker 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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants