From 4c5bc13c7fe86e7e45638c5f26899e8891238747 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 24 Sep 2014 19:11:53 +0200 Subject: [PATCH 1/8] Check style fixes --- lib/cartodb/template_maps.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/template_maps.js b/lib/cartodb/template_maps.js index c5ca91d4..8e61a42c 100644 --- a/lib/cartodb/template_maps.js +++ b/lib/cartodb/template_maps.js @@ -38,7 +38,7 @@ function TemplateMaps(redis_pool, signed_maps, opts) { // // We have the following datastores: // - // 1. User teplates: set of per-user map templates + // 1. User templates: set of per-user map templates // NOTE: each template would have an associated auth // reference, see signed_maps.js @@ -150,7 +150,7 @@ o._checkInvalidTemplate = function(template) { if ( ! phold[k].hasOwnProperty('type') ) { return new Error("Missing type for placeholder '" + k + "'"); } - }; + } // Check certificate validity var cert = this.getTemplateCertificate(template); @@ -168,12 +168,11 @@ o._checkInvalidTemplate = function(template) { // SignedMaps.addCertificate or SignedMaps.authorizedByCert // o.getTemplateCertificate = function(template) { - var cert = { - version: '0.0.1', - template_id: template.name, - auth: template.auth + return { + version: '0.0.1', + template_id: template.name, + auth: template.auth }; - return cert; }; // Add a template @@ -519,9 +518,8 @@ o.getTemplate = function(owner, tpl_id, callback) { }, function parseTemplate(err, tpl_val) { if ( err ) throw err; - var tpl = JSON.parse(tpl_val); // Should we strip auth_id ? - return tpl; + return JSON.parse(tpl_val); }, function finish(err, tpl) { callback(err, tpl); From abf0fa1b32203569e7791879612ee33a3199a2c1 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 24 Sep 2014 19:12:43 +0200 Subject: [PATCH 2/8] Remove unused var --- lib/cartodb/template_maps.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/cartodb/template_maps.js b/lib/cartodb/template_maps.js index 8e61a42c..255f367a 100644 --- a/lib/cartodb/template_maps.js +++ b/lib/cartodb/template_maps.js @@ -2,9 +2,6 @@ var crypto = require('crypto'); var Step = require('step'); var _ = require('underscore'); -// Templates in this hash (keyed as @) -// are being worked on. -var user_template_locks = {}; // Class handling map templates // From 1c10b8193ba26d38d7247d2c59f7df4821f5d138 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 24 Sep 2014 19:17:51 +0200 Subject: [PATCH 3/8] Adds dot to compile templates --- lib/cartodb/template_maps.js | 33 ++++++++++++++------------------- package.json | 1 + 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/lib/cartodb/template_maps.js b/lib/cartodb/template_maps.js index 255f367a..63b16971 100644 --- a/lib/cartodb/template_maps.js +++ b/lib/cartodb/template_maps.js @@ -1,7 +1,7 @@ -var crypto = require('crypto'); -var Step = require('step'); -var _ = require('underscore'); - +var crypto = require('crypto'), + Step = require('step'), + _ = require('underscore'), + dot = require('dot'); // Class handling map templates // @@ -40,12 +40,11 @@ function TemplateMaps(redis_pool, signed_maps, opts) { // reference, see signed_maps.js // User templates (HASH:tpl_id->tpl_val) - this.key_usr_tpl = "map_tpl|<%= owner %>"; + this.key_usr_tpl = dot.template("map_tpl|{{=it.owner}}"); // User template locks (HASH:tpl_id->ctime) - this.key_usr_tpl_lck = "map_tpl|<%= owner %>|locks"; - -}; + this.key_usr_tpl_lck = dot.template("map_tpl|{{=it.owner}}|locks"); +} var o = TemplateMaps.prototype; @@ -94,13 +93,12 @@ o._redisCmd = function(redisFunc, redisArgs, callback) { // @param callback function(err, obtained) o._obtainTemplateLock = function(owner, tpl_id, callback) { - var usr_tpl_lck_key = _.template(this.key_usr_tpl_lck, {owner:owner}); var that = this; var gotLock = false; Step ( function obtainLock() { var ctime = Date.now(); - that._redisCmd('HSETNX', [usr_tpl_lck_key, tpl_id, ctime], this); + that._redisCmd('HSETNX', [that.key_usr_tpl_lck({owner:owner}), tpl_id, ctime], this); }, function checkLock(err, locked) { if ( err ) throw err; @@ -119,8 +117,7 @@ o._obtainTemplateLock = function(owner, tpl_id, callback) { // @param callback function(err, deleted) o._releaseTemplateLock = function(owner, tpl_id, callback) { - var usr_tpl_lck_key = _.template(this.key_usr_tpl_lck, {owner:owner}); - this._redisCmd('HDEL', [usr_tpl_lck_key, tpl_id], callback); + this._redisCmd('HDEL', [this.key_usr_tpl_lck({owner:owner}), tpl_id], callback); }; o._reValidIdentifier = /^[a-zA-Z][0-9a-zA-Z_]*$/; @@ -205,7 +202,7 @@ o.addTemplate = function(owner, template, callback) { // // - var usr_tpl_key = _.template(this.key_usr_tpl, {owner:owner}); + var usr_tpl_key = this.key_usr_tpl({owner:owner}); var gotLock = false; var that = this; var limit = that._userTemplateLimit(); @@ -289,7 +286,7 @@ o.addTemplate = function(owner, template, callback) { // @param callback function(err) // o.delTemplate = function(owner, tpl_id, callback) { - var usr_tpl_key = _.template(this.key_usr_tpl, {owner:owner}); + var usr_tpl_key = this.key_usr_tpl({owner:owner}); var gotLock = false; var that = this; Step( @@ -398,7 +395,7 @@ o.updTemplate = function(owner, tpl_id, template, callback) { return; } - var usr_tpl_key = _.template(this.key_usr_tpl, {owner:owner}); + var usr_tpl_key = this.key_usr_tpl({owner:owner}); var gotLock = false; var that = this; Step( @@ -492,8 +489,7 @@ o.updTemplate = function(owner, tpl_id, template, callback) { // Returns a list of template identifiers // o.listTemplates = function(owner, callback) { - var usr_tpl_key = _.template(this.key_usr_tpl, {owner:owner}); - this._redisCmd('HKEYS', [ usr_tpl_key ], callback); + this._redisCmd('HKEYS', [ this.key_usr_tpl({owner:owner}) ], callback); }; // Get a templates @@ -507,11 +503,10 @@ o.listTemplates = function(owner, callback) { // Return full template definition // o.getTemplate = function(owner, tpl_id, callback) { - var usr_tpl_key = _.template(this.key_usr_tpl, {owner:owner}); var that = this; Step( function getTemplate() { - that._redisCmd('HGET', [ usr_tpl_key, tpl_id ], this); + that._redisCmd('HGET', [ that.key_usr_tpl({owner:owner}), tpl_id ], this); }, function parseTemplate(err, tpl_val) { if ( err ) throw err; diff --git a/package.json b/package.json index 1f542da7..f126a5d8 100644 --- a/package.json +++ b/package.json @@ -24,6 +24,7 @@ "dependencies": { "node-varnish": "https://github.com/Vizzuality/node-varnish/tarball/0.3.0", "underscore" : "~1.6.0", + "dot": "~1.0.2", "windshaft": "https://github.com/CartoDB/Windshaft/tarball/0.27.0", "step": "~0.0.5", "request": "~2.9.203", From ac2a3243b5427e0d5aefcb49251ae73a8bd5790a Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 25 Sep 2014 12:04:52 +0200 Subject: [PATCH 4/8] Don't cache regexes and avoid the _re hack --- lib/cartodb/template_maps.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/cartodb/template_maps.js b/lib/cartodb/template_maps.js index 63b16971..6b76f9d9 100644 --- a/lib/cartodb/template_maps.js +++ b/lib/cartodb/template_maps.js @@ -537,14 +537,10 @@ o._reCSSColorVal = /^#[0-9a-fA-F]{3,6}$/; o._replaceVars = function(str, params) { //return _.template(str, params); // lazy way, possibly dangerous // Construct regular expressions for each param - if ( ! params._re ) { - params._re = {}; - for (var k in params) { - params._re[k] = RegExp("<%=\\s*" + k + "\\s*%>", "g"); - } - } - for (var k in params) str = str.replace(params._re[k], params[k]); - return str; + Object.keys(params).forEach(function(k) { + str = str.replace(new RegExp("<%=\\s*" + k + "\\s*%>", "g"), params[k]); + }); + return str; }; o.instance = function(template, params) { var all_params = {}; From fb8fd5121ebf1932abb628ce39a893ce73aaf469 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 25 Sep 2014 12:16:34 +0200 Subject: [PATCH 5/8] Do not expose internal implementation --- lib/cartodb/template_maps.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/cartodb/template_maps.js b/lib/cartodb/template_maps.js index 6b76f9d9..04cac79e 100644 --- a/lib/cartodb/template_maps.js +++ b/lib/cartodb/template_maps.js @@ -531,10 +531,11 @@ o.getTemplate = function(owner, tpl_id, callback) { // // @throws Error on malformed template or parameter // -o._reNumber = /^([-+]?[\d\.]?\d+([eE][+-]?\d+)?)$/; -o._reCSSColorName = /^[a-zA-Z]+$/; -o._reCSSColorVal = /^#[0-9a-fA-F]{3,6}$/; -o._replaceVars = function(str, params) { +var _reNumber = /^([-+]?[\d\.]?\d+([eE][+-]?\d+)?)$/, + _reCSSColorName = /^[a-zA-Z]+$/, + _reCSSColorVal = /^#[0-9a-fA-F]{3,6}$/; + +_replaceVars = function(str, params) { //return _.template(str, params); // lazy way, possibly dangerous // Construct regular expressions for each param Object.keys(params).forEach(function(k) { @@ -559,7 +560,7 @@ o.instance = function(template, params) { } else if ( type === 'number' ) { // check it's a number - if ( typeof(val) !== 'number' && ! val.match(this._reNumber) ) { + if ( typeof(val) !== 'number' && ! val.match(_reNumber) ) { throw new Error("Invalid number value for template parameter '" + k + "': " + val); } @@ -567,7 +568,7 @@ o.instance = function(template, params) { else if ( type === 'css_color' ) { // check it only contains letters or // starts with # and only contains hexdigits - if ( ! val.match(this._reCSSColorName) && ! val.match(this._reCSSColorVal) ) { + if ( ! val.match(_reCSSColorName) && ! val.match(_reCSSColorVal) ) { throw new Error("Invalid css_color value for template parameter '" + k + "': " + val); } @@ -583,8 +584,8 @@ o.instance = function(template, params) { var layergroup = JSON.parse(JSON.stringify(template.layergroup)); for (var i=0; i Date: Thu, 25 Sep 2014 12:17:32 +0200 Subject: [PATCH 6/8] Use object.keys to iterate over objects --- lib/cartodb/template_maps.js | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/lib/cartodb/template_maps.js b/lib/cartodb/template_maps.js index 04cac79e..15565e2b 100644 --- a/lib/cartodb/template_maps.js +++ b/lib/cartodb/template_maps.js @@ -120,7 +120,7 @@ o._releaseTemplateLock = function(owner, tpl_id, callback) { this._redisCmd('HDEL', [this.key_usr_tpl_lck({owner:owner}), tpl_id], callback); }; -o._reValidIdentifier = /^[a-zA-Z][0-9a-zA-Z_]*$/; +var _reValidIdentifier = /^[a-zA-Z][0-9a-zA-Z_]*$/; o._checkInvalidTemplate = function(template) { if ( template.version != '0.0.1' ) { return new Error("Unsupported template version " + template.version); @@ -129,21 +129,25 @@ o._checkInvalidTemplate = function(template) { if ( ! tplname ) { return new Error("Missing template name"); } - if ( ! tplname.match(this._reValidIdentifier) ) { + if ( ! tplname.match(_reValidIdentifier) ) { return new Error("Invalid characters in template name '" + tplname + "'"); } - var phold = template.placeholders; - for (var k in phold) { - if ( ! k.match(this._reValidIdentifier) ) { - return new Error("Invalid characters in placeholder name '" + k + "'"); - } - if ( ! phold[k].hasOwnProperty('default') ) { - return new Error("Missing default for placeholder '" + k + "'"); - } - if ( ! phold[k].hasOwnProperty('type') ) { - return new Error("Missing type for placeholder '" + k + "'"); - } + var placeholders = template.placeholders || {}; + + var placeholderKeys = Object.keys(placeholders); + for (var i = 0, len = placeholderKeys.length; i < len; i++) { + var placeholderKey = placeholderKeys[i]; + + if (!placeholderKey.match(_reValidIdentifier)) { + return new Error("Invalid characters in placeholder name '" + placeholderKey + "'"); + } + if ( ! placeholders[placeholderKey].hasOwnProperty('default') ) { + return new Error("Missing default for placeholder '" + placeholderKey + "'"); + } + if ( ! placeholders[placeholderKey].hasOwnProperty('type') ) { + return new Error("Missing type for placeholder '" + placeholderKey + "'"); + } } // Check certificate validity @@ -545,8 +549,8 @@ _replaceVars = function(str, params) { }; o.instance = function(template, params) { var all_params = {}; - var phold = template.placeholders; - for (var k in phold) { + var phold = template.placeholders || {}; + Object.keys(phold).forEach(function(k) { var val = params.hasOwnProperty(k) ? params[k] : phold[k].default; var type = phold[k].type; // properly escape @@ -578,7 +582,7 @@ o.instance = function(template, params) { throw new Error("Invalid placeholder type '" + type + "'"); } all_params[k] = val; - } + }); // NOTE: we're deep-cloning the layergroup here var layergroup = JSON.parse(JSON.stringify(template.layergroup)); From b4bee864d2b5279839ce171487af06bb5830812e Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 25 Sep 2014 19:00:35 +0200 Subject: [PATCH 7/8] Lock now considers the creation time and compares against a ttl so a lock is not keep forever in case of failure. Pending: lazy removal of expired locks. --- lib/cartodb/template_maps.js | 42 +++++------ test/unit/cartodb/template_maps.test.js | 94 ++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 21 deletions(-) diff --git a/lib/cartodb/template_maps.js b/lib/cartodb/template_maps.js index 15565e2b..9b4cfba4 100644 --- a/lib/cartodb/template_maps.js +++ b/lib/cartodb/template_maps.js @@ -44,6 +44,8 @@ function TemplateMaps(redis_pool, signed_maps, opts) { // User template locks (HASH:tpl_id->ctime) this.key_usr_tpl_lck = dot.template("map_tpl|{{=it.owner}}|locks"); + + this.lock_ttl = this.opts['lock_ttl'] || 5000; } var o = TemplateMaps.prototype; @@ -93,26 +95,26 @@ o._redisCmd = function(redisFunc, redisArgs, callback) { // @param callback function(err, obtained) o._obtainTemplateLock = function(owner, tpl_id, callback) { - var that = this; - var gotLock = false; - Step ( - function obtainLock() { - var ctime = Date.now(); - that._redisCmd('HSETNX', [that.key_usr_tpl_lck({owner:owner}), tpl_id, ctime], this); - }, - function checkLock(err, locked) { - if ( err ) throw err; - if ( ! locked ) { - // Already locked - // TODO: unlock if expired ? - throw new Error("Template '" + tpl_id + "' of user '" + owner + "' is locked"); - } - return gotLock = true; - }, - function finish(err) { - callback(err, gotLock); - } - ); + var that = this, + lockKey = this.key_usr_tpl_lck({owner:owner}); + Step ( + function obtainLock() { + that._redisCmd('HGET', [lockKey, tpl_id], this); + }, + function checkLock(err, lockTime) { + if (err) { throw err; } + + var _newLockTime = Date.now(); + if (!lockTime || ((_newLockTime - lockTime) > that.lock_ttl)) { + that._redisCmd('HSET', [lockKey, tpl_id, _newLockTime], this); + } else { + throw new Error("Template '" + tpl_id + "' of user '" + owner + "' is locked"); + } + }, + function finish(err, hsetValue) { + callback(err, !!hsetValue); + } + ); }; // @param callback function(err, deleted) diff --git a/test/unit/cartodb/template_maps.test.js b/test/unit/cartodb/template_maps.test.js index 1314dbe6..928d68fe 100644 --- a/test/unit/cartodb/template_maps.test.js +++ b/test/unit/cartodb/template_maps.test.js @@ -12,6 +12,14 @@ suite('template_maps', function() { // configure redis pool instance to use in tests var redis_pool = RedisPool(global.environment.redis); var signed_maps = new SignedMaps(redis_pool); + + var validTemplate = { + version:'0.0.1', + name: 'first', + auth: {}, + layergroup: {} + }; + var owner = 'me'; test('does not accept template with unsupported version', function(done) { var tmap = new TemplateMaps(redis_pool, signed_maps); @@ -502,5 +510,89 @@ suite('template_maps', function() { } ); }); - + + var redisCmdFunc = TemplateMaps.prototype._redisCmd; + + function runWithRedisStubbed(stubbedCommands, func) { + TemplateMaps.prototype._redisCmd = function(redisFunc, redisArgs, callback) { + redisFunc = redisFunc.toLowerCase(); + if (stubbedCommands.hasOwnProperty(redisFunc)) { + callback(null, stubbedCommands[redisFunc]); + } else { + throw 'Unknown command'; + } + }; + + func(); + + TemplateMaps.prototype._redisCmd = redisCmdFunc; + } + + test('_obtainTemplateLock with no previous value, happy case', function(done) { + runWithRedisStubbed({hget: null, hset: 1}, function() { + var templateMaps = new TemplateMaps(redis_pool, signed_maps); + + templateMaps._obtainTemplateLock(owner, validTemplate.name, function(err, gotLock) { + assert.ok(!err); + assert.ok(gotLock); + done(); + }); + }); + }); + + test('_obtainTemplateLock no lock for non expired ttl, simulates obtaining two locks at same time', function(done) { + runWithRedisStubbed({hget: Date.now()}, function() { + var templateMaps = new TemplateMaps(redis_pool, signed_maps); + + templateMaps._obtainTemplateLock(owner, validTemplate.name, function(err, gotLock) { + assert.ok(!!err); + assert.equal(gotLock, false); + done(); + }); + }); + }); + + + test('_obtainTemplateLock no lock for non expired ttl, last millisecond of valid ttl', function(done) { + var nowValue = Date.now(), + nowFunc = Date.now; + Date.now = function() { + return nowValue; + }; + var lockTtl = 1000; + runWithRedisStubbed({hget: Date.now() - lockTtl, hset: true}, function() { + var templateMaps = new TemplateMaps(redis_pool, signed_maps, {lock_ttl: lockTtl}); + + templateMaps._obtainTemplateLock(owner, validTemplate.name, function(err, gotLock) { + assert.ok(!!err); + assert.equal(gotLock, false); + + Date.now = nowFunc; + + done(); + }); + }); + }); + + test('_obtainTemplateLock gets lock for expired ttl, first millisecond of invalid ttl', function(done) { + var nowValue = Date.now(), + nowFunc = Date.now; + Date.now = function() { + return nowValue; + }; + var lockTtl = 1000; + runWithRedisStubbed({hget: Date.now() - lockTtl - 1, hset: true}, function() { + var templateMaps = new TemplateMaps(redis_pool, signed_maps, {lock_ttl: lockTtl}); + + templateMaps._obtainTemplateLock(owner, validTemplate.name, function(err, gotLock) { + assert.ok(!err); + assert.ok(gotLock); + + Date.now = nowFunc; + + done(); + }); + }); + }); + }); From e8e03585ffa196f30ceb99d774b27daddae8819b Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 25 Sep 2014 19:17:02 +0200 Subject: [PATCH 8/8] Adds dot.js dependency --- npm-shrinkwrap.json | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index a21dfbb5..2f68cd70 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -9,6 +9,9 @@ "underscore": { "version": "1.6.0" }, + "dot": { + "version": "1.0.2" + }, "windshaft": { "version": "0.27.0", "from": "https://github.com/CartoDB/Windshaft/tarball/0.27.0", @@ -1230,12 +1233,7 @@ }, "cartodb-redis": { "version": "0.11.0", - "from": "https://github.com/CartoDB/node-cartodb-redis/tarball/0.11.0", - "dependencies": { - "dot": { - "version": "1.0.2" - } - } + "from": "https://github.com/CartoDB/node-cartodb-redis/tarball/0.11.0" }, "cartodb-psql": { "version": "0.4.0",