3
\$\begingroup\$

I am writing a lightweight JSON API, and I come from a PHP background, so I have some questions/reviews about async node.js code.

This is my code so far:

main.js

var http = require('http');
var api = require('./api.js');

api.addRoute({
    'method': 'POST',
    'url': '/users',
    'input': {
        'fullName': {
            type: String,
            required: true,
            min: 2,
            max: 64
        },
        'email': {
            type: String,
            required: true,
            match: /^(.*)@(.*).(.*)$/,
            min: 2,
            max: 64
        },
        'password': {
            type: String,
            required: true,
            min: 2,
            max: 64
        }
    },
    'description': 'Create an user account.'
});

var server = http.createServer(api.handleRequest).listen(3000);

api.js

var api = exports;
var routes = [];

var postMaxSize = 1000;
var postTotalReceived = 0;

function getInput(req, res, callback){
    if(req.method === 'POST' || req.method === 'PUT'){
        var queryString = require('querystring');
        var inputData = '';

        req.on('data', function(data){
            if(data.length < postMaxSize && postTotalReceived < postMaxSize){
                inputData += data;
                postTotalReceived += data.length;
            } else {
                res.statusCode = 413;
                res.end('max-size');
            }
        });

        req.on('end', function(){
            callback(queryString.parse(inputData));
        });
    } else {
        callback(false);
    }
};

api.addRoute = function(route){
    routes.push(route);
}

function getRoute(req, res, callback){
    for(var i = 0; i < routes.length; i++){
        if(routes[i]['method'] === req.method){ // If the request method matches.
            if(typeof(routes[i]['url'] === 'string') && routes[i]['url'] === req.url){
                callback(routes[i]);
            }
        } else {
            var matches = req.url.match(routes[i]['url']);

            if(matches){
                req.urlParameters = [];

                for(var j = 1; j < matches.length; j++){ // Skip the first match, which is the url.
                    req.urlParameters.push(matches[j]);
                }

                callback(routes[i]);
            }
        }
    }
};

api.handleRequest = function(req, res){
    res.setHeader('Content-Type', 'application/json'); // Always return JSON since this is a rest JSON api.

    getInput(req, res, function(inputData){
        getRoute(req, res, function(route){
            if(route){

                // Do something.

                res.end();
            } else {
                res.statusCode = 404;
                res.end('not-found');
            }
        });
    });
};

I am just wondering, Node.js sells itself with "Non-blocking", but if on each request I have to do a regex to match the route for example:

/^\/users\/(.*)\//$ -> /users/mike

The regex has to be executed since Node.js is single threaded. How does it do Regex non blocking? The CPU has to execute the Regex, right?

About the code, my idea was:

  • I don't want to use any framework like express, since routing etc. can be done much more lightweight and I can customize everything, full control etc.

  • Each route has automatic input validation, which can be combined with auto generating documentation, for example if a user would request 'GET /' he would get a list of all the routes fully documented with required parameters, optional parameters, description, authentication requirements etc.

Please take a look at the api.handleRequest function. Am I correctly using callbacks at getInput and getRoute? Or should I only use callbacks at I/O, db calls etc?

The API I am about the build is basically a simple wrapper around Amazon Web Services, MongoDb and maybe later Redis or some other message queue.

\$\endgroup\$
2
\$\begingroup\$

From a once over:

  • Not sure why you are not parsing the querystring for GET requests? It seems wrong
  • if(data.length < postMaxSize && postTotalReceived < postMaxSize) could be if(data.length + postTotalReceived < postMaxSize)
  • Not sure what happens in your code after setting the 413, it seems you have some loose ends there. You could consider redefining callback to function(){}
  • I would not store the routes as an array, the lookup time is too slow in my mind, instead I would store the routes in an object like this:

    api.addRoute = function(route){
      routes[route.method][route.url] = route;
    }
    
  • I read your code several times, it seems as if even if the method of a route does not match you will still execute the route if the url matches through a regex. If so, then that is completely wrong.
\$\endgroup\$
  • \$\begingroup\$ Point 2 is not correct \$\endgroup\$ – megawac Feb 3 '14 at 15:48
  • \$\begingroup\$ @megawac Why do you think so? \$\endgroup\$ – konijn Feb 3 '14 at 15:55
  • \$\begingroup\$ I haven't read his code but the first case will be true for data.length = postTotalReceived = postMaxSize - 1 while yours will be false or am I missing something \$\endgroup\$ – megawac Feb 3 '14 at 15:59
  • 2
    \$\begingroup\$ You are correct, in my mind that is a bug in the OP code. The OP code will allow the size to be larger than postMaxSize. My suggestion does not allow it. \$\endgroup\$ – konijn Feb 3 '14 at 16:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.