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: make the include option of Model.create default to all provided associations #16922

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
16 changes: 16 additions & 0 deletions packages/core/src/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -1749,6 +1749,10 @@ ${associationOwner._getAssociationDebugList()}`);
*/
static async create(values, options) {
options = cloneDeep(options) ?? {};
const association = this._searchInclude(values);
if (association.length > 0 && !options.include) {
options.include = association;
Copy link
Member

Choose a reason for hiding this comment

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

only call _searchInclude if options.include is not set, to avoid computing it if not necessary

The association.length > 0 check should not be necessary and can be removed

association should be plural, but entire code can be simplified to this:

options.include ??= getDefaultCreateInclude(this, values);

}

return await this.build(values, {
isNewRecord: true,
Expand All @@ -1759,6 +1763,18 @@ ${associationOwner._getAssociationDebugList()}`);
}).save(options);
}

static _searchInclude(values) {
Copy link
Member

Choose a reason for hiding this comment

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

New functions like this one should go in model-internals.ts instead

I think getDefaultCreateInclude may be a more explicit name

const association = [];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const association = [];
const associations = [];

const associationKeys = Object.keys(this.associations);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const associationKeys = Object.keys(this.associations);
const associationNames = Object.keys(this.associations);

for (const value in values) {
if (associationKeys.includes(value)) {
association.push({ association: value });
Copy link
Member

Choose a reason for hiding this comment

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

Also I think you can just push value and that the { association } wrapper is redundant

}
}

return association;
}

/**
* Find an entity that matches the query, or build (but don't save) the entity if none is found.
* The successful result of the promise will be the tuple [instance, initialized].
Expand Down
30 changes: 30 additions & 0 deletions packages/core/test/integration/model/create.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1508,6 +1508,36 @@ describe(Support.getTestDialectTeaser('Model'), () => {
});
});
});
it('should create associated objects without include option', async function () {
const Player = this.customSequelize.define('player', {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const Player = this.customSequelize.define('player', {
const Player = this.customSequelize.define('Player', {

name: {
type: DataTypes.STRING,
},
teamId: {
type: DataTypes.INTEGER,
},
});
const Team = this.customSequelize.define('team', {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const Team = this.customSequelize.define('team', {
const Team = this.customSequelize.define('Team', {

id: {
type: DataTypes.INTEGER,
primaryKey: true,
},
name: {
type: DataTypes.STRING,
},
});
Player.team = Player.belongsTo(Team, {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Player.team = Player.belongsTo(Team, {
Player.belongsTo(Team, {

as: 'team',
foreignKey: 'teamId',
targetKey: 'id',
});
await this.customSequelize.sync({ force: true });
const createdPlayer = await Player.create({
name: 'Player 1', team: { id: 1, name: 'My new team' },
});
expect(createdPlayer.team instanceof Team).to.be.ok;
expect(createdPlayer.teamId).to.equal(1);
});
});

it('should return autoIncrement primary key (create)', async function () {
Expand Down