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.

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

feat: support include option in bulkInsert #11307

Merged
merged 2 commits into from Aug 13, 2019

Conversation

javiertury
Copy link
Contributor

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

Closes #3277.

This PR makes bulkInsert capable of handling associations. It uses the include option, just like other model methods.

It requires only 1 insert statement for each association model. E.g. 100 instances with 100 children and 100 grandchildren are created in 3 insert statements.

@codecov
Copy link

codecov bot commented Aug 11, 2019

Codecov Report

Merging #11307 into master will decrease coverage by 0.01%.
The diff coverage is 93.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11307      +/-   ##
==========================================
- Coverage   96.32%   96.31%   -0.02%     
==========================================
  Files          94       94              
  Lines        9069     9142      +73     
==========================================
+ Hits         8736     8805      +69     
- Misses        333      337       +4
Impacted Files Coverage Δ
lib/model.js 96.46% <93.38%> (-0.23%) ⬇️
lib/dialects/postgres/connection-manager.js 95.77% <0%> (+1.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a39c63a...819c0ef. Read the comment docs.

@papb
Copy link
Member

papb commented Aug 11, 2019

Hey, thanks for the PR. Can you show a code snippet showing the usage as an example?

@papb papb added status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action type: feature For issues and PRs. For new features. Never breaking changes. labels Aug 11, 2019
@javiertury
Copy link
Contributor Author

The same as regular create method.

        const Product = this.sequelize.define('Product', {
          title: Sequelize.STRING
        });
        const Tag = this.sequelize.define('Tag', {
          name: Sequelize.STRING
        });

        Product.hasMany(Tag);

        return this.sequelize.sync({ force: true }).then(() => {
          return Product.bulkCreate([{
            id: 1,
            title: 'Chair',
            Tags: [
              { id: 1, name: 'Alpha' },
              { id: 2, name: 'Beta' }
            ]
          }, {
            id: 2,
            title: 'Table',
            Tags: [
              { id: 3, name: 'Gamma' },
              { id: 4, name: 'Delta' }
            ]
          }], {
            include: [{
              model: Tag,
              myOption: 'option'
            }]
          }).then(savedProducts => {
            // ...
          });

@sushantdhiman
Copy link
Contributor

Looks good, can you update typings https://github.com/sequelize/sequelize/blob/master/types/lib/model.d.ts#L707

if (!instance || key === model.primaryKeyAttribute &&
instance.get(model.primaryKeyAttribute) &&
['mysql', 'mariadb', 'sqlite'].includes(dialect)) {
// The query.js for these DBs is blind, it autoincrements the
Copy link
Contributor

Choose a reason for hiding this comment

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

For MySQL I have seen this behaviour with updateOnDuplicate #11223 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can reliably support this unless MySQL adds support for RETURNING * equivalent

That is exactly the root of the problem. PostgreSQL and SQL Server both support RETURNING and therefore don't have any issue.

@javiertury
Copy link
Contributor Author

@sushantdhiman typings added

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

I haven't given new code a deep look but new tests looks good and old one passed :)

@papb
Copy link
Member

papb commented Aug 11, 2019

I haven't given new code a deep look but new tests looks good and old one passed :)

Same from me! 👍

@papb papb added status: awaiting maintainer and removed status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action labels Aug 11, 2019
@sushantdhiman sushantdhiman merged commit 4f09899 into sequelize:master Aug 13, 2019
@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 5.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@simonbrent
Copy link

Thanks a lot for implementing this feature, it will be very helpful.

I have one observation regarding it though - the options passed to recursiveBulkCreate are as follows:

const includeOptions =  _(Utils.cloneDeep(include))
              .omit(['association'])
              .defaults({
                transaction: options.transaction,
                logging: options.logging
              }).value();

but this means that if I want to validate at every stage of creation, I cannot just set validate: true as a top level option - instead I must set validate: true both at the top level and in every include.

Would you be willing to either allow the includeOptions to default validate to options.validate, or maybe if that is undesirable, support a validateAll option, which is used to set options.validate to true at the start of recursiveBulkCreate?

@javiertury javiertury deleted the bulk_create_include branch September 3, 2019 14:05
@javiertury
Copy link
Contributor Author

I see your point about validation. I'm now wondering why validation is disabled by default. Is bulkCreate assumed to not receive user input? Is it about performance?

@simonbrent, are you using bulkCreate with user input?

@simonbrent
Copy link

@javiertury yes I am.
I've got a workaround for now in a beforeBulkCreate hook, which modifies options.include to set validate: options.validate for each entry in the include array.

I assume validation is disabled by default because it can be expensive.

mrrinot pushed a commit to mrrinot/sequelize that referenced this pull request Feb 19, 2020
@marinalohova
Copy link

marinalohova commented Apr 3, 2020

Hi, I'm getting
"instance[include.association.accessors.set] is not a function"

My code is as following:

await this.models.products.bulkCreate(products, {
        updateOnDuplicate: ['name', 'description', 'subscriptionId', 'creditId'],
        returning: true,
        include: {
          model: this.models.subscriptions,
          as: 'subscription',
          updateOnDuplicate: ['subscriptionFee', 'subscriptionReoccurrence'],
          returning: true,
        },
      });

What could be the issue? Thanks!

@jeffersonveloso
Copy link

Hi, I'm getting
"instance[include.association.accessors.set] is not a function"

My code is as following:

await this.models.products.bulkCreate(products, {
        updateOnDuplicate: ['name', 'description', 'subscriptionId', 'creditId'],
        returning: true,
        include: {
          model: this.models.subscriptions,
          as: 'subscription',
          updateOnDuplicate: ['subscriptionFee', 'subscriptionReoccurrence'],
          returning: true,
        },
      });

What could be the issue? Thanks!

Have any solution for this problem ? @srslyimthebest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released type: feature For issues and PRs. For new features. Never breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there any option for create bulk asociations?
6 participants