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

Feature/alter column #1759

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@RWOverdijk
Contributor

RWOverdijk commented Oct 27, 2016

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 knex/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 comment has been minimized.

Collaborator

elhigu commented Oct 27, 2016

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

@bas080

This comment has been minimized.

bas080 commented Oct 27, 2016

Good feature 👍. Indeed best to update the tests.

@RWOverdijk

This comment has been minimized.

Contributor

RWOverdijk commented Oct 27, 2016

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

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Oct 27, 2016

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

@RWOverdijk

This comment has been minimized.

Contributor

RWOverdijk commented Oct 27, 2016

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

@RWOverdijk

This comment has been minimized.

Contributor

RWOverdijk commented Oct 27, 2016

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

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Oct 27, 2016

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

This comment has been minimized.

Collaborator

wubzz commented Oct 27, 2016

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

@RWOverdijk

This comment has been minimized.

Contributor

RWOverdijk commented Oct 27, 2016

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

This comment has been minimized.

Contributor

RWOverdijk commented Oct 27, 2016

@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

This comment has been minimized.

Collaborator

wubzz commented Oct 27, 2016

@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

This comment has been minimized.

Contributor

RWOverdijk commented Oct 27, 2016

@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

This comment has been minimized.

Contributor

RWOverdijk commented Nov 3, 2016

Is anyone looking at this still?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 3, 2016

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

This comment has been minimized.

Contributor

RWOverdijk commented Nov 4, 2016

@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

This comment has been minimized.

Collaborator

elhigu commented Nov 13, 2016

@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

This comment has been minimized.

Collaborator

elhigu commented Nov 13, 2016

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

This comment has been minimized.

Collaborator

elhigu commented Nov 23, 2016

@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

This comment has been minimized.

Contributor

RWOverdijk commented Nov 23, 2016

@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

This comment has been minimized.

Collaborator

elhigu commented Nov 24, 2016

@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

This comment has been minimized.

eigenmannmartin commented Jan 31, 2017

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

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 31, 2017

@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

This comment has been minimized.

Contributor

RWOverdijk commented Jan 31, 2017

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

This comment has been minimized.

Collaborator

elhigu commented Feb 14, 2017

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

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 15, 2017

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

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