find({ ... }) doesn't intelligently handle associations

[notheotherben]

Currently it is impossible to use a find({ association: instance }) condition correctly, as such conditions are passed directly to node-sql-query without any kind of intelligent filtering.

Repro

var person = db.define('person', {
    name: String
});

var pet = db.define('pet', {
    name: String
});

pet.hasOne('owner', person, {
    required: true
});

// Load some stuff into the database...

person.get(1, function(err, person) {
    pet.find({ owner: person }, function(err, pets) {
        // Generates an invalid SQL query
    });
});

Problem

Instead of using owner_id, and the id of the passed person, to run the query (as one would expect) we find node-orm2 passing the conditions in raw form to node-sql-query (not sql-query's fault, since it is supposed to be low level) which results in a SQL query like the following:

SELECT `id`, `name`, `owner_id` FROM `pet` WHERE `owner` = `id` = 1, `name` = 'John Doe'

What we would expect it to do is generate a SQL query like this:

SELECT `id`, `name`, `owner_id` FROM `pet` WHERE `owner_id` = 1

Possible Solutions

• Preprocess conditions as they are received, mapping conditions who's identifiers map to properties on the current model to the identifiers required to identify the relationship.
• Preprocess conditions, mapping if the value of a condition is an Instance object and the condition identifier maps to a property on the current model.

[dresende]

I'm working on this right now.

[notheotherben]

Ah, was also looking into it. Appears somewhat more complicated to fix nicely than I first expected - since conditions can also be used in table joins, and there doesn't appear to be any central point that they all pass through.

Currently working on adding a method to Model which preprocesses conditions, and then I'll try to get merge options to pass through that as well (or something similar).

It also appears that this only affects hasOne associations, since hasMany and extendsTo don't appear to add properties which could be used as conditions.

[dresende]

I'm just pointing my batteries to hasOne associations. I'm just going to update to support Array of instances. Other things can then be added and this function used in more places.

[notheotherben]

Appears to be working correctly for hasOne relationships, I'll look into adding support for hasMany and extendsTo when I get the chance.

[dresende]

Please add some tests if you have time :)

[notheotherben]

Will do

[notheotherben]

Hmm, just looked through your implementation fully and looks like there is currently no support for multiple primary keys. Unfortunately, adding support for multiple values AND primary keys requires that we add some additional functionality to node-sql-query.

Basically, if we want to support something like this

Pet.find({ owner: [owner1, owner2] }, function(err, pets) {
    //pets with either owner1 or owner2 as their owner, implied by same syntax as { owner_id: [owner1.id, owner2.id] }
});

we would need to add support for generating of a SQL query like

SELECT * FROM `pets` WHERE (`owner_id1` = `1` AND `owner_id2` = `1`) OR (`owner_id1` = `2` AND `owner_id2` = `2`)

My proposal would be for a custom conditions object which provides logical handling along the lines of

var conditions = ORM.conditions({ cond1: 'val1' }).and({ cond2: 'val2' }).or(ORM.conditions({ cond1: 'val11'}).and({ cond2: 'val22' }));

Thus, it should support chaining on the and and or methods, and support either anonymous condition objects (as the current implementation uses) or other ORM.conditions instances to allow for complex conditional queries.

The and and or methods should wrap their conditions in brackets when generating the query, unless only one key is present in the condition, in which case it should be added to the previous condition set (if it's of the same type - AND + AND = AND, OR + OR = OR, AND(x) + OR(y) = (x) OR (y))

var conditions = ORM.conditions({ name: 'John'});
// `name` = 'John'
conditions = conditions.and({ surname: 'Doe' });
// `name` = 'John' AND `surname` = 'Doe'
conditions = conditions.or({ name: 'Sally', surname: 'Black'});
// (`name` = 'John' AND `surname` = 'Doe') OR (`name` = 'Sally' AND `surname` = 'Black')

[dresende]

sql-query supports OR but I'm not sure if it's easy to add this.

https://github.com/dresende/node-sql-query/blob/master/test/integration/test-where-advanced.js

[notheotherben]

Ah, neat. Let me see what I can do about adding it now

[notheotherben]

Okay, just a quick question - is it fine if I use Model.table to determine equality between models? Can't think of any other nice way to do it, since it is necessary to ensure that only instances of the type required by the relevant property are processed.

[dresende]

I think it's OK for now, but you can create an uid value or something (like Instance has using hat module).

[notheotherben]

Okay, let me do that instead then. Having issues at the moment getting tests to pass for SQLite, it appears to dislike returning a valid owner object when matching on pet_id (although all the other DB engines appear to work so far). Been bashing my head against a wall for hours on it, and can't seem to figure out the problem (doesn't help that node-sqlite won't build on my local machine)

[notheotherben]

Okay, another question: Why are we using a different primarykey format for MongoDB?

[dresende]

MongoDB default id is "_id" (automatically added). I don't have enough experience to know how to change this name.

[notheotherben]

Hmm, okay. Also looks like MongoDB automatically increments that ID on changes to the model - which makes some of the tests in my suite fail. I'm tempted to just check the name property (which is unique in this case) but it doesn't solve the underlying issue.

[dresende]

The _id does never change. If it's changing it might be because of recreating instances.

[notheotherben]

Well here's the test I was running, and _id was being incremented on the same model by the looks of it.

var setup = function () {
        return function (done) {
            Person = db.define('person', {
                name     : String
            });
            Pet = db.define('pet', {
                name     : String
            });
            Person.hasOne('pet', Pet, {
                reverse: 'owner',
                field: 'pet_id'
            });

            return helper.dropSync([ Person, Pet ], function () {
                async.parallel([
                    Person.create.bind(Person, { name: "John Doe" }),
                    Person.create.bind(Person, { name: "Jane Doe" }),
                    Pet.create.bind(Pet, { name: "Deco" }),
                    Pet.create.bind(Pet, { name: "Fido" }),
                ], done);
            });
        };
    };

    // Other tests...

    it("should be able to find given an association instance", function (done) {
            Person.find({ name: "John Doe" }).first(function (err, John) {
                should.not.exist(err);
                should.exist(John);
                Pet.find({ name: "Deco" }).first(function (err, Deco) {
                    should.not.exist(err);
                    should.exist(Deco);
                    Deco.hasOwner(function (err, has_owner) {
                        should.not.exist(err);
                        has_owner.should.be.false;

                        Deco.setOwner(John, function (err) {
                            should.not.exist(err);

                            Person.find({ pet: Deco }).first(function (err, owner) {
                                should.not.exist(err);
                                should.exist(owner);
                                should.equal(owner[Person.id[0]], John[Person.id[0]]);
                                done();
                            });

                        });
                    });
                });
            });
        });

    // And 2 other tests which are variants thereof

The Person.id[0] is just to get id for *SQL and _id for MongoDB. If you can spot anything in there which would be recreating the instances on Mongo (but not SQL) then let me know and I'll change it, otherwise we may have a bug in our Mongo implementation...

[notheotherben]

Okay, I think we can mark this as resolved as of the latest code in master. We currently handle both multiple instance and multiple identifier key query generation, and haven't had any test failures as a result of the new code.

If any issues are found then we'll re-open this, but in the mean time I'm gonna close it.