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

Modifying columns #46

Closed
ErisDS opened this issue Aug 23, 2013 · 61 comments
Closed

Modifying columns #46

ErisDS opened this issue Aug 23, 2013 · 61 comments

Comments

@ErisDS
Copy link
Contributor

@ErisDS ErisDS commented Aug 23, 2013

I think it is a reasonable expectation that it would be possible to:

  • rename a column
  • change the data type
  • add or drop the null constraint
  • change the default

And probably other things that I have not thought of.

I would imagine the syntax for doing this would be something like

 knex.Schema.table('table_name', function (t) {
        t.string('my_column').renameTo('something_else');
        t.string('my_column').changeTo('text');
        t.string('my_column').nullable() < adds nullable if not already present
        t.string('my_column').notNullable() < removes nullable if present
        t.string('my_column').defaultTo('whatever') < adds or updates the default
  });

Maybe there needs to be something extra in the chain, possibly at the knex.Schema.table level to indicate that this is a modify statement not an add.

I realise that this is tricky to implement across various databases, especially in SQLite where you have to create a whole new table, but in a system which requires migrations, without these tools it is going to be necessary to use knex.raw and write all the migrations for each DB supported, which sort of defeats the point of having a nice ORM especially one which is about to support migrations.

@tgriesser
Copy link
Member

@tgriesser tgriesser commented Sep 13, 2013

Checklist:

  • Renaming Columns
  • Change Null
  • Change Default
  • Change Datatype
@fjorgemota
Copy link

@fjorgemota fjorgemota commented Jan 17, 2014

The support to renaming columns is very basic, it does not support renaming fields that are a foreign key, for example, as I reported at #157.

@bendrucker
Copy link
Member

@bendrucker bendrucker commented Jan 17, 2014

Acknowledged—take a look at the checklist in Tim's post two above yours. If this is a priority for you and you need it immediately, we'd really appreciate a PR.

@gregberge
Copy link

@gregberge gregberge commented Jan 24, 2014

We need this functionality too for @lemonde CMS.

Adding an index or alter an enum should be possible without executing raw queries.

@bendrucker
Copy link
Member

@bendrucker bendrucker commented Jan 24, 2014

@neoziro Right now you're limited to to setting a particular column as an index. Other schema modifications are on the todo list, and as always PRs are appreciated if you urgently need something.

On another note, I just started a thread to collect featured use cases for Knex/Bookshelf. I'd love to hear how you guys are using knex at @lemonde. You can share over here: #170.

@ErisDS
Copy link
Contributor Author

@ErisDS ErisDS commented Jan 24, 2014

@bendrucker I understand completely your stance on submitting a PR. I am hoping to try to divert some of Ghost's resources to getting this done, but we have very limited resource at the moment.

Seeing as there are a few of us hoping to get this feature, perhaps it would be good to get some thoughts from @bendrucker / @tgriesser about what the API should look like (is my suggestion above correct) and perhaps brain dump any thoughts or ideas about how it can be achieved / the preferred approach.

At the moment, a lot of the ideas/principles about how stuff does and should work in knex/bookshelf are still (as far as I can find) in @tgriesser's head... I realise he doesn't have a great deal of time, but perhaps a wiki page full of where the inspiration for different bits came from, ideas for the future, clever-ass stuff that's in the library and why, all the insider info so we can all catch up to what you were thinking, and try to continue on in a way that you would approve of 😎

Or perhaps you could blog it all.. you know.. on Ghost 😉

@tgriesser
Copy link
Member

@tgriesser tgriesser commented Jan 24, 2014

Haha definitely... sorry @ErisDS and everyone... I really do need to dedicate some time to sit down and write a bunch of the stuff out that's still in my head. Hopefully a ton of stuff including all of the above will be cleared up once I get more internal consistency around building queries in Knex, at which point I can focus more on organizing features I have planned for Bookshelf (model typecasting, soft deletes, a composite key plugin, etc).

I think it'd also be really great to get an open source example app together (other than ghost) that showcases some of the different features of knex/bookshelf for folks to take a look at. I'd started on a hacker news clone, but haven't had the time to finish it... @bendrucker, @johanneslumpe, @tkellen or anyone else have any interest on helping with something like that?

@ErisDS
Copy link
Contributor Author

@ErisDS ErisDS commented Jan 24, 2014

@tgriesser I highly recommend just sitting down and spewing out whatever comes to mind, rather than trying to clarify through code. At a certain point, the principles are far easier communicated with words.

I personally find knex & bookshelf daunting to contribute to, because I don't feel I have enough background info or guidance on how to get stuck in - despite relying heavily on these libraries and knowing them reasonably well.

@bendrucker
Copy link
Member

@bendrucker bendrucker commented Jan 24, 2014

@ErisDS I second a wiki for "clever-ass stuff" 😄

@tgriesser I'd help w/ the OS example app for sure

Big changes should probably wait on other refactors, but I think it's worth consider a non-breaking refactor of the schema builder API as well. There's a weird mix right now of promises, chainable sync functions, and callbacks. Maybe table.column(name) returns a ChainableColumn and then all the type functions (integer, text, etc.) chain off of it.

Then there'd be methods create, drop, rename to set the actual SQL column operation (ADD, ALTER, DROP). create would be the default.

To me that fits better with the rest of Knex style-wise. Just chain up all your query components and let the magic happen rather than dealing with a ton of camelcased syntax like dropColumn et al.

@tkellen
Copy link
Contributor

@tkellen tkellen commented Jan 24, 2014

@tgriesser I could probably get a Knex/Bookshelf hacker news clone up at BocoupFest in Feburary (4th-7th). Maybe @tbranyen and I could whip one up. I'll let ya know.

@johanneslumpe
Copy link
Contributor

@johanneslumpe johanneslumpe commented Jan 24, 2014

@tgriesser Of course! I will help where I can!

Sent from my iPhone

On Jan 24, 2014, at 5:25 PM, Tyler Kellen notifications@github.com wrote:

@tgriesser I could probably get a Knex/Bookshelf hacker news clone up at BocoupFest in Feburary (4th-7th). Maybe @tbranyen and I could whip one up. I'll let ya know.


Reply to this email directly or view it on GitHub.

@p-baleine
Copy link
Contributor

@p-baleine p-baleine commented Feb 19, 2014

@tgriesser

As @ErisDS has mentioned by the original post, I think that it would be nice that if I could do dropping or renaming of columns on SQLite3 by knex's migration function.

lib/migration.js and lib/cli/migrate.js do not know what client they are targeting and what action is performed on migration time. They implement migration via knex's query building functions. But in order to implement dropping or renaming of columns on SQLite3, only when the client is SQLite3 and the performed action is one of renaming or dropping, we have to create a table for copy data, copy data from original table to the table for copy, drop the original table, create a new table, copy data to the new table and drop the table for copy data.

If this migration function were to be implemented, where do you think this switching stuff should be placed?

Note: currently on my project, as workaround, I rename(or drop) of columns in migration files as follow:

exports.up = function(knex, Promise) {
  if (require('config').database.client === 'mysql') {
    return knex.schema.table('posts', function(t) {
      t.renameColumn('foo', 'bar');
    });
  } else if (require('config').database.client === 'sqlite3') {
    // copy original table
    return knex.raw('create table copy_posts AS select * from posts')
      .then(function() {
        return knex.schema.dropTable('posts');
      })
      .then(function() {
        // create new posts table
        return knex.schema.createTable('posts', function(t) {
          // ...
          // other column definitions
          // ...
          t.string('bar');
        });
      })
      .then(function() {
        return knex.raw('insert into posts select * from copy_posts');
      })
      .then(function() {
        return knex.schema.dropTable('copy_posts');
      });
  }
};
@tgriesser
Copy link
Member

@tgriesser tgriesser commented Feb 19, 2014

Yeah, dropping/changing would be nice. Basically it's just a pain to implement, and because stuff hasn't been quite consistent internally I've held off, because the current implementation (I'm pretty sure rename column works currently) is sort of a huge hack.

This is the new version of renameColumn in the upcoming 0.6.0-wip branch:
https://github.com/tgriesser/knex/blob/6a2e3352a7cb9e6f989e7f572ef7ea1e363ef8e4/lib/clients/sqlite3/schema/tablecompiler.js#L81

I'm going to re-use most of the code there as well in dropping a column / changing types which is something I know @ErisDS was also after. Hopefully should have this release done in the coming weeks.

@p-baleine
Copy link
Contributor

@p-baleine p-baleine commented Feb 19, 2014

@tgriesser

Thanks for your reply.
Your new version of renameColumn is very great! 👍

I am looking forward to your new release.

@ErisDS
Copy link
Contributor Author

@ErisDS ErisDS commented Feb 20, 2014

This is excellent news 👍

Other than watching issues, is there a good way to follow along with progress? A roadmap, mailing list, irc channel or such where all this progress is organised? I'm super keen to keep up to date & help out where possible.

@tgriesser
Copy link
Member

@tgriesser tgriesser commented Feb 20, 2014

Yeah if you want to drop by #bookshelf I'm typically in there (need to advertise that channel a bit). Will look into a roadmap shortly after big changes with the internals have stabilized.

tgriesser added a commit that referenced this issue Jun 9, 2014
Don't throw when trying to add foreign key
Allow dropping a column in sqlite3
@nikku
Copy link

@nikku nikku commented Jul 3, 2014

Looks like dropping foreign key columns is possible nowadays, too.

@ErisDS
Copy link
Contributor Author

@ErisDS ErisDS commented Oct 1, 2014

Not sure if being able to change the length of TEXT columns is on the radar as part of this, so just dropping a note that this is something Ghost needs.

I know that 0.7 is in the works, does that get us any closer towards the 'stablised internals' that were a pre-requisite for getting some 'clever-ass shit' documentation? 😜

@whatyoubendoing
Copy link

@whatyoubendoing whatyoubendoing commented Nov 5, 2014

+1 for changing null & not null on columns

@dkushner
Copy link

@dkushner dkushner commented Feb 1, 2015

@tgriesser, @bendrucker: Has there been any update on this functionality? Writing raw queries in every migration I have that modifies a column (virtually all of them), is getting a bit frustrating.

@bendrucker
Copy link
Member

@bendrucker bendrucker commented Feb 1, 2015

No, sorry

@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented Apr 6, 2016

Ah right, I was just responding to "and would appreciate documentation on how to go about doing this.". No stress. :)

@ivawzh
Copy link

@ivawzh ivawzh commented Apr 16, 2016

+1 for column date type modification

@tkellen
Copy link
Contributor

@tkellen tkellen commented Apr 16, 2016

Honest question. Is this abstraction actually serving a useful purpose? Just write SQL.

Knex should be a query builder, not a migration system. Here is an example of a migration system that makes at least a little sense:

https://github.com/tkellen/node-postgres-ansible-api-boilerplate/blob/master/careen.js
https://github.com/tkellen/node-postgres-ansible-api-boilerplate/blob/master/package.json#L6-L9
https://github.com/tkellen/node-postgres-ansible-api-boilerplate/tree/master/migrations

@ivawzh
Copy link

@ivawzh ivawzh commented Apr 16, 2016

Why do I have to touch SQL? Imagine a case that if one day I want to switch from PostgreSQL to MySQL. How long will it take for me to learn all the gotchas on both side and rewrite all the raw SQL migrations. Plus, if ActiveRecord can do migration right, why Knex can not?

@tkellen
Copy link
Contributor

@tkellen tkellen commented Apr 16, 2016

Switching between MySQL and PostgreSQL seamlessly is a pipe dream that will never happen for anyone on a meaningful scale ever. All database systems have strengths and weaknesses. It makes absolutely no sense to limit yourself to a subset that works across both so you can arbitrarily decide to switch between one or the other.

ActiveRecord doesn't do migrations "right". There is a huge set of functionality it doesn't provide a DSL for. You are right that more human effort has been dumped into ActiveRecord's migration system though. The reason for that is simple numbers.

Knex:
screen shot 2016-04-16 at 10 44 59 am

ActiveRecord:
screen shot 2016-04-16 at 10 44 33 am

My question remains. Where is the value in these DSLs? Anything you want to migrate can be expressed in SQL. You don't need to build some half-baked sugar on top of it. People have been talking about altering columns here for 3 years. Meanwhile SQL has been trucking along doing the same thing just fine for 30.

Also, RE: switching between PostgreSQL and MySQL, what happens if you decide to move from javascript to ruby? How much time will it take you to transcribe all of your SQL migrations from one silly pointless DSL to another? Maybe we should write a portable DSL, you say? That's SQL.

@ricardograca
Copy link
Member

@ricardograca ricardograca commented Apr 17, 2016

@tkellen While I'm more inclined to agree with you these days, I think there is still value in having an interface for easily changing a database's structure from the javascript side.

While the argument about easily changing database servers often comes up, I think realistically that's not the main use case, due to the reasons you mentioned, and because when you create an application you do it with a set of dependencies in mind, including the database server. From my point of view the main benefit is just having a standard interface that allows this functionality in all your apps without having to constantly write the same thing over and over again, or constantly including your own private helper methods.

That is, instead of everybody writing their own code for this, and ending up with a lot of different ways it is accomplished, we would have a sort of standard way of doing it. I know that SQL is already something standard, but it's too generic. Using SQL alone you can do it anywhere and at any time, but this feature is about tailoring it to something more specific, making it more predictable.

Changing a database's structure is probably something that is shared by a great number of programs that use a database, so it makes a good candidate for having a module that does all the heavy lifting.

What's your suggestion to this problem?

@tkellen
Copy link
Contributor

@tkellen tkellen commented Apr 17, 2016

My suggestion is to use something like careen, db-migrate, etc.

Basically, find the most minimal abstraction possible and use it. That means something to run arbitrary SQL files, a facility to track which have been run, and some functionality for rolling them up or down.

In my opinion, migrations never belonged in Knex. I tried to use them years ago, coming from a ruby background using Sequel and ActiveRecord. Like most people in this thread, I imagined that some day we'd have the same level of support here. I even landed some PRs adding functionality to the migration system.

At some point, I recognized two things:

  1. A monumental effort would be required to achieve feature parity with more mature tools.
  2. Mature tools are still wildly incomplete and provide zero value over SQL.

For example, here is a check constraint that prevents an employee from being double-booked on a schedule.

ALTER TABLE utilization ADD CONSTRAINT employee_over_utilized EXCLUDE USING gist (
  employee_id
  WITH =,COALESCE(sketch_calendar_id::text, 'null')
  WITH =,BOX(
    POINT(EXTRACT(EPOCH FROM first_day), EXTRACT(EPOCH FROM first_day)),
    POINT(EXTRACT(EPOCH FROM last_day), EXTRACT(EPOCH FROM last_day))
  )
  WITH &&
);

You can't build constraints like this fluently using any DSL I know of. Even if you could, I still wouldn't expend the effort learning it. SQL does everything you want it to. Use it. Putting a crappy javascript veneer over it isn't helping anyone get anything done.

@ErisDS
Copy link
Contributor Author

@ErisDS ErisDS commented Apr 18, 2016

Switching between MySQL and PostgreSQL seamlessly is a pipe dream that will never happen for anyone on a meaningful scale ever.

Whilst switching a production system from one db to another is hard, it's not impossible. Nonetheless, I think you're totally missing an entire ORM use case here. What about software that allows you to choose your DB when you install it? That's what I'm working on 😉

Knex works really nicely in most places as it provides a great API (or javascript veneer if that's what you want to call it) for the most common use cases, and then it has an escape valve of knex.raw that lets you dropdown to the magic of SQL when you need to go beyond those basics. All good JavaScript libraries work this way - they wrap things up in magic but let you escape when you need to.

When it comes to the migration system, this issue is talking about a handful of common cases that it would be great to have a veneer for because of the complexity of the differences between SQL versions - that is, there is no one SQL query that will work for multiple DBs. It doesn't propose that every possible migration ever should have an abstraction.

@ivawzh
Copy link

@ivawzh ivawzh commented Apr 18, 2016

Thanks, I'm trying out db-migrate now. So far so good.

@tkellen
Copy link
Contributor

@tkellen tkellen commented Apr 18, 2016

Whilst switching a production system from one db to another is hard, it's not impossible. Nonetheless, I think you're totally missing an entire ORM use case here. What about software that allows you to choose your DB when you install it? That's what I'm working on 😉

If I were maintaining Ghost, I would have separate migration files for each RDBMS. Yes, you'd have some minor duplication in your codebase, but how much clarity would it provide for new contributors? Would your existing test suite easily cover failures? Would you be able to more easily leverage the strengths of each respective database you support by dropping this edifice?

More pedantically, this is not an ORM use case.

Knex works really nicely in most places as it provides a great API (or javascript veneer if that's what you want to call it) for the most common use cases, and then it has an escape valve of knex.raw that lets you dropdown to the magic of SQL when you need to go beyond those basics. All good JavaScript libraries work this way - they wrap things up in magic but let you escape when you need to.

Agreed--I see the value in a query builder over munging SQL strings, Knex is great for that!

When it comes to the migration system, this issue is talking about a handful of common cases that it would be great to have a veneer for because of the complexity of the differences between SQL versions - that is, there is no one SQL query that will work for multiple DBs. It doesn't propose that every possible migration ever should have an abstraction.

I see the point you're making here, but it's been three years and we're still talking about altering a column. Also, where do you draw the line at basic? For example, I consider robust check constraints to be a basic tenet of good database design.

@joepie91
Copy link
Contributor

@joepie91 joepie91 commented Sep 7, 2016

So, I've just read through the entire thread, and it's not clear to me what the current status is. Could a maintainer answer the following?

  1. What is the current state of internals documentation? That is, information about how the codebase is structured, why it is structured that way, and so on.
  2. What is the current state of this feature request? What parts have been implemented, what parts have not? Where are potential 'attachment points' for implementing the remainder?
  3. What, if any, are the blockers at this moment for implementing this and sending in a PR? What parts of the internal architecture need changing and how, to make this a possibility?

I'm potentially interested in implementing this and submitting a PR, but before I can commit to doing that, I need to have a better idea of what the current situation is like, and how much work I can expect it to be.

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 7, 2016

@joepie91

  1. There is no other developer documentation, except the code. CONTRIBUTING.md describes some stuff how to run tests etc.
  2. As far as I know there has been no effort to implement this. If one starts to implement this I can help by trying to find some related pull requests where e.g. column renaming is implemented.
  3. I'm not aware of any blockers, why this couldn't be implemented. I don't believe that one has to change internal architecture for this. I would recommend to choose one specific thing what you like to modify and implement that first and then select next thing what to modify and implement that. I suppose that the biggest challenge is to find a correct query for every dialect for certain change.

People who knows better feel free to correct / add info here, I just wanted to answer something to start with so that question won't just stay hanging there in silence...

@ricardograca
Copy link
Member

@ricardograca ricardograca commented Sep 7, 2016

@elhigu What you say is mostly right, but there is some resistance from some collaborators for getting this merged. Tim himself seems to be ok with it if anyone actually does all the work including tests.

The biggest challenge will be in regards to SQLite which doesn't natively support modifying columns, so it requires all sorts of workarounds.

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 8, 2016

@ricardograca We have been supporting APIs which doesn't work on every dialect. Specially with sqlite there are some features, which just throw an exception of not being supported or just ignored like .returning().

So I don't mind if e.g. certain column modify methods doesn't work on sqlite if it is actually pretty much impossible to implement without creating some temporary columns + data migrations to support it.

@RWOverdijk
Copy link
Contributor

@RWOverdijk RWOverdijk commented Oct 27, 2016

A huge 👍 here.

@elhigu
Copy link
Member

@elhigu elhigu commented Feb 16, 2017

Implemented in #1759

@elhigu elhigu closed this Feb 16, 2017
@ErisDS
Copy link
Contributor Author

@ErisDS ErisDS commented Feb 17, 2017

@elhigu I think this is a mistake? There are 4 features requested here:

  1. rename a column
  2. change the data type
  3. add or drop the null constraint
  4. change the default

Only 1 and 3 are implemented. 2 is still a pretty enormous missing feature?

Wait, ignore me - I see the PR that adds this is #1914 - the link confused me totally! This is super exciting

@elhigu
Copy link
Member

@elhigu elhigu commented Feb 17, 2017

@ErisDS yeah, I wanted to link the original PR with most off the discussion, should have linked both :)

@PierBover
Copy link

@PierBover PierBover commented Jun 11, 2017

Ok, so we can alter a table, a column, etc.

Is there a simple way of seeding only to a column?

@elhigu
Copy link
Member

@elhigu elhigu commented Jun 20, 2017

@PierBover what do you mean by "seeding only to a column"?

@PierBover
Copy link

@PierBover PierBover commented Jun 20, 2017

If for example we add a new column, is there a simple way of filling it with values other than using raw queries?

@elhigu
Copy link
Member

@elhigu elhigu commented Jun 20, 2017

@PierBover you either use defaultTo() when creating the column or first create data and then update it with normal knex queries. There is no additional magic in knex for this (and I don't think there should be either).

return knex.schema.alterTable('MyTable', t => {
  // deafault value / expression to seed (one cannot create select queries here)
  t.integer('newColumn').defaultTo(1);
  t.integer('newColumn2').defaultTo(knex.raw('some expression e.g to create random data')); 
}).then(() => {
  // just create populate query after creating new column like normally done with SQL
  return knex('MyTable').update({ newColumn: 2, newColumn2: 3 });
});
@ricardograca
Copy link
Member

@ricardograca ricardograca commented Jun 20, 2017

@PierBover
Copy link

@PierBover PierBover commented Jun 20, 2017

Right @ricardograca but that is for seeding the whole table, no?

Thanks for the suggestion @elhigu 👍

@2rhop
Copy link

@2rhop 2rhop commented Jun 12, 2018

I am starting a new project to seed tables with knex.js, here. Any help will be nice!

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.

None yet
You can’t perform that action at this time.