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

fix(associations): enable overwrite the name in unique constraint #9914

Merged
merged 9 commits into from Sep 16, 2018

Conversation

@lgaticaq
Copy link
Contributor

@lgaticaq lgaticaq commented Sep 12, 2018

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 follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

fix #9913

@codecov
Copy link

@codecov codecov bot commented Sep 12, 2018

Codecov Report

Merging #9914 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9914      +/-   ##
==========================================
+ Coverage   96.04%   96.04%   +<.01%     
==========================================
  Files          63       63              
  Lines        9430     9433       +3     
==========================================
+ Hits         9057     9060       +3     
  Misses        373      373
Impacted Files Coverage Δ
lib/associations/belongs-to-many.js 97.03% <100%> (+0.02%) ⬆️

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 fefe374...32cf738. Read the comment docs.

});

return this.sequelize.sync({ force: true }).bind({}).then(() => {
expect(this.Task.associations.MyUsers.through.model.rawAttributes.id_user_very_long_field.unique).to.have.lengthOf(24);

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 13, 2018
Contributor

no need to check length

@@ -2235,6 +2235,65 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), () => {
expect(ut2).to.have.length(1);
});
});

it('throw error with unique identifier very long', function() {

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 13, 2018
Contributor

Remove this test

uniqueKey: 'custom_user_group_unique'
});

return this.sequelize.sync({ force: true }).bind({}).then(() => {

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 13, 2018
Contributor

No bind with arrow functions

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 14, 2018
Contributor

Use arrow functions I meant, get rid of bind

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 14, 2018
Contributor

return this.sequelize.sync({ force: true }).then(() => {

@@ -252,7 +252,12 @@ class BelongsToMany extends Association {
if (this.primaryKeyDeleted === true) {
targetAttribute.primaryKey = sourceAttribute.primaryKey = true;
} else if (this.through.unique !== false) {
const uniqueKey = [this.through.model.tableName, this.foreignKey, this.otherKey, 'unique'].join('_');
let uniqueKey;
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Some minor changes and it can be merged

@@ -522,6 +522,13 @@ User.findAll({
}]
});
```
Is possible rewrite the unique constraint name using **uniqueKey** option.

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 14, 2018
Contributor

Belongs-To-Many creates a unique key when primary key is not present on through model. This unique key name can be overridden using uniqueKey option.

uniqueKey: 'custom_user_group_unique'
});

return this.sequelize.sync({ force: true }).bind({}).then(() => {

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 14, 2018
Contributor

return this.sequelize.sync({ force: true }).then(() => {

Project.belongsToMany(User, { through: UserProjects, uniqueKey: 'my_custom_unique' })
```

**Note:** _This option prevent error when the unique constraint name is too long._

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 14, 2018
Contributor

No need for this note

@sushantdhiman sushantdhiman merged commit aa92764 into sequelize:master Sep 16, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 96.04%)
Details
codecov/project 96.04% (+<.01%) compared to fefe374
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 16, 2018

Thanks @lgaticaq

RobertYoung pushed a commit to RobertYoung/sequelize that referenced this pull request Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants