From f0c82f21d23e874fb21687ec9d015b69525bc65c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Mar 2019 17:01:13 +0100 Subject: [PATCH 1/8] Reduce complexity by extracting a complex condition to a function --- lib/cartodb/backends/cluster.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index c9776eba..31a2c7e9 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -7,7 +7,7 @@ const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfi module.exports = class ClusterBackend { // TODO: reduce complexity - // jshint maxcomplexity: 17 + // jshint maxcomplexity: 16 getClusterFeatures (mapConfigProvider, params, callback) { mapConfigProvider.getMapConfig((err, _mapConfig) => { if (err) { @@ -26,7 +26,7 @@ module.exports = class ClusterBackend { const layer = mapConfig.getLayer(layerIndex); - if (layer.options.aggregation === false || !mapConfig.isAggregationLayer(layerIndex)) { + if (!hasAggregationLayer(mapConfig, layerIndex)) { const error = new Error(`Map ${token} has no aggregation defined for layer ${layerIndex}`); error.http_status = 400; error.type = 'layer'; @@ -37,6 +37,7 @@ module.exports = class ClusterBackend { }; debug(error); + return callback(error); } @@ -249,3 +250,14 @@ const aggregationQuery = ctx => ` FROM (${ctx.query}) __cdb_aggregation ${ctx.columns.length ? `GROUP BY ${ctx.columns.join(', ')}` : ''} `; + +// TODO: update when https://github.com/CartoDB/Windshaft-cartodb/pull/1082 is merged +function hasAggregationLayer (mapConfig, layerIndex) { + const layer = mapConfig.getLayer(layerIndex); + + if (typeof layer.options.aggregation === 'boolean') { + return layer.options.aggregation; + } + + return mapConfig.isAggregationLayer(layerIndex); +} From fecedfdc68b19e132522b4d72842a0f7522741be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Mar 2019 17:06:04 +0100 Subject: [PATCH 2/8] Reduce complexity by extracting validation to a function --- lib/cartodb/backends/cluster.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 31a2c7e9..8ad2343c 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -7,7 +7,7 @@ const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfi module.exports = class ClusterBackend { // TODO: reduce complexity - // jshint maxcomplexity: 16 + // jshint maxcomplexity: 15 getClusterFeatures (mapConfigProvider, params, callback) { mapConfigProvider.getMapConfig((err, _mapConfig) => { if (err) { @@ -61,7 +61,7 @@ module.exports = class ClusterBackend { const { columns, expressions } = aggregation; - if (!Array.isArray(columns) || !columns.length) { + if (!hasColumns(columns)) { const error = new Error( `Invalid aggregation input, columns should be and array of column names` ); @@ -261,3 +261,7 @@ function hasAggregationLayer (mapConfig, layerIndex) { return mapConfig.isAggregationLayer(layerIndex); } + +function hasColumns (columns) { + return Array.isArray(columns) && columns.length; +} From 8051dc5110074d399b31331a4882b33bff2ff0a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Mar 2019 17:14:07 +0100 Subject: [PATCH 3/8] Reduce complexity by extracting validation condition to its function --- lib/cartodb/backends/cluster.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 8ad2343c..e305eb28 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -7,7 +7,7 @@ const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfi module.exports = class ClusterBackend { // TODO: reduce complexity - // jshint maxcomplexity: 15 + // jshint maxcomplexity: 13 getClusterFeatures (mapConfigProvider, params, callback) { mapConfigProvider.getMapConfig((err, _mapConfig) => { if (err) { @@ -77,9 +77,7 @@ module.exports = class ClusterBackend { } if (expressions !== undefined) { - if (expressions === null || - Array.isArray(expressions) || - ['string', 'number', 'boolean'].includes(typeof expressions)) { + if (!isValidExpression(expressions)) { const error = new Error( `Invalid aggregation input, expressions should be and object with valid functions` ); @@ -265,3 +263,9 @@ function hasAggregationLayer (mapConfig, layerIndex) { function hasColumns (columns) { return Array.isArray(columns) && columns.length; } + +function isValidExpression (expressions) { + const invalidTypes = ['string', 'number', 'boolean']; + + return expressions !== null && !Array.isArray(expressions) && !invalidTypes.includes(typeof expressions); +} From 49104a6add53be5b39d9a93ea78267b618f300f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Mar 2019 17:25:29 +0100 Subject: [PATCH 4/8] Reduce complexity by extracting function to validate expressions --- lib/cartodb/backends/cluster.js | 57 ++++++++++++--------------------- 1 file changed, 21 insertions(+), 36 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index e305eb28..f858b27a 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -7,7 +7,7 @@ const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfi module.exports = class ClusterBackend { // TODO: reduce complexity - // jshint maxcomplexity: 13 + // jshint maxcomplexity: 10 getClusterFeatures (mapConfigProvider, params, callback) { mapConfigProvider.getMapConfig((err, _mapConfig) => { if (err) { @@ -43,7 +43,7 @@ module.exports = class ClusterBackend { let { aggregation } = params; - if ( aggregation !== undefined) { + if (aggregation !== undefined) { try { aggregation = JSON.parse(aggregation); } catch (err) { @@ -77,10 +77,9 @@ module.exports = class ClusterBackend { } if (expressions !== undefined) { - if (!isValidExpression(expressions)) { - const error = new Error( - `Invalid aggregation input, expressions should be and object with valid functions` - ); + try { + validateExpressions(expressions); + } catch (error) { error.http_status = 400; error.type = 'layer'; error.subtype = 'aggregation'; @@ -91,36 +90,6 @@ module.exports = class ClusterBackend { return callback(error); } - - for (const { aggregate_function, aggregated_column } of Object.values(expressions)) { - if (typeof aggregated_column !== 'string') { - const error = new Error(`Invalid aggregation input, aggregated column should be an string`); - error.http_status = 400; - error.type = 'layer'; - error.subtype = 'aggregation'; - error.layer = { - index: layerIndex, - type: layer.type - }; - - return callback(error); - } - - if (typeof aggregate_function !== 'string') { - const error = new Error( - `Invalid aggregation input, aggregate function should be an string` - ); - error.http_status = 400; - error.type = 'layer'; - error.subtype = 'aggregation'; - error.layer = { - index: layerIndex, - type: layer.type - }; - - return callback(error); - } - } } } @@ -264,6 +233,22 @@ function hasColumns (columns) { return Array.isArray(columns) && columns.length; } +function validateExpressions (expressions) { + if (!isValidExpression(expressions)) { + throw new Error(`Invalid aggregation input, expressions should be and object with valid functions`); + } + + for (const { aggregate_function, aggregated_column } of Object.values(expressions)) { + if (typeof aggregated_column !== 'string') { + throw new Error(`Invalid aggregation input, aggregated column should be an string`); + } + + if (typeof aggregate_function !== 'string') { + throw new Error(`Invalid aggregation input, aggregate function should be an string`); + } + } +} + function isValidExpression (expressions) { const invalidTypes = ['string', 'number', 'boolean']; From 589996b79c679049ca1a7be44ec32271d1c4ea8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Mar 2019 17:53:34 +0100 Subject: [PATCH 5/8] Reduce complexity --- lib/cartodb/backends/cluster.js | 106 +++++++++++++++----------------- 1 file changed, 48 insertions(+), 58 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index f858b27a..113dde65 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -6,8 +6,6 @@ const debug = require('debug')('backend:cluster'); const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfig'); module.exports = class ClusterBackend { - // TODO: reduce complexity - // jshint maxcomplexity: 10 getClusterFeatures (mapConfigProvider, params, callback) { mapConfigProvider.getMapConfig((err, _mapConfig) => { if (err) { @@ -43,54 +41,19 @@ module.exports = class ClusterBackend { let { aggregation } = params; - if (aggregation !== undefined) { - try { - aggregation = JSON.parse(aggregation); - } catch (err) { - const error = new Error(`Invalid aggregation input, should be a a valid JSON`); - error.http_status = 400; - error.type = 'layer'; - error.subtype = 'aggregation'; - error.layer = { - index: layerIndex, - type: layer.type - }; + try { + aggregation = parseAggregation(aggregation); + validateAggregation(aggregation); + } catch (error) { + error.http_status = 400; + error.type = 'layer'; + error.subtype = 'aggregation'; + error.layer = { + index: layerIndex, + type: layer.type + }; - return callback(error); - } - - const { columns, expressions } = aggregation; - - if (!hasColumns(columns)) { - const error = new Error( - `Invalid aggregation input, columns should be and array of column names` - ); - error.http_status = 400; - error.type = 'layer'; - error.subtype = 'aggregation'; - error.layer = { - index: layerIndex, - type: layer.type - }; - - return callback(error); - } - - if (expressions !== undefined) { - try { - validateExpressions(expressions); - } catch (error) { - error.http_status = 400; - error.type = 'layer'; - error.subtype = 'aggregation'; - error.layer = { - index: layerIndex, - type: layer.type - }; - - return callback(error); - } - } + return callback(error); } const query = layer.options.sql_raw; @@ -229,22 +192,49 @@ function hasAggregationLayer (mapConfig, layerIndex) { return mapConfig.isAggregationLayer(layerIndex); } +function parseAggregation (aggregation) { + if (aggregation !== undefined) { + try { + aggregation = JSON.parse(aggregation); + } catch (err) { + throw new Error(`Invalid aggregation input, should be a a valid JSON`); + } + + } + + return aggregation; +} + +function validateAggregation (aggregation) { + if (aggregation !== undefined) { + const { columns, expressions } = aggregation; + + if (!hasColumns(columns)) { + throw new Error(`Invalid aggregation input, columns should be and array of column names`); + } + + validateExpressions(expressions); + } +} + function hasColumns (columns) { return Array.isArray(columns) && columns.length; } function validateExpressions (expressions) { - if (!isValidExpression(expressions)) { - throw new Error(`Invalid aggregation input, expressions should be and object with valid functions`); - } - - for (const { aggregate_function, aggregated_column } of Object.values(expressions)) { - if (typeof aggregated_column !== 'string') { - throw new Error(`Invalid aggregation input, aggregated column should be an string`); + if (expressions !== undefined) { + if (!isValidExpression(expressions)) { + throw new Error(`Invalid aggregation input, expressions should be and object with valid functions`); } - if (typeof aggregate_function !== 'string') { - throw new Error(`Invalid aggregation input, aggregate function should be an string`); + for (const { aggregate_function, aggregated_column } of Object.values(expressions)) { + if (typeof aggregated_column !== 'string') { + throw new Error(`Invalid aggregation input, aggregated column should be an string`); + } + + if (typeof aggregate_function !== 'string') { + throw new Error(`Invalid aggregation input, aggregate function should be an string`); + } } } } From 0aa3b288a03d962370a0221370f9cdb835ab4147 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Mar 2019 18:04:47 +0100 Subject: [PATCH 6/8] Improve validation --- lib/cartodb/backends/cluster.js | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 113dde65..ba998c5d 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -19,29 +19,13 @@ module.exports = class ClusterBackend { return callback(error); } - const { user, token, layer: layerIndex } = params; + const { user, layer: layerIndex } = params; const mapConfig = new AggregationMapConfig(user, _mapConfig.obj(), pg); - const layer = mapConfig.getLayer(layerIndex); - - if (!hasAggregationLayer(mapConfig, layerIndex)) { - const error = new Error(`Map ${token} has no aggregation defined for layer ${layerIndex}`); - error.http_status = 400; - error.type = 'layer'; - error.subtype = 'aggregation'; - error.layer = { - index: layerIndex, - type: layer.type - }; - - debug(error); - - return callback(error); - } - let { aggregation } = params; try { + validateAggregationLayer(mapConfig, layerIndex); aggregation = parseAggregation(aggregation); validateAggregation(aggregation); } catch (error) { @@ -53,6 +37,8 @@ module.exports = class ClusterBackend { type: layer.type }; + debug(error); + return callback(error); } @@ -181,6 +167,12 @@ const aggregationQuery = ctx => ` ${ctx.columns.length ? `GROUP BY ${ctx.columns.join(', ')}` : ''} `; +function validateAggregationLayer (mapConfig, layerIndex) { + if (!hasAggregationLayer(mapConfig, layerIndex)) { + throw new Error(`Map ${mapConfig.id()} has no aggregation defined for layer ${layerIndex}`); + } +} + // TODO: update when https://github.com/CartoDB/Windshaft-cartodb/pull/1082 is merged function hasAggregationLayer (mapConfig, layerIndex) { const layer = mapConfig.getLayer(layerIndex); From d86b01ba330be36c1a91e57319bb5096863ad516 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Mar 2019 18:09:41 +0100 Subject: [PATCH 7/8] Add debug statement --- lib/cartodb/backends/cluster.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index ba998c5d..87b3b27d 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -16,6 +16,8 @@ module.exports = class ClusterBackend { try { pg = new PSQL(dbParamsFromReqParams(params)); } catch (error) { + debug(error); + return callback(error); } From 367ca399c8a507695ccdc065ed94bb60b416e13d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Mar 2019 18:53:47 +0100 Subject: [PATCH 8/8] Improve readability --- lib/cartodb/backends/cluster.js | 39 +++++++++++++++++---------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 87b3b27d..ab663997 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -24,6 +24,7 @@ module.exports = class ClusterBackend { const { user, layer: layerIndex } = params; const mapConfig = new AggregationMapConfig(user, _mapConfig.obj(), pg); const layer = mapConfig.getLayer(layerIndex); + let { aggregation } = params; try { @@ -44,35 +45,35 @@ module.exports = class ClusterBackend { return callback(error); } - const query = layer.options.sql_raw; - const resolution = layer.options.aggregation.resolution || 1; + params.aggregation = aggregation; - getColumnsName(pg, query, (err, columns) => { - if (err) { - return callback(err); - } - - const { zoom, clusterId } = params; - - getClusterFeatures(pg, zoom, clusterId, columns, query, resolution, aggregation, (err, features) => { - if (err) { - return callback(err); - } - - return callback(null, features); - }); - }); + getFeatures(pg, layer, params, callback); }); } }; +function getFeatures (pg, layer, params, callback) { + const query = layer.options.sql_raw; + const resolution = layer.options.aggregation.resolution || 1; + + getColumnsName(pg, query, (err, columns) => { + if (err) { + return callback(err); + } + + const { zoom, clusterId, aggregation } = params; + + getClusterFeatures(pg, zoom, clusterId, columns, query, resolution, aggregation, callback); + }); +} + const SKIP_COLUMNS = { 'the_geom': true, 'the_geom_webmercator': true }; function getColumnsName (pg, query, callback) { - const sql = limitedQuery({ + const sql = schemaQuery({ query: query }); @@ -126,7 +127,7 @@ function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, ag } , true); // use read-only transaction } -const limitedQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_schema LIMIT 0`; +const schemaQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_schema LIMIT 0`; const clusterFeaturesQuery = ctx => ` WITH _cdb_params AS (