in bulkCreate, updateOnDuplicate doesn’t work on included table

[John6Connor]

Issue Creation Checklist

• I understand that my issue will be automatically closed if I don't fill in the requested information
• I have read the contribution guidelines

Bug Description

When I execute the method bulkCreate with the options updateOnDuplicate and include, the updateOnDuplicate option is applied only in the main table and not in the included one. This imply that if I run two times bulkCreate with the same arguments it produce an error because, in the included table, it tries to insert records that already exist instead to try to update them.

Reproducible Example

I defined two models Teams and Players with the relation one-to-many.

Team model:

//file Team.ts

export class Team{
    code!:string;
    name?: string;
    country?: string;
    color?: string;
    trophies?: number;
    players?: Player[];
}

@Table
export class TeamModel extends Model<Team>{

    @Column({primaryKey: true})
    code!:string;

    @Column
    name?: string;

    @Column
    country?: string;

    @Column
    color?: string;

    @Column
    trophies?: number;

    @HasMany(() => PlayerModel)
    players: Player[];
}

Player model:

//file Player.ts
export class Player{
    code:string;
    name: string;
    surname: string;
    dateBirth: Date;
    country: string;
}

@Table
export class PlayerModel extends Model<Player>{

    @Column({primaryKey: true})
    code!:string;

    @Column
    name?: string;

    @Column
    surname?: string;

    @Column
    dateBirth?: Date;

    @Column
    country?: string;

    @ForeignKey(() => TeamModel)
    @Column
    teamCode?: string;

    @BelongsTo(() => TeamModel)
    team?: Team;
}

When I try to execute the following code, at the second time it produce an error

  TeamModel.bulkCreate([{
                code:'team001',
                name:'nameTeam0001',
                country:'countryTeam001',
                color:'colorTeam001',
                trophies:1,
                players:[{
                    code:'player0001',
                    name:'playername0001',
                    surname:'playernSurname0001',
                    dateBirth:new Date(),
                    country:'country0001',
                }]
            },
        ],
        {   
            include: [{model:PlayerModel,as:'players'}],
            updateOnDuplicate: ["country", "color", "thophies",]
        },
    );

the error is: parent: Error: Duplicate entry 'player0001' for key 'PlayerModels.PRIMARY'

By looking the queries that Sequelize produces, we can see that it puts the string ON DUPLICATE KEY UPDATE ... only in the INSERT query of the main table and not in the one that is included:

Executing (default): INSERT INTO `TeamModels` (`code`,`name`,`country`,`color`,`trophies`,`createdAt`,`updatedAt`) VALUES ('team001','nameTeam0001','countryTeam001','colorTeam001',1,'2022-06-14 15:37:18','2022-06-14 15:37:18') ON DUPLICATE KEY UPDATE `country`=VALUES(`country`),`color`=VALUES(`color`);
Executing (default): INSERT INTO `PlayerModels` (`code`,`name`,`surname`,`dateBirth`,`country`,`teamCode`,`createdAt`,`updatedAt`) VALUES ('player0001','playername0001','playernSurname0001','2022-06-14 15:37:18','country0001','team001','2022-06-14 15:37:18','2022-06-14 15:37:18');

What do you expect to happen?

I expect that also the included table receive the option updateOnDuplicate and therefore the ON DUPLICATE KEY UPDATE ...` is applied as well in the included query

What is actually happening?

Sequelize tries to insert records that are already present in included table instead to try to update them

Environment

• Sequelize version: sequelize@6.19.2
• Node.js version: v12.22.10
• If TypeScript related: TypeScript version:typescript@4.6.4
• Database & Version: MySQL 8.0.29

Would you be willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time and I know how to start.
• [] Yes, I have the time but I will need guidance.
• No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
• No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in resolving my issue.

---

Indicate your interest in the resolution of this issue by adding the 👍 reaction. Comments such as "+1" will be removed.

[evanrittenhouse]

@John6Connor I'll take this one. May ask you for some clarifications as I move forward.

[evanrittenhouse]

@ephys Quick question as I move into this - I notice this seems to be using sequelize-typescript. Is that going to have an effect on the code, or should the same issue happen in vanilla Sequelize?

[ephys]

The fact that it is written in sequelize-typescript shouldn't impact this, but I think the first step would be writing a short test that tries to replicate the issue without sequelize-typescript

[github-actions]

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

[evanrittenhouse]

@ephys Seems like I've found the problem, but I'm not sure how we want to solve it. In bulkCreate, we define a helper function called recursiveBulkCreate which walks down the include tree and bulkCreates the included models. In doing so, we actually only take the options for bulkCreating off of the included model, not off the parent model. This is why updateOnDuplicate doesn't get copied to the Player model. We need to decide which of the parent model's fields to add onto the child - in this case, which of the Team bulkCreateOptions should carry over to the Player.

The code in question can be found here. As a side note, how would you feel about a refactor of model.js to be more readable (maybe a /model folder that exports the class, and we can have all the helper functions as separate files)? I'd be happy to take that on if so.

[evanrittenhouse]

@John6Connor Just to clarify, are you expecting that the Players query should include the same updateOnDuplicate options in that query? Or that the included model, players, should be added to the Teams query?

[ephys]

If I understand the issue correctly, I think we can classify this as a missing feature rather than a bug

In terms of API design, I don't think we should copy the updateOnDuplicate option over. Should we maybe add it to include?

I think we have multiple options (I've trimmed the example a bit):

Design 1

include has its own updateOnDuplicate for that association

await TeamModel.bulkCreate([{
  code: 'team001',
  trophies:1,
  
  players: [{
    code: 'player0001',
    name: 'playername0001',
  }],
}], {   
  // this applies to the TeamModel bulk insert query
  updateOnDuplicate: ['trophies'],
  include: [{
    association: 'players',
    // this applies to the TeamModel.players bulk insert query
    updateOnDuplicate: ['name'],
  }],
});

Design 2

Only one updateOnDuplicate, but can 'update on duplicate' association elements by using a syntax associationName.associatedModelAttribute

await TeamModel.bulkCreate([{
  code: 'team001',
  trophies:1,
  
  players: [{
    code: 'player0001',
    name: 'playername0001',
  }],
}], {
  updateOnDuplicate: [
    // this applies to the TeamModel bulk insert query
    'trophies', 
    // this applies to the TeamModel.players bulk insert query
    'players.name',
  ],
  include: [{
    association: 'players',
  }],
});

[evanrittenhouse]

@ephys Personally, I prefer design 1. I think it's cleaner to have include-specific options residing in the include object itself. IMO, it also players nicer with the current recursive structure of bulkCreate.

It also allows for some future-proofing where we could create a general "override" mechanism in case something like this occurs with something other than updateOnDuplicate - we would just check all keys in include, and prioritize those over the ones that are in the parent object.

[ephys]

I also think design 1 is better

[SkycladObserver]

Hello! Following up on this. Seems the PR is already created but not yet merged so I just wanted to ping

[ramseyrashid]

Voting/checking for this as well. Please advise.

[ephys]

The relevant PR is #14739, it's pending a review

[mahantesh-maningol]

https://github.com/sequelize/sequelize/pull/14739 not yet merged