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

sqlite self table FOREIGN KEY constraint failed with ON DELETE RESTRICT when drop()/sync({force:true}) #11583

Open
2 of 7 tasks
Suisse00 opened this issue Oct 20, 2019 · 11 comments
Labels
dialect: sqlite For issues and PRs. Things that involve SQLite (and do not involve all dialects). Great SSCCE This issue has a great SSCCE/MCVE/reprex posted and therefore deserves extra attention! :) P4: nice to have For issues that are not bugs. type: feature For issues and PRs. For new features. Never breaking changes.

Comments

@Suisse00
Copy link

Suisse00 commented Oct 20, 2019

Issue Description

I created a nested table that refers to itself.
When it contains data, trying to execute the .drop() or .sync({force: true}) will trigger a constraint failure from sqlite.

What are you doing?

const Sequelize = require('sequelize');

const sequelize = new Sequelize({dialect: 'sqlite', storage: 'database.sqlite'});

class MyTable extends Sequelize.Model {
}

MyTable.init({
    parentId: {
        type: Sequelize.INTEGER,
        onDelete: 'RESTRICT',
        references: {
            model: MyTable,
            key: 'id'
        }
    }
}, { sequelize, modelName: 'Alarms'});

sequelize
    .sync({force: true})
    // root
    .then(_ => MyTable.build().save()
        // child
        .then(rootModel => MyTable.build({parentId: rootModel.id}).save()))

    // BOOM
    .then(_ => sequelize.sync({force: true}))
    .catch(error => console.error(error));

will output

CREATE TABLE "Alarms" (
	"id"	INTEGER PRIMARY KEY AUTOINCREMENT,
	"parentId"	INTEGER,
	"createdAt"	DATETIME NOT NULL,
	"updatedAt"	DATETIME NOT NULL,
	FOREIGN KEY("parentId") REFERENCES "Alarms"("id") ON DELETE RESTRICT
);

INSERT INTO "Alarms" (id, parentId, createdAt, updatedAt) VALUES(1, NULL, datetime('now'), datetime('now'));
INSERT INTO "Alarms" (id, parentId, createdAt, updatedAt) VALUES(2, 1, datetime('now'), datetime('now'));

What do you expect to happen?

Since I'm deleting the whole table, FK to the same table shouldn't trigger this FK constraint.

Note: I don't have a SQLite background, only a MSSQL/MYSQL. So this behavior is strange to me.

I did look into DROP TABLE documentation and they indeed talk about this behavior.

If foreign key constraints are enabled, a DROP TABLE command performs an implicit DELETE FROM command before removing the table from the database schema. [...] An implicit DELETE FROM does cause any configured foreign key actions to take place.

I could see why this ticket could be closed as won't do.

Environment

  • Sequelize version: 5.21.1
  • Node.js version: 10.13.0
  • Operating System: Win64

Issue Template Checklist

How does this problem relate to dialects?

  • I think this problem happens regardless of the dialect.
  • I think this problem happens only for the following dialect(s): sqlite
  • I don't know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I don't know how to start, I would need guidance.
  • No, I don't have the time, although I believe I could do it if I had the time...
  • No, I don't have the time and I wouldn't even know how to start.
@papb
Copy link
Member

papb commented Oct 21, 2019

Thanks for the report, can you please convert your code into a sequelize-sscce so that we can easily see it working on all dialects except sqlite?

@papb papb added the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Oct 21, 2019
@Suisse00
Copy link
Author

Done ( sequelize/sequelize-sscce#18 )
Out of the 4 dialects only the SQLite failed.

@papb
Copy link
Member

papb commented Oct 21, 2019

Thanks! Great SSCCE.

Do you have any suggestion on how Sequelize should handle this? Should Sequelize remove the foreign key constraint before deleting?

@papb papb added dialect: sqlite For issues and PRs. Things that involve SQLite (and do not involve all dialects). Great SSCCE This issue has a great SSCCE/MCVE/reprex posted and therefore deserves extra attention! :) type: feature For issues and PRs. For new features. Never breaking changes. labels Oct 21, 2019
@papb
Copy link
Member

papb commented Oct 21, 2019

I've tagged this as a feature because it would be something extra that Sequelize is doing since SQLite doesn't do it by itself.

@Suisse00
Copy link
Author

Suisse00 commented Oct 21, 2019

I'm new to both SQLite and sequelize so I can't suggest the best approach.

I did Google a little bit before submitting my suggestions and they did make no sense... SQLite doesn't support to drop a column or FK... oh... crap...

So the only suggestion that left on my list is: Parse the FK constraint and delete rows ourselves in the right order. To this suggestion, I added the note (before googling): "at this point, it should probably be a user addon in a different NPM or the user should switch to ON DELETE CASCADE"

So I totally agree with you about the feature label! (if you still go ahead, and if there is nobody that already did this somewhere).

After thinking about it my recommendation would be: Close the issue. I should have know SQLite limit.

@papb
Copy link
Member

papb commented Oct 22, 2019

@Suisse00 I see... Thanks :)

I will leave it open because perhaps it might be interesting to try to implement a workaround for this in Sequelize, however it would be much better if SQLite supported this natively; so I will tag this as a low priority feature.

@papb papb added the P4: nice to have For issues that are not bugs. label Oct 22, 2019
@romannep
Copy link

For some cases could help turning off foreign key constrait

await sequelize.query('PRAGMA foreign_keys = false;');
sequelize.sync({...});
await sequelize.query('PRAGMA foreign_keys = true;');

@papb
Copy link
Member

papb commented Nov 4, 2019

@romannep Hmmm, perhaps this could be built-in the force: true option for SQLite? Do you think it's too risky? I think it could be done, force: true is already risky anyway

@louy2
Copy link
Contributor

louy2 commented Apr 21, 2020

I think it is reasonable to build this into force: true for SQLite.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2021

This issue has been automatically marked as stale because it has been open for 7 days without activity. It will be closed if no further activity occurs. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

@github-actions github-actions bot added the stale label Nov 6, 2021
@WikiRik WikiRik removed the stale label Nov 15, 2021
@silencej
Copy link

silencej commented Jan 6, 2022

https://stackoverflow.com/a/5987838/6191913
SQLite 2021-03-12 (3.35.0) now supports DROP COLUMN.

Shall we update this accordingly?

@github-actions github-actions bot removed the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialect: sqlite For issues and PRs. Things that involve SQLite (and do not involve all dialects). Great SSCCE This issue has a great SSCCE/MCVE/reprex posted and therefore deserves extra attention! :) P4: nice to have For issues that are not bugs. type: feature For issues and PRs. For new features. Never breaking changes.
Projects
None yet
Development

No branches or pull requests

6 participants