diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b46d5d4e..6c4e4161 100644 --- a/CONTRIBUTING.md +++ b/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). diff --git a/NEWS.md b/NEWS.md index 625d6b61..aa57784b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,9 +1,35 @@ # Changelog -## 2.87.2 -Released 2016-mm-dd +## 2.87.6 +Released 2017-mm-dd +## 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 + +Bug fixes: + - Fix overviews-related dataviews problems. See https://github.com/CartoDB/Windshaft-cartodb/pull/604 + + +## 2.87.2 +Released 2016-12-19 + +- Use exception safe Dataservices API functions. See https://github.com/CartoDB/dataservices-api/issues/314 and https://github.com/CartoDB/camshaft/issues/242 + ## 2.87.1 Released 2016-12-13 diff --git a/docs/static_maps_api.md b/docs/static_maps_api.md index de539c50..736d90e0 100644 --- a/docs/static_maps_api.md +++ b/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 diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 0be51713..e0515efe 100644 --- a/lib/cartodb/backends/dataview.js +++ b/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} diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 3b0642b9..a80efbc7 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/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 }) diff --git a/lib/cartodb/models/dataview/overviews/aggregation.js b/lib/cartodb/models/dataview/overviews/aggregation.js index 41685ff5..da63b27f 100644 --- a/lib/cartodb/models/dataview/overviews/aggregation.js +++ b/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')); @@ -65,7 +77,7 @@ Aggregation.prototype.constructor = Aggregation; module.exports = Aggregation; -Aggregation.prototype.sql = function(psql, filters, override, callback) { +Aggregation.prototype.sql = function(psql, override, callback) { if (!callback) { callback = override; override = {}; @@ -85,9 +97,14 @@ Aggregation.prototype.sql = function(psql, filters, 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, filters, 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 }) diff --git a/lib/cartodb/models/dataview/overviews/base.js b/lib/cartodb/models/dataview/overviews/base.js index bbf70742..1425e2d1 100644 --- a/lib/cartodb/models/dataview/overviews/base.js +++ b/lib/cartodb/models/dataview/overviews/base.js @@ -55,20 +55,20 @@ BaseOverviewsDataview.prototype.rewrittenQuery = function(query) { }; // Default behaviour -BaseOverviewsDataview.prototype.defaultSql = function(psql, filters, override, callback) { +BaseOverviewsDataview.prototype.defaultSql = function(psql, override, callback) { var query = this.query; var dataview = this.baseDataview; if ( SETTINGS.defaultOverviews ) { query = this.rewrittenQuery(query); dataview = new this.BaseDataview(query, this.queryOptions); } - return dataview.sql(psql, filters, override, callback); + return dataview.sql(psql, override, callback); }; // default implementation that can be override in derived classes: -BaseOverviewsDataview.prototype.sql = function(psql, filters, override, callback) { - return this.defaultSql(psql, filters, override, callback); +BaseOverviewsDataview.prototype.sql = function(psql, override, callback) { + return this.defaultSql(psql, override, callback); }; BaseOverviewsDataview.prototype.search = function(psql, userQuery, callback) { diff --git a/lib/cartodb/models/dataview/overviews/formula.js b/lib/cartodb/models/dataview/overviews/formula.js index adad5f58..9e331f0b 100644 --- a/lib/cartodb/models/dataview/overviews/formula.js +++ b/lib/cartodb/models/dataview/overviews/formula.js @@ -36,7 +36,7 @@ Formula.prototype.constructor = Formula; module.exports = Formula; -Formula.prototype.sql = function(psql, filters, override, callback) { +Formula.prototype.sql = function(psql, override, callback) { var formulaQueryTpl = formulaQueryTpls[this.operation]; if ( formulaQueryTpl ) { @@ -52,5 +52,5 @@ Formula.prototype.sql = function(psql, filters, override, callback) { } // default behaviour - return this.defaultSql(psql, filters, override, callback); + return this.defaultSql(psql, override, callback); }; diff --git a/lib/cartodb/models/dataview/overviews/histogram.js b/lib/cartodb/models/dataview/overviews/histogram.js index 20dd7712..5f4d97d9 100644 --- a/lib/cartodb/models/dataview/overviews/histogram.js +++ b/lib/cartodb/models/dataview/overviews/histogram.js @@ -8,7 +8,6 @@ dot.templateSettings.strip = false; var columnTypeQueryTpl = dot.template( 'SELECT pg_typeof({{=it.column}})::oid FROM ({{=it.query}}) _cdb_histogram_column_type limit 1' ); -var columnCastTpl = dot.template("date_part('epoch', {{=it.column}})"); var BIN_MIN_NUMBER = 6; var BIN_MAX_NUMBER = 48; @@ -149,7 +148,9 @@ Histogram.prototype.sql = function(psql, override, callback) { } if (this._columnType === 'date') { - _column = columnCastTpl({column: _column}); + // overviews currently aggregate dates to NULL + // to avoid problem we don't use overviews for histograms of date columns + return this.defaultSql(psql, override, callback); } var _query = this.rewrittenQuery(this.query); diff --git a/package.json b/package.json index 8db76ef7..b563194f 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "private": true, "name": "windshaft-cartodb", - "version": "2.87.2", + "version": "2.87.6", "description": "A map tile server for CartoDB", "keywords": [ "cartodb" @@ -40,7 +40,8 @@ "turbo-carto": "0.19.0", "underscore": "~1.6.0", "windshaft": "cartodb/windshaft#mapnik-migration", - "yargs": "~5.0.0" + "yargs": "~5.0.0", + "zipfile": "cartodb/node-zipfile#0.5.11-cdb1" }, "devDependencies": { "istanbul": "~0.4.3", diff --git a/test/acceptance/dataviews/aggregation.js b/test/acceptance/dataviews/aggregation.js index 2986a174..9fc9a219 100644 --- a/test/acceptance/dataviews/aggregation.js +++ b/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(); + }); + }); + }); }); diff --git a/test/acceptance/dataviews/histogram.js b/test/acceptance/dataviews/histogram.js index 5d5ff000..abbaef61 100644 --- a/test/acceptance/dataviews/histogram.js +++ b/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(); + }); + }); }); diff --git a/test/support/test-client.js b/test/support/test-client.js index f5d3789c..75fa51e0 100644 --- a/test/support/test-client.js +++ b/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);