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

Conversation

@mickhansen
Contributor

mickhansen commented Mar 21, 2015

No description provided.

mbroadst and others added some commits Feb 18, 2015

initial support for relations in Instance.save
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});

This comment has been minimized.

@mickhansen

mickhansen Mar 21, 2015

Contributor

whoops

@mbroadst

This comment has been minimized.

Contributor

mbroadst commented Mar 21, 2015

LGTM

mickhansen added a commit that referenced this pull request Mar 21, 2015

@mickhansen mickhansen merged commit 95f2107 into master Mar 21, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@mickhansen mickhansen referenced this pull request Mar 24, 2015

Closed

Create with includes #1838

@pilsy

This comment has been minimized.

pilsy commented Mar 29, 2015

nice 👍

@troyastorino

This comment has been minimized.

troyastorino commented Apr 9, 2015

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

This comment has been minimized.

Contributor

mickhansen commented Apr 10, 2015

@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

This comment has been minimized.

troyastorino commented Apr 10, 2015

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

This comment has been minimized.

Contributor

mickhansen commented Apr 10, 2015

@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 Apr 10, 2015

@fixe

This comment has been minimized.

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

This comment has been minimized.

Contributor

mickhansen commented Apr 28, 2015

@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

This comment has been minimized.

Contributor

fixe commented Apr 28, 2015

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

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 28, 2015

@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

This comment has been minimized.

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

This comment has been minimized.

Member

janmeier commented Apr 30, 2015

@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

This comment has been minimized.

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

This comment has been minimized.

mshahriarinia commented Jun 5, 2015

Great feature! +1 for update.

@bpmckee

This comment has been minimized.

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

This comment has been minimized.

Contributor

mickhansen commented Dec 21, 2015

@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

This comment has been minimized.

Contributor

hendrul commented Jan 6, 2016

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

@hendrul

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

hendrul commented Jun 21, 2016

This doesn't supports association scopes

@Ajaxy

This comment has been minimized.

Ajaxy commented Feb 6, 2017

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

@jasonbodily

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

mickhansen commented Jan 29, 2018

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

@hendrul

This comment has been minimized.

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

This comment has been minimized.

Contributor

mickhansen commented Jan 29, 2018

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