Skip to content

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

Merged
dhmlau merged 1 commit intoloopbackio:masterfrom
samarpanB:create-all-optimize
Nov 1, 2022
Merged

feat: add capability for insert multiple rows in single query#1985
dhmlau merged 1 commit intoloopbackio:masterfrom
samarpanB:create-all-optimize

Conversation

@samarpanB
Copy link
Contributor

@samarpanB samarpanB commented Sep 15, 2022

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

Adds new createAll method to support insert multiple rows in single query in memory connector
Enables multiInsertSupport in memory connector
Adds capability to createAll in dao.js for models to do multiple insert in one query via the supported connectors

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

Checklist

  • Sign off your commits with DCO (Developer Certificate of Origin)
  • 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 September 15, 2022 11:43
@coveralls
Copy link

coveralls commented Sep 15, 2022

Pull Request Test Coverage Report for Build 3167967406

  • 91 of 106 (85.85%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 84.667%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/connectors/memory.js 11 13 84.62%
lib/dao.js 80 93 86.02%
Totals Coverage Status
Change from base Build 3155109076: -0.09%
Covered Lines: 7250
Relevant Lines: 8255

💛 - Coveralls

@samarpanB
Copy link
Contributor Author

Added persistence hooks test cases as well. While adding those, I found some issues, fixed them as well. Please review @dhmlau @achrinza

@samarpanB samarpanB force-pushed the create-all-optimize branch 2 times, most recently from fd20c9d to a86b063 Compare September 25, 2022 19:32
@samarpanB
Copy link
Contributor Author

loopback-connector release is needed before it can be merged. However, this can be reviewed until then.

Signed-off-by: Samarpan Bhattacharya <this.is.samy@gmail.com>
@samarpanB samarpanB force-pushed the create-all-optimize branch from a86b063 to 6ed63ea Compare October 2, 2022 08:54
@samarpanB
Copy link
Contributor Author

Updated connector version after rebase.

@samarpanB
Copy link
Contributor Author

@raymondfeng @achrinza @dhmlau Please review.

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 the PR. Your changes look reasonable to me. I have 2 minor comments, please review.

Before merging this PR, I'd like to get @raymondfeng or one more maintainer to approve. Thanks.


hookMonitor.names.should.eql([
'before save',
'before save',
Copy link
Member

Choose a reason for hiding this comment

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

It duplicates with the line above. Do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thats how the hooks used to work previously as well, when inserting multiple records in create method. For each entry in the array of records to be inserted, before save and after save hooks independently. SO as many records, that many hooks triggered. I kept the behaviour same. There are some data validation logic which is useful if triggered for each entry separately.

'persist',
'loaded',
'after save',
'after save',
Copy link
Member

Choose a reason for hiding this comment

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

same comment. Duplicate line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied above

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 @dhmlau's remarks, everything else LGTM 👍

@samarpanB
Copy link
Contributor Author

@dhmlau I replied to your comment. Can you please check if I need to change the behaviour ?

@dhmlau
Copy link
Member

dhmlau commented Nov 1, 2022

@samarpanB, thanks for your response. LGTM. Since I have the "go ahead" from @achrinza as well, I'll be merging this PR. Thanks!

@dhmlau dhmlau merged commit d29bec7 into loopbackio:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add capability for creating multiple rows in single query using insert into values (), (), () dialect

4 participants