Sequelize getter produce invalid/repeating query when given an options object

[mlegenhausen]

var co = require('co');
var Sequelize = require('sequelize');

var sequelize = new Sequelize('sequelize-test', 'root', '');

var A = sequelize.define('A', {});
var B = sequelize.define('B', {});
var C = sequelize.define('C', {});

A.hasMany(B);
B.belongsTo(A);

A.hasMany(C);
C.belongsTo(A);

co(function* () {
    yield sequelize.sync({
        force: true
    });

    var options = {
    };

    var a = yield A.create(options);
    yield a.getBs(options);
    yield a.getCs(options);
})();

Generated sql:

Executing (default): INSERT INTO `As` (`id`,`createdAt`,`updatedAt`) VALUES (DEFAULT,'2014-09-20 19:25:09','2014-09-20 19:25:09');
Executing (default): SELECT `id`, `createdAt`, `updatedAt`, `AId` FROM `Bs` AS `B` WHERE (`B`.`AId`=1 AND 1=1);
Executing (default): SELECT `id`, `createdAt`, `updatedAt`, `AId` FROM `Cs` AS `C` WHERE (`C`.`AId`=1 AND 1=1 AND (`B`.`AId`=1 AND 1=1));

It seems query information is attached to the options object you can repeat the get calls to make the SELECT query as long as you want.

Example with 3 getBs(options) calls:

Executing (default): INSERT INTO `As` (`id`,`createdAt`,`updatedAt`) VALUES (DEFAULT,'2014-09-20 19:27:24','2014-09-20 19:27:24');
Executing (default): SELECT `id`, `createdAt`, `updatedAt`, `AId` FROM `Bs` AS `B` WHERE (`B`.`AId`=1 AND 1=1);
Executing (default): SELECT `id`, `createdAt`, `updatedAt`, `AId` FROM `Bs` AS `B` WHERE (`B`.`AId`=1 AND 1=1 AND (`B`.`AId`=1 AND 1=1));
Executing (default): SELECT `id`, `createdAt`, `updatedAt`, `AId` FROM `Bs` AS `B` WHERE (`B`.`AId`=1 AND 1=1 AND (`B`.`AId`=1 AND 1=1 AND (`B`.`AId`=1 AND 1=1)));

And what does the 1=1 do?

[janmeier]

When the where object is empty / has an unexpected structure it defaults to 1=1 which is always true. It seems the options are added into the object.

Ideally we should of course be cloning it each time but it's a question of performance vs being able to reuse the the object

[janmeier]

Which version?

[mlegenhausen]

Current master

[mlegenhausen]

Changing given objects from the user can produce so many side effects and is really frustrating.

[mickhansen]

@janmeier we should already be cloning options on all find calls and i think all association getters use find internally now.

Edit: But looking at the SQL we might not be.

In any case the performance overhead of cloning on getters is negligible, they are not an often called method in the execution change of fetching something.

[janmeier]

@mickhansen I though the same, but the problem is that the getter adds an and. https://github.com/sequelize/sequelize/blob/master/lib/associations/has-many-double-linked.js#L28-L32 Since the options.where is empty it resolves to 1=1, and does so every time :).

[mickhansen]

@janmeier the 1=1 one thing is known, but the bug here is that it keeps modifying and reusing the same object right?

[janmeier]

Yep, each call does a sequelize.and with the current where object in the getter, which is before it is cloned in find

[mickhansen]

@janmeier oh right, yea so we need to clone it in the getter.

[janmeier]

Closed in d3bf724