From acecb88efb10bb2fc3adf7596f0d49fdff841514 Mon Sep 17 00:00:00 2001 From: Esther Lozano Date: Mon, 11 Nov 2019 18:14:30 +0100 Subject: [PATCH 1/6] Validate aggregation method is either sum or count --- lib/models/dataview/overviews/aggregation.js | 22 +++ test/acceptance/dataviews/overviews-test.js | 188 +++++++++++++++++++ 2 files changed, 210 insertions(+) diff --git a/lib/models/dataview/overviews/aggregation.js b/lib/models/dataview/overviews/aggregation.js index 1d0f7de6..bc0b9116 100644 --- a/lib/models/dataview/overviews/aggregation.js +++ b/lib/models/dataview/overviews/aggregation.js @@ -94,6 +94,8 @@ var CATEGORIES_LIMIT = 6; function Aggregation(query, options, queryRewriter, queryRewriteData, params, queries) { BaseOverviewsDataview.call(this, query, options, BaseDataview, queryRewriter, queryRewriteData, params, queries); + this._checkOptions(options); + this.query = query; this.queries = queries; this.column = options.column; @@ -219,6 +221,26 @@ var aggregationFnQueryTpl = { sum: dot.template('sum({{=it._aggregationColumn}}*_feature_count)') }; +const VALID_OPERATIONS = { + count: [], + sum: ['aggregationColumn'] +}; + +Aggregation.prototype._checkOptions = function (options) { + if (!VALID_OPERATIONS[options.aggregation]) { + throw new Error(`Aggregation does not support '${options.aggregation}' operation in dataview overview options`); + } + + const requiredOptions = VALID_OPERATIONS[options.aggregation]; + const missingOptions = requiredOptions.filter(requiredOption => !options.hasOwnProperty(requiredOption)); + + if (missingOptions.length > 0) { + throw new Error( + `Aggregation '${options.aggregation}' is missing some options for overview: ${missingOptions.join(',')}` + ); + } +}; + Aggregation.prototype.getAggregationSql = function() { return aggregationFnQueryTpl[this.aggregation]({ _aggregationFn: this.aggregation, diff --git a/test/acceptance/dataviews/overviews-test.js b/test/acceptance/dataviews/overviews-test.js index 8df298b3..ea688882 100644 --- a/test/acceptance/dataviews/overviews-test.js +++ b/test/acceptance/dataviews/overviews-test.js @@ -665,6 +665,194 @@ describe('dataviews using tables with overviews', function() { }); }); }); + + describe.only('agreggation validation', function (){ + const params = { + response: { + status: 400, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + } + }; + + function createMapConfig(options) { + return { + version: '1.5.0', + analyses: [ + { id: 'data-source', + type: 'source', + params: { + query: 'select * from test_table_overviews' + } + }, + { + id: 'data-source-special-float-values', + type: 'source', + params: { + query: 'select * from test_special_float_values_table_overviews' + } + } + ], + dataviews: { + test_invalid_aggregation: { + type: 'aggregation', + source: {id: 'data-source'}, + options: options + } + }, + layers: [ + { + type: 'mapnik', + options: { + sql: 'select * from test_table_overviews', + cartocss: '#layer { marker-fill: red; marker-width: 32; marker-allow-overlap: true; }', + cartocss_version: '2.3.0', + source: { id: 'data-source' } + } + }, + { + type: 'mapnik', + options: { + sql: 'select * from test_special_float_values_table_overviews', + cartocss: '#layer { marker-fill: red; marker-width: 32; marker-allow-overlap: true; }', + cartocss_version: '2.3.0', + source: { + id: 'data-source-special-float-values' + } + } + } + ] + }; + } + + it('should fail if missing column', function (done) { + var options = { + aggregation: "sum", + aggregationColumn: "value" + }; + var missingCOlumnMapConfig = createMapConfig(options); + + var testClient = new TestClient(missingCOlumnMapConfig); + testClient.getDataview('test_invalid_aggregation', params, function (err, dataview) { + if (err) { + return done(err); + } + + assert.deepStrictEqual(dataview, { + errors: ["Aggregation expects 'column' in dataview options"], + errors_with_context: [{ + type: 'unknown', + message: "Aggregation expects 'column' in dataview options" + }] + }); + + testClient.drain(done); + }); + }); + + it('should fail if no aggregation operation', function (done) { + var options = { + column: "value", + aggregationColumn: "value" + }; + var missingOperationMapConfig = createMapConfig(options); + + var testClient = new TestClient(missingOperationMapConfig); + testClient.getDataview('test_invalid_aggregation', params, function (err, dataview) { + if (err) { + return done(err); + } + + assert.deepStrictEqual(dataview, { + errors: ["Aggregation expects 'aggregation' operation in dataview options"], + errors_with_context: [{ + type: 'unknown', + message: "Aggregation expects 'aggregation' operation in dataview options" + }] + }); + + testClient.drain(done); + }); + }); + + it('should fail if fake operation', function (done) { + var options = { + column: "value", + aggregation: "wadus", + aggregationColumn: "value" + }; + var wrongOperationMapConfig = createMapConfig(options); + + var testClient = new TestClient(wrongOperationMapConfig); + testClient.getDataview('test_invalid_aggregation', params, function (err, dataview) { + if (err) { + return done(err); + } + + assert.deepStrictEqual(dataview, { + errors: ["Aggregation does not support 'wadus' operation"], + errors_with_context: [{ + type: 'unknown', + message: "Aggregation does not support 'wadus' operation" + }] + }); + + testClient.drain(done); + }); + }); + + it('should fail if invalid operation for overview', function (done) { + var options = { + column: "value", + aggregation: "avg", + aggregationColumn: "value" + }; + var wrongOperationMapConfig = createMapConfig(options); + + var testClient = new TestClient(wrongOperationMapConfig); + testClient.getDataview('test_invalid_aggregation', params, function (err, dataview) { + if (err) { + return done(err); + } + + assert.deepStrictEqual(dataview, { + errors: ["Aggregation does not support 'avg' operation in dataview overview options"], + errors_with_context: [{ + type: 'unknown', + message: "Aggregation does not support 'avg' operation in dataview overview options" + }] + }); + + testClient.drain(done); + }); + }); + + it('should fail if no aggregation column when needed', function (done) { + var options = { + column: "value", + aggregation: "sum" + }; + var missingOptionMapConfig = createMapConfig(options); + + var testClient = new TestClient(missingOptionMapConfig); + testClient.getDataview('test_invalid_aggregation', params, function (err, dataview) { + if (err) { + return done(err); + } + + assert.deepStrictEqual(dataview, { + errors: ["Aggregation 'sum' is missing some options: aggregationColumn"], + errors_with_context: [{ + type: 'unknown', + message: "Aggregation 'sum' is missing some options: aggregationColumn" + }] + }); + + testClient.drain(done); + }); + }); + }); }); }); From 2b5ed2120734a51969b847364a6b4b378eeffd04 Mon Sep 17 00:00:00 2001 From: Esther Lozano Date: Tue, 12 Nov 2019 12:37:24 +0100 Subject: [PATCH 2/6] Remove only in tests :P --- test/acceptance/dataviews/overviews-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/dataviews/overviews-test.js b/test/acceptance/dataviews/overviews-test.js index ea688882..0bcf4049 100644 --- a/test/acceptance/dataviews/overviews-test.js +++ b/test/acceptance/dataviews/overviews-test.js @@ -666,7 +666,7 @@ describe('dataviews using tables with overviews', function() { }); }); - describe.only('agreggation validation', function (){ + describe('agreggation validation', function (){ const params = { response: { status: 400, From b05740048c413fb6b9243998a7b1fa12b8f10358 Mon Sep 17 00:00:00 2001 From: Esther Lozano Date: Tue, 12 Nov 2019 12:48:22 +0100 Subject: [PATCH 3/6] Update NEWS.md --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 058c65bc..b8980a70 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,7 +7,7 @@ Breaking changes: - Schema change for "routes" in configuration file, each "router" is now an array instead of an object. See [`dd06de2`](https://github.com/CartoDB/Windshaft-cartodb/pull/1126/commits/dd06de2632661e19d64c9fbc2be0ba1a8059f54c) for more details. Announcements: - +- Added validation to only allow "count" and "sum" aggregations in dataview overview. - Added mechanism to inject custom middlewares through configuration. - Stop requiring unused config properties: "base_url", "base_url_mapconfig", and "base_url_templated". From bb745b0318caf2e54a35e8f4b4c574c22a553b52 Mon Sep 17 00:00:00 2001 From: Esther Lozano Date: Tue, 12 Nov 2019 14:48:37 +0100 Subject: [PATCH 4/6] Update test/acceptance/dataviews/overviews-test.js Co-Authored-By: Daniel G. Aubert --- test/acceptance/dataviews/overviews-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/dataviews/overviews-test.js b/test/acceptance/dataviews/overviews-test.js index 0bcf4049..64a071f9 100644 --- a/test/acceptance/dataviews/overviews-test.js +++ b/test/acceptance/dataviews/overviews-test.js @@ -731,7 +731,7 @@ describe('dataviews using tables with overviews', function() { aggregation: "sum", aggregationColumn: "value" }; - var missingCOlumnMapConfig = createMapConfig(options); + var missingColumnMapConfig = createMapConfig(options); var testClient = new TestClient(missingCOlumnMapConfig); testClient.getDataview('test_invalid_aggregation', params, function (err, dataview) { From 75583f67c54e8cbd96d0366f5da3cae32f8c28a3 Mon Sep 17 00:00:00 2001 From: Esther Lozano Date: Tue, 12 Nov 2019 14:54:25 +0100 Subject: [PATCH 5/6] Use last version for map config in tests --- test/acceptance/dataviews/overviews-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/acceptance/dataviews/overviews-test.js b/test/acceptance/dataviews/overviews-test.js index 64a071f9..c75433e1 100644 --- a/test/acceptance/dataviews/overviews-test.js +++ b/test/acceptance/dataviews/overviews-test.js @@ -678,7 +678,7 @@ describe('dataviews using tables with overviews', function() { function createMapConfig(options) { return { - version: '1.5.0', + version: '1.8.0', analyses: [ { id: 'data-source', type: 'source', @@ -733,7 +733,7 @@ describe('dataviews using tables with overviews', function() { }; var missingColumnMapConfig = createMapConfig(options); - var testClient = new TestClient(missingCOlumnMapConfig); + var testClient = new TestClient(missingColumnMapConfig); testClient.getDataview('test_invalid_aggregation', params, function (err, dataview) { if (err) { return done(err); From f17411916ffd0905bc7fbf812558e5e4beece7fa Mon Sep 17 00:00:00 2001 From: Esther Lozano Date: Tue, 12 Nov 2019 17:43:03 +0100 Subject: [PATCH 6/6] Remove unnecessary config in tests --- test/acceptance/dataviews/overviews-test.js | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/test/acceptance/dataviews/overviews-test.js b/test/acceptance/dataviews/overviews-test.js index c75433e1..aa97a28f 100644 --- a/test/acceptance/dataviews/overviews-test.js +++ b/test/acceptance/dataviews/overviews-test.js @@ -685,13 +685,6 @@ describe('dataviews using tables with overviews', function() { params: { query: 'select * from test_table_overviews' } - }, - { - id: 'data-source-special-float-values', - type: 'source', - params: { - query: 'select * from test_special_float_values_table_overviews' - } } ], dataviews: { @@ -710,17 +703,6 @@ describe('dataviews using tables with overviews', function() { cartocss_version: '2.3.0', source: { id: 'data-source' } } - }, - { - type: 'mapnik', - options: { - sql: 'select * from test_special_float_values_table_overviews', - cartocss: '#layer { marker-fill: red; marker-width: 32; marker-allow-overlap: true; }', - cartocss_version: '2.3.0', - source: { - id: 'data-source-special-float-values' - } - } } ] };