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

bulkCreate would have problems with a disparate field list. Closes #736 #738

Merged
merged 3 commits into from Jul 16, 2013

Conversation

3 participants
@durango
Member

durango commented Jun 29, 2013

I'm not sure if this affects performance much, probably a little bit especially with a lot of rows... but there are much bigger things to blame for performance issues than this PR. Sequelize needs a little bit of remodeling pretty soon ;)

@durango

This comment has been minimized.

Member

durango commented Jun 29, 2013

I believe the tests in hasMany associations should be changed to (2,1) from (1,2)... please check travis logs to see what I'm talking about. This might be a false flag in the current test suite :)

@janmeier

This comment has been minimized.

Member

janmeier commented Jun 29, 2013

I believe it was changed in a recent PR. Will try to figure it whats correct :D

@janmeier

This comment has been minimized.

Member

janmeier commented Jun 29, 2013

I would say that

INSERT INTO `TasksUsers` (`TaskId`,`UserId`) VALUES (1,1),(2,1)

is correct. We create one user and assigns two tasks. The userid is the same, but taskid is different

Actually, to make this test completely valid we should also check the correct order of the columns (i.e. also assert that the order is TaskId, UserId and not the other way around). The problem then becomes that we have to take into account different qouting methods. Perhaps we could assert that indexof taskid < indexof userid?

@durango

This comment has been minimized.

Member

durango commented Jul 1, 2013

@sdepold (or @mickhansen or anyone else for that matter) you mind voicing your opinion on this issue? Only reason why I'm pinging is because this PR would be changing a test case. Just want to make sure @janmeier and myself aren't crazy ;)

@durango

This comment has been minimized.

Member

durango commented Jul 8, 2013

@janmeier updated :)

@@ -385,7 +385,7 @@ describe(Helpers.getTestDialectTeaser("HasMany"), function() {
this.Task.create({ title: 'task2' }).success(function(task2) {
user.setTasks([task1, task2]).on('sql', spy).on('sql', _.after(2, function (sql) {

This comment has been minimized.

@sdepold

sdepold Jul 16, 2013

Member

wow smart :)

@sdepold

This comment has been minimized.

Member

sdepold commented Jul 16, 2013

looks good. also 2,1 seems to be the more correct version

sdepold added a commit that referenced this pull request Jul 16, 2013

Merge pull request #738 from durango/bulkcreate
bulkCreate would have problems with a disparate field list. Closes #736

@sdepold sdepold merged commit 93016a0 into sequelize:master Jul 16, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment