1602 / jugglingdb

Multi-database ORM for nodejs: redis, mongodb, mysql, sqlite3, postgresql, arango, in-memory...
http://1602.github.io/jugglingdb/
2.04k stars 241 forks source link

Text fields values don't persist #340

Closed stansm closed 10 years ago

stansm commented 10 years ago

Schema.Text() function returns empty value and this doesn't allow to persist values from Text fields.

spericas commented 10 years ago

I have found this issue using the MySQL adapter and the proposed solution works. It doesn't appear to happen with other adapters like MongoDB.

dgsan commented 10 years ago

If it happens in mysql adapter maybe write a test case to include in mysql adapter's unit tests and make a pull request to the adapter?

spericas commented 10 years ago

I'll try that. A quick run of the tests gives me a failure that can be resolved replacing 'UserData' by 'userdata' (all lowercase) in migration.test.js. MySQL version 5.6.14.

dgsan commented 10 years ago

That actually seems pretty odd. I can confirm that the tests pass as-is on my machine. If you can track down why it's going lowercase for you it'd be useful. It might make sense to make the adapter always lowercase table table names or something anyway. Right now I think it just uses the model name itself as is.

spericas commented 10 years ago

Here is a more detailed description of the problem. I'm not sure if the issue is in jugglingdb or in the MySQL adapter, hopefully someone can shed some light on this. Steps:

(1) In schema.js: Schema.Text = function Text() {} ;

(2) In application: var Post = { ... content: { type: Schema.Text } ... }

(3) In mysql.js (around 248): this.client.escape(prop.type(val))

where prop.type === Schema.Text. Thus, undefined is passed to this.client.escape and the column is NULL in the DB. At first glance, it does not appear that other adapters (e.g., postgres) call prop.type(val) but, instead, they simply call val.toString(). I'm not sure what the "contract" is here. As a side note, Schema.prototype.define does use the identify function in place of the original definition of Schema.Text.

dgsan commented 10 years ago

(2) In application: var Post = { ... content: { type: Schema.Text } ... }

Is that shorthand for

db.define('Post', {
        content: {type: Schema.Text} 
});

pretty much?

spericas commented 10 years ago

Yes, sorry. Similar to the example here: http://jsdoc.info/1602/jugglingdb/

dgsan commented 10 years ago

For now try this:

var DummyModel = db.define('DummyModel', {string: {type: String, dataType: 'Text'}});
db.automigrate(function(){
    DummyModel.create({string: 'This is my string!'}, function(err, obj){
        console.log(obj);
        console.log('done');
    });
});

I'm pretty sure in the long run Schema.Text is supposed to die in favor of dataType and just have String and JSON, but check with 1602.

There should probably be a unit test.