fix (mssql): bulkUpdate returning values

[ShaharHD]

Pull Request check-list

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

• Does npm run test or npm run test-DIALECT pass with this change (including linting)?
• Does the description below contain a link to an 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)?
• Did you update the typescript typings accordingly (if applicable)?
• Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Fix MSSQL returning values for bulkUpdate

The issue creates a major performance hit for v5 - as bulkUpdates right now does not work correctly and requires to run another query to get the model data - which is returning already in the update itself.

• bulkUpdate will now return the updates if options.returning is true.
• updateQuery removed redundant setting check
• Fix documentation for model.update

[ShaharHD]

The CI fails on an npm install error: mariadb requires at least version 10.13 of Node, please upgrade

[sushantdhiman]

You need to submit a fix to master branch before this can be merged

[ShaharHD]

You need to submit a fix to master branch before this can be merged

#12413 was created as requested.
But the issue has a major performance hit for v5 - as bulkUpdates right now does not work correctly and requires in the code to run another query to get the model data - which is returning already in the update itself.

[sushantdhiman]

Keep this check, first check test if dialect supports returnings like MySQL which doesn't support it. Second if user is requesting to return records

[ShaharHD]

Look at line 367 - you won't even get into this if statement if this._dialect.supports.returnValues was false. why recheck for it?

[sushantdhiman]

The issue creates a major performance hit for v5

I didn't understand this, are you saying this change will improve performance?

[ShaharHD]

The issue creates a major performance hit for v5

I didn't understand this, are you saying this change will improve performance?

Yes.

Consider this scenario:

const [rowsUpdate, [updatedBook]] = Book.update(
   {price: 100},
   {returning: true, where: {price: 0} }
 )

Without this PR - updatedBook will be undefined (and not populated with the updated records returned by MSSQL OUTPUT INSERTED.*) which means you'll need to to issue another query to pull the data (and in some cases it is not possible as the data might have already changed)

[ShaharHD]

@sunshinewyin Added the new test to this branch as well as master

Look for should output the updated record in the tests.

Seems the test now fail on an unrelated issue bash : curl: (3) <url> malformed

[sushantdhiman]

🎉 This PR is included in version 5.22.1 🎉

The release is available on:

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

Your semantic-release bot 📦🚀