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

feat: fix multiple bugs related to sync() and indexes. Add Model.getIndexes() #14619

Merged
merged 23 commits into from Jun 13, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Jun 10, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

Closes #12889

This PR fixes the following bugs:

  • DB2 forced columns that had a unique index to be non-null, this has been removed (you still need to make your column non-null, but we don't silently make that happen).
  • When defining an index through the indexes option on a model, then calling sync(), the dialect would modify the model to mark all attributes that are part of the index as unique. (DB2 only)
  • Calling sync({ alter: true }) with an attribute that has unique: true set would generate a query such as ALTER TABLE users ALTER COLUMN emails SET VARCHAR UNIQUE. DB2 and MSSQL do not allow UNIQUE to be specified there. This has been changed to use a separate CREATE INDEX query.
  • SQLite supports specifying an index name during CREATE TABLE, but that name is ignored by sqlite. Causing subsequent sync() to try to recreate the index. The first sync does not try to create the index as part of CREATE TABLE anymore, but executes CREATE INDEX afterwards to create them (sqlite only, other dialects still do it during CREATE TABLE).
  • This PR adds Model.getIndexes() which lists all indexes defined on the model (it regroups all indexes, the ones defined through ModelOptions.indexes, as well as the ones coming from ModelAttributeColumnOptions.unique)

This PR blocks #14572 (comment)

TODO

@ephys ephys added type: bug dialect: db2 For issues and PRs. Things that involve db2 (and do not involve all dialects). labels Jun 10, 2022
@ephys ephys self-assigned this Jun 10, 2022
@ephys ephys changed the title fix: fix multiple bugs related to sync() and indexes in db2 fix: fix multiple bugs related to sync() and indexes in db2, mssql, sqlite Jun 10, 2022
@ephys ephys marked this pull request as draft June 10, 2022 20:59
@ephys

This comment was marked as outdated.

@ephys ephys changed the title fix: fix multiple bugs related to sync() and indexes in db2, mssql, sqlite fix: fix multiple bugs related to sync() and indexes Jun 10, 2022
@ephys ephys changed the title fix: fix multiple bugs related to sync() and indexes feat: fix multiple bugs related to sync() and indexes. Add Model.getIndexes() Jun 11, 2022
@ephys ephys marked this pull request as ready for review June 11, 2022 19:01
@ephys
Copy link
Member Author

ephys commented Jun 11, 2022

You're going to laugh. I've deadlocked my PRs.

One of the tests is failing because DB2 is recreating the table instead of altering it.
This is a DB2 bug I fixed in #14572
That PR is blocked because it relies on this one to fix DB2 bugs, and this one is blocked because it relies on that one.

This isn't a regression, so I'm requesting to force-merge this PR, and continue over at #14572

@ephys ephys requested review from sdepold and WikiRik June 11, 2022 19:02
@ephys ephys requested a review from a team June 11, 2022 19:02
@WikiRik
Copy link
Member

WikiRik commented Jun 11, 2022

Can we temporarily disable the failing unit tests so only DB2 checks fail for this PR and re-enable them in the other PR? Not a fan of force merging

Will review tomorrow

@ephys
Copy link
Member Author

ephys commented Jun 11, 2022

Can we temporarily disable the failing unit tests

I fixed them, these weren't supposed to fail.
The only tests that were 'supposed' to fail were the DB2 integration ones, which funnily enough I cannot reproduce locally so there is more flakiness going on (still need to complete #14453)

Edit: looks like all remaining tests are flaky. I guess we're good to go

@ephys ephys mentioned this pull request Jun 12, 2022
3 tasks
Copy link
Member

@sdepold sdepold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always surprised by how much you know about the library and in which corners you can stir :)

src/dialects/sqlite/query-generator.js Show resolved Hide resolved
src/model.js Show resolved Hide resolved
src/dialects/sqlite/query-generator.js Show resolved Hide resolved
src/model.js Show resolved Hide resolved
src/model.js Show resolved Hide resolved
test/integration/model/sync.test.js Show resolved Hide resolved
@ephys
Copy link
Member Author

ephys commented Jun 13, 2022

Let me handle the merging of this PR, I'll add a detailed message for the changelog

@ephys ephys merged commit f5c9a90 into main Jun 13, 2022
@ephys ephys deleted the ephys/fix-db2-unique branch June 13, 2022 10:02
@github-actions
Copy link
Contributor

🎉 This PR is included in version 7.0.0-alpha.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Duplicated unique constraints and indexes on sequelize.sync({ alter: true })
3 participants