Merge pull request #1139 from CartoDB/fix/bad-aggregation-method-overview

Validate aggregation method is either sum or count

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".
 - Upgraded cartodb-query-tables to version [0.7.0](https://github.com/CartoDB/node-cartodb-query-tables/blob/0.7.0/NEWS.md#version-0.7.0).

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,

test/acceptance/dataviews/overviews-test.js

@@ -665,6 +665,176 @@ describe('dataviews using tables with overviews', function() {
                 });
             });
         });
+
+        describe('agreggation validation', function (){
+            const params = {
+                response: {
+                    status: 400,
+                    headers: {
+                        'Content-Type': 'application/json; charset=utf-8'
+                    }
+                }
+            };
+
+            function createMapConfig(options) {
+                return {
+                    version: '1.8.0',
+                    analyses: [
+                        { id: 'data-source',
+                            type: 'source',
+                            params: {
+                                query: 'select * from test_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' }
+                            }
+                        }
+                    ]
+                };
+            }
+
+            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);
+                });
+            });
+        });
     });
 });