UPSERT functionality for MSSQL

[harshithkashyap]

Pull Request check-list

• Does npm run test or npm run test-DIALECT pass with this change (including linting)?
• Does your issue contain a link to existing issue (Closes #[issue]) or a description of the issue you are solving?
• Have you added new tests to prevent regressions?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Have you added an entry under Future in the changelog?

Description of change

This MR enables upsert query for MSSQL dialect using SQL Server MERGE statements.

• Adds IDENTITY_INSERT(autoIncrement) wrapper if autoIncrement columns are given a value
• Tries to use PrimaryKey for the join clause. Else uses Unique keys
• Handles both primary and unique composite key case
• Partial Primary/Unique Keys are filtered out as we cannot find a unique row if specified
• MySQL returns 1 for inserted, 2 for updated. Similar output is achieved in MSSQL using $action which returns Insert/Update based on the executed query.
• Modified hooks test to explicitly pass either primaryKey/uniqueKey as either of them should be passed for upsert.
• Used const in places as suggested by eslint
• Added WITH(HOLDLOCK) hint to prevent concurrency issue

[mention-bot]

@harshithkashyap, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mbroadst, @felixfbecker and @sushantdhiman to be potential reviewers.

[felixfbecker]

LGTM otherwise

[felixfbecker]

Out of scope for this PR. You can add this to .git/info/exclude

[sushantdhiman]

No this is ok, we are already ignoring .DS_STORE and .idea files

[harshithkashyap]

Removed the parens from arrow functions. Keeping the .gitignore change for now

[felixfbecker]

Unneeded parens (applies to all arrow functions you used)

[codecov-io]

Current coverage is 93.68% (diff: 100%)

Merging #6842 into master will decrease coverage by 0.90%

Powered by Codecov. Last update 7029d2d...62aa2a3

[smassin]

Great job on submitting a completed upsert for MSSQL. 👍

This is one edge case bug I found trying this: If a table has 2 non-PK unique indexes and the PK was not one of the merge attributes it will always default to use the PK and cause a SQL error. The following code always assumes that if 2 clauses were found that one must be the PK which is not strictly true.

else if (clauses.length === 2) {
//Both PKs & UKs are passed. Use Primary Key for join
joinCondition = getJoinSnippet(primaryKeysAttrs).join(' AND ');
}

While it is an unlikely case there is nothing stopping people from setting up their tables in this manner. Consider revising the clause generation section with this:

if (clauses.length === 0) {
throw new Error('Primary Key or Unique key should be passed to upsert query');
} else {
// Search for primary key attribute in clauses
for (const i in clauses) {
const keys = Object.keys(clauses[i]);
if (primaryKeysAttrs.indexOf(keys[0]) !== -1) {
joinCondition = getJoinSnippet(primaryKeysAttrs).join(' AND ');
break;
}
}
// Primary key was not found in clauses so use unique attributes
if(!joinCondition) {
joinCondition = getJoinSnippet(uniqueAttrs).join(' AND ');
}
}

[harshithkashyap]

@smassin Thanks for the review. :) Yes, this would fail when there are two unique keys on table. I'll push a commit with the changes.

[smassin]

Additionally, and this is not an error in your code, but we really should use a lock to ensure atomicity of the MERGE statement.

See:
http://weblogs.sqlteam.com/dang/archive/2009/01/31/UPSERT-Race-Condition-With-MERGE.aspx
https://www.mssqltips.com/sqlservertip/3074/use-caution-with-sql-servers-merge-statement/

The strong recommendation is use WITH (HOLDLOCK) to ensure atomicity in concurrent situations. Ex.

MERGE dbo.TableName WITH (HOLDLOCK) AS target ...

[sushantdhiman]

@harshithkashyap Please rebase and we can merge it

[becuz]

Hi there,
I spotted a couple of bugs on the upsert implementation.

The check of the unique columns ignores the definitions through the model indexes option.
With shis user model definition:

sequelize.define('user', {}, {
  ...
  indexes: [
    {
      unique: true,
      fields: ['email']
    }]
})

the upsert uniqueAttrs list will end up without the email field.

The code always uses all unique columns to generate the ON condition, regardless of their presence in the upsert values parameter.
The call upsert.user(values) will throw "Invalid column name 'email'" if email is not in values, because upsert will create an invalid ON condition:

[[model_target].[email] = [model_source].[email]]

source has no column email

[harshithkashyap]

@janmeier @sushantdhiman We pass model.rawAttributes to upsert query.
It looks like sequelize.define does not add indexes defined via options to rawAttributes.
https://github.com/sequelize/sequelize/blob/master/lib/model.js#L783-L796
I'll submit a patch, unless its intended?