Release 2.87.5

Cast all dataview overrides values to Number

CONTRIBUTING.md

@@ -8,4 +8,4 @@ We love pull requests from everyone, see [Contributing to Open Source on GitHub]
 
 ## Submitting Contributions
 
-* You will need to sign a Contributor License Agreement (CLA) before making a submission. [Learn more here](https://cartodb.com/contributing).
+* You will need to sign a Contributor License Agreement (CLA) before making a submission. [Learn more here](https://carto.com/contributions).

NEWS.md

@@ -1,5 +1,19 @@
 # Changelog
 
+## 2.87.5
+Released 2017-02-02
+
+Bug fixes:
+ - Cast dataview override values to Number or throw error.
+
+
+## 2.87.4
+Released 2017-01-20
+
+Bug fixes:
+ - Be able to not compute NULL categories and null values  in aggregation dataviews #617.
+
+
 ## 2.87.3
 Released 2016-12-19
 

docs/static_maps_api.md

@@ -146,9 +146,9 @@ It is important to note that generated images are cached from the live data refe
 
 ### Limits
 
-* While images can encompass an entirety of a map, the default limit for pixel range is 8192 x 8192.
-* Image resolution by default is set to 72 DPI
-* JPEG quality by default is 85%
+* While images can encompass an entirety of a map, the limit for pixel range is 8192 x 8192.
+* Image resolution is set to 72 DPI
+* JPEG quality is 85%
 * Timeout limits for generating static maps are the same across CARTO Builder and CARTO Engine. It is important to ensure timely processing of queries.
 
 ## Examples

lib/cartodb/backends/dataview.js

@@ -79,7 +79,10 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param
 
             var overrideParams = _.reduce(_.pick(params, 'start', 'end', 'bins'),
                 function castNumbers(overrides, val, k) {
-                    overrides[k] = Number.isFinite(+val) ? +val : val;
+                    if (!Number.isFinite(+val)) {
+                        throw new Error('Invalid number format for parameter \'' + k + '\'');
+                    }
+                    overrides[k] = +val;
                     return overrides;
                 },
                 {ownFilter: ownFilter}

lib/cartodb/models/dataview/aggregation.js

@@ -19,25 +19,37 @@ var rankedCategoriesQueryTpl = dot.template([
     '  SELECT {{=it._column}} AS category, {{=it._aggregation}} AS value,',
     '    row_number() OVER (ORDER BY {{=it._aggregation}} desc) as rank',
     '  FROM ({{=it._query}}) _cdb_aggregation_all',
+    '  {{?it._aggregationColumn!==null}}WHERE {{=it._aggregationColumn}} IS NOT NULL{{?}}',
     '  GROUP BY {{=it._column}}',
     '  ORDER BY 2 DESC',
     ')'
 ].join('\n'));
 
-var categoriesSummaryQueryTpl = dot.template([
-    'categories_summary AS(',
-    '  SELECT count(1) categories_count, max(value) max_val, min(value) min_val',
+var categoriesSummaryMinMaxQueryTpl = dot.template([
+    'categories_summary_min_max AS(',
+    '  SELECT max(value) max_val, min(value) min_val',
     '  FROM categories',
     ')'
 ].join('\n'));
 
+var categoriesSummaryCountQueryTpl = dot.template([
+    'categories_summary_count AS(',
+    '  SELECT count(1) AS categories_count',
+    '  FROM (',
+    '    SELECT {{=it._column}} AS category',
+    '    FROM ({{=it._query}}) _cdb_categories',
+    '    GROUP BY {{=it._column}}',
+    '  ) _cdb_categories_count',
+    ')'
+].join('\n'));
+
 var rankedAggregationQueryTpl = dot.template([
     'SELECT CAST(category AS text), value, false as agg, nulls_count, min_val, max_val, count, categories_count',
-    '  FROM categories, summary, categories_summary',
+    '  FROM categories, summary, categories_summary_min_max, categories_summary_count',
     '  WHERE rank < {{=it._limit}}',
     'UNION ALL',
     'SELECT \'Other\' category, sum(value), true as agg, nulls_count, min_val, max_val, count, categories_count',
-    '  FROM categories, summary, categories_summary',
+    '  FROM categories, summary, categories_summary_min_max, categories_summary_count',
     '  WHERE rank >= {{=it._limit}}',
     'GROUP BY nulls_count, min_val, max_val, count, categories_count'
 ].join('\n'));
@@ -45,7 +57,7 @@ var rankedAggregationQueryTpl = dot.template([
 var aggregationQueryTpl = dot.template([
     'SELECT CAST({{=it._column}} AS text) AS category, {{=it._aggregation}} AS value, false as agg,',
     '  nulls_count, min_val, max_val, count, categories_count',
-    'FROM ({{=it._query}}) _cdb_aggregation_all, summary, categories_summary',
+    'FROM ({{=it._query}}) _cdb_aggregation_all, summary, categories_summary_min_max, categories_summary_count',
     'GROUP BY category, nulls_count, min_val, max_val, count, categories_count',
     'ORDER BY value DESC'
 ].join('\n'));
@@ -114,6 +126,7 @@ Aggregation.prototype.sql = function(psql, override, callback) {
     var _query = this.query;
 
     var aggregationSql;
+
     if (!!override.ownFilter) {
         aggregationSql = [
             "WITH",
@@ -125,9 +138,14 @@ Aggregation.prototype.sql = function(psql, override, callback) {
                 rankedCategoriesQueryTpl({
                     _query: _query,
                     _column: this.column,
-                    _aggregation: this.getAggregationSql()
+                    _aggregation: this.getAggregationSql(),
+                    _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null
                 }),
-                categoriesSummaryQueryTpl({
+                categoriesSummaryMinMaxQueryTpl({
+                    _query: _query,
+                    _column: this.column
+                }),
+                categoriesSummaryCountQueryTpl({
                     _query: _query,
                     _column: this.column
                 })
@@ -150,9 +168,14 @@ Aggregation.prototype.sql = function(psql, override, callback) {
                 rankedCategoriesQueryTpl({
                     _query: _query,
                     _column: this.column,
-                    _aggregation: this.getAggregationSql()
+                    _aggregation: this.getAggregationSql(),
+                    _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null
                 }),
-                categoriesSummaryQueryTpl({
+                categoriesSummaryMinMaxQueryTpl({
+                    _query: _query,
+                    _column: this.column
+                }),
+                categoriesSummaryCountQueryTpl({
                     _query: _query,
                     _column: this.column
                 })

lib/cartodb/models/dataview/overviews/aggregation.js

@@ -18,25 +18,37 @@ var rankedCategoriesQueryTpl = dot.template([
     '  SELECT {{=it._column}} AS category, {{=it._aggregation}} AS value,',
     '    row_number() OVER (ORDER BY {{=it._aggregation}} desc) as rank',
     '  FROM ({{=it._query}}) _cdb_aggregation_all',
+    '  {{?it._aggregationColumn!==null}}WHERE {{=it._aggregationColumn}} IS NOT NULL{{?}}',
     '  GROUP BY {{=it._column}}',
     '  ORDER BY 2 DESC',
     ')'
 ].join('\n'));
 
-var categoriesSummaryQueryTpl = dot.template([
-    'categories_summary AS(',
-    '  SELECT count(1) categories_count, max(value) max_val, min(value) min_val',
+var categoriesSummaryMinMaxQueryTpl = dot.template([
+    'categories_summary_min_max AS(',
+    '  SELECT max(value) max_val, min(value) min_val',
     '  FROM categories',
     ')'
 ].join('\n'));
 
+var categoriesSummaryCountQueryTpl = dot.template([
+    'categories_summary_count AS(',
+    '  SELECT count(1) AS categories_count',
+    '  FROM (',
+    '    SELECT {{=it._column}} AS category',
+    '    FROM ({{=it._query}}) _cdb_categories',
+    '    GROUP BY {{=it._column}}',
+    '  ) _cdb_categories_count',
+    ')'
+].join('\n'));
+
 var rankedAggregationQueryTpl = dot.template([
     'SELECT CAST(category AS text), value, false as agg, nulls_count, min_val, max_val, count, categories_count',
-    '  FROM categories, summary, categories_summary',
+    '  FROM categories, summary, categories_summary_min_max, categories_summary_count',
     '  WHERE rank < {{=it._limit}}',
     'UNION ALL',
     'SELECT \'Other\' category, sum(value), true as agg, nulls_count, min_val, max_val, count, categories_count',
-    '  FROM categories, summary, categories_summary',
+    '  FROM categories, summary, categories_summary_min_max, categories_summary_count',
     '  WHERE rank >= {{=it._limit}}',
     'GROUP BY nulls_count, min_val, max_val, count, categories_count'
 ].join('\n'));
@@ -44,7 +56,7 @@ var rankedAggregationQueryTpl = dot.template([
 var aggregationQueryTpl = dot.template([
     'SELECT CAST({{=it._column}} AS text) AS category, {{=it._aggregation}} AS value, false as agg,',
     '  nulls_count, min_val, max_val, count, categories_count',
-    'FROM ({{=it._query}}) _cdb_aggregation_all, summary, categories_summary',
+    'FROM ({{=it._query}}) _cdb_aggregation_all, summary, categories_summary_min_max, categories_summary_count',
     'GROUP BY category, nulls_count, min_val, max_val, count, categories_count',
     'ORDER BY value DESC'
 ].join('\n'));
@@ -85,9 +97,14 @@ Aggregation.prototype.sql = function(psql, override, callback) {
                 rankedCategoriesQueryTpl({
                     _query: _query,
                     _column: this.column,
-                    _aggregation: this.getAggregationSql()
+                    _aggregation: this.getAggregationSql(),
+                    _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null
                 }),
-                categoriesSummaryQueryTpl({
+                categoriesSummaryMinMaxQueryTpl({
+                    _query: _query,
+                    _column: this.column
+                }),
+                categoriesSummaryCountQueryTpl({
                     _query: _query,
                     _column: this.column
                 })
@@ -110,9 +127,14 @@ Aggregation.prototype.sql = function(psql, override, callback) {
                 rankedCategoriesQueryTpl({
                     _query: _query,
                     _column: this.column,
-                    _aggregation: this.getAggregationSql()
+                    _aggregation: this.getAggregationSql(),
+                    _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null
                 }),
-                categoriesSummaryQueryTpl({
+                categoriesSummaryMinMaxQueryTpl({
+                    _query: _query,
+                    _column: this.column
+                }),
+                categoriesSummaryCountQueryTpl({
                     _query: _query,
                     _column: this.column
                 })

package.json

@@ -1,7 +1,7 @@
 {
   "private": true,
   "name": "windshaft-cartodb",
-  "version": "2.87.3",
+  "version": "2.87.5",
   "description": "A map tile server for CartoDB",
   "keywords": [
     "cartodb"
@@ -40,7 +40,8 @@
     "turbo-carto": "0.19.0",
     "underscore": "~1.6.0",
     "windshaft": "2.6.5",
-    "yargs": "~5.0.0"
+    "yargs": "~5.0.0",
+    "zipfile": "cartodb/node-zipfile#0.5.11-cdb1"
   },
   "devDependencies": {
     "istanbul": "~0.4.3",

test/acceptance/dataviews/aggregation.js

@@ -3,7 +3,7 @@ require('../../support/test_helper');
 var assert = require('../../support/assert');
 var TestClient = require('../../support/test-client');
 
-describe('aggregations', function() {
+describe('aggregations happy cases', function() {
 
     afterEach(function(done) {
         if (this.testClient) {
@@ -13,30 +13,36 @@ describe('aggregations', function() {
         }
     });
 
-    function aggregationOperationMapConfig(operation) {
-        return {
+    function aggregationOperationMapConfig(operation, query, column, aggregationColumn) {
+        column = column || 'adm0name';
+        aggregationColumn = aggregationColumn || 'pop_max';
+        query = query || 'select * from populated_places_simple_reduced';
+
+        var mapConfig = {
             version: '1.5.0',
             layers: [
                 {
                     type: 'mapnik',
                     options: {
-                        sql: 'select * from populated_places_simple_reduced',
+                        sql: query,
                         cartocss: '#layer0 { marker-fill: red; marker-width: 10; }',
                         cartocss_version: '2.0.1',
-                        widgets: {
-                            adm0name: {
-                                type: 'aggregation',
-                                options: {
-                                    column: 'adm0name',
-                                    aggregation: operation,
-                                    aggregationColumn: 'pop_max'
-                                }
-                            }
-                        }
+                        widgets: {}
                     }
                 }
             ]
         };
+
+        mapConfig.layers[0].options.widgets[column] = {
+            type: 'aggregation',
+            options: {
+                column: column,
+                aggregation: operation,
+                aggregationColumn: aggregationColumn
+            }
+        };
+
+        return mapConfig;
     }
 
     var operations = ['count', 'sum', 'avg', 'max', 'min'];
@@ -56,4 +62,49 @@ describe('aggregations', function() {
             });
         });
     });
+
+    var query = [
+        'select 1 as val, \'a\' as cat, ST_Transform(ST_SetSRID(ST_MakePoint(0,0),4326),3857) as the_geom_webmercator',
+        'select null, \'b\', ST_Transform(ST_SetSRID(ST_MakePoint(0,1),4326),3857)',
+        'select null, \'b\', ST_Transform(ST_SetSRID(ST_MakePoint(1,0),4326),3857)',
+        'select null, null, ST_Transform(ST_SetSRID(ST_MakePoint(1,1),4326),3857)'
+    ].join(' UNION ALL ');
+
+    operations.forEach(function (operation) {
+        var not = operation === 'count' ? ' not ' : ' ';
+        var description = 'should' +
+            not +
+            'handle NULL values in category and aggregation columns using "' +
+            operation +
+            '" as aggregation operation';
+
+        it(description, function (done) {
+            this.testClient = new TestClient(aggregationOperationMapConfig(operation, query, 'cat', 'val'));
+            this.testClient.getDataview('cat', { own_filter: 0 }, function (err, aggregation) {
+                assert.ifError(err);
+
+                assert.ok(aggregation);
+                assert.equal(aggregation.type, 'aggregation');
+                assert.ok(aggregation.categories);
+                assert.equal(aggregation.categoriesCount, 3);
+                assert.equal(aggregation.count, 4);
+                assert.equal(aggregation.nulls, 1);
+
+                var hasNullCategory = false;
+                aggregation.categories.forEach(function (category) {
+                    if (category.category === null) {
+                        hasNullCategory = true;
+                    }
+                });
+
+                if (operation === 'count') {
+                    assert.ok(hasNullCategory, 'aggregation has not a category NULL');
+                } else {
+                    assert.ok(!hasNullCategory, 'aggregation has category NULL');
+                }
+
+                done();
+            });
+        });
+    });
 });

test/acceptance/dataviews/histogram.js

@@ -77,4 +77,24 @@ describe('histogram-dataview', function() {
             done();
         });
     });
+
+    it('should cast all overridable params to numbers', function(done) {
+        var params = {
+            bins: '256 AS other, (select 256 * 2) AS bins_number--',
+            start: 1e3,
+            end: 0,
+            response: TestClient.RESPONSE.ERROR
+        };
+
+        this.testClient = new TestClient(mapConfig, 1234);
+        this.testClient.getDataview('pop_max_histogram', params, function(err, res) {
+            assert.ok(!err, err);
+
+            assert.ok(res.errors);
+            assert.equal(res.errors.length, 1);
+            assert.ok(res.errors[0].match(/Invalid number format for parameter 'bins'/));
+
+            done();
+        });
+    });
 });

test/support/test-client.js

@@ -307,6 +307,13 @@ TestClient.prototype.getDataview = function(dataviewName, params, callback) {
         url += '?' + qs.stringify(extraParams);
     }
 
+    var expectedResponse = params.response || {
+        status: 200,
+        headers: {
+            'Content-Type': 'application/json; charset=utf-8'
+        }
+    };
+
     var layergroupId;
     step(
         function createLayergroup() {
@@ -372,12 +379,7 @@ TestClient.prototype.getDataview = function(dataviewName, params, callback) {
                         host: 'localhost'
                     }
                 },
-                {
-                    status: 200,
-                    headers: {
-                        'Content-Type': 'application/json; charset=utf-8'
-                    }
-                },
+                expectedResponse,
                 function(res, err) {
                     if (err) {
                         return next(err);