From 98d61708706f40f9b0e90f8b54c841b0744b8811 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 8 Sep 2015 16:44:44 +0200 Subject: [PATCH 01/14] Adds notes about contributing --- CONTRIBUTING.md | 11 +++++++++++ LICENSE | 2 +- README.md | 6 ++++++ 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..b46d5d4e --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,11 @@ +Contributing +--- + +The issue tracker is at [github.com/CartoDB/Windshaft-cartodb](https://github.com/CartoDB/Windshaft-cartodb). + +We love pull requests from everyone, see [Contributing to Open Source on GitHub](https://guides.github.com/activities/contributing-to-open-source/#contributing). + + +## Submitting Contributions + +* You will need to sign a Contributor License Agreement (CLA) before making a submission. [Learn more here](https://cartodb.com/contributing). diff --git a/LICENSE b/LICENSE index 91eaff9a..d939626d 100644 --- a/LICENSE +++ b/LICENSE @@ -1,4 +1,4 @@ -Copyright (c) 2014, Vizzuality +Copyright (c) 2015, CartoDB All rights reserved. Redistribution and use in source and binary forms, with or without diff --git a/README.md b/README.md index dc28f532..b724e1e9 100644 --- a/README.md +++ b/README.md @@ -95,3 +95,9 @@ Examples -------- [CartoDB's Map Gallery](http://cartodb.com/gallery/) showcases several examples of visualisations built on top of this. + + +Contributing +--- + +See [CONTRIBUTING.md](CONTRIBUTING.md). From 469b602484a9e6b2c29824e563ddd0a6563c7bd4 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 14 Sep 2015 18:46:22 +0200 Subject: [PATCH 02/14] Attributes backend using mapconfig provider to access attributes --- lib/cartodb/controllers/layergroup.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index e0aa6b44..845f7eaf 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -59,7 +59,10 @@ LayergroupController.prototype.attributes = function(req, res) { assert.ifError(err); - self.attributesBackend.getFeatureAttributes(req.params, false, this); + var mapConfigProvider = new MapStoreMapConfigProvider( + self.mapStore, req.context.user, self.userLimitsApi, req.params + ); + self.attributesBackend.getFeatureAttributes(mapConfigProvider, req.params, false, this); }, function finish(err, tile, stats) { req.profiler.add(stats || {}); From 2d3088ba27715f599b6b07de8811508a060a6476 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 14 Sep 2015 18:47:01 +0200 Subject: [PATCH 03/14] Port everything related to stats from windshaft --- lib/cartodb/server.js | 10 ++- lib/cartodb/stats/client.js | 74 +++++++++++++++++++++ lib/cartodb/stats/profiler_proxy.js | 53 +++++++++++++++ lib/cartodb/stats/reporter/renderer.js | 70 +++++++++++++++++++ npm-shrinkwrap.json | 60 ++++++++--------- package.json | 3 + test/unit/cartodb/ported/tile_stats.test.js | 2 +- 7 files changed, 239 insertions(+), 33 deletions(-) create mode 100644 lib/cartodb/stats/client.js create mode 100644 lib/cartodb/stats/profiler_proxy.js create mode 100644 lib/cartodb/stats/reporter/renderer.js diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 178f3014..ad4ce2f9 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -13,6 +13,10 @@ var NamedMapsCacheEntry = require('./cache/model/named_maps_entry'); var VarnishHttpCacheBackend = require('./cache/backend/varnish_http'); var FastlyCacheBackend = require('./cache/backend/fastly'); +var StatsClient = require('./stats/client'); +var Profiler = require('./stats/profiler_proxy'); +var RendererStatsReporter = require('./stats/reporter/renderer'); + var windshaft = require('windshaft'); var mapnik = windshaft.mapnik; @@ -46,7 +50,7 @@ var timeoutErrorTile = require('fs').readFileSync(timeoutErrorTilePath, {encodin module.exports = function(serverOptions) { // Make stats client globally accessible - global.statsClient = windshaft.stats.Client.getInstance(serverOptions.statsd); + global.statsClient = StatsClient.getInstance(serverOptions.statsd); var redisPool = new RedisPool(_.defaults(global.environment.redis, { name: 'windshaft:server', @@ -156,6 +160,8 @@ module.exports = function(serverOptions) { statsInterval: 60000 // reports stats every milliseconds defined here }); var rendererCache = new windshaft.cache.RendererCache(rendererFactory, rendererCacheOpts); + var rendererStatsReporter = new RendererStatsReporter(rendererCache, rendererCacheOpts.statsInterval); + rendererStatsReporter.start(); var attributesBackend = new windshaft.backend.Attributes(mapStore); var previewBackend = new windshaft.backend.Preview(rendererCache); @@ -472,7 +478,7 @@ function bootstrap(opts) { app.use(function bootstrap$prepareRequestResponse(req, res, next) { req.context = req.context || {}; - req.profiler = new windshaft.stats.Profiler({ + req.profiler = new Profiler({ statsd_client: global.statsClient, profile: opts.useProfiler }); diff --git a/lib/cartodb/stats/client.js b/lib/cartodb/stats/client.js new file mode 100644 index 00000000..b8b09a89 --- /dev/null +++ b/lib/cartodb/stats/client.js @@ -0,0 +1,74 @@ +var _ = require('underscore'); +var debug = require('debug')('windshaft:stats_client'); +var StatsD = require('node-statsd').StatsD; + +module.exports = { + /** + * Returns an StatsD instance or an stub object that replicates the StatsD public interface so there is no need to + * keep checking if the stats_client is instantiated or not. + * + * The first call to this method implies all future calls will use the config specified in the very first call. + * + * TODO: It's far from ideal to use make this a singleton, improvement desired. + * We proceed this way to be able to use StatsD from several places sharing one single StatsD instance. + * + * @param config Configuration for StatsD, if undefined it will return an stub + * @returns {StatsD|Object} + */ + getInstance: function(config) { + + if (!this.instance) { + + var instance; + + if (config) { + instance = new StatsD(config); + instance.last_error = { msg: '', count: 0 }; + instance.socket.on('error', function (err) { + var last_err = instance.last_error; + var last_msg = last_err.msg; + var this_msg = '' + err; + if (this_msg !== last_msg) { + debug("statsd client socket error: " + err); + instance.last_error.count = 1; + instance.last_error.msg = this_msg; + } else { + ++last_err.count; + if (!last_err.interval) { + instance.last_error.interval = setInterval(function () { + var count = instance.last_error.count; + if (count > 1) { + debug("last statsd client socket error repeated " + count + " times"); + instance.last_error.count = 1; + //console.log("Clearing interval"); + clearInterval(instance.last_error.interval); + instance.last_error.interval = null; + } + }, 1000); + } + } + }); + } else { + var stubFunc = function (stat, value, sampleRate, callback) { + if (_.isFunction(callback)) { + callback(null, 0); + } + }; + instance = { + timing: stubFunc, + increment: stubFunc, + decrement: stubFunc, + gauge: stubFunc, + unique: stubFunc, + set: stubFunc, + sendAll: stubFunc, + send: stubFunc + }; + } + + this.instance = instance; + } + + return this.instance; + } +}; \ No newline at end of file diff --git a/lib/cartodb/stats/profiler_proxy.js b/lib/cartodb/stats/profiler_proxy.js new file mode 100644 index 00000000..1e1f91e7 --- /dev/null +++ b/lib/cartodb/stats/profiler_proxy.js @@ -0,0 +1,53 @@ +var Profiler = require('step-profiler'); + +/** + * Proxy to encapsulate node-step-profiler module so there is no need to check if there is an instance + */ +function ProfilerProxy(opts) { + this.profile = !!opts.profile; + + this.profiler = null; + if (!!opts.profile) { + this.profiler = new Profiler({statsd_client: opts.statsd_client}); + } +} + +ProfilerProxy.prototype.done = function(what) { + if (this.profile) { + this.profiler.done(what); + } +}; + +ProfilerProxy.prototype.end = function() { + if (this.profile) { + this.profiler.end(); + } +}; + +ProfilerProxy.prototype.start = function(what) { + if (this.profile) { + this.profiler.start(what); + } +}; + +ProfilerProxy.prototype.add = function(what) { + if (this.profile) { + this.profiler.add(what || {}); + } +}; + +ProfilerProxy.prototype.sendStats = function() { + if (this.profile) { + this.profiler.sendStats(); + } +}; + +ProfilerProxy.prototype.toString = function() { + return this.profile ? this.profiler.toString() : ""; +}; + +ProfilerProxy.prototype.toJSONString = function() { + return this.profile ? this.profiler.toJSONString() : "{}"; +}; + +module.exports = ProfilerProxy; diff --git a/lib/cartodb/stats/reporter/renderer.js b/lib/cartodb/stats/reporter/renderer.js new file mode 100644 index 00000000..5c01d960 --- /dev/null +++ b/lib/cartodb/stats/reporter/renderer.js @@ -0,0 +1,70 @@ +// - Reports stats about: +// * Total number of renderers +// * For mapnik renderers: +// - the mapnik-pool status: count, unused and waiting +// - the internally cached objects: png and grid + +var _ = require('underscore'); + +function RendererStatsReporter(rendererCache, statsInterval) { + this.rendererCache = rendererCache; + this.statsInterval = statsInterval || 6e4; + this.renderersStatsIntervalId = null; +} + +module.exports = RendererStatsReporter; + +RendererStatsReporter.prototype.start = function() { + var self = this; + this.renderersStatsIntervalId = setInterval(function() { + var rendererCacheEntries = self.rendererCache.renderers; + + if (!rendererCacheEntries) { + return null; + } + + global.statsClient.gauge('windshaft.rendercache.count', _.keys(rendererCacheEntries).length); + + var renderersStats = _.reduce(rendererCacheEntries, function(_rendererStats, cacheEntry) { + var stats = cacheEntry.renderer && cacheEntry.renderer.getStats && cacheEntry.renderer.getStats(); + if (!stats) { + return _rendererStats; + } + + _rendererStats.pool.count += stats.pool.count; + _rendererStats.pool.unused += stats.pool.unused; + _rendererStats.pool.waiting += stats.pool.waiting; + + _rendererStats.cache.grid += stats.cache.grid; + _rendererStats.cache.png += stats.cache.png; + + return _rendererStats; + }, + { + pool: { + count: 0, + unused: 0, + waiting: 0 + }, + cache: { + png: 0, + grid: 0 + } + } + ); + + global.statsClient.gauge('windshaft.mapnik-cache.png', renderersStats.cache.png); + global.statsClient.gauge('windshaft.mapnik-cache.grid', renderersStats.cache.grid); + + global.statsClient.gauge('windshaft.mapnik-pool.count', renderersStats.pool.count); + global.statsClient.gauge('windshaft.mapnik-pool.unused', renderersStats.pool.unused); + global.statsClient.gauge('windshaft.mapnik-pool.waiting', renderersStats.pool.waiting); + }, this.statsInterval); + +}; + + +RendererStatsReporter.prototype.stop = function() { + clearInterval(this.renderersStatsIntervalId); + this.renderersStatsIntervalId = null; +}; diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index dc3d8f5c..27b21ac8 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -66,6 +66,18 @@ } } }, + "debug": { + "version": "2.2.0", + "from": "debug@~2.2.0", + "resolved": "https://registry.npmjs.org/debug/-/debug-2.2.0.tgz", + "dependencies": { + "ms": { + "version": "0.7.1", + "from": "ms@0.7.1", + "resolved": "https://registry.npmjs.org/ms/-/ms-0.7.1.tgz" + } + } + }, "dot": { "version": "1.0.3", "from": "dot@~1.0.2", @@ -140,9 +152,9 @@ "resolved": "https://registry.npmjs.org/isarray/-/isarray-0.0.1.tgz" }, "process-nextick-args": { - "version": "1.0.2", + "version": "1.0.3", "from": "process-nextick-args@~1.0.0", - "resolved": "https://registry.npmjs.org/process-nextick-args/-/process-nextick-args-1.0.2.tgz" + "resolved": "https://registry.npmjs.org/process-nextick-args/-/process-nextick-args-1.0.3.tgz" }, "string_decoder": { "version": "0.10.31", @@ -255,9 +267,9 @@ "resolved": "https://registry.npmjs.org/hawk/-/hawk-3.1.0.tgz", "dependencies": { "hoek": { - "version": "2.14.0", + "version": "2.15.0", "from": "hoek@2.x.x", - "resolved": "https://registry.npmjs.org/hoek/-/hoek-2.14.0.tgz" + "resolved": "https://registry.npmjs.org/hoek/-/hoek-2.15.0.tgz" }, "boom": { "version": "2.8.0", @@ -265,9 +277,9 @@ "resolved": "https://registry.npmjs.org/boom/-/boom-2.8.0.tgz" }, "cryptiles": { - "version": "2.0.4", + "version": "2.0.5", "from": "cryptiles@2.x.x", - "resolved": "https://registry.npmjs.org/cryptiles/-/cryptiles-2.0.4.tgz" + "resolved": "https://registry.npmjs.org/cryptiles/-/cryptiles-2.0.5.tgz" }, "sntp": { "version": "1.0.9", @@ -309,9 +321,9 @@ "resolved": "https://registry.npmjs.org/har-validator/-/har-validator-1.8.0.tgz", "dependencies": { "bluebird": { - "version": "2.9.34", + "version": "2.10.0", "from": "bluebird@^2.9.30", - "resolved": "https://registry.npmjs.org/bluebird/-/bluebird-2.9.34.tgz" + "resolved": "https://registry.npmjs.org/bluebird/-/bluebird-2.10.0.tgz" }, "chalk": { "version": "1.1.1", @@ -470,6 +482,11 @@ "from": "lzma@~1.3.7", "resolved": "https://registry.npmjs.org/lzma/-/lzma-1.3.7.tgz" }, + "node-statsd": { + "version": "0.0.7", + "from": "node-statsd@~0.0.7", + "resolved": "https://registry.npmjs.org/node-statsd/-/node-statsd-0.0.7.tgz" + }, "queue-async": { "version": "1.0.7", "from": "queue-async@~1.0.7", @@ -518,6 +535,11 @@ "from": "step@~0.0.5", "resolved": "https://registry.npmjs.org/step/-/step-0.0.6.tgz" }, + "step-profiler": { + "version": "0.2.1", + "from": "step-profiler@~0.2.1", + "resolved": "https://registry.npmjs.org/step-profiler/-/step-profiler-0.2.1.tgz" + }, "underscore": { "version": "1.6.0", "from": "underscore@~1.6.0", @@ -2287,18 +2309,6 @@ } } }, - "debug": { - "version": "2.2.0", - "from": "debug@~2.2.0", - "resolved": "https://registry.npmjs.org/debug/-/debug-2.2.0.tgz", - "dependencies": { - "ms": { - "version": "0.7.1", - "from": "ms@0.7.1", - "resolved": "https://registry.npmjs.org/ms/-/ms-0.7.1.tgz" - } - } - }, "tilelive": { "version": "4.5.3", "from": "tilelive@~4.5.3", @@ -2766,11 +2776,6 @@ } } }, - "step-profiler": { - "version": "0.2.1", - "from": "step-profiler@~0.2.1", - "resolved": "https://registry.npmjs.org/step-profiler/-/step-profiler-0.2.1.tgz" - }, "torque.js": { "version": "2.11.0", "from": "torque.js@~2.11.0", @@ -2959,11 +2964,6 @@ "version": "1.0.2", "from": "sphericalmercator@1.0.2", "resolved": "https://registry.npmjs.org/sphericalmercator/-/sphericalmercator-1.0.2.tgz" - }, - "node-statsd": { - "version": "0.0.7", - "from": "node-statsd@~0.0.7", - "resolved": "https://registry.npmjs.org/node-statsd/-/node-statsd-0.0.7.tgz" } } } diff --git a/package.json b/package.json index b476eda4..6a0fc47c 100644 --- a/package.json +++ b/package.json @@ -23,6 +23,9 @@ ], "dependencies": { "express": "~2.5.11", + "debug": "~2.2.0", + "step-profiler": "~0.2.1", + "node-statsd": "~0.0.7", "underscore" : "~1.6.0", "dot": "~1.0.2", "windshaft": "https://github.com/CartoDB/Windshaft/tarball/backend-foundations", diff --git a/test/unit/cartodb/ported/tile_stats.test.js b/test/unit/cartodb/ported/tile_stats.test.js index 0f570829..433daddf 100644 --- a/test/unit/cartodb/ported/tile_stats.test.js +++ b/test/unit/cartodb/ported/tile_stats.test.js @@ -3,7 +3,7 @@ require('../../../support/test_helper.js'); var assert = require('assert'); var cartodbServer = require('../../../../lib/cartodb/server'); var serverOptions = require('../../../../lib/cartodb/server_options'); -var StatsClient = require('windshaft').stats.Client; +var StatsClient = require('../../../../lib/cartodb/stats/client'); var LayergroupController = require('../../../../lib/cartodb/controllers/layergroup'); From 3f7202d89c698878bfd39a441247ce09d5d338bc Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 14 Sep 2015 19:07:53 +0200 Subject: [PATCH 04/14] Port tests for stats --- Makefile | 2 + test/unit/cartodb/ported/profiler.test.js | 17 ++++++++ test/unit/cartodb/ported/stats_client.test.js | 40 +++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 test/unit/cartodb/ported/profiler.test.js create mode 100644 test/unit/cartodb/ported/stats_client.test.js diff --git a/Makefile b/Makefile index 3f6ebf43..5ca7973a 100644 --- a/Makefile +++ b/Makefile @@ -22,6 +22,7 @@ test: config/environments/test.js @echo "***tests***" @$(SHELL) ./run_tests.sh ${RUNTESTFLAGS} \ test/unit/cartodb/*.js \ + test/unit/cartodb/ported/*.js \ test/unit/cartodb/cache/model/*.js \ test/integration/*.js \ test/acceptance/*.js \ @@ -33,6 +34,7 @@ test-unit: config/environments/test.js @echo "***tests***" @$(SHELL) ./run_tests.sh ${RUNTESTFLAGS} \ test/unit/cartodb/*.js \ + test/unit/cartodb/ported/*.js \ test/unit/cartodb/cache/model/*.js test-integration: config/environments/test.js diff --git a/test/unit/cartodb/ported/profiler.test.js b/test/unit/cartodb/ported/profiler.test.js new file mode 100644 index 00000000..29d9b485 --- /dev/null +++ b/test/unit/cartodb/ported/profiler.test.js @@ -0,0 +1,17 @@ +require('../../../support/test_helper'); + +var assert = require('assert'); +var ProfilerProxy = require('../../../../lib/cartodb/stats/profiler_proxy'); + +describe('profiler', function() { + + it('Profiler is null in ProfilerProxy when profiling is not enabled', function() { + var profilerProxy = new ProfilerProxy({profile: false}); + assert.equal(profilerProxy.profiler, null); + }); + + it('Profiler is NOT null in ProfilerProxy when profiling is enabled', function() { + var profilerProxy = new ProfilerProxy({profile: true}); + assert.notEqual(profilerProxy.profiler, null); + }); +}); diff --git a/test/unit/cartodb/ported/stats_client.test.js b/test/unit/cartodb/ported/stats_client.test.js new file mode 100644 index 00000000..088206a0 --- /dev/null +++ b/test/unit/cartodb/ported/stats_client.test.js @@ -0,0 +1,40 @@ +require('../../../support/test_helper'); + +var assert = require('assert'); + +var StatsClient = require('../../../../lib/cartodb/stats/client'); + +describe('stats client', function() { + var statsInstance; + + before(function() { + statsInstance = StatsClient.instance; + StatsClient.instance = null; + }); + + after(function() { + StatsClient.instance = statsInstance; + }); + + it('reports errors when they repeat', function(done) { + var WADUS_ERROR = 'wadus_error'; + var statsClient = StatsClient.getInstance({ host: '127.0.0.1', port: 8033 }); + + statsClient.socket.emit('error', 'other_error'); + assert.ok(statsClient.last_error); + assert.equal(statsClient.last_error.msg, 'other_error'); + assert.ok(!statsClient.last_error.interval); + + statsClient.socket.emit('error', WADUS_ERROR); + assert.ok(statsClient.last_error); + assert.equal(statsClient.last_error.msg, WADUS_ERROR); + assert.ok(!statsClient.last_error.interval); + + statsClient.socket.emit('error', WADUS_ERROR); + assert.ok(statsClient.last_error); + assert.equal(statsClient.last_error.msg, WADUS_ERROR); + assert.ok(statsClient.last_error.interval); + + done(); + }); +}); From ea872d96f8a2505abb523a5abac8661192fd7c1c Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 14 Sep 2015 19:24:24 +0200 Subject: [PATCH 05/14] Listen to renderer cache events and log stats --- lib/cartodb/stats/reporter/renderer.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/cartodb/stats/reporter/renderer.js b/lib/cartodb/stats/reporter/renderer.js index 5c01d960..c6843a25 100644 --- a/lib/cartodb/stats/reporter/renderer.js +++ b/lib/cartodb/stats/reporter/renderer.js @@ -61,10 +61,23 @@ RendererStatsReporter.prototype.start = function() { global.statsClient.gauge('windshaft.mapnik-pool.waiting', renderersStats.pool.waiting); }, this.statsInterval); + this.rendererCache.on('err', rendererCacheErrorListener); + this.rendererCache.on('gc', gcTimingListener); }; +function rendererCacheErrorListener() { + global.statsClient.increment('windshaft.rendercache.error'); +} + +function gcTimingListener(gcTime) { + console.log('gctime'); + global.statsClient.timing('windshaft.rendercache.gc', gcTime); +} RendererStatsReporter.prototype.stop = function() { + this.rendererCache.removeListener('err', rendererCacheErrorListener); + this.rendererCache.removeListener('gc', gcTimingListener); + clearInterval(this.renderersStatsIntervalId); this.renderersStatsIntervalId = null; }; From 126491fc933636c0ceabb31a9d8138b1e8995c85 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 15 Sep 2015 11:46:19 +0200 Subject: [PATCH 06/14] Remove unused xml file that was used in health check --- lib/cartodb/monitoring/map-config.xml | 4 ---- 1 file changed, 4 deletions(-) delete mode 100644 lib/cartodb/monitoring/map-config.xml diff --git a/lib/cartodb/monitoring/map-config.xml b/lib/cartodb/monitoring/map-config.xml deleted file mode 100644 index 91a271e1..00000000 --- a/lib/cartodb/monitoring/map-config.xml +++ /dev/null @@ -1,4 +0,0 @@ - - From f5660667c8337eb709536907e1aee7a399b7be96 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 15 Sep 2015 18:16:50 +0200 Subject: [PATCH 07/14] Move req.profiler call to req2params itself --- lib/cartodb/controllers/layergroup.js | 8 +------- lib/cartodb/server.js | 4 ++++ test/unit/cartodb/req2params.test.js | 25 ++++++++++++++++++------- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 845f7eaf..1925225e 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -55,8 +55,6 @@ LayergroupController.prototype.attributes = function(req, res) { self.app.req2params(req, this); }, function retrieveFeatureAttributes(err) { - req.profiler.done('req2params'); - assert.ifError(err); var mapConfigProvider = new MapStoreMapConfigProvider( @@ -103,10 +101,7 @@ LayergroupController.prototype.tileOrLayer = function (req, res) { self.app.req2params(req, this); }, function mapController$getTileOrGrid(err) { - req.profiler.done('req2params'); - if ( err ) { - throw err; - } + assert.ifError(err); self.tileBackend.getTile( new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, req.params), req.params, this @@ -192,7 +187,6 @@ LayergroupController.prototype.staticMap = function(req, res, width, height, zoo self.app.req2params(req, this); }, function(err) { - req.profiler.done('req2params'); assert.ifError(err); if (center) { self.previewBackend.getImage( diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index ad4ce2f9..6cad6466 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -348,6 +348,7 @@ module.exports = function(serverOptions) { _.extend(req.query, JSON.parse(result)); app.req2params(req, callback); } catch (err) { + req.profiler.done('req2params'); callback(new Error('Error parsing lzma as JSON: ' + err)); } } @@ -380,6 +381,7 @@ module.exports = function(serverOptions) { 'Cannot use map signature of user "' + req.params.signer + '" on db of user "' + user + '"' ); err.http_status = 403; + req.profiler.done('req2params'); callback(err); return; } @@ -420,6 +422,7 @@ module.exports = function(serverOptions) { }, function finishSetup(err) { if ( err ) { + req.profiler.done('req2params'); return callback(err, req); } @@ -432,6 +435,7 @@ module.exports = function(serverOptions) { dbport: global.environment.postgres.port }); + req.profiler.done('req2params'); callback(null, req); } ); diff --git a/test/unit/cartodb/req2params.test.js b/test/unit/cartodb/req2params.test.js index d0c83416..a46941a7 100644 --- a/test/unit/cartodb/req2params.test.js +++ b/test/unit/cartodb/req2params.test.js @@ -18,14 +18,17 @@ suite('req2params', function() { assert.ok(_.isFunction(server.req2params)); }); - function addContext(req) { + function prepareRequest(req) { + req.profiler = { + done: function() {} + }; req.context = { user: 'localhost' }; return req; } test('cleans up request', function(done){ var req = {headers: { host:'localhost' }, query: {dbuser:'hacker',dbname:'secret'}}; - server.req2params(addContext(req), function(err, req) { + server.req2params(prepareRequest(req), function(err, req) { if ( err ) { done(err); return; } assert.ok(_.isObject(req.query), 'request has query'); assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); @@ -39,7 +42,7 @@ suite('req2params', function() { test('sets dbname from redis metadata', function(done){ var req = {headers: { host:'localhost' }, query: {} }; - server.req2params(addContext(req), function(err, req) { + server.req2params(prepareRequest(req), function(err, req) { if ( err ) { done(err); return; } //console.dir(req); assert.ok(_.isObject(req.query), 'request has query'); @@ -54,7 +57,7 @@ suite('req2params', function() { test('sets also dbuser for authenticated requests', function(done){ var req = {headers: { host:'localhost' }, query: {map_key: '1234'} }; - server.req2params(addContext(req), function(err, req) { + server.req2params(prepareRequest(req), function(err, req) { if ( err ) { done(err); return; } //console.dir(req); assert.ok(_.isObject(req.query), 'request has query'); @@ -63,8 +66,16 @@ suite('req2params', function() { assert.ok(!req.params.hasOwnProperty('interactivity'), 'request params do not have interactivity'); assert.equal(req.params.dbname, test_database); assert.equal(req.params.dbuser, test_user); - - server.req2params(addContext({headers: { host:'localhost' }, query: {map_key: '1235'} }), function(err, req) { + + req = { + headers: { + host:'localhost' + }, + query: { + map_key: '1235' + } + }; + server.req2params(prepareRequest(req), function(err, req) { // wrong key resets params to no user assert.ok(req.params.dbuser === test_pubuser, 'could inject dbuser ('+req.params.dbuser+')'); done(); @@ -90,7 +101,7 @@ suite('req2params', function() { lzma: data } }; - server.req2params(addContext(req), function(err, req) { + server.req2params(prepareRequest(req), function(err, req) { if ( err ) { return done(err); } From e2e5e40ea91e94bf12ee47c81a17d329ffa73bfe Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 15 Sep 2015 19:01:34 +0200 Subject: [PATCH 08/14] Fix maxcomplexity --- lib/cartodb/server.js | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 6cad6466..225b61c4 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -80,22 +80,7 @@ module.exports = function(serverOptions) { max_user_templates: global.environment.maxUserTemplates }); - var surrogateKeysCacheBackends = []; - - if (serverOptions.varnish_purge_enabled) { - surrogateKeysCacheBackends.push( - new VarnishHttpCacheBackend(serverOptions.varnish_host, serverOptions.varnish_http_port) - ); - } - - if (serverOptions.fastly && - !!serverOptions.fastly.enabled && !!serverOptions.fastly.apiKey && !!serverOptions.fastly.serviceId) { - surrogateKeysCacheBackends.push( - new FastlyCacheBackend(serverOptions.fastly.apiKey, serverOptions.fastly.serviceId) - ); - } - - var surrogateKeysCache = new SurrogateKeysCache(surrogateKeysCacheBackends); + var surrogateKeysCache = new SurrogateKeysCache(surrogateKeysCacheBackends(serverOptions)); function invalidateNamedMap (owner, templateName) { var startTime = Date.now(); @@ -440,6 +425,7 @@ module.exports = function(serverOptions) { } ); }; + // jshint maxcomplexity:6 return app; }; @@ -515,6 +501,25 @@ function setupLogger(app, opts) { } } +function surrogateKeysCacheBackends(serverOptions) { + var cacheBackends = []; + + if (serverOptions.varnish_purge_enabled) { + cacheBackends.push( + new VarnishHttpCacheBackend(serverOptions.varnish_host, serverOptions.varnish_http_port) + ); + } + + if (serverOptions.fastly && + !!serverOptions.fastly.enabled && !!serverOptions.fastly.apiKey && !!serverOptions.fastly.serviceId) { + cacheBackends.push( + new FastlyCacheBackend(serverOptions.fastly.apiKey, serverOptions.fastly.serviceId) + ); + } + + return cacheBackends; +} + function statusFromErrorMessage(errMsg) { // Find an appropriate statusCode based on message var statusCode = 400; From fba5a3551469b62271c7e2b1751cf493d66e4cfa Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 15 Sep 2015 19:28:02 +0200 Subject: [PATCH 09/14] Move sendResponse and sendError to response object --- lib/cartodb/controllers/layergroup.js | 8 +- lib/cartodb/controllers/map.js | 8 +- lib/cartodb/controllers/named_maps.js | 6 +- lib/cartodb/controllers/named_maps_admin.js | 4 +- lib/cartodb/server.js | 148 ++++++++++---------- test/unit/cartodb/ported/tile_stats.test.js | 20 +-- 6 files changed, 96 insertions(+), 98 deletions(-) diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 1925225e..1e5ecc9a 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -69,7 +69,7 @@ LayergroupController.prototype.attributes = function(req, res) { // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 var errMsg = err.message ? ( '' + err.message ) : ( '' + err ); var statusCode = self.app.findStatusCode(err); - self.app.sendError(res, { errors: [errMsg] }, statusCode, 'GET ATTRIBUTES', err); + res.sendError(res, { errors: [errMsg] }, statusCode, 'GET ATTRIBUTES', err); } else { self.sendResponse(req, res, [tile, 200]); } @@ -149,7 +149,7 @@ LayergroupController.prototype.finalizeGetTileOrGrid = function(err, req, res, t errMsg = 'style'+matches[2]+': ' + matches[1]; } - this.app.sendError(res, { errors: ['' + errMsg] }, statusCode, 'TILE RENDER', err); + res.sendError(res, { errors: ['' + errMsg] }, statusCode, 'TILE RENDER', err); global.statsClient.increment('windshaft.tiles.error'); global.statsClient.increment('windshaft.tiles.' + formatStat + '.error'); } else { @@ -206,7 +206,7 @@ LayergroupController.prototype.staticMap = function(req, res, width, height, zoo if (!err.error) { err.error = err.message; } - self.app.sendError(res, {errors: ['' + err] }, self.app.findStatusCode(err), 'STATIC_MAP', err); + res.sendError(res, {errors: ['' + err] }, self.app.findStatusCode(err), 'STATIC_MAP', err); } else { res.setHeader('Content-Type', headers['Content-Type'] || 'image/' + format); self.sendResponse(req, res, [image, 200]); @@ -245,7 +245,7 @@ LayergroupController.prototype.sendResponse = function(req, res, args) { res.header('X-Cache-Channel', tablesCacheEntry.getCacheChannel()); self.surrogateKeysCache.tag(res, tablesCacheEntry); } - self.app.sendResponse(res, args); + res.sendResponse(res, args); } ); diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 45f17797..2a43a3ce 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -156,10 +156,10 @@ MapController.prototype.create = function(req, res, prepareConfigFn) { function finish(err, layergroup) { if (err) { var statusCode = self.app.findStatusCode(err); - self.app.sendError(res, { errors: [ err.message ] }, statusCode, 'ANONYMOUS LAYERGROUP', err); + res.sendError(res, { errors: [ err.message ] }, statusCode, 'ANONYMOUS LAYERGROUP', err); } else { res.header('X-Layergroup-Id', layergroup.layergroupid); - self.app.sendResponse(res, [layergroup, 200]); + res.sendResponse(res, [layergroup, 200]); } } ); @@ -211,7 +211,7 @@ MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn function finishTemplateInstantiation(err, layergroup) { if (err) { var statusCode = self.app.findStatusCode(err); - self.app.sendError(res, { errors: [ err.message ] }, statusCode, 'NAMED MAP LAYERGROUP', err); + res.sendError(res, { errors: [ err.message ] }, statusCode, 'NAMED MAP LAYERGROUP', err); } else { var templateHash = self.templateMaps.fingerPrint(mapConfigProvider.template).substring(0, 8); layergroup.layergroupid = cdbuser + '@' + templateHash + '@' + layergroup.layergroupid; @@ -219,7 +219,7 @@ MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn res.header('X-Layergroup-Id', layergroup.layergroupid); self.surrogateKeysCache.tag(res, new NamedMapsCacheEntry(cdbuser, mapConfigProvider.getTemplateName())); - self.app.sendResponse(res, [layergroup, 200]); + res.sendResponse(res, [layergroup, 200]); } } ); diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index 8dbc27d1..a751b5d0 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -60,7 +60,7 @@ NamedMapsController.prototype.sendResponse = function(req, res, resource, header self.surrogateKeysCache.tag(res, tablesCacheEntry); } } - self.app.sendResponse(res, [resource, 200]); + res.sendResponse(res, [resource, 200]); } ); }; @@ -93,7 +93,7 @@ NamedMapsController.prototype.tile = function(req, res) { if (!err.error) { err.error = err.message; } - self.app.sendError(res, err, self.app.findStatusCode(err), 'NAMED_MAP_TILE', err); + res.sendError(res, err, self.app.findStatusCode(err), 'NAMED_MAP_TILE', err); } else { self.sendResponse(req, res, tile, headers, namedMapProvider); } @@ -185,7 +185,7 @@ NamedMapsController.prototype.staticMap = function(req, res) { if (!err.error) { err.error = err.message; } - self.app.sendError(res, err, self.app.findStatusCode(err), 'STATIC_VIZ_MAP', err); + res.sendError(res, err, self.app.findStatusCode(err), 'STATIC_VIZ_MAP', err); } else { self.sendResponse(req, res, image, headers, namedMapProvider); } diff --git a/lib/cartodb/controllers/named_maps_admin.js b/lib/cartodb/controllers/named_maps_admin.js index f2fb1c8a..31dffcad 100644 --- a/lib/cartodb/controllers/named_maps_admin.js +++ b/lib/cartodb/controllers/named_maps_admin.js @@ -179,9 +179,9 @@ function finishFn(app, res, description, okResponse) { if ( ! _.isUndefined(err.http_status) ) { statusCode = err.http_status; } - app.sendError(res, response, statusCode, description, err); + res.sendError(res, response, statusCode, description, err); } else { - app.sendResponse(res, okResponse || [response, statusCode]); + res.sendResponse(res, okResponse || [response, statusCode]); } }; } diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 225b61c4..4ee6464f 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -227,81 +227,6 @@ module.exports = function(serverOptions) { * END Routing ******************************************************************************************************************/ - // temporary measure until we upgrade to newer version expressjs so we can check err.status - app.use(function(err, req, res, next) { - if (err) { - if (err.name === 'SyntaxError') { - app.sendError(res, { errors: [err.name + ': ' + err.message] }, 400, 'JSON', err); - } else { - next(err); - } - } else { - next(); - } - }); - - app.sendResponse = function(res, args) { - var req = res.req; - - if (global.environment && global.environment.api_hostname) { - res.header('X-Served-By-Host', global.environment.api_hostname); - } - - if (req && req.params && req.params.dbhost) { - res.header('X-Served-By-DB-Host', req.params.dbhost); - } - - if ( req && req.profiler ) { - res.header('X-Tiler-Profiler', req.profiler.toJSONString()); - } - -// res.send(body|status[, headers|status[, status]]) - res.send.apply(res, args); - - if ( req && req.profiler ) { - try { - // May throw due to dns, see - // See http://github.com/CartoDB/Windshaft/issues/166 - req.profiler.sendStats(); - } catch (err) { - console.error("error sending profiling stats: " + err); - } - } - }; - - app.sendError = function(res, err, statusCode, label, tolog) { - res._windshaftStatusCode = statusCode; - - var olabel = '['; - if ( label ) { - olabel += label + ' '; - } - olabel += 'ERROR]'; - if ( ! tolog ) { - tolog = err; - } - var log_msg = olabel + " -- " + statusCode + ": " + tolog; - //if ( tolog.stack ) log_msg += "\n" + tolog.stack; - console.error(log_msg); // use console.log for statusCode != 500 ? - // If a callback was requested, force status to 200 - if ( res.req ) { - // NOTE: res.req can be undefined when we fake a call to - // ourself from POST to /layergroup - if ( res.req.query.callback ) { - statusCode = 200; - } - } - // Strip connection info, if any - // See https://github.com/CartoDB/Windshaft/issues/173 - err = JSON.stringify(err); - err = err.replace(/Connection string: '[^']*'\\n/, ''); - // See https://travis-ci.org/CartoDB/Windshaft/jobs/20703062#L1644 - err = err.replace(/is the server.*encountered/im, 'encountered'); - err = JSON.parse(err); - - app.sendResponse(res, [err, statusCode]); - }; - // jshint maxcomplexity:10 /** * Whitelist input and get database name & default geometry type from @@ -472,10 +397,83 @@ function bootstrap(opts) { statsd_client: global.statsClient, profile: opts.useProfiler }); + res.removeHeader('x-powered-by'); + + res.sendResponse = function(res, args) { + if (global.environment && global.environment.api_hostname) { + res.header('X-Served-By-Host', global.environment.api_hostname); + } + + if (req && req.params && req.params.dbhost) { + res.header('X-Served-By-DB-Host', req.params.dbhost); + } + + if (req && req.profiler ) { + res.header('X-Tiler-Profiler', req.profiler.toJSONString()); + } + +// res.send(body|status[, headers|status[, status]]) + res.send.apply(res, args); + + if ( req && req.profiler ) { + try { + // May throw due to dns, see + // See http://github.com/CartoDB/Windshaft/issues/166 + req.profiler.sendStats(); + } catch (err) { + console.error("error sending profiling stats: " + err); + } + } + }; + res.sendError = function(res, err, statusCode, label, tolog) { + res._windshaftStatusCode = statusCode; + + var olabel = '['; + if ( label ) { + olabel += label + ' '; + } + olabel += 'ERROR]'; + if ( ! tolog ) { + tolog = err; + } + var log_msg = olabel + " -- " + statusCode + ": " + tolog; + //if ( tolog.stack ) log_msg += "\n" + tolog.stack; + console.error(log_msg); // use console.log for statusCode != 500 ? + // If a callback was requested, force status to 200 + if ( res.req ) { + // NOTE: res.req can be undefined when we fake a call to + // ourself from POST to /layergroup + if ( res.req.query.callback ) { + statusCode = 200; + } + } + // Strip connection info, if any + // See https://github.com/CartoDB/Windshaft/issues/173 + err = JSON.stringify(err); + err = err.replace(/Connection string: '[^']*'\\n/, ''); + // See https://travis-ci.org/CartoDB/Windshaft/jobs/20703062#L1644 + err = err.replace(/is the server.*encountered/im, 'encountered'); + err = JSON.parse(err); + + res.sendResponse(res, [err, statusCode]); + }; next(); }); + // temporary measure until we upgrade to newer version expressjs so we can check err.status + app.use(function(err, req, res, next) { + if (err) { + if (err.name === 'SyntaxError') { + res.send({ errors: [err.name + ': ' + err.message] }, 400); + } else { + next(err); + } + } else { + next(); + } + }); + setupLogger(app, opts); return app; diff --git a/test/unit/cartodb/ported/tile_stats.test.js b/test/unit/cartodb/ported/tile_stats.test.js index 433daddf..b0bbf79c 100644 --- a/test/unit/cartodb/ported/tile_stats.test.js +++ b/test/unit/cartodb/ported/tile_stats.test.js @@ -28,17 +28,17 @@ describe('tile stats', function() { } }); - var ws = cartodbServer(serverOptions); - ws.sendError = function(){}; - - var layergroupController = new LayergroupController(ws, null); + var layergroupController = new LayergroupController(cartodbServer(serverOptions)); var reqMock = { params: { format: invalidFormat } }; - layergroupController.finalizeGetTileOrGrid('Unsupported format png2', reqMock, {}, null, null); + var resMock = { + sendError: function() {} + }; + layergroupController.finalizeGetTileOrGrid('Unsupported format png2', reqMock, resMock, null, null); assert.ok(formatMatched, 'Format was never matched in increment method'); assert.equal(expectedCalls, 0, 'Unexpected number of calls to increment method'); @@ -60,13 +60,13 @@ describe('tile stats', function() { format: validFormat } }; + var resMock = { + sendError: function() {} + }; - var ws = cartodbServer(serverOptions); - ws.sendError = function(){}; + var layergroupController = new LayergroupController(cartodbServer(serverOptions)); - var layergroupController = new LayergroupController(ws, null); - - layergroupController.finalizeGetTileOrGrid('Another error happened', reqMock, {}, null, null); + layergroupController.finalizeGetTileOrGrid('Another error happened', reqMock, resMock, null, null); assert.ok(formatMatched, 'Format was never matched in increment method'); assert.equal(expectedCalls, 0, 'Unexpected number of calls to increment method'); From 72a0c4a487783720b1be0a9072a956f5e03bfebf Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 16 Sep 2015 01:36:51 +0200 Subject: [PATCH 10/14] New sendResponse and sendError methods - fixes response for static named map error cases --- lib/cartodb/controllers/layergroup.js | 18 +++---- lib/cartodb/controllers/map.js | 10 ++-- lib/cartodb/controllers/named_maps.js | 12 ++--- lib/cartodb/controllers/named_maps_admin.js | 22 +++----- lib/cartodb/server.js | 60 ++++++++++----------- test/acceptance/named_static_maps.js | 9 ++-- 6 files changed, 55 insertions(+), 76 deletions(-) diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 1e5ecc9a..57a7628f 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -66,10 +66,7 @@ LayergroupController.prototype.attributes = function(req, res) { req.profiler.add(stats || {}); if (err) { - // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 - var errMsg = err.message ? ( '' + err.message ) : ( '' + err ); - var statusCode = self.app.findStatusCode(err); - res.sendError(res, { errors: [errMsg] }, statusCode, 'GET ATTRIBUTES', err); + res.sendError(err, 'GET ATTRIBUTES'); } else { self.sendResponse(req, res, [tile, 200]); } @@ -138,18 +135,18 @@ LayergroupController.prototype.finalizeGetTileOrGrid = function(err, req, res, t } } - if (err){ + if (err) { // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 var errMsg = err.message ? ( '' + err.message ) : ( '' + err ); - var statusCode = this.app.findStatusCode(err); // Rewrite mapnik parsing errors to start with layer number var matches = errMsg.match("(.*) in style 'layer([0-9]+)'"); if (matches) { errMsg = 'style'+matches[2]+': ' + matches[1]; } + err.message = errMsg; - res.sendError(res, { errors: ['' + errMsg] }, statusCode, 'TILE RENDER', err); + res.sendError(err, 'TILE RENDER'); global.statsClient.increment('windshaft.tiles.error'); global.statsClient.increment('windshaft.tiles.' + formatStat + '.error'); } else { @@ -203,10 +200,7 @@ LayergroupController.prototype.staticMap = function(req, res, width, height, zoo req.profiler.add(stats || {}); if (err) { - if (!err.error) { - err.error = err.message; - } - res.sendError(res, {errors: ['' + err] }, self.app.findStatusCode(err), 'STATIC_MAP', err); + res.sendError(err, 'STATIC_MAP'); } else { res.setHeader('Content-Type', headers['Content-Type'] || 'image/' + format); self.sendResponse(req, res, [image, 200]); @@ -245,7 +239,7 @@ LayergroupController.prototype.sendResponse = function(req, res, args) { res.header('X-Cache-Channel', tablesCacheEntry.getCacheChannel()); self.surrogateKeysCache.tag(res, tablesCacheEntry); } - res.sendResponse(res, args); + res.sendResponse(args); } ); diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 2a43a3ce..73182387 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -155,11 +155,10 @@ MapController.prototype.create = function(req, res, prepareConfigFn) { }, function finish(err, layergroup) { if (err) { - var statusCode = self.app.findStatusCode(err); - res.sendError(res, { errors: [ err.message ] }, statusCode, 'ANONYMOUS LAYERGROUP', err); + res.sendError(err, 'ANONYMOUS LAYERGROUP'); } else { res.header('X-Layergroup-Id', layergroup.layergroupid); - res.sendResponse(res, [layergroup, 200]); + res.sendResponse([layergroup, 200]); } } ); @@ -210,8 +209,7 @@ MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn }, function finishTemplateInstantiation(err, layergroup) { if (err) { - var statusCode = self.app.findStatusCode(err); - res.sendError(res, { errors: [ err.message ] }, statusCode, 'NAMED MAP LAYERGROUP', err); + res.sendError(err, 'NAMED MAP LAYERGROUP'); } else { var templateHash = self.templateMaps.fingerPrint(mapConfigProvider.template).substring(0, 8); layergroup.layergroupid = cdbuser + '@' + templateHash + '@' + layergroup.layergroupid; @@ -219,7 +217,7 @@ MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn res.header('X-Layergroup-Id', layergroup.layergroupid); self.surrogateKeysCache.tag(res, new NamedMapsCacheEntry(cdbuser, mapConfigProvider.getTemplateName())); - res.sendResponse(res, [layergroup, 200]); + res.sendResponse([layergroup, 200]); } } ); diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index a751b5d0..c123c461 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -60,7 +60,7 @@ NamedMapsController.prototype.sendResponse = function(req, res, resource, header self.surrogateKeysCache.tag(res, tablesCacheEntry); } } - res.sendResponse(res, [resource, 200]); + res.sendResponse([resource, 200]); } ); }; @@ -90,10 +90,7 @@ NamedMapsController.prototype.tile = function(req, res) { req.profiler.add(stats); } if (err) { - if (!err.error) { - err.error = err.message; - } - res.sendError(res, err, self.app.findStatusCode(err), 'NAMED_MAP_TILE', err); + res.sendError(err, 'NAMED_MAP_TILE'); } else { self.sendResponse(req, res, tile, headers, namedMapProvider); } @@ -182,10 +179,7 @@ NamedMapsController.prototype.staticMap = function(req, res) { } if (err) { - if (!err.error) { - err.error = err.message; - } - res.sendError(res, err, self.app.findStatusCode(err), 'STATIC_VIZ_MAP', err); + res.sendError(err, 'STATIC_VIZ_MAP'); } else { self.sendResponse(req, res, image, headers, namedMapProvider); } diff --git a/lib/cartodb/controllers/named_maps_admin.js b/lib/cartodb/controllers/named_maps_admin.js index 31dffcad..8e971b1f 100644 --- a/lib/cartodb/controllers/named_maps_admin.js +++ b/lib/cartodb/controllers/named_maps_admin.js @@ -48,7 +48,7 @@ NamedMapsAdminController.prototype.create = function(req, res) { assert.ifError(err); return { template_id: tpl_id }; }, - finishFn(self.app, res, 'POST TEMPLATE') + finishFn(res, 'POST TEMPLATE') ); }; @@ -76,7 +76,7 @@ NamedMapsAdminController.prototype.update = function(req, res) { return { template_id: tpl_id }; }, - finishFn(self.app, res, 'PUT TEMPLATE') + finishFn(res, 'PUT TEMPLATE') ); }; @@ -112,7 +112,7 @@ NamedMapsAdminController.prototype.retrieve = function(req, res) { delete tpl_val.auth_id; return { template: tpl_val }; }, - finishFn(self.app, res, 'GET TEMPLATE') + finishFn(res, 'GET TEMPLATE') ); }; @@ -140,7 +140,7 @@ NamedMapsAdminController.prototype.destroy = function(req, res) { assert.ifError(err); return { status: 'ok' }; }, - finishFn(self.app, res, 'DELETE TEMPLATE', ['', 204]) + finishFn(res, 'DELETE TEMPLATE', ['', 204]) ); }; @@ -166,22 +166,16 @@ NamedMapsAdminController.prototype.list = function(req, res) { assert.ifError(err); return { template_ids: tpl_ids }; }, - finishFn(self.app, res, 'GET TEMPLATE LIST') + finishFn(res, 'GET TEMPLATE LIST') ); }; -function finishFn(app, res, description, okResponse) { +function finishFn(res, description, okResponse) { return function finish(err, response){ - var statusCode = 200; if (err) { - statusCode = 400; - response = { errors: ['' + err] }; - if ( ! _.isUndefined(err.http_status) ) { - statusCode = err.http_status; - } - res.sendError(res, response, statusCode, description, err); + res.sendError(err, description); } else { - res.sendResponse(res, okResponse || [response, statusCode]); + res.sendResponse(okResponse || [response, 200]); } }; } diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 4ee6464f..9d8594a0 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -400,23 +400,22 @@ function bootstrap(opts) { res.removeHeader('x-powered-by'); - res.sendResponse = function(res, args) { + res.sendResponse = function(args) { if (global.environment && global.environment.api_hostname) { res.header('X-Served-By-Host', global.environment.api_hostname); } - if (req && req.params && req.params.dbhost) { + if (req.params && req.params.dbhost) { res.header('X-Served-By-DB-Host', req.params.dbhost); } - if (req && req.profiler ) { + if (req.profiler) { res.header('X-Tiler-Profiler', req.profiler.toJSONString()); } -// res.send(body|status[, headers|status[, status]]) res.send.apply(res, args); - if ( req && req.profiler ) { + if (req.profiler ) { try { // May throw due to dns, see // See http://github.com/CartoDB/Windshaft/issues/166 @@ -426,37 +425,36 @@ function bootstrap(opts) { } } }; - res.sendError = function(res, err, statusCode, label, tolog) { - res._windshaftStatusCode = statusCode; + res.sendError = function(err, label) { - var olabel = '['; - if ( label ) { - olabel += label + ' '; + label = label || 'UNKNOWN'; + + var statusCode = app.findStatusCode(err); + + // use console.log for statusCode != 500 ? + if (statusCode >= 500) { + console.error('[%s ERROR] -- %d: %s', label, statusCode, err); + } else { + console.warn('[%s WARN] -- %d: %s', label, statusCode, err); } - olabel += 'ERROR]'; - if ( ! tolog ) { - tolog = err; - } - var log_msg = olabel + " -- " + statusCode + ": " + tolog; - //if ( tolog.stack ) log_msg += "\n" + tolog.stack; - console.error(log_msg); // use console.log for statusCode != 500 ? + // If a callback was requested, force status to 200 - if ( res.req ) { - // NOTE: res.req can be undefined when we fake a call to - // ourself from POST to /layergroup - if ( res.req.query.callback ) { - statusCode = 200; - } + if (req.query.callback) { + statusCode = 200; } - // Strip connection info, if any - // See https://github.com/CartoDB/Windshaft/issues/173 - err = JSON.stringify(err); - err = err.replace(/Connection string: '[^']*'\\n/, ''); - // See https://travis-ci.org/CartoDB/Windshaft/jobs/20703062#L1644 - err = err.replace(/is the server.*encountered/im, 'encountered'); - err = JSON.parse(err); - res.sendResponse(res, [err, statusCode]); + // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 + var message = (_.isString(err) ? err : err.message) || 'Unknown error'; + // Strip connection info, if any + message = message + // See https://github.com/CartoDB/Windshaft/issues/173 + .replace(/Connection string: '[^']*'\\n/, '') + // See https://travis-ci.org/CartoDB/Windshaft/jobs/20703062#L1644 + .replace(/is the server.*encountered/im, 'encountered'); + + var errorResponseBody = { errors: [message] }; + + res.sendResponse([errorResponseBody, statusCode]); }; next(); }); diff --git a/test/acceptance/named_static_maps.js b/test/acceptance/named_static_maps.js index 3df35b22..3acd4bee 100644 --- a/test/acceptance/named_static_maps.js +++ b/test/acceptance/named_static_maps.js @@ -163,8 +163,10 @@ describe('named static maps', function() { var nonexistentName = 'nonexistent'; getStaticMap(nonexistentName, { status: 404 }, function(err, res) { assert.ok(!err); - var parsed = JSON.parse(res.body); - assert.equal(parsed.error, "Template '" + nonexistentName + "' of user '" + username + "' not found"); + assert.deepEqual( + JSON.parse(res.body), + { errors: ["Template '" + nonexistentName + "' of user '" + username + "' not found"] } + ); done(); }); }); @@ -172,8 +174,7 @@ describe('named static maps', function() { it('should return 403 if not properly authorized', function(done) { getStaticMap(tokenAuthTemplateName, { status: 403 }, function(err, res) { assert.ok(!err); - var parsed = JSON.parse(res.body); - assert.equal(parsed.error, 'Unauthorized template instantiation'); + assert.deepEqual(JSON.parse(res.body), { errors: ['Unauthorized template instantiation'] }); done(); }); }); From ad2ebc11ddc9f03b88320fd02cd10c45cdf6fc0a Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 16 Sep 2015 01:39:15 +0200 Subject: [PATCH 11/14] Remove unused require --- lib/cartodb/controllers/named_maps_admin.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cartodb/controllers/named_maps_admin.js b/lib/cartodb/controllers/named_maps_admin.js index 8e971b1f..42a4cd31 100644 --- a/lib/cartodb/controllers/named_maps_admin.js +++ b/lib/cartodb/controllers/named_maps_admin.js @@ -1,6 +1,5 @@ var step = require('step'); var assert = require('assert'); -var _ = require('underscore'); var templateName = require('../backends/template_maps').templateName; var cors = require('../middleware/cors'); From 713ad03c3b30635cca820ad64e3d48a1ac6ab4a4 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 16 Sep 2015 01:44:30 +0200 Subject: [PATCH 12/14] No need to expose findStatusCode at app level --- lib/cartodb/server.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 9d8594a0..21d34f0c 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -164,16 +164,6 @@ module.exports = function(serverOptions) { var authApi = new AuthApi(pgConnection, metadataBackend, mapStore, templateMaps); - app.findStatusCode = function(err) { - var statusCode; - if ( err.http_status ) { - statusCode = err.http_status; - } else { - statusCode = statusFromErrorMessage('' + err); - } - return statusCode; - }; - var TablesExtentApi = require('./api/tables_extent_api'); var tablesExtentApi = new TablesExtentApi(pgQueryRunner); @@ -429,7 +419,7 @@ function bootstrap(opts) { label = label || 'UNKNOWN'; - var statusCode = app.findStatusCode(err); + var statusCode = findStatusCode(err); // use console.log for statusCode != 500 ? if (statusCode >= 500) { @@ -516,6 +506,16 @@ function surrogateKeysCacheBackends(serverOptions) { return cacheBackends; } +function findStatusCode(err) { + var statusCode; + if ( err.http_status ) { + statusCode = err.http_status; + } else { + statusCode = statusFromErrorMessage('' + err); + } + return statusCode; +} + function statusFromErrorMessage(errMsg) { // Find an appropriate statusCode based on message var statusCode = 400; From 62f428f434a1e35dc862cdbd46c2c1120d2f1cb1 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 16 Sep 2015 01:48:54 +0200 Subject: [PATCH 13/14] Remove app dependency --- lib/cartodb/controllers/named_maps_admin.js | 4 +--- lib/cartodb/server.js | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/controllers/named_maps_admin.js b/lib/cartodb/controllers/named_maps_admin.js index 42a4cd31..53265cd5 100644 --- a/lib/cartodb/controllers/named_maps_admin.js +++ b/lib/cartodb/controllers/named_maps_admin.js @@ -5,13 +5,11 @@ var cors = require('../middleware/cors'); /** - * @param app * @param {TemplateMaps} templateMaps * @param {AuthApi} authApi * @constructor */ -function NamedMapsAdminController(app, templateMaps, authApi) { - this.app = app; +function NamedMapsAdminController(templateMaps, authApi) { this.templateMaps = templateMaps; this.authApi = authApi; } diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 21d34f0c..99ea6a8f 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -209,7 +209,7 @@ module.exports = function(serverOptions) { tablesExtentApi ).register(app); - new controller.NamedMapsAdmin(app, templateMaps, authApi).register(app); + new controller.NamedMapsAdmin(templateMaps, authApi).register(app); new controller.ServerInfo().register(app); From 66f94d945286e4aa6f030f72bf86e3787b2b4af8 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Wed, 16 Sep 2015 02:49:18 +0200 Subject: [PATCH 14/14] Fix test --- lib/cartodb/server.js | 1 + test/unit/cartodb/ported/windshaft_server.test.js | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 99ea6a8f..d2006ead 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -515,6 +515,7 @@ function findStatusCode(err) { } return statusCode; } +module.exports.findStatusCode = findStatusCode; function statusFromErrorMessage(errMsg) { // Find an appropriate statusCode based on message diff --git a/test/unit/cartodb/ported/windshaft_server.test.js b/test/unit/cartodb/ported/windshaft_server.test.js index 9685e71e..2fee4c27 100644 --- a/test/unit/cartodb/ported/windshaft_server.test.js +++ b/test/unit/cartodb/ported/windshaft_server.test.js @@ -41,16 +41,16 @@ describe('windshaft', function() { }); it('different formats for postgis plugin error returns 400 as status code', function() { - var ws = cartodbServer(serverOptions); + var expectedStatusCode = 400; assert.equal( - ws.findStatusCode("Postgis Plugin: ERROR: column \"missing\" does not exist\n"), + cartodbServer.findStatusCode("Postgis Plugin: ERROR: column \"missing\" does not exist\n"), expectedStatusCode, "Error status code for single line does not match" ); assert.equal( - ws.findStatusCode("Postgis Plugin: PSQL error:\nERROR: column \"missing\" does not exist\n"), + cartodbServer.findStatusCode("Postgis Plugin: PSQL error:\nERROR: column \"missing\" does not exist\n"), expectedStatusCode, "Error status code for multiline/PSQL does not match" );