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\$

1 Answer 1

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\$
4
  • \$\begingroup\$ Point 2 is not correct \$\endgroup\$
    – megawac
    Feb 3, 2014 at 15:48
  • \$\begingroup\$ @megawac Why do you think so? \$\endgroup\$
    – konijn
    Feb 3, 2014 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, 2014 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, 2014 at 16:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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