-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix(query_generator.js): fixes duplicate unique index #8582
Conversation
Removes the unique index from the dataType script, thus not causing the duplicate indexes to be created issue-6134
@@ -117,6 +117,10 @@ if (dialect === 'mysql') { | |||
expectation: 'CREATE TABLE IF NOT EXISTS `myTable` (`title` VARCHAR(255), `name` VARCHAR(255)) ENGINE=MyISAM;' | |||
}, | |||
{ | |||
arguments: ['myTable', {title: 'VARCHAR(255) UNIQUE', name: 'VARCHAR(255)'}, {engine: 'MyISAM'}], | |||
expectation: 'CREATE TABLE IF NOT EXISTS `myTable` (`title` VARCHAR(255), `name` VARCHAR(255)) ENGINE=MyISAM;' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't UNIQUE be added to this query? Do we create an index now, if thats the case you need to write an integration test that will create a table and assert only one index is created per unique
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood correctly the UNIQUE here causes a duplication as it is added in
sequelize/lib/model.js
Line 806 in 47b3f61
if (definition.hasOwnProperty('unique') && definition.unique !== false) {
As far as I can see, integration/create.test.js
already contains tests for the unique index and the change doesn't break them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid now if anyone try to use createTable
with unique: true
attribute it wont create any constraint. Sync handle this but createTable
doesnt.
Can you add a test for this, create a table with createTable
having a unique attribute and verify if unique index is still created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sushantdhiman you are right in case of createTable
it doesn't and the unique
constraint, do you have any suggestions maybe? The naive one would be to check the caller, if I will come up with better one will let you know :)
#6134
Pull Request check-list
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
Removes the unique index from the dataType script, thus not causing the duplicate indexes to be
created