From f9e5d9d0a976771ca448aaae78e182a4b4289d6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 17:02:06 +0100 Subject: [PATCH] Validate aggregation expresions --- lib/cartodb/backends/cluster.js | 33 +++++++- test/acceptance/cluster.js | 142 ++++++++++++++++++++++++++++---- 2 files changed, 160 insertions(+), 15 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index c9108da9..650b42ce 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -90,6 +90,36 @@ module.exports = class ClusterBackend { return callback(error); } + + for (const [columnName, exp] of Object.entries(expressions)) { + const { aggregate_function, aggregated_column } = exp; + + 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); + } + } } } @@ -153,7 +183,8 @@ function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, ag let { columns = [], expressions = [] } = aggregation; if (expressions) { - expressions = Object.entries(expressions).map(entries =>`${entries[1].aggregated_function}(${entries[1].aggregated_column}) AS ${entries[0]}`); + expressions = Object.entries(expressions) + .map(([columnName, exp]) => `${exp.aggregate_function}(${exp.aggregated_column}) AS ${columnName}`); } sql = aggregationQuery({ diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index 881dbf8e..87ff63f5 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -403,7 +403,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -418,7 +418,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -433,7 +433,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -448,7 +448,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -463,7 +463,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -478,7 +478,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -493,7 +493,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -508,7 +508,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -526,7 +526,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -567,7 +567,7 @@ describe('cluster', function () { }); }); - describe('invalid aggregation', function () { + describe.only('invalid aggregation', function () { const expectedColumnsError = { errors:[ 'Invalid aggregation input, columns should be and array of column names' ], errors_with_context:[ @@ -598,6 +598,57 @@ describe('cluster', function () { ] }; + const invalidFunctionExpressionsError = { + errors:[ 'function wadus(integer) does not exist' ], + errors_with_context:[ + { + message: 'function wadus(integer) does not exist', + type: 'unknown' + } + ] + }; + + const invalidColumnExpressionsError = { + errors:[ 'column \"wadus\" does not exist' ], + errors_with_context:[ + { + message: 'column \"wadus\" does not exist', + type: 'unknown' + } + ] + }; + + + const expectedAggregatedColumnError = { + errors:[ 'Invalid aggregation input, aggregated column should be an string' ], + errors_with_context:[ + { + layer: { + index: '0', + type: 'cartodb' + }, + message: 'Invalid aggregation input, aggregated column should be an string', + subtype: 'aggregation', + type: 'layer' + } + ] + }; + + const expectedAggregateFunctionError = { + errors:[ 'Invalid aggregation input, aggregate function should be an string' ], + errors_with_context:[ + { + layer: { + index: '0', + type: 'cartodb' + }, + message: 'Invalid aggregation input, aggregate function should be an string', + subtype: 'aggregation', + type: 'layer' + } + ] + }; + const suite = [ { description: 'empty aggregation object should respond with error', @@ -686,10 +737,75 @@ describe('cluster', function () { resolution: 1, aggregation: { columns: [ 'type' ], expressions: null }, expected: expectedExpressionsError + }, + { + description: 'invalid aggregation function should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregate_function: 'wadus', + aggregated_column: 'value' + } + } + }, + expected: invalidFunctionExpressionsError + }, + { + description: 'invalid aggregation column should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregate_function: 'max', + aggregated_column: 'wadus' + } + } + }, + status: 404, + expected: invalidColumnExpressionsError + }, + { + description: 'aggregated column as non string should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregate_function: 'max', + aggregated_column: 1 + } + } + }, + expected: expectedAggregatedColumnError + }, + { + description: 'aggregate function as non string should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregate_function: 1, + aggregated_column: 'value' + } + } + }, + expected: expectedAggregateFunctionError } ]; - suite.forEach(({ description, zoom, cartodb_id, resolution, aggregation, expected }) => { + suite.forEach(({ description, zoom, cartodb_id, resolution, aggregation, expected, status = 400 }) => { it(description, function (done) { const mapConfig = createVectorMapConfig([{ type: 'cartodb', @@ -704,9 +820,7 @@ describe('cluster', function () { const testClient = new TestClient(mapConfig); const layerId = 0; const params = { - response: { - status: 400 - }, + response: { status }, aggregation };