Feature/alter column

[RWOverdijk]

Fixes some issues from #46

After this PR, a couple of things will have to be done:

1. Update the docs to document this method Added docs for .alter() documentation#8
2. Add to dialects (alter column works for most, and I already added mysql)
3. Add tests

The code in this PR works as follows:

knex.alterTable('user', table => {
  table.string('something', 50).notNullable();
  table.string('username', 35).notNullable().alter();
  table.dropColumn('reasons');
});

which generates:

~/projects/wetland [ time node app
alter table `user` add `something` varchar(50) not null;
alter table `user` modify `username` varchar(35) not null;
alter table `user` drop `reasons`;

[elhigu]

This needs automatic tests and documentation see.https://github.com/tgriesser/knex/blob/master/CONTRIBUTING.md

[bas080]

Good feature 👍. Indeed best to update the tests.

[RWOverdijk]

@elhigu Yep. I'll accept a PR on my branch 👍

[elhigu]

@RWOverdijk do you mean that this pull request won't be finished by you?

[RWOverdijk]

@elhigu If nobody PRs the docs and tests, I'll add them. Until then I'm using our fork.

[RWOverdijk]

@elhigu Added docs and tests :) Also updated the dialects.

[elhigu]

Thanks for starting work on this! From issue #48 Tim had added check list of stuff it should implement so at least those should be tested.

Checklist:
 Renaming Columns (already done)
 Change Null
 Change Default 
 Change Datatype

As far as I know add column and alter / modify column syntax is not the same and wit this implementation just add is changed to alter / modify... so how this should work exactly in those cases?

Here are some of the syntaxes (sqlite cannot be supported):

http://dev.mysql.com/doc/refman/5.7/en/alter-table.html
https://www.postgresql.org/docs/9.4/static/sql-altertable.html
https://www.techonthenet.com/oracle/tables/alter_table.php

Tests were currently very minimal and tests just some trivial cases for some dialects (didn't see postgresql test). What kind of altering this PR supports?

[wubzz]

Pinging in some related issues/PRs.
Tim's Checklist: #46 (comment)
Change nullable: #1327

[RWOverdijk]

I'm not sure if I'm in agreement with the way columns get renamed. It's an async process and the rest of the instructions don't wait for it. So if one of my columns has changes, and needs the change to happen first, I'm out of luck.

[RWOverdijk]

@elhigu This PR is just so you can alter a column's definition. It doesn't do more than that. So now you don't have to drop columns when you change the size or type.

[wubzz]

@RWOverdijk @elhigu Changing a columns default value / nullable state should undoubtedly be considered part of the column definition, and right now these additions do not cover all dialects. For instance, postgres: https://www.postgresql.org/docs/9.4/static/sql-altertable.html

var knex = require('./lib/index')({
    client: 'pg'
});

console.log(knex.schema.alterTable('user', (table) => {
    table.integer('id').defaultTo(50).alter();
}).toString());
//alter table "user" alter column "id" integer default '50'

Given the .alter() call one would expect ALTER TABLE "user" ALTER COLUMN "id" SET DEFAULT '50' for postgres.

var knex = require('./lib/index')({
    client: 'pg'
});

console.log(knex.schema.alterTable('user', (table) => {
    table.integer('id').nullable().alter();
}).toString());
//alter table "user" alter column "id" integer null

For postgres: ALTER TABLE "user" ALTER COLUMN "id" DROP NOT NULL

var knex = require('./lib/index')({
    client: 'pg'
});

console.log(knex.schema.alterTable('user', (table) => {
    table.decimal('id').alter();
}).toString());
//alter table "user" alter column "id" decimal(8, 2)

For postgres: ALTER TABLE user" ALTER COLUMN "id" TYPE decimal(8, 2)

Modifications like these don't just need unit tests for string comparisons, it needs integration tests to ensure the queries are actually functioning.

I'm not too familiar with mysql syntax, but don't you need the entire column definition in the modify statement otherwise it will overwite type/default/null ? I could be dead wrong..

[RWOverdijk]

@wubzz You might be, but I'm probably wrong about postgres. In my opinion, it's up to the user to define that behavior. You alter the column, you know the database, you define the correct definition.

As far as I can tell, for mysql it's enough to just say "this is the new definition now" so you just use modify. I didn't realize the postgres syntax was this verbose. I guess we can change the definitions for postgres based on the _method value (alter / add).

Until we figure out what to do with this PR, I'll monkey patch this behavior in wetland.

[RWOverdijk]

Is anyone looking at this still?

[elhigu]

I suppose this works for mysql and for oracle to redefine column type, default and nullability... So it adds value to knex even that it doesn't work with postgresql this way yet.

Postgresql support might work by creating multiple alter column statements to first drop old defaults, null constraints etc. and then redeclare type, nullability and default...

So this

table.integer('bar').nullable().defaultTo(50).alter();

Would output following queries

ALTER TABLE foo ALTER COLUMN bar DROP DEFAULT;
ALTER TABLE foo ALTER COLUMN bar DROP NOT NULL;
ALTER TABLE foo ALTER COLUMN bar TYPE integer;
ALTER TABLE foo ALTER COLUMN bar SET DEFAULT 50;
ALTER TABLE foo ALTER COLUMN bar SET NOT NULL;

@wubzz @RWOverdijk would this sound reasonable solution? About MSSQL and sqlite3 then we still need to figure out if there are solution for this for sqlite3 and mssql to decide if those should just throw an exception.

[RWOverdijk]

@elhigu I already have exceptions for sqlite3 in wetland due to foreign keys. I now allow two approaches:

• disabling foreign keys
• drop / recreate and reinsert

So another exception wouldn't hurt our project that much.

For sqlite, I'm pretty sure we can recreate the schema (maybe group alters so we only have to do it once for all changes) and copy the data over, dropping the old table. But I guess it should then be solved with queries alone; unlike the rename column functionality which causes problems because it is asynchronous.

As to mssql, I'm not sure. I've used it twice to get data from an old application so I have little to no knowledge there.

And finally for postgres, I think that's the best way to do it. Otherwise we'd have to get the schema, diff it, calculate the changes and apply them. This is a lot more readable and easier to implement. Although I'm not sure where or how this would then be implemented yet.

[elhigu]

@RWOverdijk I'm fine this implementation if you add exceptions for sqlite and postgres that feature is not supported by them. Also new issue about adding support for postgres for this feature would be needed, with link to this issue...

But as @wubzz mentioned, integration test is needed for this, otherwise one may change query implementation and change unit test how query is outputted, but new query might not work after all on real DB. So having integration test is more about making sure that in the future, no one breaks this feature by accident.

[elhigu]

I wouldn't like to see this feature being rotten here, since even that it is not complete it already adds value to knex and shows direction how to support this more widely.

[elhigu]

@RWOverdijk do you still have plans for doing those last changes to complete this one? After integration test and couple of error messages of unsupported dialects I'm good to merge this :)

[RWOverdijk]

@elhigu Sorry, yes I would love to. I'm just not able to right now due to work load. I have plans to tackle this as soon as I'm able to, but it might take a while. I also want to look at the async rename column issue I noticed :)

[elhigu]

@RWOverdijk Ok, great! No worries about the schedule, I was just checking to see if there was still plans to continue at some point in the future 👍

[eigenmannmartin]

Can we help in some way to get this pr to land?

[elhigu]

@eigenmannmartin yes please, in comment #1759 (comment) is mentioned changes that still should be done.

If postgresql support is implemented like described in #1759 (comment) , it would be nice, but not obligatory to get this PR merged.

[RWOverdijk]

I'm really sorry for the delay... I do want to finish this but there's a bunch of stuff I need to do before I get here. But yes, any help is welcome!

[elhigu]

I'll finish this with postgres support to get it to next release. Thanks @RWOverdijk for making this happen!

[elhigu]

#1914 I opened finished pull request. I'll close this now.