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

Database migrations fail to add foreign key #966

Closed
sebastianhoitz opened this Issue Oct 7, 2013 · 65 comments

Comments

@sebastianhoitz
Contributor

sebastianhoitz commented Oct 7, 2013

I have an integer column called orderId.

Now I want to add a foreign key to it:

module.exports = {
  up: function(migration, DataTypes, done) {
    migration.changeColumn("orderitems", "orderId", 
      {
        type: DataTypes.INTEGER,
        references: "orders",
        referenceKey: "id",
        onUpdate: "CASCADE",
        onDelete: "RESTRICT"
      }
    ).complete(done);
  }
}

but the migration fails with this:

{ '0':
   { [error: syntax error at or near "REFERENCES"]
     length: 93,
     name: 'error',
     severity: 'ERROR',
     code: '42601',
     detail: undefined,
     hint: undefined,
     position: '185',
     internalPosition: undefined,
     internalQuery: undefined,
     where: undefined,
     file: 'scan.l',
     line: '1002',
     routine: 'scanner_yyerror' } }
Completed in 9ms

events.js:74
        throw TypeError('Uncaught, unspecified "error" event.');
              ^
TypeError: Uncaught, unspecified "error" event.
    at TypeError (<anonymous>)
    at EventEmitter.emit (events.js:74:15)
    at null.<anonymous> (/Users/hoitz/develop/salad/node_modules/sequelize/lib/migrator.js:95:44)
    at EventEmitter.emit (events.js:98:17)
    at module.exports.finish (/Users/hoitz/develop/salad/node_modules/sequelize/lib/query-chainer.js:138:30)
    at exec (/Users/hoitz/develop/salad/node_modules/sequelize/lib/query-chainer.js:92:16)
    at onError (/Users/hoitz/develop/salad/node_modules/sequelize/lib/query-chainer.js:72:11)
    at EventEmitter.emit (events.js:95:17)
    at /Users/hoitz/develop/salad/node_modules/sequelize/lib/migration.js:65:19
    at null.<anonymous> (/Users/hoitz/develop/salad/node_modules/sequelize/lib/emitters/custom-event-emitter.js:52:38)

The error is caused by this query:

ALTER TABLE "orderitems" 
ALTER COLUMN "orderId" TYPE INTEGER REFERENCES "orders" ("id") 
ON DELETE RESTRICT 
ON UPDATE CASCADE;

which is invalid SQL. It should add a constraint to the column.

@theoptz

This comment has been minimized.

theoptz commented May 19, 2014

I try to add new column and foreign key

migration.addColumn('user', 'level_id', {
  type: DataTypes.INTEGER.UNSIGNED,
  references: 'level',
  referenceKey: 'id',
  onUpdate: 'cascade',
  onDelete: 'restrict'
});

The column will be successfully created but without foreign key.
Does it support now?
Thanks

@janmeier

This comment has been minimized.

Member

janmeier commented May 19, 2014

@theoptz No, this issues is not resolved yet. The current code only handles foreign keys in the context of table creation, not column updates.

@dgmike

This comment has been minimized.

dgmike commented May 26, 2014

👍

👀 Waiting for this issue

@ee99ee

This comment has been minimized.

ee99ee commented Jul 24, 2014

👍

2 similar comments
@eanglay

This comment has been minimized.

eanglay commented Jul 25, 2014

👍

@TimmyCarbone

This comment has been minimized.

TimmyCarbone commented Aug 26, 2014

👍

@yangchristian

This comment has been minimized.

yangchristian commented Aug 27, 2014

👍 Would be great to be able to remove foreign keys as well. I am converting a 1:many relationship to a many:many and need to remove the original foreign key constraint.

@sampatbadhe

This comment has been minimized.

sampatbadhe commented Aug 28, 2014

👍

2 similar comments
@jlouthan

This comment has been minimized.

jlouthan commented Aug 29, 2014

👍

@cusspvz

This comment has been minimized.

cusspvz commented Sep 1, 2014

👍

@cusspvz

This comment has been minimized.

cusspvz commented Sep 1, 2014

Also fail on migration.renameColumn if it has a constraint.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 9, 2014

Not related to original issue but addColumn should now support foreign key constraints as of 0c1ec1c

@mickhansen mickhansen removed this from the 2.0.0 release candidate milestone Sep 10, 2014

@noisyneuron

This comment has been minimized.

noisyneuron commented Sep 21, 2014

I'm using v2.0-rc1, and addColumn still doesn't create the foreign key - is this supposed to functional in this release?

This is the migration code:

migration.addColumn('Paintings', 'ArtistId', {
  type: DataTypes.INTEGER,
  references: 'Artists',
  referencesKey: 'id',
  onUpdate: 'CASCADE',
  onDelete: 'RESTRICT'
});
@janmeier

This comment has been minimized.

Member

janmeier commented Sep 21, 2014

The issue is still open, so yes, it's still an issue :)

@noisyneuron

This comment has been minimized.

noisyneuron commented Sep 22, 2014

:) That was in reference to the comment above : Not related to original issue but addColumn should now support foreign key constraints as of 0c1ec1c.

Original issue was about changeColumn , comment seemed to suggest using addColumn instead would work now.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 22, 2014

Well the unit test added in that commit passes, so addColumn is partially implemented atleast.
Please provide a pull request with a failing unit test and i'll get a fix in for you.

@herlon214

This comment has been minimized.

herlon214 commented Oct 9, 2014

Waiting too
👍

@WoLfulus

This comment has been minimized.

WoLfulus commented Oct 16, 2014

Me too.

@jfacorro

This comment has been minimized.

jfacorro commented Dec 9, 2014

+1

4 similar comments
@burakkilic

This comment has been minimized.

burakkilic commented Dec 24, 2014

+1

@phillip-elm

This comment has been minimized.

phillip-elm commented Dec 29, 2014

+1

@cesarandreu

This comment has been minimized.

cesarandreu commented Jan 5, 2015

+1

@Rastopyr

This comment has been minimized.

Rastopyr commented Jan 9, 2015

+1

@jlk227

This comment has been minimized.

jlk227 commented Jan 9, 2015

+1 +1

@jlk227

This comment has been minimized.

jlk227 commented Jan 12, 2015

I'm using sequelize "^2.0.0-rc2" and sequelize-cli "^0.3.3". I just added foreign key constrain on my table using migration.createTable.
migration.createTable('order_items',{order_id: {references:"orders", "referencesKey": "id"}}). Both words have "s".

@andrewhassan

This comment has been minimized.

andrewhassan commented Jun 9, 2015

Awesome!

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jun 10, 2015

@corbanb You are right, the original issue had a typo referenceKey instead of referencesKey.
I'll close this unless someone posts new code that still doesn't work.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jun 10, 2015

@corbanb Hmm, although the original issue deals with changeColumn not addColumn, so still might not work.

@bottleofwater

This comment has been minimized.

bottleofwater commented Jun 11, 2015

@mickhansen nope, changeColumn still doesn't work:

Executing (default): ALTER TABLE "Table" ALTER COLUMN "Column" TYPE UUID REFERENCES "OtherTable" ("uid") ON DELETE CASCADE ON UPDATE CASCADE;

SequelizeDatabaseError: syntax error at or near "REFERENCES"
@dantman

This comment has been minimized.

Contributor

dantman commented Aug 28, 2015

Annoyingly this applies to any change to a column with a foreign key, not just addition/removal of a foreign key from a column.

So it's impossible to create allowsNull: false if your table already has rows. (Since you can't set it on addColumn since all rows in the table would violate the constraint and attempting to change it to addNull after filling up all the rows lands you in this issue).

@dantman

This comment has been minimized.

Contributor

dantman commented Aug 28, 2015

Ok, I've come to a realization. Me and everyone else here are wrong.

Neither pg nor MySQL actually support REFERENCES in ALTER COLUMN. pg emits an error. MySQL may "say" it's valid syntax, but InnoDB actually ignores it.

You can modify a column that has a reference, all you do is exclude the reference information from the column options. The ALTER COLUMN operation does not modify the foreign key.

The proper way to modify foreign key references is using ADD [CONSTRAINT [symbol]] FOREIGN KEY ( column_name [, ... ] ) REFERENCES ... and DROP FOREIGN KEY (mysql) / DROP CONSTRAINT [ IF EXISTS ] constraint_name (pg).

So the real result for this bug should be:

  • An extra set of migration functions should be added to handle references, something like addColumnReference / dropColumnReference.
  • changeColumn should throw an error if the options contains references. Something like "reference cannot be used in changeColumn, use addColumnReference to add new foreign key references to a column".
@legomind

This comment has been minimized.

Contributor

legomind commented Sep 12, 2015

Is there anyone working on a pull request for this, or should I work on a PR myself?

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 14, 2015

@legomind No work is currently being done, a PR would be great!

@nestorneto

This comment has been minimized.

nestorneto commented Nov 7, 2015

nothing yet?

@andyskw

This comment has been minimized.

andyskw commented Nov 29, 2015

+1

1 similar comment
@dgurkaynak

This comment has been minimized.

dgurkaynak commented Dec 1, 2015

+1

@theptrk

This comment has been minimized.

theptrk commented Dec 3, 2015

@corbanb Thanks for that code snippet. I tried to associate a new model with my User model, like you did with Post and it only worked when I changed User to the plural form "Users".
Is this the desired API? Because I flipped the table when I finally figured this out.

@jocull

This comment has been minimized.

jocull commented Dec 9, 2015

+1 still broken in ^3.12.1

@jocull

This comment has been minimized.

jocull commented Dec 9, 2015

Can anyone chime in with what dialects they had issues with specifically? We hit this in MySQL (and related issues in SQLite which has no alter table support). I'd like for a pull request to tackle each affected dialect.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Dec 10, 2015

@jocull All dialects fail. Constraints can't be added in a single add column call - So the logic needs to be changed to do two calls internally, or we add addConstraint methods as we've planned and force users to use that :)

@jocull

This comment has been minimized.

jocull commented Dec 10, 2015

I submitted pull request #5014 in an attempt to deal with this. Can anyone else help me finally fix this?

@sushantdhiman sushantdhiman referenced this issue Jan 15, 2016

Merged

Fix 966 #5227

@Wtower

This comment has been minimized.

Wtower commented Jan 15, 2016

+1

@jifeon jifeon referenced this issue Feb 24, 2016

Merged

Postgres version and base image updated #4

1 of 1 task complete
@jamespedid

This comment has been minimized.

jamespedid commented Jun 9, 2016

Not sure if this should be reopened or if a new ticket should be made, but the constraint isn't properly removed when a migration undo is done:

module.exports = {
    up: function (queryInterface, Sequelize) {
        return queryInterface.changeColumn('access_tokens', 'user_id', {
            type: Sequelize.UUID,
            allowNull: Sequelize,
            field: 'user_id',
            references: {
                model: 'users',
                key: 'id'
            }
        })
    },

    down: function (queryInterface, Sequelize) {
        return queryInterface.changeColumn('access_tokens', 'user_id', {
            type: Sequelize.UUID,
            allowNull: Sequelize,
            field: 'user_id'
        })
    }
};

The up-migration here properly adds the keys, but the down migration doesn't remove them.

@Budry

This comment has been minimized.

Budry commented Aug 25, 2016

I have a same problem as @jamespedid. I use version 3.24.11 and mysql

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Aug 25, 2016

@jamespedid open a new ticket

@sushantdhiman

This comment has been minimized.

Member

sushantdhiman commented Aug 25, 2016

No need to open ticket @jamespedid , Its already in milestone v4 , #5212

@Budry

This comment has been minimized.

Budry commented Aug 29, 2016

@sushantdhiman I'm not sure if it solves my problem too. I have this code:

module.exports = {
  up: function(queryInterface, Sequelize) {
    return queryInterface.changeColumn('accounts', 'user_id', {
      type: Sequelize.INTEGER,
      onDelete: 'CASCADE',
      onUpdate: 'SET NULL'
    });
  },

  down: function(queryInterface, Sequelize) {
    return queryInterface.changeColumn('accounts', 'user_id', {
      type: Sequelize.INTEGER,
      onDelete: 'CASCADE',
      onUpdate: 'CASCADE'
    });
  }
};

Change is only on onUpdate from CASCADE to SET NULL, but after run sequelize db:migrate is created a new foreign key on user_id columns and old and wrong fk is still there

@sushantdhiman

This comment has been minimized.

Member

sushantdhiman commented Aug 29, 2016

@Budry with #5212 you will be able to do that by pairing removeConstraint and addConstraint

@Budry

This comment has been minimized.

Budry commented Aug 30, 2016

@sushantdhiman thanks, you are right. Can you help me with this problem now? Exist any options how do this now? I can of course remove column and add again with correct properties, but I need keep data in table during migrations and this would be unpleasantly.

@sushantdhiman

This comment has been minimized.

Member

sushantdhiman commented Aug 30, 2016

@Budry AFAIK no one is working on #5212, For now you can issue raw constraint change queries if you want to. Or you can help by working on #5212

@Budry

This comment has been minimized.

Budry commented Aug 30, 2016

@sushantdhiman thanks a lot.

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