BulkCreate with composite unique constraint does not work

[cjancsar]

Issue Description

What are you doing?

I am trying to use bulkCreate to updateOnDuplicate when a duplicate is detected. Duplicate should be detected by a "multi column unique index".

"use strict";

const Sequelize = require("./index");
const sequelize = require("./test/support").createSequelizeInstance();
const chai = require("chai"),
  expect = chai.expect;

class Bar extends Sequelize.Model {}
Bar.init(
  {
    value: Sequelize.DataTypes.STRING,
    timestamp: {
      type: Sequelize.DataTypes.DATE,
      allowNull: false
    }
  },
  {
    indexes: [
      {
        name: "unique_timestamp_per_foo",
        unique: true,
        fields: ["timestamp", "FooId"]
      }
    ],
    sequelize
  }
);

class Foo extends Sequelize.Model {}
Foo.init({}, { sequelize });

Bar.belongsTo(Foo, {
  foreignKey: { allowNull: false },
  onDelete: "CASCADE"
});
Foo.hasMany(Bar, { foreignKey: { allowNull: false }, onDelete: "CASCADE" });

(async () => {
  await sequelize.sync({ force: true });

  // Create a _parent_ `Foo` model
  const foo = await Foo.create();

  // Create a `Bar` object
  const originalBar = await Bar.create({
    value: "original",
    timestamp: new Date(Date.UTC(2016, 0, 1)),
    FooId: foo.id
  });

  // We obviously expect a `Bar` object to exist with the unique constraints of `timestamp` and `FooId`
  expect(
    await Bar.count({
      where: { FooId: foo.id, timestamp: new Date(Date.UTC(2016, 0, 1)) }
    })
  ).to.equal(1);

  // We create a modified `Bar` object that has an overlapping `timestamp` and `FooId`
  // This should be detected as duplicate by the `uniqueTimestampPerFoo` unique constraint/index,
  // during the next `bulkCreate` operation with `updateOnDuplicate` set.
  const modifiedBar = Bar.build({
    value: "modified",
    timestamp: new Date(Date.UTC(2016, 0, 1)),
    FooId: foo.id
  });

  // "Bulk" create to "upsert" a modified `existingBar` when conflict detected.
  // We only want `value` and `updatedAt` to be updated on conflict.
  await expect(
    Bar.bulkCreate([modifiedBar.toJSON()], {
      updateOnDuplicate: ["value", "updatedAt"],
      logging: console.log
    })
  ).not.to.be.rejectedWith(Sequelize.SequelizeUniqueConstraintError);

  // *** The above function should not have been rejected with a `SequelizeUniqueConstraintError` ***

  // *** The below is what should have been expected ***

  // Reload the modified original `Bar` object by `id`
  const refreshedOriginalBar = await Bar.findByPk(originalBar.id);

  // The value should have been updated
  expect(refreshedOriginalBar.value).to.equal("modified");

  // The refreshed updatedAt should be greater than the original updatedAt
  expect(refreshedOriginalBar.updatedAt > originalBar.updatedAt).be.true;
})();

What do you expect to happen?

See the expected assertions from my SSCCE:

  // *** The below is what should have been expected ***

  // Reload the modified original `Bar` object by `id`
  const refreshedOriginalBar = await Bar.findByPk(originalBar.id);

  // The value should have been updated
  expect(refreshedOriginalBar.value).to.equal("modified");

  // The refreshed updatedAt should be greater than the original updatedAt
  expect(refreshedOriginalBar.updatedAt > originalBar.updatedAt).be.true;

I wanted a duplicate to be detected by the unique_timestamp_per_foo unique key when calling the bulkCreate method with updateOnDuplicate array set.

Furthermore, I wanted the insert SQL to be ON CONFLICT ("timestamp", "FooId") instead of ON CONFLICT ("id")

What is actually happening?

A SequelizeUniqueConstraintError is thrown because the expected multi-field index does not get added to upsertKeys here (

sequelize/lib/model.js

Line 2705 in 3517eb7

options.upsertKeys = _.chain(model.uniqueKeys).values().filter(c => c.fields.length === 1).map('column').value();

) because it is filtered out.

Environment

• Sequelize version: sequelize@0.0.0-development
• Node.js v10.16.3
• Operating System: Windows 10

Issue Template Checklist

How does this problem relate to dialects?

• I think this problem happens regardless of the dialect.
• I think this problem happens only for the following dialect(s): Postgres

Would you be willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time and I know how to start.
• Yes, I have the time but I don't know how to start, I would need guidance.
• No, I don't have the time, although I believe I could do it if I had the time...
• No, I don't have the time and I wouldn't even know how to start.

[cjancsar]

It is not clear why line

sequelize/lib/model.js

Line 2705 in 3517eb7

options.upsertKeys = _.chain(model.uniqueKeys).values().filter(c => c.fields.length === 1).map('column').value();

is filtering the unique keys by length === 1.

I have locally added a patch to allow an explicit option to be set called upsertOn when calling Model.bulkCreate.

upsertOn can either be:

• An array of model fieldNames that are either primary keys or composite members of a unique key.
• A string of a custom unique composite constraint (i.e. where fields have been defined with unique: 'some_string'

Can be called like:

Bar.bulkCreate([... data ...], {
      updateOnDuplicate: [ ... fields to update ... ],
      // upsertOn: "unique_timestamp_per_foo", -> unique composite key string from model def 
      // upsertOn: ['FooId', 'timestamp'] -> fields that have a unique composite key
    })

Let me know if you agree, and want me to make a PR with this enhancement.

This has solved the gap of not allowing composite unique keys with onDuplicateUpdate.

Replacing these existing lines:

sequelize/lib/model.js

Lines 2701 to 2708 in 2b9baa2

	if (options.updateOnDuplicate) {
	options.updateOnDuplicate = options.updateOnDuplicate.map(attr => model.rawAttributes[attr].field || attr);
	// 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('column').value();
	}
	}

With:

// Map updateOnDuplicate attributes to fields
if (options.updateOnDuplicate) {
    options.updateOnDuplicate = options.updateOnDuplicate.map(attr => model.rawAttributes[attr].field || attr);

    // If `upsertOn` is passed as a `bulkCreate` option (allows explicit mapping of `upsertKeys`)
    if (Object.prototype.hasOwnProperty.call(options, 'upsertOn')) {

        // If a string is passed will search for "custom" unique constraint names.
        if (typeof options.upsertOn === 'string' || options.upsertOn instanceof String) {
            if (Object.keys(model.uniqueKeys).length > 0) {
                options.upsertKeys = _.chain(model.uniqueKeys).values().filter(c => c.customIndex).find(c => c.name === options.upsertOn).get('fields').value();
            }

            // If an array of `fieldNames` are passed, will check for existence of unique contraint of those fields
            // Or it that field exists in a primary key.
        } else if (Array.isArray(options.upsertOn) && options.upsertOn.length) {
            if (!_.difference(options.upsertOn, Object.keys(model.tableAttributes)).length) {
                // Convert upsertOn 'fieldName' array into `field` values.
                const _upsertOnFields = options.upsertOn.map(c => model.tableAttributes[c].field);
                if (options.upsertOn.length === 1) {
                    const _upsertKey = _.chain(model.primaryKeys).values().find(c => c.field === _upsertOnFields[0]).get('field').value();
                    if (_upsertKey) {
                        options.upsertKeys = [_upsertKey];
                    }
                } else if (Object.keys(model.uniqueKeys).length > 0) {
                    options.upsertKeys = _.chain(model.uniqueKeys).values().find(c => _.isEqual(c.fields.sort(), _upsertOnFields.sort())).get('fields').value();
                }
            }
        } else {
            throw Error('upsertOn option must be an Array or String.');
        }
    }

    if (options.upsertKeys === undefined) {
        // 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('column').value();
        }
    }
}

[cjancsar]

Code in fork here: cjancsar@45f6792

[cjancsar]

@papb is this a useful feature? Should I make a PR?

[fnimick]

Note: this doesn't work for composite keys with a partial index, where you need to specify ON CONFLICT ("a","b") WHERE "deleted_at" IS NULL, for example.

[fnimick]

@papb I have a fix working for my use case where I allow the user to specify upsertKeys explicitly, which can either be an array of columns or an object { fields: <columns>, where: ... } with the where expression mapping to a partial index. Should I make a PR?

[Rochet2]

I have same issue. Seems this issue is duplicate of #11534

[fnimick]

@Rochet2 I've created a patch for my fix: https://gist.github.com/fnimick/5dd3c6855cea6e000c86d1bc38876958

PR coming shortly.

[papb]

Hi everyone, sorry for taking long, I will take a look at the PR by @fnimick now. Is it the same as the work by @cjancsar?

[cjancsar]

@papb no it is not, but I have not had time to create a PR with my work. @fnimick work is a subfeature of the feature that my patch would add (his add ability for partial index as well). I prefer my approach as it only introduces one option upsertOn instead of two upsertKeys and upsertIndex. There is currently no tests on the PR so I don't really see it moving. I am using my patch at the moment, and will try to get a PR at some point next week.

[papb]

@cjancsar Thanks for the reply, I've been very busy so no time to look at your implementation and @fnimick implementation yet... Thanks for the patience :)

[fnimick]

@cjancsar @papb I can also remove upsertKeys from the PR entirely - the only use case for it was to specify an index that is defined on the table but not in the model itself, for flexibility. I am actually only using upsertIndex in my code.

[papb]

@fnimick If upsertKeys is well documented, you can keep it, flexibility is good :)

[vbhakta8]

Do we have an ETA on when this PR might get looked at? Or how we can get it merged in? Is there any work required on contributors end to get this pushed through?

[sushantdhiman]

Fixed by #11984