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.

Sign up for GitHub

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

Jump to bottom

fix(sqlite): describeTable now returns unique constraint and references #12420

Merged
merged 2 commits into from
Jun 28, 2020
Merged

fix(sqlite): describeTable now returns unique constraint and references #12420

merged 2 commits into from
Jun 28, 2020

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented Jun 24, 2020 •
edited

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Fixes #7078

For the sqlite dialect, I added an additional query for describeTable to get the constraints for the columns via showIndexes. I also added a new test for this behavior, and updated existing tests to reflect the new behavior. Currently, this only fixes unique constraints.

I'm intentionally making this a draft until we can fix the foreign key thing because I don't know how those work, and so we can put this fix in v5 as well.

I'm also not really sure if the changes I made to the tests are valid

@dyc3 dyc3 changed the base branch from master to v5 June 24, 2020 20:44
@dyc3 dyc3 changed the base branch from v5 to master June 24, 2020 20:44
@sushantdhiman
Copy link
Contributor

Tests are failing

@dyc3
Copy link
Contributor Author

dyc3 commented Jun 26, 2020

Other dialects appear to be failing on the new tests that I created. It appears other dialects don't return the unique constraint on columns when you run describeTables(). Should we add that to other dialects? Or just only run the new tests for the sqlite dialect?

@dyc3 dyc3 force-pushed the fix-sqlite-change-column branch from be3dff4 to 250c74e Compare June 27, 2020 02:11
@dyc3
Copy link
Contributor Author

dyc3 commented Jun 27, 2020

Also, should I rebase this branch to target v5 instead? Or would it be easier to make an additional PR targeting v5?

@dyc3 dyc3 force-pushed the fix-sqlite-change-column branch 2 times, most recently from ed46562 to 5bceb3f Compare June 27, 2020 04:33
@sushantdhiman
Copy link
Contributor

master is fine for now, after this is merged you may open similar PR for v5 branch

@dyc3 dyc3 force-pushed the fix-sqlite-change-column branch from 5bceb3f to 3be8877 Compare June 27, 2020 05:10
@dyc3
Copy link
Contributor Author

dyc3 commented Jun 27, 2020

I'm unsure why the tests are failing now, I tried running the postgres dialect integration tests on the master branch and those aren't successful. I'm unable to run the integration tests for mssql, mysql, and mariadb for some reason.

@dyc3
Copy link
Contributor Author

dyc3 commented Jun 27, 2020

Oh, nevermind. It looks like just appveyor is failing now. Something to do with uploading code coverage?

@dyc3
Copy link
Contributor Author

dyc3 commented Jun 27, 2020

I'll start working on preserving foreign keys tomorrow, if I can figure out how that works.

@dyc3 dyc3 force-pushed the fix-sqlite-change-column branch from ae8cef1 to b5f8711 Compare June 27, 2020 19:38
@dyc3 dyc3 changed the title fix(sqlite): describeTable now returns unique constraint fix(sqlite): describeTable now returns unique constraint and references Jun 27, 2020
sushantdhiman
sushantdhiman reviewed Jun 28, 2020
View reviewed changes
lib/dialects/sqlite/query-generator.js
@@ -418,7 +418,8 @@ class SQLiteQueryGenerator extends MySqlQueryGenerator {
).join(', ');
const attributeNamesExport = Object.keys(attributes).map(attr => this.quoteIdentifier(attr)).join(', ');

return `${this.createTableQuery(backupTableName, attributes).replace('CREATE TABLE', 'CREATE TEMPORARY TABLE')
// Temporary tables don't support foreign keys, so creating a temporary table will not allow foreign keys to be preserved
return `${this.createTableQuery(backupTableName, attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@sushantdhiman
Copy link
Contributor

LGTM @dyc3 , ready for merge?

@dyc3 dyc3 marked this pull request as ready for review June 28, 2020 04:15
@dyc3
Copy link
Contributor Author

dyc3 commented Jun 28, 2020

@sushantdhiman Yup!

@sushantdhiman sushantdhiman merged commit 2de3377 into sequelize:master Jun 28, 2020
@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 6.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dyc3 dyc3 mentioned this pull request Jun 28, 2020
Merged
6 tasks
This was referenced Jul 10, 2020
Open
Open
@rickbergfalk rickbergfalk mentioned this pull request Nov 10, 2021
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sushantdhiman sushantdhiman sushantdhiman approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sqlite remove/change/rename column don't pass foreign key references along in the new table
2 participants
@dyc3 @sushantdhiman