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(model): updateOnDuplicate handles composite keys now #11984

Merged
merged 2 commits into from
Mar 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions lib/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,7 @@ class Model {
*
* @see
* <a href="/master/manual/model-basics.html">Model Basics</a> guide
*
*
* @see
* <a href="/master/manual/model-basics.html">Hooks</a> guide
*
Expand Down Expand Up @@ -2708,7 +2708,7 @@ class Model {
// Get primary keys for postgres to enable updateOnDuplicate
options.upsertKeys = _.chain(model.primaryKeys).values().map('field').value();
if (Object.keys(model.uniqueKeys).length > 0) {
options.upsertKeys = _.chain(model.uniqueKeys).values().filter(c => c.fields.length === 1).map(c => c.fields[0]).value();
options.upsertKeys = _.chain(model.uniqueKeys).values().filter(c => c.fields.length >= 1).map(c => c.fields).reduce(c => c[0]).value();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about the reduce here, would this not effectively end up returning undefined all the time?

cc @eseliger @sushantdhiman

Copy link
Member

Choose a reason for hiding this comment

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

I think you are correct. The initializer for the reduce is undefined (not specified), and c is the previous argument, so it would effectively always return the undefined, leaving the actual values untouched.

Choose a reason for hiding this comment

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

Due to the .reduce updateOnDuplicate does not work for Composite unique keys or multiple unique keys. Can you recheck for the same.

}
}

Expand Down Expand Up @@ -3842,13 +3842,13 @@ class Model {

/**
* Validates this instance, and if the validation passes, persists it to the database.
*
*
* Returns a Promise that resolves to the saved instance (or rejects with a `Sequelize.ValidationError`, which will have a property for each of the fields for which the validation failed, with the error message for that field).
*
*
* This method is optimized to perform an UPDATE only into the fields that changed. If nothing has changed, no SQL query will be performed.
*
*
* This method is not aware of eager loaded associations. In other words, if some other model instance (child) was eager loaded with this instance (parent), and you change something in the child, calling `save()` will simply ignore the change that happened on the child.
*
*
* @param {object} [options] save options
* @param {string[]} [options.fields] An optional array of strings, representing database columns. If fields is provided, only those columns will be validated and saved.
* @param {boolean} [options.silent=false] If true, the updatedAt timestamp will not be updated.
Expand Down
111 changes: 111 additions & 0 deletions test/integration/model/bulk-create.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,117 @@ describe(Support.getTestDialectTeaser('Model'), () => {
expect(people[1].name).to.equal('Bob');
});
});

it('when the composite primary key column names and model field names are different', function() {
const Person = this.sequelize.define('Person', {
systemId: {
type: DataTypes.INTEGER,
allowNull: false,
primaryKey: true,
field: 'system_id'
},
system: {
type: DataTypes.STRING,
allowNull: false,
primaryKey: true,
field: 'system'
},
name: {
type: DataTypes.STRING,
allowNull: false,
field: 'name'
}
}, {});

return Person.sync({ force: true })
.then(() => {
const inserts = [
{ systemId: 1, system: 'system1', name: 'Alice' }
];
return Person.bulkCreate(inserts);
})
.then(people => {
expect(people.length).to.equal(1);
expect(people[0].systemId).to.equal(1);
expect(people[0].system).to.equal('system1');
expect(people[0].name).to.equal('Alice');

const updates = [
{ systemId: 1, system: 'system1', name: 'CHANGED NAME' },
{ systemId: 1, system: 'system2', name: 'Bob' }
];

return Person.bulkCreate(updates, { updateOnDuplicate: ['systemId', 'system', 'name'] });
})
.then(people => {
expect(people.length).to.equal(2);
expect(people[0].systemId).to.equal(1);
expect(people[0].system).to.equal('system1');
expect(people[0].name).to.equal('CHANGED NAME');
expect(people[1].systemId).to.equal(1);
expect(people[1].system).to.equal('system2');
expect(people[1].name).to.equal('Bob');
});
});

it('when the primary key column names and model field names are different and have composite unique constraints', function() {
const Person = this.sequelize.define('Person', {
id: {
type: DataTypes.INTEGER,
allowNull: false,
primaryKey: true,
field: 'id'
},
systemId: {
type: DataTypes.INTEGER,
allowNull: false,
unique: 'system_id_system_unique',
field: 'system_id'
},
system: {
type: DataTypes.STRING,
allowNull: false,
unique: 'system_id_system_unique',
field: 'system'
},
name: {
type: DataTypes.STRING,
allowNull: false,
field: 'name'
}
}, {});

return Person.sync({ force: true })
.then(() => {
const inserts = [
{ id: 1, systemId: 1, system: 'system1', name: 'Alice' }
];
return Person.bulkCreate(inserts);
})
.then(people => {
expect(people.length).to.equal(1);
expect(people[0].systemId).to.equal(1);
expect(people[0].system).to.equal('system1');
expect(people[0].name).to.equal('Alice');

const updates = [
{ id: 1, systemId: 1, system: 'system1', name: 'CHANGED NAME' },
{ id: 2, systemId: 1, system: 'system2', name: 'Bob' }
];

return Person.bulkCreate(updates, { updateOnDuplicate: ['systemId', 'system', 'name'] });
})
.then(people => {
expect(people.length).to.equal(2);
expect(people[0].systemId).to.equal(1);
expect(people[0].system).to.equal('system1');
expect(people[0].name).to.equal('CHANGED NAME');
expect(people[1].systemId).to.equal(1);
expect(people[1].system).to.equal('system2');
expect(people[1].name).to.equal('Bob');
});
});

});


Expand Down