Support for foreign key constraints (#389) #595

Merged
merged 18 commits into from May 8, 2013

Projects

None yet

9 participants

Contributor
optilude commented May 6, 2013

This implements support for creating FOREIGN KEY ... REFERENCES declarations with ON DELETE and ON UPDATE triggers when defining associations, for all supported databases.

The basic syntax is:

var Task = this.sequelize.define('Task', { title: Sequelize.STRING })
  , User = this.sequelize.define('User', { username: Sequelize.STRING })

User.hasMany(Task, {onDelete: 'restrict'})

The same works for hasOne() and belongsTo(). Valid options are:

  • onDelete -- set to e.g. restrict or cascade
  • onUpdate -- set to e.g. restrict or cascade
  • foreignKeyConstraint -- set to true to gain a REFERENCES declaration without either onDelete or onUpdate (that is, foreignKeyConstraint is implied if onDelete or onUpdate are set). This may be a helpful performance optimisation in some cases.

Some key implementation notes:

  • Enabling constraints is opt-in: they are only created if one or more of the options above are used when defining an association.
  • MySQL (with InnoDB tables) and Postgres support foreign key references by default. SQLite does not unless PRAGMA FOREIGN_KEYS=ON is issued. We do so when opening the database connection, unless explicitly disabled with a global option, to get parity across implementations. Note that just enabling this doesn't make any difference unless there are actual constraints in place.
  • Only simple foreign keys are supported (because associations only support simple keys). This is the "80%" use case of course. Setting one of foreign key options in a situation where there is more than one primary key defined will cause the option to be ignored.
  • SQL allows two ways to define foreign keys: "inline" (someId INTEGER REFERENCES someTable(id) ON DELETE CASCADE) or "standalone" (FOREIGN KEY(someId) REFERENCES someTable(id) ON DELETE CASCADE placed as a separate clause inside a CREATE TABLE statement). Since associations in sequelize create foreign key attributes the "inline" syntax is used, and attributesToSQL is taught how to add the relevant suffix to any "raw" attribute with the relevant metadata attached. This works for Postgres and SQLite, but MySQL ignores this syntax, requiring the "standalone" approach. For MySQL, we move the declaration to the end with a bit of string manipulation. This is analogous to how PRIMARY KEY is handled and allows this to be done without major refactoring.
  • If we have foreign key constraints, the order in which tables are created matters: if foo has a foreign key to bar with a constraint, then bar has to exist before foo can be created. To make sure this happens, we use a topological sort of relationships (via the Toposort-class module, added as a dependency) to sequence calls to CREATE TABLE in sync(). This also necessitates sync() being serialised, but given it's an "on startup" operation that shouldn't be too much of an issue.
  • A similar concern happens with dropAllTables(), but here we don't have enough information to sort the list. Instead, we do one of two things: for SQLite and MySQL, we temporarily disable constraint checking. For Postgres, we use DROP TABLE ... CASCADE to drop relevant constraints when required. (MySQL and SQLite only support the former and Postgres only supports the latter). This is blunt, but OK given that the function is attempting to drop all the tables.
  • For other calls to dropTable() the caller is expected to sequence calls appropriately, or wrap the call in disableForeignKeyConstraints() and enableForeignKeyConstraints() (MySQL and SQLite; no-ops in Postgres) and/or pass {cascade: true} in options (Postgres; no-op in MySQL and SQLite).
  • There are (passing) tests of the various dialect QueryGenerators and of cascade and restrict behaviour at the integration test level.
  • I will add documentation (unless you'd prefer to do it yourself) if/once this PR is accepted, I just don't want to do it yet if there's going to be challenge.
Owner
durango commented May 6, 2013

:+1 @sdepold and @mickhansen this has my vote (looked through the code. The styling seems OK, the only concern is that we're running synchronously in a few places that were previously async, but this I actually support within the PR's context).

@optilude nice job man, this is a huge win, I'd just like to get some other opinions in :)

Contributor
optilude commented May 6, 2013

@durango, glad you could look at it so quickly :)

The serialisation is a necessary evil (you can't create a table with a foreign key constraint to another table if the other table doesn't exist yet - see implementation note on the PR for more details), but it only happens during operations that are very unlikely to be on any sort of performance critical path: on sync() when we're creating new tables, and on dropAllTables().

If you like this one, you can thank me by merging #569 and then Sequelize will (hopefully) do everything I need for my project so I can go back to hacking on that one. ;)

Owner
sdepold commented May 7, 2013

looks awesome. will take a closer look at it this evening. thanks for your work. best PR description ever :)

@sdepold sdepold commented on an outdated diff May 7, 2013
lib/dialects/sqlite/connector-manager.js
@@ -5,7 +5,13 @@ var Utils = require("../../utils")
module.exports = (function() {
var ConnectorManager = function(sequelize) {
this.sequelize = sequelize
- this.database = new sqlite3.Database(sequelize.options.storage || ':memory:')
+ this.database = db = new sqlite3.Database(sequelize.options.storage || ':memory:', function(err) {
+ if(!err && sequelize.options.foreignKeys !== false) {
+ // Make it possible to define and use foreign key constraints unelss
sdepold
sdepold May 7, 2013 Owner

typo "unless"

Owner
sdepold commented May 7, 2013

rocksolid work. @mickhansen @janmeier any thoughts on this ? +1 from me

@janmeier janmeier and 1 other commented on an outdated diff May 7, 2013
spec-jasmine/sqlite/query-generator.spec.js
+ },
+ ],
+
+ createTableQuery: [
+ {
+ arguments: ['myTable', {title: 'VARCHAR(255)', name: 'VARCHAR(255)'}],
+ expectation: "CREATE TABLE IF NOT EXISTS `myTable` (`title` VARCHAR(255), `name` VARCHAR(255));"
+ },
+ {
+ arguments: ['myTable', {title: 'VARCHAR(255)', name: 'VARCHAR(255)'}],
+ expectation: "CREATE TABLE IF NOT EXISTS `myTable` (`title` VARCHAR(255), `name` VARCHAR(255));"
+ },
+ {
+ arguments: ['myTable', {title: 'VARCHAR(255)', name: 'VARCHAR(255)'}],
+ expectation: "CREATE TABLE IF NOT EXISTS `myTable` (`title` VARCHAR(255), `name` VARCHAR(255));"
+ },
janmeier
janmeier May 7, 2013 Owner

This testcase seems to be duplicated thrice or did i miss something?

optilude
optilude May 7, 2013 Contributor

D'uh. It was a test copied from the MySQL implementation and then changed to remove MySQL-specific setting of the table engine, which of course left the three all the same. Corrected.

@janmeier janmeier commented on the diff May 7, 2013
spec/associations/belongs-to.spec.js
+ })
+ })
+ })
+
+ it("can restrict deletes", function(done) {
+ var Task = this.sequelize.define('Task', { title: Sequelize.STRING })
+ , User = this.sequelize.define('User', { username: Sequelize.STRING })
+
+ Task.belongsTo(User, {onDelete: 'restrict'})
+
+ this.sequelize.sync({ force: true }).success(function() {
+ User.create({ username: 'foo' }).success(function(user) {
+ Task.create({ title: 'task' }).success(function(task) {
+ task.setUser(user).success(function() {
+ user.destroy().error(function() {
+ // Should fail due to FK restriction
janmeier
janmeier May 7, 2013 Owner

We might want to check err.code here to make 100% sure that the right error is triggered

optilude
optilude May 7, 2013 Contributor

Do we know what it's supposed to be (in a database-agnostic way)?

@janmeier janmeier commented on an outdated diff May 7, 2013
spec/associations/belongs-to.spec.js
+
+ Task.belongsTo(User, {onUpdate: 'cascade'})
+
+ this.sequelize.sync({ force: true }).success(function() {
+ User.create({ username: 'foo' }).success(function(user) {
+ Task.create({ title: 'task' }).success(function(task) {
+ task.setUser(user).success(function() {
+
+ // Changing the id of a DAO requires a little dance since
+ // the `UPDATE` query generated by `save()` uses `id` in the
+ // `WHERE` clause
+
+ var tableName = user.QueryInterface.QueryGenerator.addSchema(user.__factory)
+ user.QueryInterface.update(user, tableName, {id: 999}, user.id)
+ .success(function() {
+ // Should fail due to FK restriction
janmeier
janmeier May 7, 2013 Owner

Should it? Seems you are asserting here that the update succeeded ;)

@janmeier janmeier commented on an outdated diff May 7, 2013
spec/associations/has-many.spec.js
+
+ User.hasMany(Task, {onUpdate: 'cascade'})
+
+ this.sequelize.sync({ force: true }).success(function() {
+ User.create({ username: 'foo' }).success(function(user) {
+ Task.create({ title: 'task' }).success(function(task) {
+ user.setTasks([task]).success(function() {
+
+ // Changing the id of a DAO requires a little dance since
+ // the `UPDATE` query generated by `save()` uses `id` in the
+ // `WHERE` clause
+
+ var tableName = user.QueryInterface.QueryGenerator.addSchema(user.__factory)
+ user.QueryInterface.update(user, tableName, {id: 999}, user.id)
+ .success(function() {
+ // Should fail due to FK restriction
janmeier
janmeier May 7, 2013 Owner

Seems to be another misplaced comment

@janmeier janmeier commented on the diff May 7, 2013
spec/associations/has-many.spec.js
+ })
+ })
+ })
+
+ it("can cascade updates", function(done) {
+ var Task = this.sequelize.define('Task', { title: Sequelize.STRING })
+ , User = this.sequelize.define('User', { username: Sequelize.STRING })
+
+ User.hasMany(Task, {onUpdate: 'cascade'})
+
+ this.sequelize.sync({ force: true }).success(function() {
+ User.create({ username: 'foo' }).success(function(user) {
+ Task.create({ title: 'task' }).success(function(task) {
+ user.setTasks([task]).success(function() {
+
+ // Changing the id of a DAO requires a little dance since
janmeier
janmeier May 7, 2013 Owner

I'm not quite sure what this dance is about - could you give an example of the dance-/no-dance-queries?

optilude
optilude May 7, 2013 Contributor

So basically, my first attempt was:

user.id = 9999
user.save().success(....)

The problem is that save() generates an UPDATE query like this:

UPDATE Users SET id id = 999 WHERE id = 999

because id is the primary key. So, I need to use the lower level interface.

Owner
janmeier commented May 7, 2013

Truly solid work! My comments are only very, very minor

I don't know what the others think, but given how well-written the description for this PR is, I would have no problem at all letting you write the docs as well! :)

Contributor
optilude commented May 7, 2013

@janmeier I'm happy to write some docs, but I don't understand how they're maintained/how to generate a test site. I need some meta-docs. ;)

Owner
sdepold commented May 8, 2013

@optilude https://github.com/sequelize/sequelize-doc - its a mix of erb and markdown.

@sdepold sdepold merged commit 256db5c into sequelize:master May 8, 2013

1 check passed

default The Travis CI build passed
Details
Contributor

This introduces a global db var.

Suggest:

var db = this.database =
Contributor

nice catch ... alternatively one could just not create the var at all and do this instead:

    this.database.run('PRAGMA FOREIGN_KEYS=ON')

I would love to have some documentation about how exactly to use this extra "feature". Because I believe, but I don't know for sure that I can use this. I have sequelize 2.0.0-alpha2 installed, but I really don't know how to use this.

Any help or further documentation would be great!
Edit: I have got it working right now, but there is a slight problem

I have the next code:

var User = sequelize.define("User", { /* */ });
var Group = sequelize.define("Group", { /* */ });
var Apikey = sequelize.define("Apikey", { /* */ });

Group.hasMany(User);
User.hasMany(Apikey);

Somehow when trying to sync({force:true}) it want to try to drop the tables in this order:

  1. Group
  2. User
  3. Apikey

This turns into the next error:

Cannot drop table Users because it is referenced by Apikeys

Sounds logical, because the apikeys table has a reference to User

The order of dropping should be:

  1. Group
  2. Apikey
  3. User

Because the reference from the Apikeys table to the Users table must be removed first!

Also somehow Many-to-Many relations doesn't create Foreign Keys in the database. Al One-to-Many are executing perfectly!

Owner
sdepold commented Jun 17, 2013

true. will add it later the day

Sascha Depold
Gesendet mit Sparrow (http://www.sparrowmailapp.com/?sig)

Am Donnerstag, 13. Juni 2013 um 23:04 schrieb leroybaeyens:

I would love to have some documentation about how exactly to use this extra "feature". Because I believe, but I don't know for sure that I can use this. I have sequelize 2.0.0-alpha2 installed, but I really don't know how to use this.
Any help or further documentation would be great!


Reply to this email directly or view it on GitHub (#595 (comment)).

It would be nice if we could do something like { onDelete: 'restrict', allowNull: false } to make the foreign key field non-nullable

It seems like there is a fair amount of work going on to try to add the foreign key constraints at the same time that the tables are created. This means we need to try to determine what order to add the tables in, and then insert them synchronously in the right order. Wouldn't it be much easier to just create all the tables/fields, then alter them to add the constraints?

This also would fix a problem I'm having, which is that you cannot create unary relationship constraints the way we're currently doing it (even though it's perfectly valid SQL). For example:

User.hasMany(User, {as: 'Children', foreignKey: 'parentId', useJunctionTable: false, onDelete: 'cascade'})
Error: Cyclic dependency found. 'user' is dependent of itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment