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
Nested creation #3386
Nested creation #3386
Conversation
This allows users to insert association data into the databased by specifying the proper include models in their build() data.
…elize into feat-nested-create
var values = {}; | ||
values[include.association.foreignKey] = self.get(self.Model.primaryKeyAttribute, {raw: true}); | ||
values[include.association.otherKey] = instance.get(instance.Model.primaryKeyAttribute, {raw: true}); | ||
return include.association.throughModel.create(values, {transaction: options.transaction, logging: console.log}); |
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.
whoops
LGTM |
nice 👍 |
What are the thoughts for further work on this? I would love to see it extended to work for @mickhansen I saw you said in #3169 that |
@troyastorino There are a ton more cases for update yes. Especially a case like UUID primary keys, how do you know whether it's a create or an existing - and a lot more cases like that ;p |
Ah ya, makes sense. Regardless, I'd love to see this, and hope I might be able to find some time to work on it at some point soon. Maybe a good first step would be for me to write test cases for all the situations I can think of, and then you and other maintainers could review and point out all the ones I'm missing? :) |
@troyastorino Test cases are a great help, especially if you can seperate them into a few PRs. |
This PR broke support for building models hydrated with an already-inserted relation. For example, this used to work: var user = <a pre-existing User model instance>
Product.create({
user: user,
UserId: user.id
}, { include: [User] }).then(function(product) {
// `product.user` would be set to the instance we provided above.
}); Since this PR, when _INSERT_ing the product, sequelize also attempts to insert the User, which will fail due to the unique constraint. However, the PR introduces a useful feature -- maybe it's possible to support both use-cases? |
@fixe Hmm yeah that's a case we didn't exactly consider. Product.create({
UserId: user.id
}, { include: [User] }).then(function(product) {
product.set('user', user);
// `product.user` would be set to the instance we provided above.
}); |
@mickhansen that's our current solution but it feels like sequelize should be handling this case. |
@fixe That would be the ideal :) There are quite a few edge cases we need to consider, raw data vs instances, AI PKs vs UUID pks (for instance), for non AI PK's we have no idea if the content needs to be created, updated or no-op'ed (unless wrapped in an instance which has isNewRecord) |
Is there any way to use this just to set many-to-many relations during a creation of the object? Product.create({
Users: [1, 2, 3, 4, 5]
}, { include: [User] }).then(function(product) {
}); Sequelize throws constraint error. I would just expect users to be set and returned as a |
@grabbou As noted above, this only works for new instances, not for existing ones. However, you can pass an array of ids to setUsers: Product.create().then(function(product) {
product.setUsers([1, 2, 3, 4, 5]);
}); |
Yeah, I know :) The only thing I am trying to solve is to avoid extra call to get the result of |
Great feature! +1 for update. |
How would you handle an insert that's 3 levels deep? That seems really hard to do. For exmaple, var football = {
name: 'NFL',
Teams: [
{
name: 'Minnesota Vikings',
GameDates: [ { date: '9/7/2015' }, { date: '9/22/2015' }, // the rest of the dates
]
},
{
name: 'Seattle Seahawks',
GameDates: [ { date: '9/10/2015' }, { date: '9/22/2015' }, // the rest of the dates
]
}
// ... the rest of the teams
]
}; Ideally I'd be able to do the insert like this: return Sport.create(football, {
include: [{
model: Team,
as: 'Teams',
include: [{
model: GameDate,
as: 'GameDates'
}]
}]
}); This would throw a notNull violation because I don't even know how I'd have to do this create. Probably something like this. let response;
return sequelize.transaction(t => {
return Sport.create(football, { transaction: t })
.then(sport => {
response = sport;
const teamPromises = [];
football.Teams.forEach(team => {
team.sportId = sport.id;
teamPromises = Team.Create(team, { transaction: t });
});
return Promise.all(teamPromises);
})
.then(teamResults => {
response.Teams = teamResults;
const gameDatePromises = [];
teamResults.forEach(tr => {
// figure out how to map tr to Team in the original football object
team.GameDates.forEach(gameDt => {
gameDt.teamId = teamResult.id; // teamResult was also mapped somehow.
});
gameDatePromises.push(GameDate.bulkCreate(team.GameDates, { transaction: t }));
});
return Promise.all(gameDatePromises);
})
.then(gameDateResults => {
// somehow map these results back into the response object we've been building.
return new Promise(resolve => resolve(response));
});
}); A lot more complex and there's some weird mapping steps that I haven't figured out yet. Please tell me there's an easier way to do this @mickhansen Edit: maybe the skip method in this issue will help. Either that or traversing through everything and manually setting a fake foreign key id. I'll get back on whether or not that worked. Edit 2: The build method wasn't very effective and was getting pretty complex. The setting a fake id didn't work at all, it gave me the following error. |
@bpmckee That's a known issue with creating a belongsTo with nested, you'll need to add |
+1 For update! What has been done so far? where may I start to help? |
@bpmckee I test that three levels deep creation problem you mention and it works, is it I'm missing something?. @mickhansen you respond...
But where is the belongsTo relation on his example?. var Sequelize = require('sequelize');
var sq = new Sequelize(null,null,null, {
dialect: 'sqlite'
});
var Sport = sq.define('Sport', {
name: Sequelize.STRING
});
var Team = sq.define('Team', {
name: Sequelize.STRING
});
var GameDate = sq.define('GameDate', {
date: Sequelize.DATE
});
var Coach = sq.define('Coach', {
name: Sequelize.STRING
});
Sport.hasMany(Team, {
foreignKey: {
allowNull: false
}
});
Team.hasMany(GameDate, {
foreignKey: {
allowNull: false
}
});
Team.belongsTo(Coach, {
foreignKey: {
allowNull: false
}
});
var football = {
name: 'NFL',
Teams: [
{
name: 'Minnesota Vikings',
GameDates: [{ date: '9/7/2015' }, { date: '9/22/2015' }],
Coach: { name: 'Mike Zimmer' },
CoachId: 0 //Math.random().toString() is not necessary
},
{
name: 'Seattle Seahawks',
GameDates: [{ date: '9/10/2015' }, { date: '9/22/2015' }],
Coach: { name: 'Pete Carroll' },
CoachId: 0 //just any value that help us pass the not null validation
}
]
};
sq.sync().then(function(){
Sport.create(football, {
include: [{
model: Team,
as: 'Teams',
include: [{
model: GameDate,
as: 'GameDates'
},{
model: Coach,
as: 'Coach'
}]
}]
}).then(function(instance){
console.log('All Good');
}).catch(function(err){
console.error(err);
});
}) |
@hendrul interesting. I'm not exactly sure what my differences were but the one thing I noticed was I setup the associations in both directions. Maybe I shouldn't have done that. My setup would have looked like this:
|
This doesn't supports association scopes |
Is it still impossible to add associations to existing models during create? |
So where are we with associations on build? Currently I have to do the following so as to have it all save together:
Is there a more abbreviated way of doing this? Such as:
|
Right now I need nested creation with existing rows :). Anybody interested in this case?, this PR would be great |
I ask, isn't the presence of the pk attribute enough to say that we want to associate instead of making an insert? or it's not a good idea for some reason? |
@hendrul Only in cases where you know that IDs are generated by the database |
What if we add support for some cases, this case of pks generated by db is pretty common, don't you think? |
It's common but these days i normally suggest people use UUIDs instead. |
No description provided.