maritz / nohm

node.js object relations mapper (orm) for redis
http://maritz.github.io/nohm/
MIT License
500 stars 64 forks source link

Creating a record if it does not exist - code structure help #72

Closed Morgon closed 11 years ago

Morgon commented 11 years ago

Hello!

Since I really like redis, I've been trying to use nohm to try and give some structure to the data I'm using (so I hopefully don't have to roll my own). I'm having trouble with code flow (and possibly syntax?) when it comes to looking up a model and creating one if the record doesn't exist.

Essentially, my application relies on third-party data. In this case, users in a particular chat room. When my application enters the room, it receives the list of users currently in the room. I need to make my own entries for these users (for additional application-specific information), so I'm iterating through this list of received users, and attempting to load my user record for them. What I also want to do is create a new record if I haven't seen them before.

I've tried so many different ways, but this is what I currently have:

//[snip]
    $.each(m.users, function (user) { // Underscore aliased to $ because I'm a web-based jQuery guy
        var curUser = Nohm.factory('User', user.userid, function(err) {
            if (err) {
                console.log('Creating new user');
                RoomUsers[user.userid] = this.createUser(user);
            } else {
                RoomUsers[user.userid] = curUser;
            }
        });
    });

   init(); // Which right now is just a call to console.log(RoomUsers);

Of course, this doesn't work at all, since the call to init() happens before the $.each has a chance to do anything.

If I move my init() call to inside the var curUser block, executed when an additional iterator i + 1 matches the length of the user list, then the control flow works as I logically expect -- but only if all of the users have been saved.

Testing with 1 account, if I delete the records for the user in redis, the control flow calls init() before it has had a chance to create the account. But subsequent runs seem okay.

I'm really not sure what else to do here. I really need to be able to have one code control block that checks for a record and creates one if it's not found, in one fell swoop with a proper return. Any help you can provide would be fantastic.

Also, including my User model, since that has the createUser function --

// file models/User.js
var Nohm = require('nohm').Nohm;

var user = Nohm.model('User', {
    properties: {
        userid: {
            type: 'string',
            unique: true,
            validations: [
                'notEmpty'
            ]
        },
        lastLogin: { type: 'timestamp', },
        lastLogout: { type: 'timestamp', },
        consecutiveDays: { type: 'integer', },
        sourceData: {
            type: 'json',
            validations: [
                'notEmpty'
            ]
        }
    },
    methods: {
        createUser: function (src) {
            var user = Nohm.factory('User');

            user.p({
                userid: src.userid,
                lastLogin: new Date().getTime(),
                consecutiveDays: 1,
                sourceData: src
            })

            user.save(function(err) {
                if (err) {
                    console.log('createUser error: ' + err);
                    console.log(user.errors);
                } else {
                    console.log('Returning new user');
                    return user;
                }
            });
        },
        updateLogin: function(ts) {
            if (ts - this.p('lastLogout') < 86400) {
                this.p('consecutiveDays', this.p('consecutiveDays' + 1));
            }
            this.p('lastLogin', ts);
        }
    },
    idGenerator: function (cb) {
        cb(this.p('userid'));
    }
});

module.exports = user;
maritz commented 11 years ago

This is mostly just a misunderstanding of asynchronous programming. Read more on that here or in some other resource. It's absolutely required to understand asynchronous programming for developing (good) things with node.js.

Basically it means that you don't use returns for async functions (like createUser). What you do is pass a callback (like you do with user.save and the Nohm.factory) and in that callback you then work with the results.

When you've understood these principles you can take a look at github.com/caolan/async, which helps keeping you out of callback hell (meaning nesting callbacks too many levels down).

Btw: node.js supports ECMAScript 5 features like array.forEeach, so you don't need underscore for that. More on that here.

var arr = ['asd', 'foo', 'bar'];
arr.forEach( function (value, index) {
  console.log(value);
});

// output: 
// asd
// foo
// bar

You should also check if the callback error on the factory is "not found" or something else. If it's "not found" then you create the user, otherwise it might be something bad like a db problem.

Morgon commented 11 years ago

Moritz, thank you for your response.

I know that forEach works, but I read somewhere that they determined underscore's .each was a blocking call (or at least more blocking than forEach), which I felt like I needed. :)

I did try async in my code above, as well.. I didn't post it because I was already taking up too much of your time. But I wasn't able to make anything useful from it. The problem I think is that when a user isn't found and needs to be created, it takes an extra tick before the data is available and in a place I can work with it.

Agreed on checking specifically 'not found'. I was being quick and dirty to see if I could use Nohm or have to create my own models (which won't go well, I'm sure), but I do recall seeing that error in a console.log(), and the code goes on to create the user entry correctly.

In general, would you say my syntax and logic for determining whether an account exists (var curUser[...]) and creating them if they don't exist (createUser()) are correct from a Nohm standpoint?

Thanks again for your response.

maritz commented 11 years ago

Neither Array.forEach nor _.each are blocking in the meaning that is usually attributed to that word in node.js.

Blocking basically means that some piece of code is halting all other execution while it waits for something. For example fs.readFileSync will completely halt the execution until it has completely read the file which usually means waiting for the disk IO operations.

forEach and _.each are not blocking in the sense that they aren't waiting for anything (except of course if you do a blocking call in your forEach, in which case you're blocking inside it).

The problem I think is that when a user isn't found and needs to be created, it takes an extra tick before the data is available and in a place I can work with it.

Well, an extra tick or thousands of ticks, depending on your database server and connection. Although it's rather safe to assume it will be a bit more than one tick (which could be over in way less than a microsecond).
That's why you use async code (aka. a callback). I don't like giving people code solutions when it's not something really arcane, but in this case it might help you understand:


RoomUsers = {};

var done = function () {
  if (Object.keys(RoomUsers).length === m.users.length) {
    // yep, all users loaded or created
    console.log(RoomUsers);
    init();
  } else {
    // boo, not done yet :(
  }
};

$.each(m.users, function (user) {
  // try to load the user
  var curUser = Nohm.factory('User', user.userid, function(err) {
    // we're in an async callback, hurrayy

    if (err && err === 'not found') { // ohhh
      console.log('Creating new user');
      this.createUser(user, function (err, nohm_user) {
        if (err) {
          // oh god, user creation failed. :(
          console.log(err, nohm_user.errors);
          process.exit(1); // or something less dramatic ;D
        } else {
          RoomUsers[user.userid] = nohm_user;
          done(); // we're done with this user
        }
      });

    } else if (err) {
      console.log('ERROROROROROROR', err);
    } else {
       RoomUsers[user.userid] = curUser; // or `this` instead of curUser
       done(); // we're done with this user
    }
  });
});

console.log('This will be written to the console output before any users are loaded.');

This requires changing the createUser function to accept a callback like this:


        createUser: function (src, callback) {
            var user = Nohm.factory('User');

            user.p({
                userid: src.userid,
                lastLogin: new Date().getTime(),
                consecutiveDays: 1,
                sourceData: src
            })

            user.save(callback);
        }

Now, once you understand why and how, you should look up github.com/caolan/async and for this case specifically async.forEach, which will take care of the done() stuff and error handling for you in a much nicer way.

If you want, you can always drop by in #node.js on freenode and ask around there, people are usually trying to help. (I'm mAritz there)

maritz commented 11 years ago

I'm assuming there are no problems left and will thus close this. You can of course re-open it if you have any issues with this.

Morgon commented 11 years ago

My apologies for not responding - with everything going on around the new year, I hadn't had a chance to post.

First, thank you again. That code snippet was MUCH more helpful than any tutorial or blog could have ever been, and helped me understand things a little more abstractly, as well.

The only trouble I had with your code is the createUser return referencing nohm_user - it was actually undefined for some reason. But I was able to substitute it with RoomUsers[user.userid] = this; to get the new user data.

Thanks again for your time and explanation. Perhaps adding a use case somewhere in the documentation might be helpful - I actually searched a number of your github projects looking for an example of where you might be doing a create on a failed lookup, but didn't see it.

maritz commented 11 years ago

The only trouble I had with your code is the createUser return referencing nohm_user - it was actually undefined for some reason. But I was able to substitute it with RoomUsers[user.userid] = this; to get the new user data.

Oh yeah, my bad. What I meant to do was actually getting back the nohm_user but I forgot that when I edited the createUser function. Assigning this works, using curUser would've worked as well. (and since for example jslint complains about using this in callbacks maybe the latter is the better solution.)

You're right, I've personally never done that, but with a solid understanding of how async js works it should be very doable. I don't think this specific use case needs to be mentioned. Plus I'm lazy when it comes to writing docs ;D