geddy / model

Datastore-agnostic ORM in JavaScript
265 stars 55 forks source link

fix crash on SQL datastores when using defaultAdapter #208

Open moitias opened 10 years ago

moitias commented 10 years ago

this:

var model = require('model');
// this causes a crash
model.defaultAdapter = model.createAdapter('sqlite', { database: './dev.db' }); 
// this doesnt
//model.defaultAdapter = model.createAdapter('memory', { database: './dev.db' }); 

function Test() {
    this.defineProperties({
        "foobar": { "type" : "int "}
    });
}

Test = model.register('Test',Test);
Test.first(1,function(err,first) {
    console.log(err);
    console.log(first);
})

Crashes in adapters\sql\base.js:368 due to query.model.adapter being undefined as setAdapter has not been called because we're using a defaultAdapter.

I'm guessing this happens across all SQL adapters, but only tested with SQLite and MySQL (very quickly).

This PR makes sure the adapter is set on a model even when using defaultAdapter.

mde commented 10 years ago

This was probably broken by the change that moved a bunch of config options from the base 'model' namespace object to a config object. In reality we should be distinguishing between model.config.defaultAdapter, which should be a string key to indicate the adapter to use, and model.defaultAdapter, which should be the adapter itself. We've deprecated the ability to set .adapter directly on the model definition -- people should be using setAdapter -- and I think we should do the same thing here for the lib-level default adapter (i.e., implement model.setDefaultAdapter). In the meantime I'll fix the adapter lookup so that it will fall back correctly to model.defaultAdapter.

mde commented 10 years ago

This problem should be fixed in master, 7e5b3329a82e94edfd3de95a5c98ab3112acba86. Can you please verify?

moitias commented 10 years ago

This doesn't fix it, no.

EventedQueryProcessor constructor (in adapters/sql/base.js) still tries to look up the adapter from query.model and fails. GetAdapterForModel isn't called for this.

I guess if you're not using model.adapter anymore, then EventedQueryProcessor should also look it up from the lib, but as it only gets query (which doesnt know about the lib as far as I can see) I'm not quite sure where it should be looking it up from.

mde commented 10 years ago

Yes, in general we should probably be relying on a method call to look up the adapter so we can do fallback to the default. We probably need to implement a getAdapter method on the model definitions rather than accessing .adapter directly. (I guess it would in turn call getAdapterForModel.) Would you be interested in taking a shot at this?