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

Nested creation #3386

Merged
merged 8 commits into from Mar 21, 2015
Merged

Nested creation #3386

merged 8 commits into from Mar 21, 2015

Conversation

mickhansen
Copy link
Contributor

No description provided.

Matt Broadstone and others added 3 commits February 19, 2015 10:31
This allows users to insert association data into the databased by
specifying the proper include models in their build() data.
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});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops

@mbroadst
Copy link
Contributor

LGTM

mickhansen added a commit that referenced this pull request Mar 21, 2015
@mickhansen mickhansen merged commit 95f2107 into master Mar 21, 2015
@mickhansen mickhansen mentioned this pull request Mar 24, 2015
@pilsy
Copy link

pilsy commented Mar 29, 2015

nice 👍

@troyastorino
Copy link

What are the thoughts for further work on this? I would love to see it extended to work for update.

@mickhansen I saw you said in #3169 that update would be way harder. Would that be because it would touch code in a lot more places, or because there are more situations you'd have to cover (update nested model if it does exist, create new one if it doesn't, etc.)?

@mickhansen
Copy link
Contributor Author

@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

@troyastorino
Copy link

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? :)

@mickhansen
Copy link
Contributor Author

@troyastorino Test cases are a great help, especially if you can seperate them into a few PRs.
We could also definitely start out by only supporting AI/SERIAL primary keys in the beginning.

@mbroadst mbroadst deleted the feat-nested-create branch April 10, 2015 18:53
@fixe
Copy link
Contributor

fixe commented Apr 28, 2015

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?

@mickhansen
Copy link
Contributor Author

@fixe Hmm yeah that's a case we didn't exactly consider.
Can you give this a shot:

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.
});

@fixe
Copy link
Contributor

fixe commented Apr 28, 2015

@mickhansen that's our current solution but it feels like sequelize should be handling this case.

@mickhansen
Copy link
Contributor Author

@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)

@grabbou
Copy link

grabbou commented Apr 30, 2015

Is there any way to use this just to set many-to-many relations during a creation of the object?
In my case

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 product.users instead of being created. Is there any relatively fast and efficient way to accomplish that?

@janmeier
Copy link
Member

@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]);
});

@grabbou
Copy link

grabbou commented Apr 30, 2015

Yeah, I know :) The only thing I am trying to solve is to avoid extra call to get the result of set operation. In my case, I am having a REST endpoint where user sends a title of a Product and passes ids of Categories. For now, I've managed to do that with create(include: Categories), setCategories(ids), reload, just because I needed to return full output to a client (with all categories filled in). (Un)fortunately, the product of setCategories is a ThroughModel, not a collection of destination model.

@mshahriarinia
Copy link

Great feature! +1 for update.

@bpmckee
Copy link

bpmckee commented Dec 19, 2015

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 sportId on the Team model would not be defined yet and it'd break there.

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.
SequelizeForeignKeyConstraintError: ER_NO_REFERENCED_ROW_2: Cannot add or update a child row: a foreign key constraint fails.
Giving up on it tonight, maybe I'll have better luck tomorrow. :/

@mickhansen
Copy link
Contributor Author

@bpmckee That's a known issue with creating a belongsTo with nested, you'll need to add foreignKey: Math.random().toString() to the parent level object as a work around.

@hendrul
Copy link
Contributor

hendrul commented Jan 6, 2016

+1 For update! What has been done so far? where may I start to help?

@hendrul
Copy link
Contributor

hendrul commented Jan 31, 2016

@bpmckee I test that three levels deep creation problem you mention and it works, is it I'm missing something?.

@mickhansen you respond...

That's a known issue with creating a belongsTo with nested

But where is the belongsTo relation on his example?.
To make justice to that answer I add a belongsTo relationship to the Sports example above:

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);
    });
})

@bpmckee
Copy link

bpmckee commented Jan 31, 2016

@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:

Sport.hasMany(Team, {
    foreignKey: {
        allowNull: false
    }
});
Team.belongsTo(Sport, {
    foreignKey: 'sport_id'
});
Team.hasMany(GameDate, {
    foreignKey: {
        allowNull: false
    }
});
GameDate.belongsTo(Team, {
    foreignKey: 'team_id'
});
Team.hasOne(Coach, {
    foreignKey: { allowNull: false }
});
Coach.belongsTo(Team, {
    foreignKey: 'team_id'
});

@hendrul
Copy link
Contributor

hendrul commented Jun 21, 2016

This doesn't supports association scopes

@Ajaxy
Copy link

Ajaxy commented Feb 6, 2017

Is it still impossible to add associations to existing models during create?
(example #3386 (comment))

@jasonbodily
Copy link

jasonbodily commented Feb 15, 2017

So where are we with associations on build? Currently I have to do the following so as to have it all save together:

let person = Person.build()

/* update person attributes */

let person_obj  = person.toJSON();
person_obj.cars = cars; // Association


Person.create(person_obj, {
  include: [{ model: Cars, as: 'cars'}]
}) 

Is there a more abbreviated way of doing this? Such as:

let person = Person.build()

/* update person attributes */
person.cars = cars; // Association

person.save({
  include: [{ model: Cars, as: 'cars'}]
}) 

@hendrul
Copy link
Contributor

hendrul commented Jan 29, 2018

Right now I need nested creation with existing rows :). Anybody interested in this case?, this PR would be great

@hendrul
Copy link
Contributor

hendrul commented Jan 29, 2018

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?

@mickhansen
Copy link
Contributor Author

@hendrul Only in cases where you know that IDs are generated by the database

@hendrul
Copy link
Contributor

hendrul commented Jan 29, 2018

What if we add support for some cases, this case of pks generated by db is pretty common, don't you think?

@mickhansen
Copy link
Contributor Author

It's common but these days i normally suggest people use UUIDs instead.
As for adding support, i'm not an active maintainer anymore, just adding why it's not as simple as looking for the PK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet