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

updateAttributes on jugglingdb-redis breaks index for non-string fields #349

Open ragamufin opened 10 years ago

ragamufin commented 10 years ago

Thank you for all your hard work. I ran into an issue with the jugglingdb-redis adapter. I have several fields that are defined such as:

status: { type: Number, default: 1, index: true }

When I create an instance of the object with that field it saves fine and I can see in the store that there is an index created. However if I call updateAttributes on the same instance later the index actually disappears. I've debugged this and traced it back to the following code: redis.js Line291. It compares the value being saved with the previous value and if they are different it adds the new one and removes the old one. The problem is it uses a !== for comparison so even though you could be passing the same value it will be removed if it is a number, or date or anything but a string. All keys and values in prevData are strings because they come from the hash that was retrieved with a hgetall at line 260. So when you try to do a findOne later on status it fails because it was removed from the index.

Let me know if this makes sense. I'd love to have this working so that I can use numbers, booleans, etc. and still search on them even after updating the records.

1602 commented 10 years ago

Hey @ragamufin it all makes sense. Probably converting to string before comparison should solve the issue. But before you continue with this adapter I should say, that this code was not updated for a long time, since we switched to redis-hq adapter, which uses GET + JSON instead of HMGET to store object and has a lot of improvements on indexing and other useful features. So, if JSON + GET works for your application - it is better to switch. Thanks!

ragamufin commented 10 years ago

Thanks for responding. I didn't know about redis-hq. The jugglingdb page doesn't seem to mention it at all. I just took a look at it now and the setup at the top of the page is the same as that for jugglingdb-redis. Trying to install jugglingdb-redis-hq myself with npm gave me a python error... I'm a bit lost here but I'd like to get it to work. Can you let me know or point me to some installation instructions for setting up jugglingdb with the redis-hq adapter?

anatoliychakkaev commented 10 years ago

just as usual: npm install jugglingdb-redis-hq what error are you getting?

On Wed, Dec 18, 2013 at 2:46 AM, ragamufin notifications@github.com wrote:

Thanks for responding. I didn't know about redis-hq. The jugglingdb page doesn't seem to mention it at all. I just took a look at it now and the setup at the top of the page is the same as that for jugglingdb-redis. Trying to install jugglingdb-redis-hq myself with npm gave me a python error... I'm a bit lost here but I'd like to get it to work. Can you let me know or point me to some installation instructions for setting up jugglingdb with the redis-hq adapter?

— Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/issues/349#issuecomment-30812029 .

ragamufin commented 10 years ago

I'm getting an error during the install of jugglingdb-redis-hq when it attempts to install hiredis. The error is: gyp ERR! stack Error: C:\WINDOWS\Microsoft.NET\Framework\v4.0.30319\msbuild.exe failed with exit code: 1. I've found some leads as to how to fix it that I've tried but with no luck. I am trying to follow this one now: https://github.com/TooTallNate/node-gyp/wiki/Visual-Studio-2010-Setup in hopes of fixing it. I will let you know if I get it working tomorrow when I'm back to work. Thanks.

anatoliychakkaev commented 10 years ago

As far as I remember there's way around hiredis installation for windows, but I would recommend you to use linux virtual box. Alternatively you can just remove this dependency (it will use js parser for redis then, but it has some issues).

On Wed, Dec 18, 2013 at 9:37 AM, ragamufin notifications@github.com wrote:

I'm getting an error during the install of jugglingdb-redis-hq when it attempts to install hiredis. The error is: gyp ERR! stack Error: C:\WINDOWS\Microsoft.NET\Framework\v4.0.30319\msbuild.exe failed with exit code: 1. I've found some leads as to how to fix it that I've tried but with no luck. I am trying to follow this one now: https://github.com/TooTallNate/node-gyp/wiki/Visual-Studio-2010-Setup in hopes of fixing it. I will let you know if I get it working tomorrow when I'm back to work. Thanks.

— Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/issues/349#issuecomment-30828036 .

Thanks, Anatoliy Chakkaev

ragamufin commented 10 years ago

This is all new to me so I'm not sure of the pros and cons of using the js parser versus hiredis. I have Mac at home so I assume I'll have no problems there but my app will need to work on both, actually more so on Windows so I will have to figure this out. Could you tell me more about what hiredis does and what the repercussions of turning it off are?

anatoliychakkaev commented 10 years ago

It's for performance: https://github.com/mranney/node_redis#performance

In addition I can say that js parser has issues in production use.

On Wed, Dec 18, 2013 at 9:55 AM, ragamufin notifications@github.com wrote:

This is all new to me so I'm not sure of the pros and cons of using the js parser versus hiredis. I have Mac at home so I assume I'll have no problems there but my app will need to work on both, actually more so on Windows so I will have to figure this out. Could you tell me more about what hiredis does and what the repercussions of turning it off are?

— Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/issues/349#issuecomment-30829041 .

Thanks, Anatoliy Chakkaev

ragamufin commented 10 years ago

Hi again. I've been struggling with this the past few days and I've finally made some progress. I installed hiredis-node and after that I was able to successfully install jugglingdb-redis-hq. However on start up of my app I get an error. I'm defining my schema like this:

new Schema('redis-hq', { port: 6379 });

And the error I'm getting is:

...\node_modules\jugglingdb-redis-hq\node_modules\redis\index.js:504 throw callback_err; ^ Error: zrange lua script is not loaded at RedisHQ.all ...\node_modules\jugglingdb-redis-hq\lib\adapter.js:839:19)

I saw here: https://github.com/jugglingdb/redis-hq-adapter/issues/8 that this error is fixed so I'm assuming I still don't have my setup done correctly. Can you advise? Thanks again for all your help so far.

anatoliychakkaev commented 10 years ago

Could you please post whole example to run?

On Fri, Dec 20, 2013 at 7:44 AM, ragamufin notifications@github.com wrote:

Hi again. I've been struggling with this the past few days and I've finally made some progress. I installed hiredis-node and after that I was able to successfully install jugglingdb-redis-hq. However on start up of my app I get an error. I'm defining my schema like this:

new Schema(schemaOptions.adapter, { adapter :'redis-hq' ,adapterOptions :{ port: 6379 } });

And the error I'm getting is:

...\node_modules\jugglingdb-redis-hq\node_modules\redis\index.js:504 throw callback_err; ^ Error: zrange lua script is not loaded at RedisHQ.all ...\node_modules\jugglingdb-redis-hq\lib\adapter.js:839:19)

I saw here: jugglingdb/redis-hq-adapter#8https://github.com/jugglingdb/redis-hq-adapter/issues/8that this error is fixed so I'm assuming I still don't have my setup done correctly. Can you advise? Thanks again for all your help so far.

— Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/issues/349#issuecomment-30993819 .

Thanks, Anatoliy Chakkaev

ragamufin commented 10 years ago

Sure, no problem. I made a simple example to test. It works on my mac but not on my Windows machine.

var Schema  = require('jugglingdb').Schema

var curr_schema = new Schema('redis-hq', { port: 6379 });

console.log('Starting...');

var Page = curr_schema.define(
    'Page'
    ,{
        identifier: { type: String, length: 255, index: true }
    }
);

// create/update the schema
curr_schema.isActual(function(err, actual) {
    if (!actual) {
        curr_schema.autoupdate(function (err) {
            if(err) {
                console.log('Error during autoupdate');
                console.log(err);
            }
        });
    }
});

Page.all(function(err, pages) {
    if (err) {
        console.log('Error:', err);
    } else if(pages){
        console.log('pages', pages);
    } else {
        console.log('No pages found');
    }

    console.log('Ready');
});

The error is the same as above and is not triggered until I do Page.all

ragamufin commented 10 years ago

And the error is:

Error: zrange lua script is not loaded
    at RedisHQ.all (...\node_modules\jugglingdb-redis-hq\lib\adapter.js:839:19)
    at Function.all (...\node_modules\jugglingdb\lib\model.js:434:25)
    at Schema.<anonymous> (...\node_modules\jugglingdb\lib\model.js:297:16)
    at Schema.g (events.js:180:16)
    at Schema.EventEmitter.emit (events.js:92:17)
    at Schema.<anonymous> (...\node_modules\jugglingdb\lib\schema.js:121:14)
    at Command.callback (...\node_modules\jugglingdb-redis-hq\lib\index.js:84:17)
    at RedisClient.return_error (...\node_modules\jugglingdb-redis-hq\node_modules\redis\index.js:500:25)
    at ReplyParser.<anonymous> (...\node_modules\jugglingdb-redis-hq\node_modules\redis\index.js:260:14)
    at ReplyParser.EventEmitter.emit (events.js:95:17)
anatoliychakkaev commented 10 years ago

Redis is schemaless, so it is not necessary to call autoupdate. If code works on mac, that means something wrong with windows support in any part of stack. If you can debug it and fix problem, I guess windows users would be thankful.

On Sat, Dec 21, 2013 at 1:49 AM, ragamufin notifications@github.com wrote:

And the error is:

Error: zrange lua script is not loaded at RedisHQ.all (...\node_modules\jugglingdb-redis-hq\lib\adapter.js:839:19) at Function.all (...\node_modules\jugglingdb\lib\model.js:434:25) at Schema. (...\node_modules\jugglingdb\lib\model.js:297:16) at Schema.g (events.js:180:16) at Schema.EventEmitter.emit (events.js:92:17) at Schema. (...\node_modules\jugglingdb\lib\schema.js:121:14) at Command.callback (...\node_modules\jugglingdb-redis-hq\lib\index.js:84:17) at RedisClient.return_error (...\node_modules\jugglingdb-redis-hq\node_modules\redis\index.js:500:25) at ReplyParser. (...\node_modules\jugglingdb-redis-hq\node_modules\redis\index.js:260:14) at ReplyParser.EventEmitter.emit (events.js:95:17)

— Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/issues/349#issuecomment-31054011 .

Thanks, Anatoliy Chakkaev

ragamufin commented 10 years ago

Ok, I'm up and running. It took days of debugging and fighting wild animals in the jungle but I finally figured it out. The problem comes up at: https://github.com/jugglingdb/redis-hq-adapter/blob/master/lib/zrange-mget.lua#L8.

It seems Lua scripting (whatever that is) was added to redis in version 2.6.17 as stated in the notes at:http://redis.io/download. The general port of redis to windows is 2.4 which is what I had so that particular line was throwing an error. So an upgrade was in order. I followed the instructions here: http://stackoverflow.com/questions/6476945/how-do-i-run-redis-on-windows/20200022#20200022 and I was able to get things running and can now continue with app development.

Thanks again for your help on this.

ragamufin commented 10 years ago

Also just to add to this, the indexes behave properly under redis-hq and the original problem I was having is gone under this adapter. Thanks!