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

Database migrations fail to add foreign key #966

Closed
sebastianhoitz opened this issue Oct 7, 2013 · 67 comments · Fixed by #5227
Closed

Database migrations fail to add foreign key #966

sebastianhoitz opened this issue Oct 7, 2013 · 67 comments · Fixed by #5227
Assignees
Labels
P1: important For issues and PRs. type: bug

Comments

@sebastianhoitz
Copy link
Contributor

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

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

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

dgmike commented May 26, 2014

👍

👀 Waiting for this issue

@ee99ee
Copy link

ee99ee commented Jul 24, 2014

👍

2 similar comments
@eanglay
Copy link

eanglay commented Jul 25, 2014

👍

@TimmyCarbone
Copy link

👍

@yangchristian
Copy link

👍 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
Copy link

👍

2 similar comments
@jlouthan
Copy link

👍

@cusspvz
Copy link

cusspvz commented Sep 1, 2014

👍

@cusspvz
Copy link

cusspvz commented Sep 1, 2014

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

@mickhansen
Copy link
Contributor

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

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

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

@noisyneuron
Copy link

:) 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
Copy link
Contributor

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

Waiting too
👍

@WoLfulus
Copy link

Me too.

@jfacorro
Copy link

jfacorro commented Dec 9, 2014

+1

4 similar comments
@burakkilic
Copy link

+1

@phillip-elm
Copy link

+1

@cesarandreu
Copy link

+1

@Rastopyr
Copy link

Rastopyr commented Jan 9, 2015

+1

@jlk227
Copy link

jlk227 commented Jan 9, 2015

+1 +1

@jlk227
Copy link

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".

@mickhansen
Copy link
Contributor

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

@bottleofwater
Copy link

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

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

@janmeier
Copy link
Member

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

@nextor2k
Copy link

nextor2k commented Nov 7, 2015

nothing yet?

@andyskw
Copy link

andyskw commented Nov 29, 2015

+1

1 similar comment
@dgurkaynak
Copy link

+1

@theptrk
Copy link

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

jocull commented Dec 9, 2015

+1 still broken in ^3.12.1

@jocull
Copy link

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
Copy link
Contributor

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

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 mentioned this issue Jan 15, 2016
@Wtower
Copy link

Wtower commented Jan 15, 2016

+1

@jamespedid
Copy link

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

Budry commented Aug 25, 2016

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

@felixfbecker
Copy link
Contributor

@jamespedid open a new ticket

@sushantdhiman
Copy link
Contributor

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

@Budry
Copy link

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
Copy link
Contributor

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

@Budry
Copy link

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
Copy link
Contributor

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

Budry commented Aug 30, 2016

@sushantdhiman thanks a lot.

@flouet-company
Copy link

After reading through this thread, it doesn't seem this has been implemented yet or am I incorrect?

@theptrk
Copy link

theptrk commented Oct 5, 2020

@flouet-company should be implemented based on #7108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1: important For issues and PRs. type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.