bulkCreate returns invalid records instead of throwing when unique indexes are violated

[CADBOT]

Let's say we have a table defined with a unique index that is not the primary key.

'use strict';

module.exports = {
  up: (queryInterface, Sequelize) =>
    queryInterface
      .createTable('some_table', {
        id: {
          allowNull: false,
          autoIncrement: true,
          primaryKey: true,
          type: Sequelize.INTEGER,
        },
        md5: {
          type: Sequelize.CHAR(32), // eslint-disable-line new-cap
        },    
      })
      .then(
        () => queryInterface.addIndex('some_table', { fields: ['md5'], unique: true, name: 'unique_rg_md5' }), // eslint-disable-line no-unused-expressions
      )
 
  down: queryInterface => queryInterface.dropTable('some_table'),
};

Then we create some entries

const r1 = await someTable.bulkCreate([{md5: "75b55bb34dac9d02740b9ad6b6820360"}], { ignoreDuplicates: true }); 
console.log(r1.dataValues) // { id: RANDOMLY_GENERATED, md5: 75b55bb34dac9d02740b9ad6b6820360 }

const r2 = await someTable.bulkCreate([{md5: "75b55bb34dac9d02740b9ad6b6820360"}], { ignoreDuplicates: true }); 
console.log(r1.dataValues) // { id: null, md5: 75b55bb34dac9d02740b9ad6b6820360 }

In the first call to bulkCreate we create a record in the db, and there is an id created. However in the next call to bulkCreate to record is created - due to the unique constraint being violated` but a record like object with dataValues is returned that has everything the first had, but with an id of 0.

I'm glad it's not creating the entry in the DB, but this doesn't feel like the right behavior? I think it would make more sense of an error was thrown, or at least return an object that isn't ostensibly a valid row.

I've stepped through the code and see where the bug is and can craft a PR to fix, but wanted to see if throwing an error would be an acceptable way to handle this

Dialect: sqlite
Dialect version: any
Database version: any
Sequelize version: 4.37.6
Tested with latest release: No

[SimonSchick]

This would be a breaking change, if you added an option to the second argument this would be acceptable.

[papb]

@CADBOT Thanks for the issue. I agree with you that this does not seem a correct behavior, and throwing an error would be better. I also agree with SimonSchick that this would be a breaking change.

Are you still willing to create the PR? It would be very welcome! We would wait to release it with v6, since it's a breaking change, but having the PR done in advance does not hurt at all.

What do you say? Thanks!

[github-actions]

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂