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

fix crash on SQL datastores when using defaultAdapter #208

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix crash on SQL datastores when using defaultAdapter #208

wants to merge 1 commit into from

Conversation

moitias
Copy link

@moitias moitias commented Jun 9, 2014

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

mde commented Jun 9, 2014

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

mde commented Jun 9, 2014

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

@moitias
Copy link
Author

moitias commented Jun 10, 2014

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

mde commented Jun 10, 2014

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants