Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add capability for insert multiple rows in single query #228

Merged
merged 2 commits into from Sep 22, 2022
Merged

feat: add capability for insert multiple rows in single query #228

merged 2 commits into from Sep 22, 2022

Conversation

samarpanB
Copy link
Contributor

@samarpanB samarpanB commented Jul 10, 2022 •
edited

Signed-off-by: Samarpan Bhattacharya this.is.samy@gmail.com

  1. Adds new createAll method to support insert multiple rows in single query
  2. Adds a new flag multiInsertSupported to allow derived connectors to disable this feature if this SQL dialect is not supported. By default, it is false for backwards compatibility.

Fixes #227
See also loopbackio/loopback-next#3357

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@samarpanB samarpanB requested a review from dhmlau as a code owner July 10, 2022 13:00
@coveralls
Copy link

coveralls commented Jul 10, 2022 •
edited

Pull Request Test Coverage Report for Build 3005309761

  • 27 of 41 (65.85%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 58.483%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/sql.js 27 41 65.85%
Totals Coverage Status
Change from base Build 2943639583: 0.3%
Covered Lines: 693
Relevant Lines: 1119
💛 - Coveralls

dhmlau
dhmlau reviewed Jul 11, 2022
View reviewed changes
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samarpanB, thanks for contribution. The PR generally LGTM. I have a minor comment what to do with the case where multi-insert is not supported.

lib/sql.js
*/
SQLConnector.prototype.buildInsertAll = function(model, data, options) {
if (!this.multiInsertSupported) {
debug('multiple value insert SQL dialect is not supported by this connector');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we want to throw an error in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this flag will be checked at juggler level and rejected from there itself. I'll be raising that PR soon. I have added this to ensure backwards compatibility in connector code too. Let me know if you still feel we should throw error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me.

@achrinza achrinza self-requested a review July 12, 2022 01:30
dhmlau
dhmlau approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Would like to get at least one more approval before merging. Thanks.

raymondfeng
raymondfeng reviewed Jul 20, 2022
View reviewed changes
lib/sql.js Outdated
*/
SQLConnector.prototype.buildFieldsFromArray = function(model, data, excludeIds) {
const fields = [];
if (data.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use Array.isArray(data) && data.length > 0. For example, if data is string, data.length exists too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. 👍

achrinza
achrinza reviewed Aug 22, 2022
View reviewed changes
Copy link
Member

@achrinza achrinza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than @raymondfeng's comment, everything else LGTM 👍

@samarpanB
Copy link
Contributor Author

I'll fix that. Also, I am in process of raising the next PR for this feature in loopback-juggler. Will share that too.

@samarpanB
feat: add capability for insert multiple rows in single query
ac76b13 
Signed-off-by: Samarpan Bhattacharya <this.is.samy@gmail.com>
@samarpanB
fix: rebiew comments
4edc6ae 
Signed-off-by: Samarpan Bhattacharya <this.is.samy@gmail.com>
@dhmlau dhmlau merged commit 28dec15 into loopbackio:master Sep 22, 2022
@dhmlau
Copy link
Member

dhmlau commented Sep 22, 2022

Got @raymondfeng's approval per #228 (review). Merged the PR.
@samarpanB, thanks for your contribution! 🎉

@samarpanB samarpanB mentioned this pull request Nov 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achrinza achrinza achrinza left review comments

@raymondfeng raymondfeng raymondfeng approved these changes

@dhmlau dhmlau dhmlau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add capability for creating multiple rows in single query using insert into values (), (), () dialect
5 participants
@samarpanB @coveralls @dhmlau @raymondfeng @achrinza