Skip to content

Commit

Permalink
fix: review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@sushantdhiman
sushantdhiman committed May 24, 2020
1 parent 42f7b4a commit 7b174eb
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 84 deletions.
25 changes: 15 additions & 10 deletions lib/dialects/abstract/query-interface.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,24 +752,29 @@ class QueryInterface {
options = { ...options };

const model = options.model;
const primaryKeys = _.chain(model.primaryKeys).values().map('field').value();
const uniqueKeys = _.chain(model.uniqueKeys).values().filter(c => c.fields.length >= 1).map(c => c.fields).value();
const indexKeys = _.chain(model._indexes).values().filter(c => c.unique && c.fields.length >= 1).map(c => c.fields).value();
const primaryKeys = Object.values(model.primaryKeys).map(item => item.field);
const uniqueKeys = Object.values(model.uniqueKeys).filter(c => c.fields.length >= 1).map(c => c.fields);
const indexKeys = Object.values(model._indexes).filter(c => c.unique && c.fields.length >= 1).map(c => c.fields);

options.type = QueryTypes.UPSERT;
options.updateOnDuplicate = Object.keys(updateValues);
options.upsertKeys = [];

// find first valid constraint
_.each(options.updateOnDuplicate, field => {
if (options.upsertKeys.length) return;

// For fields in updateValues, try to find a constraint or unique index
// that includes given field. Only first matching upsert key is used.
for (const field of options.updateOnDuplicate) {
const uniqueKey = uniqueKeys.find(fields => fields.includes(field));
if (uniqueKey) return options.upsertKeys = options.upsertKeys.concat(uniqueKey);
if (uniqueKey) {
options.upsertKeys = uniqueKey;
break;
}

const indexKey = indexKeys.find(fields => fields.includes(field));
if (indexKey) return options.upsertKeys = options.upsertKeys.concat(indexKey);
});
if (indexKey) {
options.upsertKeys = indexKey;
break;
}
}

// Always use PK, if no constraint available OR update data contains PK
if (
Expand Down
14 changes: 3 additions & 11 deletions lib/dialects/mssql/query-interface.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,6 @@ class MSSqlQueryInterface extends QueryInterface {
async upsert(tableName, insertValues, updateValues, where, options) {
const model = options.model;
const wheres = [];
const attributes = Object.keys(insertValues);
let indexes = [];

options = { ...options };

Expand All @@ -60,16 +58,10 @@ class MSSqlQueryInterface extends QueryInterface {
}

// Lets combine unique keys and indexes into one
indexes = _.map(model.uniqueKeys, value => {
return value.fields;
});

model._indexes.forEach(value => {
if (value.unique) {
indexes.push(value.fields);
}
});
let indexes = Object.values(model.uniqueKeys).map(item => item.fields);
indexes = indexes.concat(Object.values(model._indexes).filter(item => item.unique).map(item => item.fields));

const attributes = Object.keys(insertValues);
for (const index of indexes) {
if (_.intersection(attributes, index).length === index.length) {
where = {};
Expand Down
126 changes: 63 additions & 63 deletions test/integration/model/upsert.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,19 +60,19 @@ describe(Support.getTestDialectTeaser('Model'), () => {
if (current.dialect.supports.upserts) {
describe('upsert', () => {
it('works with upsert on id', async function() {
const created0 = await this.User.upsert({ id: 42, username: 'john' });
const [, created0] = await this.User.upsert({ id: 42, username: 'john' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.true;
expect(created0).to.be.true;
}

this.clock.tick(1000);
const created = await this.User.upsert({ id: 42, username: 'doe' });
const [, created] = await this.User.upsert({ id: 42, username: 'doe' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}

const user = await this.User.findByPk(42);
Expand All @@ -82,19 +82,19 @@ describe(Support.getTestDialectTeaser('Model'), () => {
});

it('works with upsert on a composite key', async function() {
const created0 = await this.User.upsert({ foo: 'baz', bar: 19, username: 'john' });
const [, created0] = await this.User.upsert({ foo: 'baz', bar: 19, username: 'john' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.true;
expect(created0).to.be.true;
}

this.clock.tick(1000);
const created = await this.User.upsert({ foo: 'baz', bar: 19, username: 'doe' });
const [, created] = await this.User.upsert({ foo: 'baz', bar: 19, username: 'doe' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}

const user = await this.User.findOne({ where: { foo: 'baz', bar: 19 } });
Expand Down Expand Up @@ -152,11 +152,11 @@ describe(Support.getTestDialectTeaser('Model'), () => {

this.clock.tick(1000);
// Update the first one
const created = await User.upsert({ a: 'a', b: 'b', username: 'doe' });
const [, created] = await User.upsert({ a: 'a', b: 'b', username: 'doe' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}

const user1 = await User.findOne({ where: { a: 'a', b: 'b' } });
Expand Down Expand Up @@ -197,28 +197,28 @@ describe(Support.getTestDialectTeaser('Model'), () => {
const options = { validate: false };

await User.sync({ force: true });
const created = await User.upsert({ id: 1, email: 'notanemail' }, options);
const [, created] = await User.upsert({ id: 1, email: 'notanemail' }, options);
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.true;
expect(created).to.be.true;
}
});

it('works with BLOBs', async function() {
const created0 = await this.User.upsert({ id: 42, username: 'john', blob: Buffer.from('kaj') });
const [, created0] = await this.User.upsert({ id: 42, username: 'john', blob: Buffer.from('kaj') });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.ok;
expect(created0).to.be.ok;
}

this.clock.tick(1000);
const created = await this.User.upsert({ id: 42, username: 'doe', blob: Buffer.from('andrea') });
const [, created] = await this.User.upsert({ id: 42, username: 'doe', blob: Buffer.from('andrea') });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}

const user = await this.User.findByPk(42);
Expand All @@ -229,58 +229,58 @@ describe(Support.getTestDialectTeaser('Model'), () => {
});

it('works with .field', async function() {
const created0 = await this.User.upsert({ id: 42, baz: 'foo' });
const [, created0] = await this.User.upsert({ id: 42, baz: 'foo' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.ok;
expect(created0).to.be.ok;
}

const created = await this.User.upsert({ id: 42, baz: 'oof' });
const [, created] = await this.User.upsert({ id: 42, baz: 'oof' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}

const user = await this.User.findByPk(42);
expect(user.baz).to.equal('oof');
});

it('works with primary key using .field', async function() {
const created0 = await this.ModelWithFieldPK.upsert({ userId: 42, foo: 'first' });
const [, created0] = await this.ModelWithFieldPK.upsert({ userId: 42, foo: 'first' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.ok;
expect(created0).to.be.ok;
}

this.clock.tick(1000);
const created = await this.ModelWithFieldPK.upsert({ userId: 42, foo: 'second' });
const [, created] = await this.ModelWithFieldPK.upsert({ userId: 42, foo: 'second' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}

const instance = await this.ModelWithFieldPK.findOne({ where: { userId: 42 } });
expect(instance.foo).to.equal('second');
});

it('works with database functions', async function() {
const created0 = await this.User.upsert({ id: 42, username: 'john', foo: this.sequelize.fn('upper', 'mixedCase1') });
const [, created0] = await this.User.upsert({ id: 42, username: 'john', foo: this.sequelize.fn('upper', 'mixedCase1') });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.ok;
expect(created0).to.be.ok;
}

this.clock.tick(1000);
const created = await this.User.upsert({ id: 42, username: 'doe', foo: this.sequelize.fn('upper', 'mixedCase2') });
const [, created] = await this.User.upsert({ id: 42, username: 'doe', foo: this.sequelize.fn('upper', 'mixedCase2') });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}
const user = await this.User.findByPk(42);
expect(user.createdAt).to.be.ok;
Expand Down Expand Up @@ -320,14 +320,14 @@ describe(Support.getTestDialectTeaser('Model'), () => {
it('does not update when setting current values', async function() {
await this.User.create({ id: 42, username: 'john' });
const user = await this.User.findByPk(42);
const created = await this.User.upsert({ id: user.id, username: user.username });
const [, created] = await this.User.upsert({ id: user.id, username: user.username });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
// After set node-mysql flags = '-FOUND_ROWS' / foundRows=false
// result from upsert should be false when upsert a row to its current value
// https://dev.mysql.com/doc/refman/5.7/en/insert-on-duplicate.html
expect(created[1]).to.equal(false);
expect(created).to.equal(false);
}
});

Expand All @@ -347,18 +347,18 @@ describe(Support.getTestDialectTeaser('Model'), () => {
});
const clock = sinon.useFakeTimers();
await User.sync({ force: true });
const created0 = await User.upsert({ username: 'user1', email: 'user1@domain.ext', city: 'City' });
const [, created0] = await User.upsert({ username: 'user1', email: 'user1@domain.ext', city: 'City' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.ok;
expect(created0).to.be.ok;
}
clock.tick(1000);
const created = await User.upsert({ username: 'user1', email: 'user1@domain.ext', city: 'New City' });
const [, created] = await User.upsert({ username: 'user1', email: 'user1@domain.ext', city: 'New City' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}
clock.tick(1000);
const user = await User.findOne({ where: { username: 'user1', email: 'user1@domain.ext' } });
Expand All @@ -383,17 +383,17 @@ describe(Support.getTestDialectTeaser('Model'), () => {
});

await User.sync({ force: true });
const created0 = await User.upsert({ username: 'user1', email: 'user1@domain.ext', city: 'City' });
const [, created0] = await User.upsert({ username: 'user1', email: 'user1@domain.ext', city: 'City' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.ok;
expect(created0).to.be.ok;
}
const created = await User.upsert({ username: 'user1', email: 'user1@domain.ext', city: 'New City' });
const [, created] = await User.upsert({ username: 'user1', email: 'user1@domain.ext', city: 'New City' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).to.be.false;
expect(created).to.be.false;
}
const user = await User.findOne({ where: { username: 'user1', email: 'user1@domain.ext' } });
expect(user.createdAt).to.be.ok;
Expand All @@ -413,17 +413,17 @@ describe(Support.getTestDialectTeaser('Model'), () => {
});

await User.sync({ force: true });
const created0 = await User.upsert({ name: 'user1', address: 'address', city: 'City' });
const [, created0] = await User.upsert({ name: 'user1', address: 'address', city: 'City' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created0[1]).to.be.null;
expect(created0).to.be.null;
} else {
expect(created0[1]).to.be.ok;
expect(created0).to.be.ok;
}
const created = await User.upsert({ name: 'user1', address: 'address', city: 'New City' });
const [, created] = await User.upsert({ name: 'user1', address: 'address', city: 'New City' });
if (dialect === 'sqlite' || dialect === 'postgres') {
expect(created[1]).to.be.null;
expect(created).to.be.null;
} else {
expect(created[1]).not.to.be.ok;
expect(created).not.to.be.ok;
}
const user = await User.findOne({ where: { name: 'user1', address: 'address' } });
expect(user.createdAt).to.be.ok;
Expand Down

0 comments on commit 7b174eb

Please sign in to comment.