From c0ce6e7a8a85eab5c580ce509132e1cf8fe7d83e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 13 Jun 2016 12:20:56 +0200 Subject: [PATCH] WIP fixes 478, adds more error information when either analysis or turbo-carto is not well formed. --- lib/cartodb/controllers/base.js | 35 +++++- .../adapter/analysis-mapconfig-adapter.js | 22 +++- .../mapconfig/adapter/turbo-carto-adapter.js | 13 +- test/acceptance/analysis/error-cases.js | 115 ++++++++++++++++++ test/acceptance/turbo-cartocss/error-cases.js | 19 +++ 5 files changed, 192 insertions(+), 12 deletions(-) diff --git a/lib/cartodb/controllers/base.js b/lib/cartodb/controllers/base.js index e5952ff6..8b36c095 100644 --- a/lib/cartodb/controllers/base.js +++ b/lib/cartodb/controllers/base.js @@ -214,15 +214,19 @@ BaseController.prototype.sendError = function(req, res, err, label) { statusCode = 200; } - var errorResponseBody = { errors: allErrors.map(errorMessage) }; + var errorResponseBody = { + errors: allErrors.map(errorMessage) + }; + + var errorsWithContext = allErrors.map(errorMessageWithContext).filter(function (err) { return !!err; }); + if (errorsWithContext.length){ + errorResponseBody.errors_with_context = errorsWithContext; + } this.send(req, res, errorResponseBody, statusCode); }; -function errorMessage(err) { - // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 - var message = (_.isString(err) ? err : err.message) || 'Unknown error'; - +function stripConnectionInfo(message) { // Strip connection info, if any return message // See https://github.com/CartoDB/Windshaft/issues/173 @@ -230,6 +234,27 @@ function errorMessage(err) { // See https://travis-ci.org/CartoDB/Windshaft/jobs/20703062#L1644 .replace(/is the server.*encountered/im, 'encountered'); } + +function errorMessage(err) { + // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 + var message = (_.isString(err) ? err : err.message) || 'Unknown error'; + + return stripConnectionInfo(message); + +} + +function errorMessageWithContext(err) { + if (!err.context) { + return; + } + // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 + var message = (_.isString(err) ? err : err.message) || 'Unknown error'; + + return { + message: stripConnectionInfo(message), + context: err.context + }; +} module.exports.errorMessage = errorMessage; function findStatusCode(err) { diff --git a/lib/cartodb/models/mapconfig/adapter/analysis-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/analysis-mapconfig-adapter.js index 7a7150db..ffbe962b 100644 --- a/lib/cartodb/models/mapconfig/adapter/analysis-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/analysis-mapconfig-adapter.js @@ -58,13 +58,27 @@ AnalysisMapConfigAdapter.prototype.getMapConfig = function(user, requestMapConfi requestMapConfig = appendFiltersToNodes(requestMapConfig, dataviewsFiltersBySourceId); - function createAnalysis(analysisDefinition, done) { - self.analysisBackend.create(analysisConfiguration, analysisDefinition, done); + function createAnalysis(analysisDefinition, index, done) { + self.analysisBackend.create(analysisConfiguration, analysisDefinition, function (err, analysis) { + if (err) { + err.context = { + type: 'camshaft', + analysis: { + index: index, + id: analysisDefinition.id, + type: analysisDefinition.type + } + }; + return done(err); + } + + done(null, analysis); + }); } var analysesQueue = queue(requestMapConfig.analyses.length); - requestMapConfig.analyses.forEach(function(analysis) { - analysesQueue.defer(createAnalysis, analysis); + requestMapConfig.analyses.forEach(function(analysis, index) { + analysesQueue.defer(createAnalysis, analysis, index); }); analysesQueue.awaitAll(function(err, analysesResults) { diff --git a/lib/cartodb/models/mapconfig/adapter/turbo-carto-adapter.js b/lib/cartodb/models/mapconfig/adapter/turbo-carto-adapter.js index 3eb29197..dfdf6414 100644 --- a/lib/cartodb/models/mapconfig/adapter/turbo-carto-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/turbo-carto-adapter.js @@ -22,8 +22,8 @@ TurboCartoAdapter.prototype.getMapConfig = function (user, requestMapConfig, par var parseCartoQueue = queue(layers.length); - layers.forEach(function(layer) { - parseCartoQueue.defer(self._parseCartoCss.bind(self), user, layer); + layers.forEach(function(layer, index) { + parseCartoQueue.defer(self._parseCartoCss.bind(self), user, layer, index); }); parseCartoQueue.awaitAll(function (err, layers) { @@ -51,7 +51,7 @@ var pixelSizeTemplate = dot.template('40075017 * cos(ST_Y(ST_Centroid({{=it._bbo var scaleDenominatorTemplate = dot.template('({{=it._pixelSize}} / 0.00028)::numeric'); -TurboCartoAdapter.prototype._parseCartoCss = function (username, layer, callback) { +TurboCartoAdapter.prototype._parseCartoCss = function (username, layer, index, callback) { if (!shouldParseLayerCartocss(layer)) { return callback(null, layer); } @@ -86,6 +86,13 @@ TurboCartoAdapter.prototype._parseCartoCss = function (username, layer, callback if (err && err.name === 'TurboCartoError') { err = new Error('turbo-carto: ' + err.message); err.http_status = 400; + err.context = { + type: 'turbo-carto', + layer: { + index: index, + type: layer.type + } + }; return callback(err); } diff --git a/test/acceptance/analysis/error-cases.js b/test/acceptance/analysis/error-cases.js index fcfc6f13..4358b1cb 100644 --- a/test/acceptance/analysis/error-cases.js +++ b/test/acceptance/analysis/error-cases.js @@ -20,6 +20,13 @@ describe('analysis-layers error cases', function() { } }; + var AUTH_ERROR_RESPONSE = { + status: 403, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + it('should handle missing analysis nodes for layers', function(done) { var mapConfig = createMapConfig( [ @@ -64,4 +71,112 @@ describe('analysis-layers error cases', function() { testClient.drain(done); }); }); + + it('camshaft: should return error missing analysis nodes for layers with some context', function(done) { + var mapConfig = createMapConfig( + [ + { + "type": "cartodb", + "options": { + "source": { + "id": "HEAD" + }, + "cartocss": '#polygons { polygon-fill: red; }', + "cartocss_version": "2.3.0" + } + } + ], + {}, + [ + { + "id": "HEAD", + "type": "buffer", + "params": { + "source": { + "id": "HEAD", + "type": "source", + "params": { + "query": "select * from populated_places_simple_reduced" + } + }, + "radius": 50000 + } + } + ] + ); + + var testClient = new TestClient(mapConfig, 11111); + + testClient.getLayergroup(AUTH_ERROR_RESPONSE, function(err, layergroupResult) { + assert.ok(!err, err); + + assert.equal(layergroupResult.errors.length, 1); + assert.equal( + layergroupResult.errors[0], + 'Analysis requires authentication with API key: permission denied.' + ); + + assert.equal(layergroupResult.errors_with_context[0].context.type, 'camshaft'); + assert.equal(layergroupResult.errors_with_context[0].context.analysis.index, 0); + assert.equal(layergroupResult.errors_with_context[0].context.analysis.id, 'HEAD'); + assert.equal(layergroupResult.errors_with_context[0].context.analysis.type, 'buffer'); + + testClient.drain(done); + }); + }); + + + it('camshaft: should return error: Missing required param "radius"; with context', function(done) { + var mapConfig = createMapConfig( + [ + { + "type": "cartodb", + "options": { + "source": { + "id": "HEAD" + }, + "cartocss": '#polygons { polygon-fill: red; }', + "cartocss_version": "2.3.0" + } + } + ], + {}, + [ + { + "id": "HEAD", + "type": "buffer", + "params": { + "source": { + "id": "HEAD", + "type": "source", + "params": { + "query": "select * from populated_places_simple_reduced" + } + } + } + } + ] + ); + + var testClient = new TestClient(mapConfig, 1234); + + testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + assert.ok(!err, err); + + assert.equal(layergroupResult.errors.length, 1); + assert.equal( + layergroupResult.errors[0], + 'Missing required param "radius"' + ); + + assert.equal(layergroupResult.errors_with_context[0].context.type, 'camshaft'); + assert.equal(layergroupResult.errors_with_context[0].context.analysis.index, 0); + assert.equal(layergroupResult.errors_with_context[0].context.analysis.id, 'HEAD'); + assert.equal(layergroupResult.errors_with_context[0].context.analysis.type, 'buffer'); + + testClient.drain(done); + }); + }); + + }); diff --git a/test/acceptance/turbo-cartocss/error-cases.js b/test/acceptance/turbo-cartocss/error-cases.js index 3967738b..23597dd7 100644 --- a/test/acceptance/turbo-cartocss/error-cases.js +++ b/test/acceptance/turbo-cartocss/error-cases.js @@ -106,4 +106,23 @@ describe('turbo-carto error cases', function() { done(); }); }); + + it('turbo-carto: should return error invalid column from datasource with some context', function(done) { + this.testClient = new TestClient(makeMapconfig(null, 'ramp([wadus_column], (red, green, blue))')); + this.testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroup) { + assert.ok(!err, err); + + assert.ok(layergroup.hasOwnProperty('errors')); + assert.equal(layergroup.errors_with_context.length, 1); + assert.ok(layergroup.errors_with_context[0].message.match(/^turbo-carto/)); + assert.ok(layergroup.errors_with_context[0].message.match(/unable\sto\scompute\sramp/i)); + assert.ok(layergroup.errors_with_context[0].message.match(/wadus_column/)); + + assert.equal(layergroup.errors_with_context[0].context.type, 'turbo-carto'); + assert.equal(layergroup.errors_with_context[0].context.layer.index, 0); + assert.equal(layergroup.errors_with_context[0].context.layer.type, 'mapnik'); + + done(); + }); + }); });