From 855f47e446db37920de064460a08d1abed4bb30b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 4 Dec 2017 19:48:06 +0100 Subject: [PATCH] Detect incompatible CartoCSS or interactivity for raster aggregation --- .../adapter/aggregation-mapconfig-adapter.js | 65 +++++++++- test/acceptance/aggregation.js | 120 ++++++++++++++++-- 2 files changed, 170 insertions(+), 15 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 7b7468b7..f9a9c840 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -1,15 +1,25 @@ const AggregationProxy = require('../../aggregation/aggregation-proxy'); const { MapConfig } = require('windshaft').model; +const MISSING_AGGREGATION_COLUMNS = 'There are missing columns to perform aggregation'; + module.exports = class AggregationMapConfigAdapter { getMapConfig (user, requestMapConfig, params, context, callback) { - this.mapConfig = new MapConfig(requestMapConfig); + const mapConfig = new MapConfig(requestMapConfig); - if (!this._shouldAdaptLayers(requestMapConfig, params)) { + if (!this._shouldAdaptLayers(mapConfig, requestMapConfig, params)) { return callback(null, requestMapConfig); } - requestMapConfig.layers = this._adaptLayers(requestMapConfig); + if (this._hasMissingColumns(mapConfig)) { + const error = new Error(MISSING_AGGREGATION_COLUMNS); + error.http_status = 400; + error.type = 'mapconfig'; + + return callback(error); + } + + requestMapConfig.layers = this._adaptLayers(mapConfig, requestMapConfig); context.aggregation = { layers: this._getAggregationMetadata(requestMapConfig), }; @@ -17,7 +27,48 @@ module.exports = class AggregationMapConfigAdapter { callback(null, requestMapConfig); } - _shouldAdaptLayers (requestMapConfig, params) { + _hasMissingColumns (mapConfig) { + const layers = mapConfig.getLayers(); + let missingColumns = false; + + for (let index = 0; index < layers.length; index++) { + const layer = layers[index]; + const { aggregation } = layer.options; + const hasAggregationColumns = aggregation !== undefined && + typeof aggregation !== 'boolean' && + typeof aggregation.columns === 'object'; + const aggregationColumns = hasAggregationColumns ? Object.keys(aggregation.columns) : []; + const layerColumns = mapConfig.getColumnsByLayer(index); + + if (layerColumns.length === 0) { + continue; + } + + if (aggregationColumns.length === 0) { + missingColumns = true; + break; + } + + if (!this._haveSameColumns(aggregationColumns,layerColumns)) { + missingColumns = true; + break; + } + } + + return missingColumns; + } + + _haveSameColumns (aggregationColumns, layerColumns) { + if (aggregationColumns.length !== layerColumns.length) { + return false; + } + + const diff = aggregationColumns.filter(column => !layerColumns.includes(column)); + + return !diff.length; + } + + _shouldAdaptLayers (mapConfig, requestMapConfig, params) { let shouldAdapt = false; if (typeof params.aggregation === 'boolean') { @@ -25,7 +76,7 @@ module.exports = class AggregationMapConfigAdapter { } if (params.aggregation === undefined) { - if (this.mapConfig.isVectorOnlyMapConfig()) { + if (mapConfig.isVectorOnlyMapConfig()) { shouldAdapt = true; } else if (this._hasAggregation(requestMapConfig)){ shouldAdapt = true; @@ -50,10 +101,10 @@ module.exports = class AggregationMapConfigAdapter { return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); } - _adaptLayers (requestMapConfig) { + _adaptLayers (mapConfig, requestMapConfig) { return requestMapConfig.layers.map(layer => { if (this._hasLayerAggregation(layer)) { - const aggregation = new AggregationProxy(this.mapConfig, layer.options.aggregation); + const aggregation = new AggregationProxy(mapConfig, layer.options.aggregation); const sqlQueryWrap = layer.options.sql_wrap; let aggregationSql = aggregation.sql(layer.options); diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index eeee18f0..3638ff0b 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -3,6 +3,7 @@ require('../support/test_helper'); const assert = require('../support/assert'); const TestClient = require('../support/test-client'); const serverOptions = require('../../lib/cartodb/server_options'); +const MISSING_AGGREGATION_COLUMNS = 'There are missing columns to perform aggregation'; const suites = [{ desc: 'mvt (mapnik)', @@ -30,7 +31,8 @@ describe('aggregation', function () { select st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326) as the_geom, st_transform(st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326), 3857) as the_geom_webmercator, - x as value + x as value, + x*x as sqrt_value from generate_series(-3, 3) x `; @@ -70,17 +72,14 @@ describe('aggregation', function () { serverOptions.renderer.mvt.usePostGIS = originalUsePostGIS; }); - - beforeEach(function () { - this.mapConfig = createVectorMapConfig(); - this.testClient = new TestClient(this.mapConfig); - }); - afterEach(function (done) { this.testClient.drain(done); }); - it('should return a layergroup indicating that was aggregated', function (done) { + it('should return a layergroup indicating the mapconfig was aggregated', function (done) { + this.mapConfig = createVectorMapConfig(); + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup((err, body) => { if (err) { return done(err); @@ -94,6 +93,111 @@ describe('aggregation', function () { done(); }); }); + + it('should return a layergroup with aggregation and cartocss compatible', function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + columns: { + total: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + } + }, + cartocss: '#layer { marker-width: [value]*2; }', + cartocss_version: '2.3.0' + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup((err/*, body */) => { + if (err) { + return done(err); + } + + done(); + }); + }); + + it('should fail if cartocss uses "value" cloumn and it\'s not defined in the aggregation', + function (done) { + const response = { + status: 400, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: true, + cartocss: '#layer { marker-width: [value]; }', + cartocss_version: '2.3.0' + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup(response, (err, body) => { + if (err) { + return done(err); + } + + assert.equal(body.errors[0], MISSING_AGGREGATION_COLUMNS); + + done(); + }); + }); + + it('should fail if aggregation misses a column defined in interactivity', + function (done) { + const response = { + status: 400, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + columns: { + total: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + } + }, + cartocss: '#layer { marker-width: [value]; }', + cartocss_version: '2.3.0', + interactivity: ['sqrt_value'] + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup(response, (err, body) => { + if (err) { + return done(err); + } + + assert.equal(body.errors[0], MISSING_AGGREGATION_COLUMNS); + + done(); + }); + }); + }); }); });