Skip to content

Added unique indexes to rawAttributes #7196

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
Merged
merged 4 commits into from
Feb 19, 2017
Merged

Added unique indexes to rawAttributes #7196

merged 4 commits into from
Feb 19, 2017

Conversation

harshithkashyap
Copy link

@harshithkashyap harshithkashyap commented Feb 3, 2017 •
edited
Loading

Pull Request check-list

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does your issue contain a link to 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)?
  • Have you added an entry under Future in the changelog?

Description of change

indexes defined via the following snippet are not added to Model.rawAttributes.

const User = current.define('User', {
    name: DataTypes.STRING,
    address: DataTypes.STRING
}, {
    indexes: [{
        unique: true,
        fields: ['name', 'address']
    }]
});

Model.upsert query uses rawAttributes to find primary/unique indexes and hence the upsert query would fail in case of the above model definition.

Fixes #6842 (comment)

@harshithkashyap
Copy link
Author

Ping @sushantdhiman @felixfbecker @janmeier

@codecov-io
Copy link

codecov-io commented Feb 3, 2017 •
edited
Loading

Codecov Report

Merging #7196 into master will decrease coverage by -1.72%.
The diff coverage is 100%.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e22ce18...be32db4. Read the comment docs.

sushantdhiman
sushantdhiman reviewed Feb 7, 2017
View reviewed changes
lib/model.js Outdated
index = this._conformIndex(index);
//Add unique indexes to rawAttributes
if (index.unique && index.fields) {
index.fields.forEach(field => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen in case of unique index on two fields (composite unique index), will it mark both fields as unique? I would like to see a test which works in case of composite unique index

Copy link
Author

@harshithkashyap harshithkashyap Feb 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@sushantdhiman Composite index also works. I have added an unit test for it but I'll add an integration test aswell.

felixfbecker
felixfbecker suggested changes Feb 7, 2017
View reviewed changes
lib/model.js Outdated
index = this._conformIndex(index);
//Add unique indexes to rawAttributes
if (index.unique && index.fields) {
index.fields.forEach(field => {
Copy link
Contributor

Choose a reason for hiding this comment

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

use for of

lib/model.js Outdated
//Add unique indexes to rawAttributes
if (index.unique && index.fields) {
index.fields.forEach(field => {
const fieldName = (typeof field === 'string') ? field : (field.name || field.attribute);
Copy link
Contributor

Choose a reason for hiding this comment

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

unneeded parens and space

@harshithkashyap harshithkashyap force-pushed the add-indexes-to-raw-attributes branch 3 times, most recently from 30c9b1f to d90bc38 Compare February 7, 2017 16:26
@harshithkashyap harshithkashyap force-pushed the add-indexes-to-raw-attributes branch from d90bc38 to be32db4 Compare February 7, 2017 16:51
@harshithkashyap
Copy link
Author

@sushantdhiman Looks like the deadlocks still occur. Was this not fixed by #7131?

@sushantdhiman sushantdhiman merged commit 943bdf2 into sequelize:master Feb 19, 2017
@harshithkashyap harshithkashyap deleted the add-indexes-to-raw-attributes branch February 19, 2017 10:46
mkaufmaner pushed a commit to mkaufmaner/sequelize that referenced this pull request Apr 25, 2017
@mkaufmaner
Added unique indexes to rawAttributes (sequelize#7196)
6479afa 
* Added unique indexes to rawAttributes

* Add changelog entry

* Added integration test to cover composite unique keys, review fixes

* Fixes failing tests
@alitaheri alitaheri mentioned this pull request Jun 10, 2017
Closed
@alitaheri alitaheri mentioned this pull request Jun 28, 2017
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@sushantdhiman sushantdhiman sushantdhiman approved these changes

@felixfbecker felixfbecker felixfbecker approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@harshithkashyap @codecov-io @sushantdhiman @felixfbecker