-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
base: main
Are you sure you want to change the base?
feat: make the include
option of Model.create
default to all provided associations
#16922
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
packages/core/src/model.js
Outdated
@@ -1759,6 +1763,18 @@ ${associationOwner._getAssociationDebugList()}`); | |||
}).save(options); | |||
} | |||
|
|||
static _searchInclude(values) { |
There was a problem hiding this comment.
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
packages/core/src/model.js
Outdated
@@ -1759,6 +1763,18 @@ ${associationOwner._getAssociationDebugList()}`); | |||
}).save(options); | |||
} | |||
|
|||
static _searchInclude(values) { | |||
const association = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const association = []; | |
const associations = []; |
packages/core/src/model.js
Outdated
@@ -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; |
There was a problem hiding this comment.
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);
packages/core/src/model.js
Outdated
@@ -1759,6 +1763,18 @@ ${associationOwner._getAssociationDebugList()}`); | |||
}).save(options); | |||
} | |||
|
|||
static _searchInclude(values) { | |||
const association = []; | |||
const associationKeys = Object.keys(this.associations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const associationKeys = Object.keys(this.associations); | |
const associationNames = Object.keys(this.associations); |
packages/core/src/model.js
Outdated
const associationKeys = Object.keys(this.associations); | ||
for (const value in values) { | ||
if (associationKeys.includes(value)) { | ||
association.push({ association: value }); |
There was a problem hiding this comment.
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
@ephys I made several corrections. This is ready to review. |
Can you fix the prettier issue? We like to review PRs once CI has passed. |
include
option of Model.create
default to all provided associations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work on this
@@ -1916,6 +1917,7 @@ ${associationOwner._getAssociationDebugList()}`); | |||
*/ | |||
static async create(values, options) { | |||
options = cloneDeep(options) ?? {}; | |||
options.include ??= getDefaultCreateInclude(this, values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think belongs inside of save
when isNewRecord
is true, because calling build then save should work too
export function getDefaultCreateInclude(model: ModelStatic, values: Object) { | ||
const associations = []; | ||
const associationNames = Object.keys(model.associations); | ||
if (values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the typing, this condition is always true
@@ -273,3 +273,17 @@ Either add a primary key to this model, or use one of the following alternatives | |||
); | |||
} | |||
} | |||
|
|||
export function getDefaultCreateInclude(model: ModelStatic, values: Object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function getDefaultCreateInclude(model: ModelStatic, values: Object) { | |
export function getDefaultCreateInclude(model: ModelStatic, values: UnknownRecord) { |
@@ -1593,6 +1593,37 @@ describe(Support.getTestDialectTeaser('Model'), () => { | |||
}); | |||
}); | |||
}); | |||
it('should create associated objects without include option', async function () { | |||
const Player = this.customSequelize.define('player', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const Player = this.customSequelize.define('player', { | |
const Player = this.customSequelize.define('Player', { |
type: DataTypes.INTEGER, | ||
}, | ||
}); | ||
const Team = this.customSequelize.define('team', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const Team = this.customSequelize.define('team', { | |
const Team = this.customSequelize.define('Team', { |
type: DataTypes.STRING, | ||
}, | ||
}); | ||
Player.team = Player.belongsTo(Team, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Player.team = Player.belongsTo(Team, { | |
Player.belongsTo(Team, { |
Pull Request Checklist
Description Of Change
Its a feature request opened by #15233
Todos
Models.create