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

Jump to bottom

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

Closed
mlegenhausen opened this issue Sep 20, 2014 · 10 comments
Closed

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

mlegenhausen opened this issue Sep 20, 2014 · 10 comments
Assignees
@janmeier
Labels
type: bug

Comments

@mlegenhausen
Copy link
Contributor 

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
Copy link
Member

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
Copy link
Member

Which version?

@mlegenhausen
Copy link
Contributor Author

Current master

@mlegenhausen
Copy link
Contributor Author

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

@mickhansen
Copy link
Contributor

@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
Copy link
Member

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

@janmeier janmeier self-assigned this Sep 22, 2014
@mickhansen
Copy link
Contributor

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

@janmeier
Copy link
Member

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

@mickhansen
Copy link
Contributor

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

@janmeier
Copy link
Member

Closed in d3bf724

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@janmeier janmeier

Labels
type: bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mlegenhausen @mickhansen @janmeier