feat(upsert): add conflictFields option

[wbourne0]

Adds support for the conflictFields option to Model.upsert.
This is used for options.upsertKeys in QueryInterface.prototype.upsert if provided for specifying the fields
used at ON CONFLICT({fields}) rather than relying on the default logic.

Pull Request Checklist

Please make sure to review and check all of these items:

• Have you added new tests to prevent regressions?
• Does npm run test or npm run test-DIALECT pass with this change (including linting)?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Did you update the typescript typings accordingly (if applicable)?
• Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
• Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

Allows for the override of the upsertKeys value passed to the query generator via options.conflictFields (for Model.upsert).

Note that the different name is intentional - upsertKeys isn't as clear as conflictFields IMO.

Our current logic tries to guess which fields are used, but there's some cases where it isn't accurate (and with #13411, this becomes more of an issue) especially partial indexes. Though it'd be nice to have the logic which infers upsertKeys smarter, I think there's some edge cases where it wouldn't be able to (safely) infer which column to use (e.g. in cases where multiple could apply).

Example use case (using a many:many joiner table)

const Memberships: typeof Model = sequelize.define(
  'my_joiner_table',
  {
    id: {
      primaryKey: true,
      autoIncrement: true,
      type: DataTypes.INTEGER,
      allowNull: false,
    },
    group_id: {
      type: DataTypes.INTEGER,
      primaryKey: true,
      allowNull: false,
      unique: 'my_constraint',
    },
    user_id: {
      type: DataTypes.INTEGER,
      primaryKey: true,
      allowNull: false,
      unique: 'my_constraint',
    },
  },
  { timestamps: true }
);

// In usage
await Memberships.upsert(
  {
    group_id: 5,
    team_id: 3,
  },
  {
    conflictFields: ['group_id', 'user_id'],
  }
); // `ON CONFLICT ("group_id", "user_id")` instead of `ON CONFLICT ("group_id")` 

[fzn0x]

How about the performance from the old ones, is it better?

[wbourne0]

How about the performance from the old ones, is it better?

As in vs v5? This should be considerably faster, the previous logic would:

Try an insert query, then an update query if the insert query threw a unique index error.

This makes it possible to specify which fields to use in ON CONFLICT({HERE}) which can give significant performance benefits if you have the right indexes if sequelize's default logic doesn't pickup the right fields.

(though in most cases, seqeuelize's default logic will work perfectly fine, this is mostly an issue when partial indexes come into play)

[sdepold]

MySQL and MSSQL doesn't support it?

[sdepold]

Overall I don't see a particular issue with the code change. I'm just wondering if the default logic should be smarter somehow. In your tests you have multi column indexes and seem to use the same columns in the conflictFields afterwards. Should we somehow prefer the multi column indexes?! Or is that too brittle and the developer should decide for him/herself?

[wbourne0]

Overall I don't see a particular issue with the code change. I'm just wondering if the default logic should be smarter somehow. In your tests you have multi column indexes and seem to use the same columns in the conflictFields afterwards. Should we somehow prefer the multi column indexes?! Or is that too brittle and the developer should decide for him/herself?

I think we should do both, this is just the easiest one to do and works until we can take care of the other one.

I think the primary case for this (where we'll never be able to determine which index to use) would be when there are two indexes which could be used in an index but we only want to use one.

For example:

const Users: typeof Model = sequelize.define(
  'my_joiner_table',
  {
    username: {
      type: DataTypes.STRING,
      allowNull: false,
    },
    email: {
      type: DataTypes.STRING,
      allowNull: false,
    },
  },
  {
    timestamps: true,
    indexes: [
      {
        unique: true,
        fields: ['username'],
      },
      {
        unique: true,
        fields: ['email'],
      },
    ],
  }
);

// In this instance, the conflicting key is ambiguous - we could either be 
// changing the email of the user with username `myUsername` or
// changing the username of the username with email `myEmail@domain.tld`.
// This is fine for inserts, but if we're updating a user it could have unintended side effects.
await Users.upsert({
  username: 'myUsername',
  email: 'myEmail@domain.tld',
});

// In this case, if a user exists with username `myUsername`, we'll update their email
// to be `myEmail@domain.tld`; otherwise we'll create a new user.
await Users.upsert(
  {
    username: 'myUsername',
    email: 'myEmail@domain.tld',
  },
  {
    conflictFields: ['username'],
  }
);

// In this case, if a user exists with email `myEmail@domain.tld`, we'll update their username
// to be `myUsername`; otherwise we'll create a new user.
await Users.upsert(
  {
    username: 'myUsername',
    email: 'myEmail@domain.tld',
  },
  {
    conflictFields: ['email'],
  }
);

However where this issue is more relevant is when / if #13412 is merged (was approved but I accidentally dismissed that, oops).

When working with partial indexes, the where clause must be specified in ON CONFLICT for postgres / sqlite to use that partial index.

So if we don't specify { conflictWhere: conditionForIndex } the index won't apply, meaning we can't infer that the index is to be used (unless we add logic which determines if a where clause implements an index's clause but that sounds very complicated).

I definitely think that we should and can have smarter logic here, but I think there's a lot of edge cases which make having this an option very useful.

[sdepold]

Is this not supported for mysql and mssql?

[wbourne0]

Nope; same for snowflake iirc. From what I gathered whilst looking at their respective docs, they don't let / make you explicitly specify the fields which could run into a UNIQUE constraint issues. Instead, they have an ON DUPLICATE clause which is triggered when any unique constraint would error.

[sdepold]

Please clarify the question above.

[github-actions]

🎉 This PR is included in version 6.12.0-beta.3 🎉

The release is available on:

• npm package (@v6-beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 6.12.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀