From 216e7b7f1debb4061746c0c820cfccd38f722033 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 8 Jan 2016 12:07:51 +0100 Subject: [PATCH 01/43] Use attributes-substitution-tokens of Windshaft Intended for installation in a staging server --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 27972e43..6ccdec26 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "node-statsd": "~0.0.7", "underscore" : "~1.6.0", "dot": "~1.0.2", - "windshaft": "1.6.1", + "windshaft": "https://github.com/CartoDB/Windshaft/tarball/attributes-substitution-tokens", "step": "~0.0.6", "queue-async": "~1.0.7", "request": "~2.62.0", From 89590d32dfb2d407b77fc2c202a6e86af36494ce Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 19 Jan 2016 19:31:43 +0100 Subject: [PATCH 02/43] Sketch of vector overviews support --- lib/cartodb/api/overviews_api.js | 65 +++++++++++++++++++ lib/cartodb/controllers/map.js | 14 ++++ .../models/mapconfig_overviews_adapter.js | 40 ++++++++++++ 3 files changed, 119 insertions(+) create mode 100644 lib/cartodb/api/overviews_api.js create mode 100644 lib/cartodb/models/mapconfig_overviews_adapter.js diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js new file mode 100644 index 00000000..b3db3101 --- /dev/null +++ b/lib/cartodb/api/overviews_api.js @@ -0,0 +1,65 @@ +var queue = require('queue-async'); +var QueryTablesApi = require('./query_tables_api'); + +function OverviewsApi(pgQueryRunner) { + if (pgQueryRunner.pgQueryRunner != null) { + this.queryTablesApi = pgQueryRunner; + this.pgQueryRunner = this.queryTablesApi.pgQueryRunner; + } else { + this.pgQueryRunner = pgQueryRunner; + this.queryTablesApi = new QueryTablesApi(pgQueryRunner); + } +} + +module.exports = OverviewsApi; + +OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) { + this.queryTablesApi.getAffectedTablesInQuery(username, sql, function(err, tableNames){ + if (err) { + callback(err); + } else { + metadata = {}; + + var parallelism = 2; + var q = queue(parallelism); + + for ( var i=0; i < tableNames.length; ++i ) { + (function(tableName) { + q.defer(function(done){ + var sql = "SELECT * FROM CDB_Overviews('" + tableName + "');"; + this.pgQueryRunner.run(username, query, handleOverviewsRows, function(err, table_metadata){ + if (err) { + done(err); + } else { + metadata[tableName] = table_metadata; + done(null); + } + }); + }); + }(tableNames[i])); + } + + q.awaitAll(function(err, results){ + if (err) { + return callback(err); + } else { + return callback(null, metadata); + } + }); + }; + }); +}; + +function handleOverviewsRows(err, rows, callback) { + if (err){ + var msg = err.message ? err.message : err; + callback(new Error('could not get overviews metadata: ' + msg)); + return; + } + var metadata = {}; + for ( var i=0; i Date: Wed, 20 Jan 2016 10:07:19 +0100 Subject: [PATCH 03/43] Avoid wrapper-functions to capture looping variable values Use async-queue defer additional parameters --- lib/cartodb/api/overviews_api.js | 22 +++++++++---------- .../models/mapconfig_overviews_adapter.js | 12 +++++----- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index b3db3101..c5f9ff76 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -24,19 +24,17 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) var q = queue(parallelism); for ( var i=0; i < tableNames.length; ++i ) { - (function(tableName) { - q.defer(function(done){ - var sql = "SELECT * FROM CDB_Overviews('" + tableName + "');"; - this.pgQueryRunner.run(username, query, handleOverviewsRows, function(err, table_metadata){ - if (err) { - done(err); - } else { - metadata[tableName] = table_metadata; - done(null); - } - }); + q.defer(function(tableName, done){ + var sql = "SELECT * FROM CDB_Overviews('" + tableName + "');"; + this.pgQueryRunner.run(username, query, handleOverviewsRows, function(err, table_metadata){ + if (err) { + done(err); + } else { + metadata[tableName] = table_metadata; + done(null); + } }); - }(tableNames[i])); + }, tableNames[i]); } q.awaitAll(function(err, results){ diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index 74ac85e5..21fd5c37 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -16,14 +16,12 @@ MapConfigNamedLayersAdapter.prototype.getMapConfig = function(username, mapconfi var q = queue(parallelism); for ( var i=0; i < layers.length; ++i ) { - (function(layer) { - q.defer(function(done){ - this.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ - // TODO: is it legit to modify layer like this? - layer.options.overviews = metadata; - }); + q.defer(function(layer, done){ + this.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ + // TODO: is it legit to modify layer like this? + layer.options.overviews = metadata; }); - })(layers[i]); + }, layers[i]); }; q.awaitAll(function(err){ From 9feae661733a1bec970632c53758620f999a9f54 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 11:49:17 +0100 Subject: [PATCH 04/43] Bugfixes --- lib/cartodb/api/overviews_api.js | 6 +++--- lib/cartodb/controllers/map.js | 2 +- lib/cartodb/models/mapconfig_overviews_adapter.js | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index c5f9ff76..2f516353 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -18,14 +18,14 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) if (err) { callback(err); } else { - metadata = {}; + var metadata = {}; var parallelism = 2; var q = queue(parallelism); for ( var i=0; i < tableNames.length; ++i ) { q.defer(function(tableName, done){ - var sql = "SELECT * FROM CDB_Overviews('" + tableName + "');"; + var query = "SELECT * FROM CDB_Overviews('" + tableName + "');"; this.pgQueryRunner.run(username, query, handleOverviewsRows, function(err, table_metadata){ if (err) { done(err); @@ -37,7 +37,7 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) }, tableNames[i]); } - q.awaitAll(function(err, results){ + q.awaitAll(function(err){ if (err) { return callback(err); } else { diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 4ccf375d..3620ed6c 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -49,7 +49,7 @@ function MapController(authApi, pgConnection, templateMaps, mapBackend, metadata this.layergroupAffectedTables = layergroupAffectedTables; this.namedLayersAdapter = new MapConfigNamedLayersAdapter(templateMaps); - this.overviewsAdapter = new MapConfigOverviewsAdapter(overviewsApi); + this.overviewsAdapter = new MapConfigOverviewsAdapter(this.overviewsApi); } util.inherits(MapController, BaseController); diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index 21fd5c37..89b87aaa 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -20,6 +20,7 @@ MapConfigNamedLayersAdapter.prototype.getMapConfig = function(username, mapconfi this.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ // TODO: is it legit to modify layer like this? layer.options.overviews = metadata; + done(null); }); }, layers[i]); }; From 8a49e466267760757789a9e524dec61d0447c0d5 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 11:51:46 +0100 Subject: [PATCH 05/43] Accept minor jshint suggestions --- lib/cartodb/api/overviews_api.js | 6 +++--- lib/cartodb/models/mapconfig_overviews_adapter.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index 2f516353..1c52c0b6 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -2,7 +2,7 @@ var queue = require('queue-async'); var QueryTablesApi = require('./query_tables_api'); function OverviewsApi(pgQueryRunner) { - if (pgQueryRunner.pgQueryRunner != null) { + if (pgQueryRunner.pgQueryRunner !== undefined) { this.queryTablesApi = pgQueryRunner; this.pgQueryRunner = this.queryTablesApi.pgQueryRunner; } else { @@ -44,7 +44,7 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) return callback(null, metadata); } }); - }; + } }); }; @@ -58,6 +58,6 @@ function handleOverviewsRows(err, rows, callback) { for ( var i=0; i Date: Wed, 20 Jan 2016 12:42:43 +0100 Subject: [PATCH 06/43] Refactor coding style Hide the fact that we define functions in a loop from jshint! --- lib/cartodb/api/overviews_api.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index 1c52c0b6..4ccb28ce 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -23,8 +23,8 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) var parallelism = 2; var q = queue(parallelism); - for ( var i=0; i < tableNames.length; ++i ) { - q.defer(function(tableName, done){ + tableNames.forEach(function(tableName) { + q.defer(function(done){ var query = "SELECT * FROM CDB_Overviews('" + tableName + "');"; this.pgQueryRunner.run(username, query, handleOverviewsRows, function(err, table_metadata){ if (err) { @@ -34,8 +34,8 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) done(null); } }); - }, tableNames[i]); - } + }); + }); q.awaitAll(function(err){ if (err) { From 4ca8ecf64ce7eae206ff71284dc290c9950d02dc Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 12:44:00 +0100 Subject: [PATCH 07/43] Refactor/fix potential problems --- lib/cartodb/controllers/map.js | 18 ++++--- .../models/mapconfig_overviews_adapter.js | 49 +++++++++++-------- 2 files changed, 41 insertions(+), 26 deletions(-) diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 3620ed6c..769e825a 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -155,12 +155,18 @@ MapController.prototype.create = function(req, res, prepareConfigFn) { function addOverviewsInformation(err, requestMapConfig, datasource) { assert.ifError(err); var next = this; - self.overviewsAdapter.getMapConfig(req.context.user, requestMapConfig, function(err, mapconfig) { - if (err) { - return next(err); - } - return next(null, mapconfig, datasource); - }); + self.overviewsAdapter.getLayers(req.context.user, requestMapConfig.layers, + function(err, layers) { + if (err) { + return next(err); + } + + if (layers) { + requestMapConfig.layers = layers; + } + return next(null, requestMapConfig, datasource); + } + ); }, function createLayergroup(err, requestMapConfig, datasource) { assert.ifError(err); diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index 7a9bc606..00cf02e0 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -1,4 +1,5 @@ var queue = require('queue-async'); +var _ = require('underscore'); function MapConfigNamedLayersAdapter(overviewsApi) { this.overviewsApi = overviewsApi; @@ -6,32 +7,40 @@ function MapConfigNamedLayersAdapter(overviewsApi) { module.exports = MapConfigNamedLayersAdapter; -MapConfigNamedLayersAdapter.prototype.getMapConfig = function(username, mapconfig, callback) { +MapConfigNamedLayersAdapter.prototype.getLayers = function(username, layers, callback) { - // TODO: we're modifying mapconfig in place and then returning it... not very nice + if (!layers) { + return callback(null); + } - var layers = mapconfig.getlayers(); + var augmentLayersQueue = queue(layers.length); - var parallelism = 2; - var q = queue(parallelism); + function augmentLayer(layer, done) { + this.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ + if ( !_.isEmpty(metadata) ) { + layer = _.extend({}, layer, { overviews: metadata }); + } + done(null, layer); + }); + } - for ( var i=0; i < layers.length; ++i ) { - q.defer(function(layer, done){ - this.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ - // TODO: is it legit to modify layer like this? - layer.options.overviews = metadata; - done(null); - }); - }, layers[i]); + function layersAugmentQueueFinish(err, layers) { + if (err) { + return callback(err); } - q.awaitAll(function(err){ - if (err) { - return callback(err); - } else { - return callback(null, mapconfig); - } - }); + if (!layers || layers.length === 0) { + return callback(new Error('Missing layers array from layergroup config')); + } + + return callback(null, layers); + } + + layers.forEach(function(layer) { + augmentLayersQueue.defer(augmentLayer, layer); + }); + augmentLayersQueue.awaitAll(layersAugmentQueueFinish); + }; // TODO: document in https://github.com/CartoDB/Windshaft/blob/master/doc/MapConfig-1.5.0.md From 3dad2255687cb62c0a206f681c2d8a5ee93925e3 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 13:12:45 +0100 Subject: [PATCH 08/43] Fix bug --- lib/cartodb/models/mapconfig_overviews_adapter.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index 00cf02e0..b78acf38 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -8,6 +8,7 @@ function MapConfigNamedLayersAdapter(overviewsApi) { module.exports = MapConfigNamedLayersAdapter; MapConfigNamedLayersAdapter.prototype.getLayers = function(username, layers, callback) { + var self = this; if (!layers) { return callback(null); @@ -16,7 +17,7 @@ MapConfigNamedLayersAdapter.prototype.getLayers = function(username, layers, cal var augmentLayersQueue = queue(layers.length); function augmentLayer(layer, done) { - this.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ + self.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ if ( !_.isEmpty(metadata) ) { layer = _.extend({}, layer, { overviews: metadata }); } From 09568050d60a077a54d485facabc3849c7085cc3 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 13:13:02 +0100 Subject: [PATCH 09/43] Fix for changes in pgQueryRunner --- lib/cartodb/api/overviews_api.js | 33 +++++++++++++------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index 4ccb28ce..9b6dc1c6 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -14,6 +14,7 @@ function OverviewsApi(pgQueryRunner) { module.exports = OverviewsApi; OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) { + var self = this; this.queryTablesApi.getAffectedTablesInQuery(username, sql, function(err, tableNames){ if (err) { callback(err); @@ -26,13 +27,19 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) tableNames.forEach(function(tableName) { q.defer(function(done){ var query = "SELECT * FROM CDB_Overviews('" + tableName + "');"; - this.pgQueryRunner.run(username, query, handleOverviewsRows, function(err, table_metadata){ - if (err) { - done(err); - } else { - metadata[tableName] = table_metadata; - done(null); + self.pgQueryRunner.run(username, query, function handleOverviewsRows(err, rows) { + if (err){ + var msg = err.message ? err.message : err; + done(new Error('could not get overviews metadata: ' + msg)); + return; } + var table_metadata = {}; + for ( var i=0; i Date: Wed, 20 Jan 2016 17:09:15 +0100 Subject: [PATCH 10/43] Update comments --- lib/cartodb/controllers/map.js | 1 + lib/cartodb/models/mapconfig_overviews_adapter.js | 4 ---- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 769e825a..c70fca65 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -193,6 +193,7 @@ MapController.prototype.create = function(req, res, prepareConfigFn) { }; MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn) { + // TODO: use addOverviewsInformation as in create var self = this; var cdbuser = req.context.user; diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index b78acf38..f8f076a8 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -43,7 +43,3 @@ MapConfigNamedLayersAdapter.prototype.getLayers = function(username, layers, cal augmentLayersQueue.awaitAll(layersAugmentQueueFinish); }; - -// TODO: document in https://github.com/CartoDB/Windshaft/blob/master/doc/MapConfig-1.5.0.md -// (as OPTIONAL) -// overviews: { table_name: { zoom_level: { table: overview_table_name } }} From 528574c5508b3359bd1637b22314ba830b43cc98 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 17:10:12 +0100 Subject: [PATCH 11/43] Add dummy CDB_Overviews SQL function for tests --- test/support/prepare_db.sh | 2 +- test/support/sql/CDB_Overviews.sql | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 test/support/sql/CDB_Overviews.sql diff --git a/test/support/prepare_db.sh b/test/support/prepare_db.sh index ab9149a2..acc496ed 100755 --- a/test/support/prepare_db.sh +++ b/test/support/prepare_db.sh @@ -81,7 +81,7 @@ if test x"$PREPARE_PGSQL" = xyes; then psql -c "CREATE LANGUAGE plpythonu;" ${TEST_DB} curl -L -s https://github.com/CartoDB/cartodb-postgresql/raw/cdb/scripts-available/CDB_QueryStatements.sql -o sql/CDB_QueryStatements.sql curl -L -s https://github.com/CartoDB/cartodb-postgresql/raw/cdb/scripts-available/CDB_QueryTables.sql -o sql/CDB_QueryTables.sql - cat sql/CDB_QueryStatements.sql sql/CDB_QueryTables.sql | + cat sql/CDB_QueryStatements.sql sql/CDB_QueryTables.sql sql/CDB_Overviews.sql | psql -v ON_ERROR_STOP=1 ${TEST_DB} || exit 1 fi diff --git a/test/support/sql/CDB_Overviews.sql b/test/support/sql/CDB_Overviews.sql new file mode 100644 index 00000000..c093dfa9 --- /dev/null +++ b/test/support/sql/CDB_Overviews.sql @@ -0,0 +1,15 @@ +-- Mockup for CDB_Overviews +CREATE OR REPLACE FUNCTION CDB_Overviews(table_name text) +RETURNS TABLE(z integer, overview_table text) +AS $$ + BEGIN + IF table_name = 'test_table_overviews' THEN + RETURN QUERY + SELECT 1 AS z, 'test_table_overviews_ov1'::text AS overviw_table + UNION ALL + SELECT 2 AS z, 'test_table_overviews_ov2'::text AS overviw_table; + ELSE + RETURN; + END IF; + END +$$ LANGUAGE PLPGSQL; From 7c897a40bf15485741d73c3b78b4047d896d12bd Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 18:07:35 +0100 Subject: [PATCH 12/43] Fix bug in tests The invalid SQL in this test (missing comma) was unnoticed because the test was provoking a failed before the SQL was parsed, but new features may cause the SQL to be evaluated (to get affected tables) before the CartoCSS validity is checked. --- test/acceptance/multilayer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/acceptance/multilayer.js b/test/acceptance/multilayer.js index 76235303..5d735278 100644 --- a/test/acceptance/multilayer.js +++ b/test/acceptance/multilayer.js @@ -576,8 +576,8 @@ describe(suiteName, function() { version: '1.0.0', layers: [ { options: { - sql: 'select 1 as cartodb_id, !pixel_height! as h' + - 'ST_Buffer(!bbox!, -32*greatest(!pixel_width!,!pixel_height!)) as the_geom_webmercator', + sql: 'select 1 as cartodb_id, !pixel_height! as h,' + + 'ST_Buffer(!bbox!, -32*greatest(!pixel_width!,!pixel_height!)) as the_geom_webmercator', cartocss: '#layer { polygon-fit:red; }', cartocss_version: '2.0.1' } } From 5543fcb736b988e6a311993bc344da5b1c51fcf2 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 18:09:00 +0100 Subject: [PATCH 13/43] Fix: handle error properly when augmented layers with overviews --- lib/cartodb/models/mapconfig_overviews_adapter.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index f8f076a8..e38cc566 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -18,10 +18,14 @@ MapConfigNamedLayersAdapter.prototype.getLayers = function(username, layers, cal function augmentLayer(layer, done) { self.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ - if ( !_.isEmpty(metadata) ) { - layer = _.extend({}, layer, { overviews: metadata }); + if (err) { + done(err, layer); + } else { + if ( !_.isEmpty(metadata) ) { + // layer = _.extend({}, layer, { overviews: metadata }); + } + done(null, layer); } - done(null, layer); }); } From ffd89edaa7de3fceed1ce1eeadf9d61dcefeb80f Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 20 Jan 2016 18:36:06 +0100 Subject: [PATCH 14/43] Add overviews metadata to MapController instantiateTemplate As in MapController create. --- lib/cartodb/controllers/map.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index c70fca65..803f5768 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -193,7 +193,6 @@ MapController.prototype.create = function(req, res, prepareConfigFn) { }; MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn) { - // TODO: use addOverviewsInformation as in create var self = this; var cdbuser = req.context.user; @@ -224,7 +223,23 @@ MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn ); mapConfigProvider.getMapConfig(this); }, - function createLayergroup(err, mapConfig_, rendererParams/*, context*/) { + function addOverviewsInformation(err, requestMapConfig, rendererParams/*, context*/) { + assert.ifError(err); + var next = this; + self.overviewsAdapter.getLayers(req.context.user, requestMapConfig.layers, + function(err, layers) { + if (err) { + return next(err); + } + + if (layers) { + requestMapConfig.layers = layers; + } + return next(null, requestMapConfig, rendererParams); + } + ); + }, + function createLayergroup(err, mapConfig_, rendererParams) { assert.ifError(err); mapConfig = mapConfig_; self.mapBackend.createLayergroup( From ed9b3e12309fa37080541f96f11e4c1ab2fb8cf1 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 10:58:50 +0100 Subject: [PATCH 15/43] Bring in code commented out for tests --- lib/cartodb/models/mapconfig_overviews_adapter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index e38cc566..8fc3bac2 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -22,7 +22,7 @@ MapConfigNamedLayersAdapter.prototype.getLayers = function(username, layers, cal done(err, layer); } else { if ( !_.isEmpty(metadata) ) { - // layer = _.extend({}, layer, { overviews: metadata }); + layer = _.extend({}, layer, { overviews: metadata }); } done(null, layer); } From cc0385d61433c66ed81159b9ffd9091e9f1167c5 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 12:03:50 +0100 Subject: [PATCH 16/43] Fix class name --- lib/cartodb/models/mapconfig_overviews_adapter.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index 8fc3bac2..7986c907 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -1,13 +1,13 @@ var queue = require('queue-async'); var _ = require('underscore'); -function MapConfigNamedLayersAdapter(overviewsApi) { +function MapConfigOverviewsAdapter(overviewsApi) { this.overviewsApi = overviewsApi; } -module.exports = MapConfigNamedLayersAdapter; +module.exports = MapConfigOverviewsAdapter; -MapConfigNamedLayersAdapter.prototype.getLayers = function(username, layers, callback) { +MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callback) { var self = this; if (!layers) { From 094c9076be88800c9723587980b7a1c4bdfec4a8 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 12:04:40 +0100 Subject: [PATCH 17/43] Fix: only mapnik layers can have overviews --- lib/cartodb/models/mapconfig_overviews_adapter.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index 7986c907..8571f7a2 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -17,6 +17,9 @@ MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callb var augmentLayersQueue = queue(layers.length); function augmentLayer(layer, done) { + if ( layer.type != 'mapnik' && layer.type != 'cartodb' ) { + return done(null, layer); + } self.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ if (err) { done(err, layer); From 87bffb9657f5b6bb84bae003aac6c6b7b5f9378e Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 12:06:01 +0100 Subject: [PATCH 18/43] Fix: overviews entry should be inside options --- lib/cartodb/models/mapconfig_overviews_adapter.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index 8571f7a2..c98fe568 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -25,7 +25,9 @@ MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callb done(err, layer); } else { if ( !_.isEmpty(metadata) ) { - layer = _.extend({}, layer, { overviews: metadata }); + // layer.options.overviews = metadata; + layer = _.extend({}, layer); + layer.options = _.extend({}, layer.options, { overviews: metadata }); } done(null, layer); } From 532654eea8921fdb30b4be5250512450b45399f8 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 13:33:45 +0100 Subject: [PATCH 19/43] Add tests for the OverviewsApi --- test/integration/overviews-api.js | 54 +++++++++++++++++++ test/support/sql/CDB_Overviews.sql | 2 +- test/support/sql/windshaft.test.sql | 80 ++++++++++++++++++++++++++++- 3 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 test/integration/overviews-api.js diff --git a/test/integration/overviews-api.js b/test/integration/overviews-api.js new file mode 100644 index 00000000..4940955f --- /dev/null +++ b/test/integration/overviews-api.js @@ -0,0 +1,54 @@ +require('../support/test_helper'); + +var assert = require('assert'); + +var RedisPool = require('redis-mpool'); +var cartodbRedis = require('cartodb-redis'); + +var PgConnection = require('../../lib/cartodb/backends/pg_connection'); +var PgQueryRunner = require('../../lib/cartodb/backends/pg_query_runner'); +var QueryTablesApi = require('../../lib/cartodb/api/query_tables_api'); +var OverviewsApi = require('../../lib/cartodb/api/overviews_api'); + + +describe('OverviewsApi', function() { + + var queryTablesApi, overviewsApi; + + before(function() { + var redisPool = new RedisPool(global.environment.redis); + var metadataBackend = cartodbRedis({pool: redisPool}); + var pgConnection = new PgConnection(metadataBackend); + var pgQueryRunner = new PgQueryRunner(pgConnection); + queryTablesApi = new QueryTablesApi(pgQueryRunner); + overviewsApi = new OverviewsApi(queryTablesApi); + }); + + it('should return an empty relation for tables that have no overviews', function(done) { + var query = 'select * from test_table'; + overviewsApi.getOverviewsMetadata('localhost', query, function(err, result) { + assert.ok(!err, err); + + assert.deepEqual(result, {}); + + done(); + }); + }); + + it('should return overviews metadata', function(done) { + var query = 'select * from test_table_overviews'; + overviewsApi.getOverviewsMetadata('localhost', query, function(err, result) { + assert.ok(!err, err); + + assert.deepEqual(result, { + 'public.test_table_overviews': { + 1: { table: 'test_table_overviews_ov1' }, + 2: { table: 'test_table_overviews_ov2' } + } + }); + + done(); + }); + }); + +}); diff --git a/test/support/sql/CDB_Overviews.sql b/test/support/sql/CDB_Overviews.sql index c093dfa9..9e04f611 100644 --- a/test/support/sql/CDB_Overviews.sql +++ b/test/support/sql/CDB_Overviews.sql @@ -3,7 +3,7 @@ CREATE OR REPLACE FUNCTION CDB_Overviews(table_name text) RETURNS TABLE(z integer, overview_table text) AS $$ BEGIN - IF table_name = 'test_table_overviews' THEN + IF table_name = 'public.test_table_overviews' THEN RETURN QUERY SELECT 1 AS z, 'test_table_overviews_ov1'::text AS overviw_table UNION ALL diff --git a/test/support/sql/windshaft.test.sql b/test/support/sql/windshaft.test.sql index 4febe94c..3b795c01 100644 --- a/test/support/sql/windshaft.test.sql +++ b/test/support/sql/windshaft.test.sql @@ -1,6 +1,6 @@ -- -- Windshaft test database --- +-- -- To use run ../prepare_db.sh -- NOTE: requires a postgis template called template_postgis -- @@ -238,3 +238,81 @@ CREATE INDEX test_big_poly_the_geom_webmercator_idx ON test_big_poly USING gist GRANT ALL ON TABLE test_big_poly TO :TESTUSER; GRANT SELECT ON TABLE test_big_poly TO :PUBLICUSER; + +-- table with overviews + +CREATE TABLE test_table_overviews ( + updated_at timestamp without time zone DEFAULT now(), + created_at timestamp without time zone DEFAULT now(), + cartodb_id integer NOT NULL, + name character varying, + address character varying, + the_geom geometry, + the_geom_webmercator geometry, + CONSTRAINT enforce_dims_the_geom CHECK ((st_ndims(the_geom) = 2)), + CONSTRAINT enforce_dims_the_geom_webmercator CHECK ((st_ndims(the_geom_webmercator) = 2)), + CONSTRAINT enforce_geotype_the_geom CHECK (((geometrytype(the_geom) = 'POINT'::text) OR (the_geom IS NULL))), + CONSTRAINT enforce_geotype_the_geom_webmercator CHECK (((geometrytype(the_geom_webmercator) = 'POINT'::text) OR (the_geom_webmercator IS NULL))), + CONSTRAINT enforce_srid_the_geom CHECK ((st_srid(the_geom) = 4326)), + CONSTRAINT enforce_srid_the_geom_webmercator CHECK ((st_srid(the_geom_webmercator) = 3857)) +); + +CREATE SEQUENCE test_table_overviews_cartodb_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE test_table_overviews_cartodb_id_seq OWNED BY test_table_overviews.cartodb_id; + +SELECT pg_catalog.setval('test_table_overviews_cartodb_id_seq', 60, true); + +ALTER TABLE test_table_overviews ALTER COLUMN cartodb_id SET DEFAULT nextval('test_table_overviews_cartodb_id_seq'::regclass); + +INSERT INTO test_table_overviews VALUES +('2011-09-21 14:02:21.358706', '2011-09-21 14:02:21.314252', 1, 'Hawai', 'Calle de Pérez Galdós 9, Madrid, Spain', '0101000020E6100000A6B73F170D990DC064E8D84125364440', '0101000020110F000076491621312319C122D4663F1DCC5241'), +('2011-09-21 14:02:21.358706', '2011-09-21 14:02:21.319101', 2, 'El Estocolmo', 'Calle de la Palma 72, Madrid, Spain', '0101000020E6100000C90567F0F7AB0DC0AB07CC43A6364440', '0101000020110F0000C4356B29423319C15DD1092DADCC5241'), +('2011-09-21 14:02:21.358706', '2011-09-21 14:02:21.324', 3, 'El Rey del Tallarín', 'Plaza Conde de Toreno 2, Madrid, Spain', '0101000020E610000021C8410933AD0DC0CB0EF10F5B364440', '0101000020110F000053E71AC64D3419C10F664E4659CC5241'), +('2011-09-21 14:02:21.358706', '2011-09-21 14:02:21.329509', 4, 'El Lacón', 'Manuel Fernández y González 8, Madrid, Spain', '0101000020E6100000BC5983F755990DC07D923B6C22354440', '0101000020110F00005DACDB056F2319C1EC41A980FCCA5241'), +('2011-09-21 14:02:21.358706', '2011-09-21 14:02:21.334931', 5, 'El Pico', 'Calle Divino Pastor 12, Madrid, Spain', '0101000020E61000003B6D8D08C6A10DC0371B2B31CF364440', '0101000020110F00005F716E91992A19C17DAAA4D6DACC5241'); + +ALTER TABLE ONLY test_table_overviews ADD CONSTRAINT test_table_overviews_pkey PRIMARY KEY (cartodb_id); + +CREATE INDEX test_table_overviews_the_geom_idx ON test_table_overviews USING gist (the_geom); +CREATE INDEX test_table_overviews_the_geom_webmercator_idx ON test_table_overviews USING gist (the_geom_webmercator); + +GRANT ALL ON TABLE test_table_overviews TO :TESTUSER; +GRANT SELECT ON TABLE test_table_overviews TO :PUBLICUSER; + +CREATE TABLE test_table_overviews_ov1 ( + updated_at timestamp without time zone DEFAULT now(), + created_at timestamp without time zone DEFAULT now(), + cartodb_id integer NOT NULL, + name character varying, + address character varying, + the_geom geometry, + the_geom_webmercator geometry, + CONSTRAINT enforce_dims_the_geom CHECK ((st_ndims(the_geom) = 2)), + CONSTRAINT enforce_dims_the_geom_webmercator CHECK ((st_ndims(the_geom_webmercator) = 2)), + CONSTRAINT enforce_geotype_the_geom CHECK (((geometrytype(the_geom) = 'POINT'::text) OR (the_geom IS NULL))), + CONSTRAINT enforce_geotype_the_geom_webmercator CHECK (((geometrytype(the_geom_webmercator) = 'POINT'::text) OR (the_geom_webmercator IS NULL))), + CONSTRAINT enforce_srid_the_geom CHECK ((st_srid(the_geom) = 4326)), + CONSTRAINT enforce_srid_the_geom_webmercator CHECK ((st_srid(the_geom_webmercator) = 3857)) +); + +CREATE TABLE test_table_overviews_ov2 ( + updated_at timestamp without time zone DEFAULT now(), + created_at timestamp without time zone DEFAULT now(), + cartodb_id integer NOT NULL, + name character varying, + address character varying, + the_geom geometry, + the_geom_webmercator geometry, + CONSTRAINT enforce_dims_the_geom CHECK ((st_ndims(the_geom) = 2)), + CONSTRAINT enforce_dims_the_geom_webmercator CHECK ((st_ndims(the_geom_webmercator) = 2)), + CONSTRAINT enforce_geotype_the_geom CHECK (((geometrytype(the_geom) = 'POINT'::text) OR (the_geom IS NULL))), + CONSTRAINT enforce_geotype_the_geom_webmercator CHECK (((geometrytype(the_geom_webmercator) = 'POINT'::text) OR (the_geom_webmercator IS NULL))), + CONSTRAINT enforce_srid_the_geom CHECK ((st_srid(the_geom) = 4326)), + CONSTRAINT enforce_srid_the_geom_webmercator CHECK ((st_srid(the_geom_webmercator) = 3857)) +); From 62cc53228c1eb9a222d73fb602e39517ae25063a Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 13:35:56 +0100 Subject: [PATCH 20/43] OverviewApi: skip tables with no overlays in result --- lib/cartodb/api/overviews_api.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index 9b6dc1c6..81d279b1 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -33,12 +33,14 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) done(new Error('could not get overviews metadata: ' + msg)); return; } - var table_metadata = {}; - for ( var i=0; i 0 ) { + var table_metadata = {}; + for ( var i=0; i Date: Thu, 21 Jan 2016 15:44:22 +0100 Subject: [PATCH 21/43] Add tests for MapConfigOverviewsAdapter --- .../mapconfig_overviews_adapter.js | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 test/integration/mapconfig_overviews_adapter.js diff --git a/test/integration/mapconfig_overviews_adapter.js b/test/integration/mapconfig_overviews_adapter.js new file mode 100644 index 00000000..0db1c1e3 --- /dev/null +++ b/test/integration/mapconfig_overviews_adapter.js @@ -0,0 +1,88 @@ +require('../support/test_helper'); + +var assert = require('assert'); +var _ = require('underscore'); +var RedisPool = require('redis-mpool'); +var cartodbRedis = require('cartodb-redis'); +var PgConnection = require(__dirname + '/../../lib/cartodb/backends/pg_connection'); +var PgQueryRunner = require('../../lib/cartodb/backends/pg_query_runner'); +var QueryTablesApi = require('../../lib/cartodb/api/query_tables_api'); +var OverviewsApi = require('../../lib/cartodb/api/overviews_api'); +var MapConfigOverviewsAdapter = require('../../lib/cartodb/models/mapconfig_overviews_adapter'); + +// configure redis pool instance to use in tests +var redisPool = new RedisPool(global.environment.redis); +var pgConnection = new PgConnection(require('cartodb-redis')({ pool: redisPool })); + +var redisPool = new RedisPool(global.environment.redis); +var metadataBackend = cartodbRedis({pool: redisPool}); +var pgConnection = new PgConnection(metadataBackend); +var pgQueryRunner = new PgQueryRunner(pgConnection); +var queryTablesApi = new QueryTablesApi(pgQueryRunner); +var overviewsApi = new OverviewsApi(queryTablesApi); + + +var mapConfigOverviewsAdapter = new MapConfigOverviewsAdapter(overviewsApi); + +describe('MapConfigOverviewsAdapter', function() { + + it('should not modify layers for which no overviews are available', function(done) { + var sql = 'SELECT * FROM test_table'; + var cartocss = '#layer { marker-fill: black; }'; + var cartocss_version = '2.3.0'; + var layer_without_overviews = { + type: 'cartodb', + options: { + sql: sql, + cartocss: cartocss, + cartocss_version: cartocss_version + } + }; + + mapConfigOverviewsAdapter.getLayers('localhost', [layer_without_overviews], function(err, layers) { + assert.ok(!err); + assert.equal(layers.length, 1); + assert.equal(layers[0].type, 'cartodb'); + assert.equal(layers[0].options.sql, sql); + assert.equal(layers[0].options.cartocss, cartocss); + assert.equal(layers[0].options.cartocss_version, cartocss_version); + assert.equal(layers[0].options.overviews, undefined); + done(); + }); + }); +}); + +describe('MapConfigOverviewsAdapter', function() { + + it('should add overviews metadata for layers using tables with overviews', function(done) { + var sql = 'SELECT * FROM test_table_overviews'; + var cartocss = '#layer { marker-fill: black; }'; + var cartocss_version = '2.3.0'; + var layer_without_overviews = { + type: 'cartodb', + options: { + sql: sql, + cartocss: cartocss, + cartocss_version: cartocss_version + } + }; + + mapConfigOverviewsAdapter.getLayers('localhost', [layer_without_overviews], function(err, layers) { + assert.ok(!err); + assert.equal(layers.length, 1); + assert.equal(layers[0].type, 'cartodb'); + assert.equal(layers[0].options.sql, sql); + assert.equal(layers[0].options.cartocss, cartocss); + assert.equal(layers[0].options.cartocss_version, cartocss_version); + assert.ok(layers[0].options.overviews); + assert.ok(layers[0].options.overviews['public.test_table_overviews']); + assert.deepEqual(_.keys(layers[0].options.overviews), ['public.test_table_overviews']); + assert.equal(_.keys(layers[0].options.overviews['public.test_table_overviews']).length, 2); + assert.ok(layers[0].options.overviews['public.test_table_overviews'][1]); + assert.ok(layers[0].options.overviews['public.test_table_overviews'][2]); + assert.equal(layers[0].options.overviews['public.test_table_overviews'][1].table, 'test_table_overviews_ov1'); + assert.equal(layers[0].options.overviews['public.test_table_overviews'][2].table, 'test_table_overviews_ov2'); + done(); + }); + }); +}); From 77f529d519f271de9d5dfb82df975f58c67a9a5a Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 17:36:25 +0100 Subject: [PATCH 22/43] Add acceptance test for overviews --- test/acceptance/overviews.js | 115 +++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 test/acceptance/overviews.js diff --git a/test/acceptance/overviews.js b/test/acceptance/overviews.js new file mode 100644 index 00000000..01d2a994 --- /dev/null +++ b/test/acceptance/overviews.js @@ -0,0 +1,115 @@ +var _ = require('underscore'); +var test_helper = require('../support/test_helper'); + +var assert = require('../support/assert'); +var CartodbWindshaft = require(__dirname + '/../../lib/cartodb/server'); +var serverOptions = require(__dirname + '/../../lib/cartodb/server_options'); +var server = new CartodbWindshaft(serverOptions); + +var LayergroupToken = require('../../lib/cartodb/models/layergroup_token'); + +var RedisPool = require('redis-mpool'); + +var step = require('step'); + +var windshaft = require('windshaft'); + + +describe('overviews', function() { + // configure redis pool instance to use in tests + var redisPool = new RedisPool(global.environment.redis); + + var overviews_layer = { + type: 'cartodb', + options: { + sql: 'SELECT * FROM test_table_overviews', + cartocss: '#layer { marker-fill: black; }', + cartocss_version: '2.3.0' + } + }; + + var non_overviews_layer = { + type: 'cartodb', + options: { + sql: 'SELECT * FROM test_table', + cartocss: '#layer { marker-fill: black; }', + cartocss_version: '2.3.0' + } + }; + + var keysToDelete; + + beforeEach(function() { + keysToDelete = {}; + }); + + afterEach(function(done) { + test_helper.deleteRedisKeys(keysToDelete, done); + }); + + it("layers with and without overviews", function(done) { + + var layergroup = { + version: '1.0.0', + layers: [overviews_layer, non_overviews_layer] + }; + + var layergroup_url = '/api/v1/map'; + + var expected_token; + step( + function do_post() + { + var next = this; + assert.response(server, { + url: layergroup_url, + method: 'POST', + headers: {host: 'localhost', 'Content-Type': 'application/json' }, + data: JSON.stringify(layergroup) + }, {}, function(res) { + assert.equal(res.statusCode, 200, res.body); + var parsedBody = JSON.parse(res.body); + assert.equal(res.headers['x-layergroup-id'], parsedBody.layergroupid); + expected_token = parsedBody.layergroupid; + next(null, res); + }); + }, + function do_get_mapconfig(err) + { + assert.ifError(err); + var next = this; + + var mapStore = new windshaft.storage.MapStore({ + pool: redisPool, + expire_time: 500000 + }); + mapStore.load(LayergroupToken.parse(expected_token).token, function(err, mapConfig) { + assert.ifError(err); + assert.deepEqual(non_overviews_layer, mapConfig._cfg.layers[1]); + assert.equal(mapConfig._cfg.layers[0].type, 'cartodb'); + assert.ok(mapConfig._cfg.layers[0].options.overviews); + assert.ok(mapConfig._cfg.layers[0].options.overviews['public.test_table_overviews']); + assert.deepEqual(_.keys(mapConfig._cfg.layers[0].options.overviews), ['public.test_table_overviews']); + assert.equal(_.keys(mapConfig._cfg.layers[0].options.overviews['public.test_table_overviews']).length, 2); + assert.ok(mapConfig._cfg.layers[0].options.overviews['public.test_table_overviews'][1]); + assert.ok(mapConfig._cfg.layers[0].options.overviews['public.test_table_overviews'][2]); + assert.equal( + mapConfig._cfg.layers[0].options.overviews['public.test_table_overviews'][1].table, + 'test_table_overviews_ov1' + ); + assert.equal( + mapConfig._cfg.layers[0].options.overviews['public.test_table_overviews'][2].table, + 'test_table_overviews_ov2' + ); + }); + + next(err); + }, + function finish(err) { + keysToDelete['map_cfg|' + LayergroupToken.parse(expected_token).token] = 0; + keysToDelete['user:localhost:mapviews:global'] = 5; + done(err); + } + ); + }); +}); From c8033700c3627265c083722268425e732c197e5c Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 17:40:57 +0100 Subject: [PATCH 23/43] Fix equality operator use --- lib/cartodb/models/mapconfig_overviews_adapter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index c98fe568..e9116ac8 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -17,7 +17,7 @@ MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callb var augmentLayersQueue = queue(layers.length); function augmentLayer(layer, done) { - if ( layer.type != 'mapnik' && layer.type != 'cartodb' ) { + if ( layer.type !== 'mapnik' && layer.type !== 'cartodb' ) { return done(null, layer); } self.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ From a6e3b07439a004e103165636cf228c541ff773e6 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 17:42:52 +0100 Subject: [PATCH 24/43] Reformat long lines --- test/integration/mapconfig_overviews_adapter.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/integration/mapconfig_overviews_adapter.js b/test/integration/mapconfig_overviews_adapter.js index 0db1c1e3..10a466a2 100644 --- a/test/integration/mapconfig_overviews_adapter.js +++ b/test/integration/mapconfig_overviews_adapter.js @@ -80,8 +80,14 @@ describe('MapConfigOverviewsAdapter', function() { assert.equal(_.keys(layers[0].options.overviews['public.test_table_overviews']).length, 2); assert.ok(layers[0].options.overviews['public.test_table_overviews'][1]); assert.ok(layers[0].options.overviews['public.test_table_overviews'][2]); - assert.equal(layers[0].options.overviews['public.test_table_overviews'][1].table, 'test_table_overviews_ov1'); - assert.equal(layers[0].options.overviews['public.test_table_overviews'][2].table, 'test_table_overviews_ov2'); + assert.equal( + layers[0].options.overviews['public.test_table_overviews'][1].table, + 'test_table_overviews_ov1' + ); + assert.equal( + layers[0].options.overviews['public.test_table_overviews'][2].table, + 'test_table_overviews_ov2' + ); done(); }); }); From 18246418a0a2522711008631a07aa0219acb4c16 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 17:49:51 +0100 Subject: [PATCH 25/43] Adapt test to new behaviour Now an error occurs before craeeteLayergroup when checking affected tables for overviews information. This prevents the creation of the map configuration, so the corresponding redis keys need not be deleted. The error message changes also because now the error originates in a different function call, QueryTablesApi.prototype.getAffectedTablesInQuery vs getAffectedTablesAndLastUpdatedTime. --- test/acceptance/multilayer_server.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/acceptance/multilayer_server.js b/test/acceptance/multilayer_server.js index e43070f1..a3ec8300 100644 --- a/test/acceptance/multilayer_server.js +++ b/test/acceptance/multilayer_server.js @@ -332,13 +332,9 @@ describe('tests from old api translated to multilayer', function() { assert.ok(!res.headers.hasOwnProperty('x-cache-channel')); - // TODO when affected tables query makes the request to fail layergroup should be removed - keysToDelete['map_cfg|4fb7bd7008322ce66f22d20aebba1ab0'] = 0; - keysToDelete['user:localhost:mapviews:global'] = 5; - var parsed = JSON.parse(res.body); assert.deepEqual(parsed, { - errors: ["Error: could not fetch affected tables or last updated time: fake error message"] + errors: ["could not fetch source tables: fake error message"] }); done(); From 8592136683d5da89c4811c919cf73277eb3b0c79 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 18:02:21 +0100 Subject: [PATCH 26/43] Change status code assigned to some errors Errors without an explicit status code with the error message containing 'does not exist' were assigned codes 404 or 403. Now if the error message is 'function X does not exist' (originated in SQL) the error code assigned is 400. --- lib/cartodb/controllers/base.js | 2 ++ test/acceptance/multilayer.js | 52 ++++++++++++++++----------------- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/lib/cartodb/controllers/base.js b/lib/cartodb/controllers/base.js index ac3ac11c..52251602 100644 --- a/lib/cartodb/controllers/base.js +++ b/lib/cartodb/controllers/base.js @@ -252,6 +252,8 @@ function statusFromErrorMessage(errMsg) { else if ( -1 !== errMsg.indexOf('does not exist') ) { if ( -1 !== errMsg.indexOf(' role ') ) { statusCode = 403; // role 'xxx' does not exist + } else if ( errMsg.match(/function .* does not exist/) ) { + statusCode = 400; // SQL error } else { statusCode = 404; } diff --git a/test/acceptance/multilayer.js b/test/acceptance/multilayer.js index 5d735278..b37113d6 100644 --- a/test/acceptance/multilayer.js +++ b/test/acceptance/multilayer.js @@ -52,14 +52,14 @@ describe(suiteName, function() { { options: { sql: 'select cartodb_id, ST_Translate(the_geom_webmercator, 5e6, 0) as the_geom_webmercator' + ' from test_table limit 2', - cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', + cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', cartocss_version: '2.0.1', interactivity: 'cartodb_id' } }, { options: { sql: 'select cartodb_id, ST_Translate(the_geom_webmercator, -5e6, 0) as the_geom_webmercator' + ' from test_table limit 2 offset 2', - cartocss: '#layer { marker-fill:blue; marker-allow-overlap:true; }', + cartocss: '#layer { marker-fill:blue; marker-allow-overlap:true; }', cartocss_version: '2.0.2', interactivity: 'cartodb_id' } } @@ -104,7 +104,7 @@ describe(suiteName, function() { // Check X-Cache-Channel cc = res.headers['x-cache-channel']; - assert.ok(cc); + assert.ok(cc); var dbname = test_database; assert.equal(cc.substring(0, dbname.length), dbname); if (!cdbQueryTablesFromPostgresEnabledValue) { // only test if it was using the SQL API @@ -198,7 +198,7 @@ describe(suiteName, function() { { options: { sql: 'select cartodb_id, ST_Translate(the_geom_webmercator, 5e6, 0) as the_geom_webmercator' + ' from test_table limit 2', - cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', + cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', cartocss_version: '2.0.1' } } ] @@ -211,7 +211,7 @@ describe(suiteName, function() { assert.response(server, { url: layergroup_url + '?config=' + encodeURIComponent(JSON.stringify(layergroup)), method: 'GET', - headers: {host: 'localhost'} + headers: {host: 'localhost'} }, {}, function(res, err) { next(err, res); }); }, function do_check_create(err, res) { @@ -245,7 +245,7 @@ describe(suiteName, function() { ] }; - var expected_token; + var expected_token; step( function do_create_get() { @@ -253,7 +253,7 @@ describe(suiteName, function() { assert.response(server, { url: layergroup_url + '?config=' + encodeURIComponent(JSON.stringify(layergroup)), method: 'GET', - headers: {host: 'localhost'} + headers: {host: 'localhost'} }, {}, function(res, err) { next(err, res); }); }, function do_check_create(err, res) { @@ -329,7 +329,7 @@ describe(suiteName, function() { { options: { sql: 'select 1 as cartodb_id, ST_Buffer(!bbox!, -32*greatest(!pixel_width!,!pixel_height!))' + ' as the_geom_webmercator from test_table limit 1', - cartocss: '#layer { polygon-fill:red; }', + cartocss: '#layer { polygon-fill:red; }', cartocss_version: '2.0.1', interactivity: 'cartodb_id' } } @@ -413,7 +413,7 @@ describe(suiteName, function() { // Check X-Cache-Channel var cc = res.headers['x-cache-channel']; - assert.ok(cc); + assert.ok(cc); var dbname = test_database; assert.equal(cc.substring(0, dbname.length), dbname); if (!cdbQueryTablesFromPostgresEnabledValue) { // only test if it was using the SQL API @@ -485,8 +485,8 @@ describe(suiteName, function() { { options: { sql: 'select 1 as cartodb_id, !pixel_height! as h,' + ' ST_Buffer(!bbox!, -32*greatest(!pixel_width!,!pixel_height!)) as the_geom_webmercator', - cartocss: '#layer { polygon-fill:red; }', - cartocss_version: '2.0.1' + cartocss: '#layer { polygon-fill:red; }', + cartocss_version: '2.0.1' } } ] }; @@ -578,8 +578,8 @@ describe(suiteName, function() { { options: { sql: 'select 1 as cartodb_id, !pixel_height! as h,' + 'ST_Buffer(!bbox!, -32*greatest(!pixel_width!,!pixel_height!)) as the_geom_webmercator', - cartocss: '#layer { polygon-fit:red; }', - cartocss_version: '2.0.1' + cartocss: '#layer { polygon-fit:red; }', + cartocss_version: '2.0.1' } } ] }; @@ -606,8 +606,8 @@ describe(suiteName, function() { layers: [ { options: { sql: 'select bogus(0,0) as the_geom_webmercator', - cartocss: '#layer { polygon-fill:red; }', - cartocss_version: '2.0.1' + cartocss: '#layer { polygon-fill:red; }', + cartocss_version: '2.0.1' } } ] }; @@ -617,7 +617,7 @@ describe(suiteName, function() { headers: {host: 'localhost', 'Content-Type': 'application/json' }, data: JSON.stringify(layergroup) }, {}, function(res) { - assert.equal(res.statusCode, 404, res.statusCode + ": " + res.body); + assert.equal(res.statusCode, 400, res.statusCode + ": " + res.body); var parsed = JSON.parse(res.body); var msg = parsed.errors[0]; assert.ok(msg.match(/bogus.*exist/), msg); @@ -633,13 +633,13 @@ describe(suiteName, function() { layers: [ { options: { sql: 'select * from test_table_private_1 where cartodb_id=1', - cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', + cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', cartocss_version: '2.1.0', interactivity: 'cartodb_id' } }, { options: { sql: 'select * from test_table_private_1 where cartodb_id=2', - cartocss: '#layer { marker-fill:blue; marker-allow-overlap:true; }', + cartocss: '#layer { marker-fill:blue; marker-allow-overlap:true; }', cartocss_version: '2.1.0', interactivity: 'cartodb_id' } } @@ -684,7 +684,7 @@ describe(suiteName, function() { // Check X-Cache-Channel var cc = res.headers['x-cache-channel']; - assert.ok(cc); + assert.ok(cc); var dbname = test_database; assert.equal(cc.substring(0, dbname.length), dbname); next(err); @@ -780,7 +780,7 @@ describe(suiteName, function() { layers: [ { options: { sql: 'select * from test_table where cartodb_id=1', - cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', + cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', cartocss_version: '2.1.0', interactivity: 'cartodb_id' } } @@ -830,7 +830,7 @@ describe(suiteName, function() { // Check X-Cache-Channel var cc = res.headers['x-cache-channel']; - assert.ok(cc, "Missing X-Cache-Channel"); + assert.ok(cc, "Missing X-Cache-Channel"); var dbname = test_database; assert.equal(cc.substring(0, dbname.length), dbname); return null; @@ -965,7 +965,7 @@ describe(suiteName, function() { layers: [ { options: { sql: "select 'SRID=3857;POINT(0 0)'::geometry as the_geom_webmercator", - cartocss: '#layer { point-transform:"scale(20)"; }', + cartocss: '#layer { point-transform:"scale(20)"; }', cartocss_version: '2.0.1' } } ] @@ -1031,7 +1031,7 @@ describe(suiteName, function() { layers: [ { options: { sql: "select * from test_table_private_1 LIMIT 0", - cartocss: '#layer { marker-fill:red; }', + cartocss: '#layer { marker-fill:red; }', cartocss_version: '2.0.1' } } ] @@ -1105,7 +1105,7 @@ describe(suiteName, function() { layers: [ { options: { sql: sql, - cartocss: '#layer { marker-fill:red; }', + cartocss: '#layer { marker-fill:red; }', cartocss_version: '2.0.1' } } ] @@ -1194,7 +1194,7 @@ describe(suiteName, function() { layers: [ { options: { sql: "select *, 'SQLAPINOANSWER' from test_table", - cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', + cartocss: '#layer { marker-fill:red; marker-width:32; marker-allow-overlap:true; }', cartocss_version: '2.1.0' } } ] @@ -1318,7 +1318,7 @@ describe(suiteName, function() { } ); }); - + }); }); From 81cb75f821807ef8fc7fc6bb6ac59a5626ff7de1 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 18:24:49 +0100 Subject: [PATCH 27/43] Refactor statusFromErrorMessage ...to make jshint happy --- lib/cartodb/controllers/base.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/cartodb/controllers/base.js b/lib/cartodb/controllers/base.js index 52251602..b402b412 100644 --- a/lib/cartodb/controllers/base.js +++ b/lib/cartodb/controllers/base.js @@ -249,11 +249,9 @@ function statusFromErrorMessage(errMsg) { else if (errMsg.match(/Postgis Plugin.*[\s|\n].*column.*does not exist/)) { statusCode = 400; } - else if ( -1 !== errMsg.indexOf('does not exist') ) { + else if ( -1 !== errMsg.indexOf('does not exist') && !errMsg.match(/function .* does not exist/ ) ) { if ( -1 !== errMsg.indexOf(' role ') ) { statusCode = 403; // role 'xxx' does not exist - } else if ( errMsg.match(/function .* does not exist/) ) { - statusCode = 400; // SQL error } else { statusCode = 404; } From 1f6d5cfd6d85185d07dde8fc1e14e776a7219c62 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 21 Jan 2016 18:39:31 +0100 Subject: [PATCH 28/43] Fix signature of CDB_Overviews --- test/support/sql/CDB_Overviews.sql | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/support/sql/CDB_Overviews.sql b/test/support/sql/CDB_Overviews.sql index 9e04f611..b5843f98 100644 --- a/test/support/sql/CDB_Overviews.sql +++ b/test/support/sql/CDB_Overviews.sql @@ -1,13 +1,13 @@ -- Mockup for CDB_Overviews -CREATE OR REPLACE FUNCTION CDB_Overviews(table_name text) -RETURNS TABLE(z integer, overview_table text) +CREATE OR REPLACE FUNCTION CDB_Overviews(table_name regclass) +RETURNS TABLE(z integer, overview_table regclass) AS $$ BEGIN - IF table_name = 'public.test_table_overviews' THEN + IF table_name::text = 'test_table_overviews' THEN RETURN QUERY - SELECT 1 AS z, 'test_table_overviews_ov1'::text AS overviw_table + SELECT 1 AS z, 'test_table_overviews_ov1'::regclass AS overviw_table UNION ALL - SELECT 2 AS z, 'test_table_overviews_ov2'::text AS overviw_table; + SELECT 2 AS z, 'test_table_overviews_ov2'::regclass AS overviw_table; ELSE RETURN; END IF; From 8d691b20481104fb2beea5c00d59b2ec63293b4d Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 22 Jan 2016 11:03:01 +0100 Subject: [PATCH 29/43] Refactor OverviewsApi Separate metadata processing into collecting each layer's information (map) and then organizing metadata per tables/zoom levels (reduce). --- lib/cartodb/api/overviews_api.js | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index 81d279b1..2a677210 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -19,8 +19,6 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) if (err) { callback(err); } else { - var metadata = {}; - var parallelism = 2; var q = queue(parallelism); @@ -33,23 +31,28 @@ OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) done(new Error('could not get overviews metadata: ' + msg)); return; } - if ( rows.length > 0 ) { - var table_metadata = {}; - for ( var i=0; i 0 ) { + var table_metadata = {}; + for ( var i=0; i Date: Fri, 22 Jan 2016 11:15:25 +0100 Subject: [PATCH 30/43] Remove commented code --- lib/cartodb/models/mapconfig_overviews_adapter.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index e9116ac8..f4236bda 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -25,7 +25,6 @@ MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callb done(err, layer); } else { if ( !_.isEmpty(metadata) ) { - // layer.options.overviews = metadata; layer = _.extend({}, layer); layer.options = _.extend({}, layer.options, { overviews: metadata }); } From ef9e9f8c78938b6e0d9739caa53164c6d6ffa23a Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 26 Jan 2016 11:38:21 +0100 Subject: [PATCH 31/43] Adapt to changes in CDB_Overviews SQL function Now data for multiple tables is obtained in one call, simplifying the use of this function. Also base table is returned as an oid, so we now have the overview base table names with schema only when needed. --- lib/cartodb/api/overviews_api.js | 96 ++++++++----------- test/acceptance/multilayer_server.js | 2 +- test/acceptance/overviews.js | 14 +-- .../mapconfig_overviews_adapter.js | 14 +-- test/integration/overviews-api.js | 2 +- test/support/sql/CDB_Overviews.sql | 10 +- 6 files changed, 61 insertions(+), 77 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index 2a677210..305d6422 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -1,61 +1,45 @@ -var queue = require('queue-async'); -var QueryTablesApi = require('./query_tables_api'); - -function OverviewsApi(pgQueryRunner) { - if (pgQueryRunner.pgQueryRunner !== undefined) { - this.queryTablesApi = pgQueryRunner; - this.pgQueryRunner = this.queryTablesApi.pgQueryRunner; - } else { - this.pgQueryRunner = pgQueryRunner; - this.queryTablesApi = new QueryTablesApi(pgQueryRunner); - } +// TODO: use pgQueryRunner parameter directly +function OverviewsApi(queryTablesApi) { + this.pgQueryRunner = queryTablesApi.pgQueryRunner; } module.exports = OverviewsApi; -OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) { - var self = this; - this.queryTablesApi.getAffectedTablesInQuery(username, sql, function(err, tableNames){ - if (err) { - callback(err); - } else { - var parallelism = 2; - var q = queue(parallelism); - - tableNames.forEach(function(tableName) { - q.defer(function(done){ - var query = "SELECT * FROM CDB_Overviews('" + tableName + "');"; - self.pgQueryRunner.run(username, query, function handleOverviewsRows(err, rows) { - if (err){ - var msg = err.message ? err.message : err; - done(new Error('could not get overviews metadata: ' + msg)); - return; - } - done(null, [tableName, rows]); - }); - }); - }); - - var metadata = {}; - q.awaitAll(function(err, results){ - if (err) { - return callback(err); - } else { - results.forEach(function(table_rows) { - var tableName = table_rows[0]; - var rows = table_rows[1]; - if ( rows.length > 0 ) { - var table_metadata = {}; - for ( var i=0; i Date: Tue, 26 Jan 2016 11:49:41 +0100 Subject: [PATCH 32/43] Refactor construction of OverviewsApi --- lib/cartodb/api/overviews_api.js | 4 ++-- lib/cartodb/controllers/map.js | 6 +++--- lib/cartodb/server.js | 3 +++ test/integration/mapconfig_overviews_adapter.js | 4 +--- test/integration/overviews-api.js | 2 +- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_api.js index 305d6422..98ac4881 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_api.js @@ -1,6 +1,6 @@ // TODO: use pgQueryRunner parameter directly -function OverviewsApi(queryTablesApi) { - this.pgQueryRunner = queryTablesApi.pgQueryRunner; +function OverviewsApi(pgQueryRunner) { + this.pgQueryRunner = pgQueryRunner; } module.exports = OverviewsApi; diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 803f5768..ba06a48f 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -18,7 +18,6 @@ var TablesCacheEntry = require('../cache/model/database_tables_entry'); var MapConfigNamedLayersAdapter = require('../models/mapconfig_named_layers_adapter'); var NamedMapMapConfigProvider = require('../models/mapconfig/named_map_provider'); var CreateLayergroupMapConfigProvider = require('../models/mapconfig/create_layergroup_provider'); -var OverviewsApi = require('../api/overviews_api'); var MapConfigOverviewsAdapter = require('../models/mapconfig_overviews_adapter'); /** @@ -33,7 +32,8 @@ var MapConfigOverviewsAdapter = require('../models/mapconfig_overviews_adapter') * @param {LayergroupAffectedTables} layergroupAffectedTables * @constructor */ -function MapController(authApi, pgConnection, templateMaps, mapBackend, metadataBackend, queryTablesApi, +function MapController(authApi, pgConnection, templateMaps, mapBackend, metadataBackend, + queryTablesApi, overviewsApi, surrogateKeysCache, userLimitsApi, layergroupAffectedTables) { BaseController.call(this, authApi, pgConnection); @@ -43,7 +43,7 @@ function MapController(authApi, pgConnection, templateMaps, mapBackend, metadata this.mapBackend = mapBackend; this.metadataBackend = metadataBackend; this.queryTablesApi = queryTablesApi; - this.overviewsApi = new OverviewsApi(this.queryTablesApi); + this.overviewsApi = overviewsApi; this.surrogateKeysCache = surrogateKeysCache; this.userLimitsApi = userLimitsApi; this.layergroupAffectedTables = layergroupAffectedTables; diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index f4205676..0a1f92af 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -20,6 +20,7 @@ var mapnik = windshaft.mapnik; var TemplateMaps = require('./backends/template_maps.js'); var QueryTablesApi = require('./api/query_tables_api'); +var OverviewsApi = require('./api/overviews_api'); var UserLimitsApi = require('./api/user_limits_api'); var AuthApi = require('./api/auth_api'); var LayergroupAffectedTablesCache = require('./cache/layergroup_affected_tables'); @@ -52,6 +53,7 @@ module.exports = function(serverOptions) { var pgConnection = new PgConnection(metadataBackend); var pgQueryRunner = new PgQueryRunner(pgConnection); var queryTablesApi = new QueryTablesApi(pgQueryRunner); + var overviewsApi = new OverviewsApi(pgQueryRunner); var userLimitsApi = new UserLimitsApi(metadataBackend, { limits: { cacheOnTimeout: serverOptions.renderer.mapnik.limits.cacheOnTimeout || false, @@ -175,6 +177,7 @@ module.exports = function(serverOptions) { mapBackend, metadataBackend, queryTablesApi, + overviewsApi, surrogateKeysCache, userLimitsApi, layergroupAffectedTablesCache diff --git a/test/integration/mapconfig_overviews_adapter.js b/test/integration/mapconfig_overviews_adapter.js index e6178d1b..b7d95cfc 100644 --- a/test/integration/mapconfig_overviews_adapter.js +++ b/test/integration/mapconfig_overviews_adapter.js @@ -6,7 +6,6 @@ var RedisPool = require('redis-mpool'); var cartodbRedis = require('cartodb-redis'); var PgConnection = require(__dirname + '/../../lib/cartodb/backends/pg_connection'); var PgQueryRunner = require('../../lib/cartodb/backends/pg_query_runner'); -var QueryTablesApi = require('../../lib/cartodb/api/query_tables_api'); var OverviewsApi = require('../../lib/cartodb/api/overviews_api'); var MapConfigOverviewsAdapter = require('../../lib/cartodb/models/mapconfig_overviews_adapter'); @@ -18,8 +17,7 @@ var redisPool = new RedisPool(global.environment.redis); var metadataBackend = cartodbRedis({pool: redisPool}); var pgConnection = new PgConnection(metadataBackend); var pgQueryRunner = new PgQueryRunner(pgConnection); -var queryTablesApi = new QueryTablesApi(pgQueryRunner); -var overviewsApi = new OverviewsApi(queryTablesApi); +var overviewsApi = new OverviewsApi(pgQueryRunner); var mapConfigOverviewsAdapter = new MapConfigOverviewsAdapter(overviewsApi); diff --git a/test/integration/overviews-api.js b/test/integration/overviews-api.js index c7dface6..0d28f050 100644 --- a/test/integration/overviews-api.js +++ b/test/integration/overviews-api.js @@ -21,7 +21,7 @@ describe('OverviewsApi', function() { var pgConnection = new PgConnection(metadataBackend); var pgQueryRunner = new PgQueryRunner(pgConnection); queryTablesApi = new QueryTablesApi(pgQueryRunner); - overviewsApi = new OverviewsApi(queryTablesApi); + overviewsApi = new OverviewsApi(pgQueryRunner); }); it('should return an empty relation for tables that have no overviews', function(done) { From 6dbb0cb1c1779cdd924e9eae33e4d520e873b7de Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 26 Jan 2016 15:08:55 +0100 Subject: [PATCH 33/43] Emulate new overview table naming schme in the tests --- test/acceptance/overviews.js | 4 ++-- test/integration/mapconfig_overviews_adapter.js | 4 ++-- test/integration/overviews-api.js | 4 ++-- test/support/sql/CDB_Overviews.sql | 4 ++-- test/support/sql/windshaft.test.sql | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/test/acceptance/overviews.js b/test/acceptance/overviews.js index 12b23fe0..10e567f5 100644 --- a/test/acceptance/overviews.js +++ b/test/acceptance/overviews.js @@ -95,11 +95,11 @@ describe('overviews', function() { assert.ok(mapConfig._cfg.layers[0].options.overviews.test_table_overviews[2]); assert.equal( mapConfig._cfg.layers[0].options.overviews.test_table_overviews[1].table, - 'test_table_overviews_ov1' + '_vovw_1_test_table_overviews' ); assert.equal( mapConfig._cfg.layers[0].options.overviews.test_table_overviews[2].table, - 'test_table_overviews_ov2' + '_vovw_2_test_table_overviews' ); }); diff --git a/test/integration/mapconfig_overviews_adapter.js b/test/integration/mapconfig_overviews_adapter.js index b7d95cfc..5ee879c2 100644 --- a/test/integration/mapconfig_overviews_adapter.js +++ b/test/integration/mapconfig_overviews_adapter.js @@ -80,11 +80,11 @@ describe('MapConfigOverviewsAdapter', function() { assert.ok(layers[0].options.overviews.test_table_overviews[2]); assert.equal( layers[0].options.overviews.test_table_overviews[1].table, - 'test_table_overviews_ov1' + '_vovw_1_test_table_overviews' ); assert.equal( layers[0].options.overviews.test_table_overviews[2].table, - 'test_table_overviews_ov2' + '_vovw_2_test_table_overviews' ); done(); }); diff --git a/test/integration/overviews-api.js b/test/integration/overviews-api.js index 0d28f050..ff256821 100644 --- a/test/integration/overviews-api.js +++ b/test/integration/overviews-api.js @@ -42,8 +42,8 @@ describe('OverviewsApi', function() { assert.deepEqual(result, { 'test_table_overviews': { - 1: { table: 'test_table_overviews_ov1' }, - 2: { table: 'test_table_overviews_ov2' } + 1: { table: '_vovw_1_test_table_overviews' }, + 2: { table: '_vovw_2_test_table_overviews' } } }); diff --git a/test/support/sql/CDB_Overviews.sql b/test/support/sql/CDB_Overviews.sql index 0acb16b9..b081ae57 100644 --- a/test/support/sql/CDB_Overviews.sql +++ b/test/support/sql/CDB_Overviews.sql @@ -5,9 +5,9 @@ AS $$ BEGIN IF (SELECT 'test_table_overviews'::regclass = ANY (table_names)) THEN RETURN QUERY - SELECT 'test_table_overviews'::regclass AS base_table, 1 AS z, 'test_table_overviews_ov1'::regclass AS overviw_table + SELECT 'test_table_overviews'::regclass AS base_table, 1 AS z, '_vovw_1_test_table_overviews'::regclass AS overview_table UNION ALL - SELECT 'test_table_overviews'::regclass AS base_table, 2 AS z, 'test_table_overviews_ov2'::regclass AS overviw_table; + SELECT 'test_table_overviews'::regclass AS base_table, 2 AS z, '_vovw_2_test_table_overviews'::regclass AS overview_table; ELSE RETURN; END IF; diff --git a/test/support/sql/windshaft.test.sql b/test/support/sql/windshaft.test.sql index 3b795c01..85851300 100644 --- a/test/support/sql/windshaft.test.sql +++ b/test/support/sql/windshaft.test.sql @@ -285,7 +285,7 @@ CREATE INDEX test_table_overviews_the_geom_webmercator_idx ON test_table_overvie GRANT ALL ON TABLE test_table_overviews TO :TESTUSER; GRANT SELECT ON TABLE test_table_overviews TO :PUBLICUSER; -CREATE TABLE test_table_overviews_ov1 ( +CREATE TABLE _vovw_1_test_table_overviews ( updated_at timestamp without time zone DEFAULT now(), created_at timestamp without time zone DEFAULT now(), cartodb_id integer NOT NULL, @@ -301,7 +301,7 @@ CREATE TABLE test_table_overviews_ov1 ( CONSTRAINT enforce_srid_the_geom_webmercator CHECK ((st_srid(the_geom_webmercator) = 3857)) ); -CREATE TABLE test_table_overviews_ov2 ( +CREATE TABLE _vovw_2_test_table_overviews ( updated_at timestamp without time zone DEFAULT now(), created_at timestamp without time zone DEFAULT now(), cartodb_id integer NOT NULL, From 37a4aaeeb42f00466513ba0c673c9531fe91b893 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 27 Jan 2016 17:39:24 +0100 Subject: [PATCH 34/43] Refactor findStatusCode for legibility ...disregarding jshint opinion --- lib/cartodb/controllers/base.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/controllers/base.js b/lib/cartodb/controllers/base.js index b402b412..9945ff5a 100644 --- a/lib/cartodb/controllers/base.js +++ b/lib/cartodb/controllers/base.js @@ -239,6 +239,7 @@ module.exports.findStatusCode = findStatusCode; function statusFromErrorMessage(errMsg) { // Find an appropriate statusCode based on message + // jshint maxcomplexity:7 var statusCode = 400; if ( -1 !== errMsg.indexOf('permission denied') ) { statusCode = 403; @@ -249,9 +250,11 @@ function statusFromErrorMessage(errMsg) { else if (errMsg.match(/Postgis Plugin.*[\s|\n].*column.*does not exist/)) { statusCode = 400; } - else if ( -1 !== errMsg.indexOf('does not exist') && !errMsg.match(/function .* does not exist/ ) ) { + else if ( -1 !== errMsg.indexOf('does not exist') ) { if ( -1 !== errMsg.indexOf(' role ') ) { statusCode = 403; // role 'xxx' does not exist + } else if ( errMsg.match(/function .* does not exist/) ) { + statusCode = 400; // invalid SQL (SQL function does not exist) } else { statusCode = 404; } From 5038ae6b1bc140c6efd0a08b38fbd9bc43c78cf9 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 27 Jan 2016 17:40:12 +0100 Subject: [PATCH 35/43] Fix data for overviews integration tests --- test/support/sql/CDB_Overviews.sql | 6 ++++++ test/support/sql/windshaft.test.sql | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/test/support/sql/CDB_Overviews.sql b/test/support/sql/CDB_Overviews.sql index b081ae57..fb927938 100644 --- a/test/support/sql/CDB_Overviews.sql +++ b/test/support/sql/CDB_Overviews.sql @@ -13,3 +13,9 @@ AS $$ END IF; END $$ LANGUAGE PLPGSQL; + +CREATE OR REPLACE FUNCTION CDB_ZoomFromScale(scaleDenominator numeric) RETURNS int AS $$ +BEGIN + RETURN 0; +END +$$ LANGUAGE plpgsql IMMUTABLE; diff --git a/test/support/sql/windshaft.test.sql b/test/support/sql/windshaft.test.sql index 85851300..66a3de69 100644 --- a/test/support/sql/windshaft.test.sql +++ b/test/support/sql/windshaft.test.sql @@ -257,6 +257,9 @@ CREATE TABLE test_table_overviews ( CONSTRAINT enforce_srid_the_geom_webmercator CHECK ((st_srid(the_geom_webmercator) = 3857)) ); +GRANT ALL ON TABLE test_table_overviews TO :TESTUSER; +GRANT SELECT ON TABLE test_table_overviews TO :PUBLICUSER; + CREATE SEQUENCE test_table_overviews_cartodb_id_seq START WITH 1 INCREMENT BY 1 @@ -301,6 +304,9 @@ CREATE TABLE _vovw_1_test_table_overviews ( CONSTRAINT enforce_srid_the_geom_webmercator CHECK ((st_srid(the_geom_webmercator) = 3857)) ); +GRANT ALL ON TABLE _vovw_1_test_table_overviews TO :TESTUSER; +GRANT SELECT ON TABLE _vovw_1_test_table_overviews TO :PUBLICUSER; + CREATE TABLE _vovw_2_test_table_overviews ( updated_at timestamp without time zone DEFAULT now(), created_at timestamp without time zone DEFAULT now(), @@ -316,3 +322,6 @@ CREATE TABLE _vovw_2_test_table_overviews ( CONSTRAINT enforce_srid_the_geom CHECK ((st_srid(the_geom) = 4326)), CONSTRAINT enforce_srid_the_geom_webmercator CHECK ((st_srid(the_geom_webmercator) = 3857)) ); + +GRANT ALL ON TABLE _vovw_2_test_table_overviews TO :TESTUSER; +GRANT SELECT ON TABLE _vovw_2_test_table_overviews TO :PUBLICUSER; From f5b12d81ed9e80f1cc530be44219f6dad44f9d37 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 28 Jan 2016 19:49:16 +0100 Subject: [PATCH 36/43] Fix indent --- test/acceptance/multilayer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/multilayer.js b/test/acceptance/multilayer.js index b37113d6..61be1939 100644 --- a/test/acceptance/multilayer.js +++ b/test/acceptance/multilayer.js @@ -577,7 +577,7 @@ describe(suiteName, function() { layers: [ { options: { sql: 'select 1 as cartodb_id, !pixel_height! as h,' + - 'ST_Buffer(!bbox!, -32*greatest(!pixel_width!,!pixel_height!)) as the_geom_webmercator', + 'ST_Buffer(!bbox!, -32*greatest(!pixel_width!,!pixel_height!)) as the_geom_webmercator', cartocss: '#layer { polygon-fit:red; }', cartocss_version: '2.0.1' } } From 8348f745134704ef99d6af169f79d39d96aeabab Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 2 Feb 2016 19:23:07 +0100 Subject: [PATCH 37/43] Provide OverviewsHandler configuration to Windshaft A parameter has been added to Windshaft Mapnik renderer configuration to define how queries will be adapted to use overviews. Here we're using the default OverviewHandler providen in Windshaft, with a parameter to define how the zoom level is determined. --- lib/cartodb/server_options.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/server_options.js b/lib/cartodb/server_options.js index c8af69d8..2acdd2d1 100644 --- a/lib/cartodb/server_options.js +++ b/lib/cartodb/server_options.js @@ -1,5 +1,11 @@ var os = require('os'); var _ = require('underscore'); +var windshaft = require('windshaft'); +var OverviewsHandler = windshaft.OverviewsHandler; + +var overviewsHandler = new OverviewsHandler({ + zoom_level: 'CDB_ZoomFromScale(!scale_denominator!)' +}); var rendererConfig = _.defaults(global.environment.renderer || {}, { cache_ttl: 60000, // milliseconds @@ -10,11 +16,13 @@ var rendererConfig = _.defaults(global.environment.renderer || {}, { bufferSize: 64, snapToGrid: false, clipByBox2d: false, - limits: {} + limits: {}, }, http: {} }); +rendererConfig.mapnik.overviewsHandler = overviewsHandler; + // Perform keyword substitution in statsd // See https://github.com/CartoDB/Windshaft-cartodb/issues/153 if ( global.environment.statsd ) { From 870688309a35009f9c1f222c38a77cf546cfd908 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 2 Feb 2016 19:29:10 +0100 Subject: [PATCH 38/43] Fix syntax --- lib/cartodb/server_options.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/server_options.js b/lib/cartodb/server_options.js index 2acdd2d1..cbe757d5 100644 --- a/lib/cartodb/server_options.js +++ b/lib/cartodb/server_options.js @@ -16,7 +16,7 @@ var rendererConfig = _.defaults(global.environment.renderer || {}, { bufferSize: 64, snapToGrid: false, clipByBox2d: false, - limits: {}, + limits: {} }, http: {} }); From 0a218da835fe89c31d67dad77edb26ee6dc1c43c Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 4 Feb 2016 10:26:31 +0100 Subject: [PATCH 39/43] Implement an Overviews query rewriter Use the Windshaft query-rewriter interface to adapt queries so they use available overview tables. This requires a version of Windshaft that implements the query-rewriter interface (package.json/npm-shrinkwap.json have yet to be updated) --- ...views_api.js => overviews_metadata_api.js} | 9 +- lib/cartodb/controllers/map.js | 6 +- .../models/mapconfig_overviews_adapter.js | 8 +- lib/cartodb/server.js | 6 +- lib/cartodb/server_options.js | 7 +- lib/cartodb/utils/overviews_query_rewriter.js | 197 ++++++++ lib/cartodb/utils/table_name_parser.js | 106 +++++ .../{overviews.js => overviews_metadata.js} | 27 +- test/acceptance/overviews_queries.js | 72 +++ .../ported/support/ported_server_options.js | 7 +- test/fixtures/_vovw_1_test_table_1_0_0.png | Bin 0 -> 1114 bytes test/fixtures/_vovw_2_test_table_2_1_1.png | Bin 0 -> 1463 bytes test/fixtures/test_table_1_0_0.png | Bin 0 -> 888 bytes test/fixtures/test_table_2_1_1.png | Bin 0 -> 1030 bytes test/fixtures/test_table_3_3_3.png | Bin 0 -> 834 bytes .../mapconfig_overviews_adapter.js | 31 +- ...views-api.js => overviews-metadata-api.js} | 12 +- test/support/sql/CDB_Overviews.sql | 27 +- test/support/sql/windshaft.test.sql | 7 + test/unit/cartodb/overviews_query_rewriter.js | 429 ++++++++++++++++++ test/unit/cartodb/table_name_parser.js | 60 +++ 21 files changed, 950 insertions(+), 61 deletions(-) rename lib/cartodb/api/{overviews_api.js => overviews_metadata_api.js} (82%) create mode 100644 lib/cartodb/utils/overviews_query_rewriter.js create mode 100644 lib/cartodb/utils/table_name_parser.js rename test/acceptance/{overviews.js => overviews_metadata.js} (74%) create mode 100644 test/acceptance/overviews_queries.js create mode 100644 test/fixtures/_vovw_1_test_table_1_0_0.png create mode 100644 test/fixtures/_vovw_2_test_table_2_1_1.png create mode 100644 test/fixtures/test_table_1_0_0.png create mode 100644 test/fixtures/test_table_2_1_1.png create mode 100644 test/fixtures/test_table_3_3_3.png rename test/integration/{overviews-api.js => overviews-metadata-api.js} (76%) create mode 100644 test/unit/cartodb/overviews_query_rewriter.js create mode 100644 test/unit/cartodb/table_name_parser.js diff --git a/lib/cartodb/api/overviews_api.js b/lib/cartodb/api/overviews_metadata_api.js similarity index 82% rename from lib/cartodb/api/overviews_api.js rename to lib/cartodb/api/overviews_metadata_api.js index 98ac4881..f3568f5c 100644 --- a/lib/cartodb/api/overviews_api.js +++ b/lib/cartodb/api/overviews_metadata_api.js @@ -1,11 +1,10 @@ -// TODO: use pgQueryRunner parameter directly -function OverviewsApi(pgQueryRunner) { +function OverviewsMetadataApi(pgQueryRunner) { this.pgQueryRunner = pgQueryRunner; } -module.exports = OverviewsApi; +module.exports = OverviewsMetadataApi; -// TODO: share this with OverviewsApi? ... or maintain independence? +// TODO: share this with QueryTablesApi? ... or maintain independence? var affectedTableRegexCache = { bbox: /!bbox!/g, scale_denominator: /!scale_denominator!/g, @@ -22,7 +21,7 @@ function prepareSql(sql) { ; } -OverviewsApi.prototype.getOverviewsMetadata = function (username, sql, callback) { +OverviewsMetadataApi.prototype.getOverviewsMetadata = function (username, sql, callback) { var query = 'SELECT * FROM CDB_Overviews(CDB_QueryTablesText($windshaft$' + prepareSql(sql) + '$windshaft$))'; this.pgQueryRunner.run(username, query, function handleOverviewsRows(err, rows) { if (err){ diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index ba06a48f..ee49e0eb 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -33,7 +33,7 @@ var MapConfigOverviewsAdapter = require('../models/mapconfig_overviews_adapter') * @constructor */ function MapController(authApi, pgConnection, templateMaps, mapBackend, metadataBackend, - queryTablesApi, overviewsApi, + queryTablesApi, overviewsMetadataApi, surrogateKeysCache, userLimitsApi, layergroupAffectedTables) { BaseController.call(this, authApi, pgConnection); @@ -43,13 +43,13 @@ function MapController(authApi, pgConnection, templateMaps, mapBackend, metadata this.mapBackend = mapBackend; this.metadataBackend = metadataBackend; this.queryTablesApi = queryTablesApi; - this.overviewsApi = overviewsApi; + this.overviewsMetadataApi = overviewsMetadataApi; this.surrogateKeysCache = surrogateKeysCache; this.userLimitsApi = userLimitsApi; this.layergroupAffectedTables = layergroupAffectedTables; this.namedLayersAdapter = new MapConfigNamedLayersAdapter(templateMaps); - this.overviewsAdapter = new MapConfigOverviewsAdapter(this.overviewsApi); + this.overviewsAdapter = new MapConfigOverviewsAdapter(this.overviewsMetadataApi); } util.inherits(MapController, BaseController); diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index f4236bda..72a29485 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -1,8 +1,8 @@ var queue = require('queue-async'); var _ = require('underscore'); -function MapConfigOverviewsAdapter(overviewsApi) { - this.overviewsApi = overviewsApi; +function MapConfigOverviewsAdapter(overviewsMetadataApi) { + this.overviewsMetadataApi = overviewsMetadataApi; } module.exports = MapConfigOverviewsAdapter; @@ -20,13 +20,13 @@ MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callb if ( layer.type !== 'mapnik' && layer.type !== 'cartodb' ) { return done(null, layer); } - self.overviewsApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ + self.overviewsMetadataApi.getOverviewsMetadata(username, layer.options.sql, function(err, metadata){ if (err) { done(err, layer); } else { if ( !_.isEmpty(metadata) ) { layer = _.extend({}, layer); - layer.options = _.extend({}, layer.options, { overviews: metadata }); + layer.options = _.extend({}, layer.options, { query_rewrite_data: { overviews: metadata } }); } done(null, layer); } diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 0a1f92af..9f9d9f69 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -20,7 +20,7 @@ var mapnik = windshaft.mapnik; var TemplateMaps = require('./backends/template_maps.js'); var QueryTablesApi = require('./api/query_tables_api'); -var OverviewsApi = require('./api/overviews_api'); +var OverviewsMetadataApi = require('./api/overviews_metadata_api'); var UserLimitsApi = require('./api/user_limits_api'); var AuthApi = require('./api/auth_api'); var LayergroupAffectedTablesCache = require('./cache/layergroup_affected_tables'); @@ -53,7 +53,7 @@ module.exports = function(serverOptions) { var pgConnection = new PgConnection(metadataBackend); var pgQueryRunner = new PgQueryRunner(pgConnection); var queryTablesApi = new QueryTablesApi(pgQueryRunner); - var overviewsApi = new OverviewsApi(pgQueryRunner); + var overviewsMetadataApi = new OverviewsMetadataApi(pgQueryRunner); var userLimitsApi = new UserLimitsApi(metadataBackend, { limits: { cacheOnTimeout: serverOptions.renderer.mapnik.limits.cacheOnTimeout || false, @@ -177,7 +177,7 @@ module.exports = function(serverOptions) { mapBackend, metadataBackend, queryTablesApi, - overviewsApi, + overviewsMetadataApi, surrogateKeysCache, userLimitsApi, layergroupAffectedTablesCache diff --git a/lib/cartodb/server_options.js b/lib/cartodb/server_options.js index cbe757d5..ff2800f5 100644 --- a/lib/cartodb/server_options.js +++ b/lib/cartodb/server_options.js @@ -1,9 +1,8 @@ var os = require('os'); var _ = require('underscore'); -var windshaft = require('windshaft'); -var OverviewsHandler = windshaft.OverviewsHandler; +var OverviewsQueryRewriter = require('./utils/overviews_query_rewriter'); -var overviewsHandler = new OverviewsHandler({ +var overviewsQueryRewriter = new OverviewsQueryRewriter({ zoom_level: 'CDB_ZoomFromScale(!scale_denominator!)' }); @@ -21,7 +20,7 @@ var rendererConfig = _.defaults(global.environment.renderer || {}, { http: {} }); -rendererConfig.mapnik.overviewsHandler = overviewsHandler; +rendererConfig.mapnik.queryRewriter = overviewsQueryRewriter; // Perform keyword substitution in statsd // See https://github.com/CartoDB/Windshaft-cartodb/issues/153 diff --git a/lib/cartodb/utils/overviews_query_rewriter.js b/lib/cartodb/utils/overviews_query_rewriter.js new file mode 100644 index 00000000..0758fa60 --- /dev/null +++ b/lib/cartodb/utils/overviews_query_rewriter.js @@ -0,0 +1,197 @@ +var TableNameParser = require('./table_name_parser'); + +function OverviewsQueryRewriter(options) { + + this.options = options; +} + +module.exports = OverviewsQueryRewriter; + +// TODO: some names are introudced in the queries, and the +// '_vovw_' (for vector overviews) is used in them, but no check +// is performed for conflicts with existing identifiers in the query. + +// Build UNION expression to replace table, using overviews metadata +// overviews metadata: { 1: 'table_ov1', ... } +// assume table and overview names include schema if necessary and are quoted as needed +function overviews_view_for_table(table, overviews_metadata, indent) { + var condition, i, len, ov_table, overview_layers, selects, z_hi, z_lo; + var parsed_table = TableNameParser.parse(table); + + var sorted_overviews = []; // [[1, 'table_ov1'], ...] + + indent = indent || ' '; + for (var z in overviews_metadata) { + if (overviews_metadata.hasOwnProperty(z)) { + sorted_overviews.push([z, overviews_metadata[z].table]); + } + } + sorted_overviews.sort(function(a, b){ return a[0]-b[0]; }); + + overview_layers = []; + z_lo = null; + for (i = 0, len = sorted_overviews.length; i < len; i++) { + z_hi = parseInt(sorted_overviews[i][0]); + ov_table = sorted_overviews[i][1]; + overview_layers.push([overview_z_condition(z_lo, z_hi), ov_table]); + z_lo = z_hi; + } + overview_layers.push(["_vovw_z > " + z_lo, table]); + + selects = overview_layers.map(function(condition_table) { + condition = condition_table[0]; + ov_table = TableNameParser.parse(condition_table[1]); + ov_table.schema = ov_table.schema || parsed_table.schema; + var ov_identifier = TableNameParser.table_identifier(ov_table); + return indent + "SELECT * FROM " + ov_identifier + ", _vovw_scale WHERE " + condition; + }); + + return selects.join("\n"+indent+"UNION ALL\n"); +} + +function overview_z_condition(z_lo, z_hi) { + if (z_lo !== null) { + if (z_lo === z_hi - 1) { + return "_vovw_z = " + z_hi; + } else { + return "_vovw_z > " + z_lo + " AND _vovw_z <= " + z_hi; + } + } else { + if (z_hi === 0) { + return "_vovw_z = " + z_hi; + } else { + return "_vovw_z <= " + z_hi; + } + } +} + +// name to be used for the view of the table using overviews +function overviews_view_name(table) { + var parsed_table = TableNameParser.parse(table); + parsed_table.table = '_vovw_' + parsed_table.table; + parsed_table.schema = null; + return TableNameParser.table_identifier(parsed_table); +} + +// replace a table name in a query by anoter name +function replace_table_in_query(sql, old_table_name, new_table_name) { + var old_table = TableNameParser.parse(old_table_name); + var new_table = TableNameParser.parse(new_table_name); + var old_table_ident = TableNameParser.table_identifier(old_table); + var new_table_ident = TableNameParser.table_identifier(new_table); + + // text that will be substituted by the table name pattern + var replacement = new_table_ident; + + // regular expression prefix (beginning) to match a table name + function pattern_prefix(schema, identifier) { + if ( schema ) { + // to match a table name including schema prefix + // name should not be part of another name, so we require + // to start a at a word boundary + if ( identifier[0] !== '"' ) { + return '\\b'; + } else { + return ''; + } + } else { + // to match a table name without schema + // name should not begin right after a dot (i.e. have a explicit schema) + // nor be part of another name + // since the pattern matches the first character of the table + // it must be put back in the replacement text + replacement = '$01'+replacement; + return '([^\.a-z0-9_]|^)'; + } + } + + // regular expression suffix (ending) to match a table name + function pattern_suffix(identifier) { + // name shouldn't be the prefix of a longer name + if ( identifier[identifier.length-1] !== '"' ) { + return '\\b'; + } else { + return ''; + } + } + + // regular expression to match a table name + var regexp = pattern_prefix(old_table.schema, old_table_ident) + + old_table_ident + + pattern_suffix(old_table_ident); + + // replace all occurrences of the table pattern + return sql.replace(new RegExp(regexp, 'g'), replacement); +} + +function overviews_query(query, overviews, zoom_level_expression) { + var replaced_query = query; + var sql = "WITH\n _vovw_scale AS ( SELECT " + zoom_level_expression + " AS _vovw_z )"; + for ( var table in overviews ) { + if (overviews.hasOwnProperty(table)) { + var table_overviews = overviews[table]; + var table_view = overviews_view_name(table); + replaced_query = replace_table_in_query(replaced_query, table, table_view); + sql += ",\n " + table_view + " AS (\n" + overviews_view_for_table(table, table_overviews) + "\n )"; + } + } + if ( replaced_query !== query ) { + sql += "\n"; + sql += replaced_query; + } else { + sql = query; + } + return sql; +} + +// Transform an SQL query so that it uses overviews. +// overviews contains metadata about the overviews to be used: +// { 'table-name': {1: { table: 'overview-table-1' }, ... }, ... } +// +// For a given query `SELECT * FROM table`, if any of tables in it +// has overviews as defined by the provided metadat, the query will +// be transform into something similar to this: +// +// WITH _vovw_scale AS ( ... ), -- define scale level +// WITH _vovw_table AS ( ... ), -- define union of overviews and base table +// SELECT * FROM _vovw_table -- query with table replaced by _vovw_table +// +// This transformation can in principle be applied to arbitrary queries +// (except for the case of queries that include the name of tables with +// overviews inside text literals: at the current table name substitution +// doesnn't prevent substitution inside literals). +// But the transformation will currently only be applied to simple queries +// of the form detected by the overviews_supported_query function. +OverviewsQueryRewriter.prototype.query = function(query, data) { + var overviews = this.overviews_metadata(data); + if ( !overviews || !this.is_supported_query(query)) { + return query; + } + var zoom_level_expression = this.options.zoom_level || '0'; + return overviews_query(query, overviews, zoom_level_expression); +}; + +OverviewsQueryRewriter.prototype.style = function(cartocss, cartocss_version, data) { + var overviews = this.overviews_metadata(data); + if ( !overviews ) { + return cartocss; + } + for ( var table in overviews ) { + if (overviews.hasOwnProperty(table)) { + // No modification of the CartoCSS is currently performed + // var table_view = overviews_view_name(table); + // cartocss = replace_table_in_style(cartocss, table, table_view); + } + } + return cartocss; +}; + +OverviewsQueryRewriter.prototype.is_supported_query = function(sql) { + return !!sql.match( + /^\s*SELECT\s+[\*\.a-z0-9_,\s]+?\s+FROM\s+((\"[^"]+\"|[a-z0-9_]+)\.)?(\"[^"]+\"|[a-z0-9_]+)\s*;?\s*$/i + ); +}; + +OverviewsQueryRewriter.prototype.overviews_metadata = function(data) { + return data && data.overviews; +}; diff --git a/lib/cartodb/utils/table_name_parser.js b/lib/cartodb/utils/table_name_parser.js new file mode 100644 index 00000000..f71c3f34 --- /dev/null +++ b/lib/cartodb/utils/table_name_parser.js @@ -0,0 +1,106 @@ +// Quote an PostgreSQL identifier if ncecessary +function quote_identifier_if_needed(txt) { + if ( txt && !txt.match(/^[a-z_][a-z_0-9]*$/)) { + return '"' + txt.replace(/\"/g, '""') + '"'; + } else { + return txt; + } +} + +// Parse PostgreSQL table name (possibly quoted and with optional schema).+ +// Returns { schema: 'schema_name', table: 'table_name' } +function parse_table_name(table) { + + function split_as_quoted_parts(table_name) { + // parse table into 'parts' that may be quoted, each part + // in the parts array being an object { part: 'text', quoted: false/true } + var parts = []; + var splitted = table_name.split(/\"/); + for (var i=0; i 0 && i < splitted.length-1 ) { + i++; + parts[parts.length - 1].part += '"' + splitted[i]; + } + } + else { + var is_quoted = (i > 0 && splitted[i-1] === '') || + (i < splitted.length - 1 && splitted[i+1] === ''); + parts.push({ part: splitted[i], quoted: is_quoted }); + } + } + return parts; + } + + var parts = split_as_quoted_parts(table); + + function split_single_part(part) { + var schema_part = null; + var table_part = null; + if ( part.quoted ) { + table_part = part.part; + } else { + var parts = part.part.split('.'); + if ( parts.length === 1 ) { + schema_part = null; + table_part = parts[0]; + } else if ( parts.length === 2 ) { + schema_part = parts[0]; + table_part = parts[1]; + } // else invalid table name + } + return { + schema: schema_part, + table: table_part + }; + } + + function split_two_parts(part1, part2) { + var schema_part = null; + var table_part = null; + if ( part1.quoted && !part2.quoted ) { + if ( part2.part[0] === '.' ) { + schema_part = part1.part; + table_part = part2.part.slice(1); + } // else invalid table name (missing dot) + } else if ( !part1.quoted && part2.quoted ) { + if ( part1.part[part1.part.length - 1] === '.' ) { + schema_part = part1.part.slice(0, -1); + table_part = part2.part; + } // else invalid table name (missing dot) + } // else invalid table name (missing dot) + return { + schema: schema_part, + table: table_part + }; + } + + if ( parts.length === 1 ) { + return split_single_part(parts[0]); + } else if ( parts.length === 2 ) { + return split_two_parts(parts[0], parts[1]); + } else if ( parts.length === 3 && parts[1].part === '.' ) { + return { + schema: parts[0].part, + table: parts[2].part + }; + } // else invalid table name +} + +function table_identifier(parsed_name) { + if ( parsed_name && parsed_name.table ) { + if ( parsed_name.schema ) { + return quote_identifier_if_needed(parsed_name.schema) + '.' + quote_identifier_if_needed(parsed_name.table); + } else { + return quote_identifier_if_needed(parsed_name.table); + } + } else { + return null; + } +} + +module.exports = { + parse: parse_table_name, + quote: quote_identifier_if_needed, + table_identifier: table_identifier +}; diff --git a/test/acceptance/overviews.js b/test/acceptance/overviews_metadata.js similarity index 74% rename from test/acceptance/overviews.js rename to test/acceptance/overviews_metadata.js index 10e567f5..0fa99bf9 100644 --- a/test/acceptance/overviews.js +++ b/test/acceptance/overviews_metadata.js @@ -1,4 +1,3 @@ -var _ = require('underscore'); var test_helper = require('../support/test_helper'); var assert = require('../support/assert'); @@ -15,7 +14,7 @@ var step = require('step'); var windshaft = require('windshaft'); -describe('overviews', function() { +describe('overviews metadata', function() { // configure redis pool instance to use in tests var redisPool = new RedisPool(global.environment.redis); @@ -87,20 +86,16 @@ describe('overviews', function() { assert.ifError(err); assert.deepEqual(non_overviews_layer, mapConfig._cfg.layers[1]); assert.equal(mapConfig._cfg.layers[0].type, 'cartodb'); - assert.ok(mapConfig._cfg.layers[0].options.overviews); - assert.ok(mapConfig._cfg.layers[0].options.overviews.test_table_overviews); - assert.deepEqual(_.keys(mapConfig._cfg.layers[0].options.overviews), ['test_table_overviews']); - assert.equal(_.keys(mapConfig._cfg.layers[0].options.overviews.test_table_overviews).length, 2); - assert.ok(mapConfig._cfg.layers[0].options.overviews.test_table_overviews[1]); - assert.ok(mapConfig._cfg.layers[0].options.overviews.test_table_overviews[2]); - assert.equal( - mapConfig._cfg.layers[0].options.overviews.test_table_overviews[1].table, - '_vovw_1_test_table_overviews' - ); - assert.equal( - mapConfig._cfg.layers[0].options.overviews.test_table_overviews[2].table, - '_vovw_2_test_table_overviews' - ); + assert.ok(mapConfig._cfg.layers[0].options.query_rewrite_data); + var expected_data = { + overviews: { + test_table_overviews: { + 1: { table: '_vovw_1_test_table_overviews' }, + 2: { table: '_vovw_2_test_table_overviews' } + } + } + }; + assert.deepEqual(mapConfig._cfg.layers[0].options.query_rewrite_data, expected_data); }); next(err); diff --git a/test/acceptance/overviews_queries.js b/test/acceptance/overviews_queries.js new file mode 100644 index 00000000..8024ba72 --- /dev/null +++ b/test/acceptance/overviews_queries.js @@ -0,0 +1,72 @@ +var testHelper = require('../support/test_helper'); +var assert = require('../support/assert'); + +var cartodbServer = require('../../lib/cartodb/server'); +var ServerOptions = require('./ported/support/ported_server_options'); +var testClient = require('./ported/support/test_client'); +var BaseController = require('../../lib/cartodb/controllers/base'); + +describe('overviews_queries', function() { + + var server = cartodbServer(ServerOptions); + server.setMaxListeners(0); + + var IMAGE_EQUALS_TOLERANCE_PER_MIL = 2; + + var req2paramsFn; + before(function() { + req2paramsFn = BaseController.prototype.req2params; + BaseController.prototype.req2params = ServerOptions.req2params; + }); + + after(function() { + BaseController.prototype.req2params = req2paramsFn; + + testHelper.rmdirRecursiveSync(global.environment.millstone.cache_basedir); + }); + + function imageCompareFn(fixture, done) { + return function(err, tile) { + if (err) { + return done(err); + } + assert.imageEqualsFile(tile.body, './test/fixtures/' + fixture, IMAGE_EQUALS_TOLERANCE_PER_MIL, done); + }; + } + + it("should not use overview for tables without overviews", function(done){ + testClient.getTile(testClient.defaultTableMapConfig('test_table'), 1, 0, 0, + imageCompareFn('test_table_1_0_0.png', done) + ); + }); + + it("should not use overview for tables without overviews at z=2", function(done){ + testClient.getTile(testClient.defaultTableMapConfig('test_table'), 2, 1, 1, + imageCompareFn('test_table_2_1_1.png', done) + ); + }); + + it("should not use overview for tables without overviews at z=2", function(done){ + testClient.getTile(testClient.defaultTableMapConfig('test_table'), 3, 3, 3, + imageCompareFn('test_table_3_3_3.png', done) + ); + }); + + it("should use overview for zoom level 1", function(done){ + testClient.getTile(testClient.defaultTableMapConfig('test_table_overviews'), 1, 0, 0, + imageCompareFn('_vovw_1_test_table_1_0_0.png', done) + ); + }); + + it("should use overview for zoom level 1", function(done){ + testClient.getTile(testClient.defaultTableMapConfig('test_table_overviews'), 2, 1, 1, + imageCompareFn('_vovw_2_test_table_2_1_1.png', done) + ); + }); + + it("should not use overview for zoom level 3", function(done){ + testClient.getTile(testClient.defaultTableMapConfig('test_table_overviews'), 3, 3, 3, + imageCompareFn('test_table_3_3_3.png', done) + ); + }); +}); diff --git a/test/acceptance/ported/support/ported_server_options.js b/test/acceptance/ported/support/ported_server_options.js index 7894c0a9..1cc5331f 100644 --- a/test/acceptance/ported/support/ported_server_options.js +++ b/test/acceptance/ported/support/ported_server_options.js @@ -2,6 +2,10 @@ var _ = require('underscore'); var serverOptions = require('../../../../lib/cartodb/server_options'); var LayergroupToken = require('../../../../lib/cartodb/models/layergroup_token'); var mapnik = require('windshaft').mapnik; +var OverviewsQueryRewriter = require('../../../../lib/cartodb/utils/overviews_query_rewriter'); +var overviewsQueryRewriter = new OverviewsQueryRewriter({ + zoom_level: 'CDB_ZoomFromScale(!scale_denominator!)' +}); module.exports = _.extend({}, serverOptions, { base_url: '/database/:dbname/table/:table', @@ -26,7 +30,8 @@ module.exports = _.extend({}, serverOptions, { limits: { render: 0, cacheOnTimeout: true - } + }, + queryRewriter: overviewsQueryRewriter }, http: { timeout: 5000, diff --git a/test/fixtures/_vovw_1_test_table_1_0_0.png b/test/fixtures/_vovw_1_test_table_1_0_0.png new file mode 100644 index 0000000000000000000000000000000000000000..1dd602df0629fdabf3efdd04e10c9551418b480a GIT binary patch literal 1114 zcmeAS@N?(olHy`uVBq!ia0y~yU<5K5893O0R7}x|GzJD1MNb#Ukcv5PZ+mCRri!qC zho%UvoH~D4kfV=5y!wO zT1gv!D1;@;^%|b^ojiN?>^u2(=g;^$xoXZ-N!9pMSK#4PbhxJb|4d`!`#d~Ai-#gO z5UKaPCT{)p-OBsyb_RX?Jg;nbZRX|UY;zO^zpM`S(s%nSW;!beJ-y_~5%1t&?cD-Gx_2c;db3bQF7ws#5u>b1`)tweaEAF%0GURm;?s&Fw z^-EKR*CL8`5xsxcUYycjJLU7e?*I0Gw6~^b>)F-ryU!wKU4Q)f{(j8|ca~jeydrXN z??$Pow=Wl8t=M1x{N>^OLHxPrk6tL>xyL`?o=lHsLjL7!#;mRf?^eHi=ie`KRGs~r z*uSY&Z_i!TRFFD(+jl+d3e9u>)=w$_x;p#+^!)YL8RT{^y3b&>dF>B-dy~Cl3zl)b zd;V1RuhEI0FJHehZTP+INm7!*^y&Pk`&sp~s%#y21@9bQX>A|r%W(c)$a+Sb9lGMd z_0O{M9qOAeY+H7n`HH~7T0LFezHs3K2k(BBIuLw)-8;^X5A_$nL^aG(+Od1}a)z}V z;|jkNm|k1Il~LsV`}gJz|IQrn3~I5||fp z#z-dqTQ!R%X-#}B_lM`_iweIq$Nc$`sJE=`*Y!V7-|depeJ0;?{_d?W8<}S-9I8pQ zJQr@xdO9$(A~@johq>_M#>5(y>?*56z(osy7g_kL4f<;e^b!Azd@m%kI+hluxQJKlF^7*_h zmK#@3&xDu#%kC`tnFr@R-Fb#eTT@SGgVLj$zneNl4bSVQb#z}L1qXcGLZ?%ej?Isd z%PF!}I-Vqu{LX18Z>GI-+){NxJddsM-uuyjW~GhR!!8cLl*3<8THzywTJWgvN2o zaKLXFs>XYuA&47;KbD2)9-5cx%naov-iQ^k5}?=uVud^wY4O z#Tez}{zGX8%n|AQ9cr^Q4bsJhe8xvaBlb@CVF5WqThA+jk}Qp1x8+Lg-BD~is8)0o zufO;DA|~lvPE>X&3+AORe$_KwjJk&JyqTgo60i~}ZKiT=&%N9<7s#chPcjj)@3aX| z+^pck?x<=CX4qm%>3m45^cFVoT?pe>do6P{O!HmPw;CX`H=4cftq~%J7#ek-e5~?b z%sfxgnm5##o600xA84kV-^B@AqN(WF6O7)Mvouac$?IN5*vLU`oNAP=u?iT+!77~@ zRp6)BrPU4F{Ybh{skkZ4`Z?xZ{qn@AOBZlWl+~FY$Z*ZO`!2Yq(k|b{HJVEk-L?9m zBODIT{{#+YUB0YnRsTx^_RyNj?UNjPjBN2z`H^&MD|nnhE+9_w_rpd>>h2$Wgd!5! zXsX%Qx^L**&ImC$^t^yJm%A`h0~#J1^amLWr`UlcHI17Gq0W{z5XPKUe+OmMik@n* zyp_{KO?B9|{y*H=*(Ys3O|TYbh?g!RgB&Zcku{rS{l+juDD$`t!;U-phOuVb^QsG@ zx)vnCuqRba94%rl<^AzsRNS~PKRG^_Ai`CIR~2g%*@gtf#7fG;plxpyii2+Rk@Dz`lfb?ZM3jc_yX?BvEyi17 zFf#>kOLvqo3d9*c@r#_)8Y%gjLbD395c-2m7AhKNiSrOy50cvvuaNK>*$*AVp2wf1;qC>lDcXsA zL<+-<+q`R6+ANyvlUfg6S> zJNTQMS@a0vrSSM8f*4((k zzht~jY!TY%;xyp1{$z8XKxh7ist~-{{OL@s>F26I6I~ts+MQkL{DJWg1-dna2sAP_ zEee2$%L^wY7gQei5(t(zuheEGh#XqJ;XfUet!=bf{;*4`lkoX#7UPBWtUelg<3B$N Bg?|75 literal 0 HcmV?d00001 diff --git a/test/fixtures/test_table_1_0_0.png b/test/fixtures/test_table_1_0_0.png new file mode 100644 index 0000000000000000000000000000000000000000..530283373205602718d4bfac99e0feddafff0a46 GIT binary patch literal 888 zcmeAS@N?(olHy`uVBq!ia0y~yU<5K5893O0R7}x|GzJD{drud~kcv5P@9fWxO_gB( z5Tdi9r1+D+fV+76BJb->&8tg;T~r?^G&S=zEiK`>=)=ga*vz-+i0cZy)=RD&Q?+zM z&a7W%XrDjpPW8LrKkgJ~tZ6*rAjEgC#_j{p_v0V*&VRRgzVG>7Wo4k1Ll7AJQF^pE zZhiUdz^AXXBhPQ$tM{NKrP+S|*K|$QZ>#I}@1J{LGP`>10mDTGB4&ioQ*e(GA* zl&`-dyM1%*IyPNCd4Bf$Eqmi0%y`maKauf*$@gbPQ>Nb3IQ@OmogZ3{mY-Ly`=|8i z-q~k=um04y+s?$$b}-@1`t`s5#uXp^|GK|p<;9Ng^Ls0bpUkg%-Pxi2eDR-O$!!Pb zFf=4iNdBKC%OLF?D3<3QQ~UIO@BZ!gti_-Hn$E=V?>*c1zonnw=UjbU>l0_Sv;qsDIt9Tmbrd8*^Sp?*%s5h*2-_G#yD?1Z|-(;Q!>)rND=X}ho{{p$~ zdE2dT-+$jXea^V**~AA2*ByA2 zf7Wrq%kzu=6zsHQ4v=S?wR*lxPW4$}_gTe~DWM4f21Q0S literal 0 HcmV?d00001 diff --git a/test/fixtures/test_table_2_1_1.png b/test/fixtures/test_table_2_1_1.png new file mode 100644 index 0000000000000000000000000000000000000000..361c52b9c81b74556c661e3016de63b161e3211a GIT binary patch literal 1030 zcmeAS@N?(olHy`uVBq!ia0y~yU<5K5893O0R7}x|GzJFdvz{)FAr*7p-m%Y!4V7X4 zuwsXXhpfm!SD7v=%a==LKX{^KVHG8Cq-jM*hPYv%xa^8S7KiTM%u|te@)j-MC;hfa|MBnK{Ob5+%Ye2H2$1UT zc+J*)(E9PmivO&+yGr*rtkz)vfALP=@+ps!bj6#Jf9A_&-3?EWewy(9F2}E5QA@5a z^;rJT^8TsU4-*y(q^#fi&Ugc_+v1B0^=>RqekCvYF5g}8>R5;RvUA&)=XZU1_;{;< z&i}m|rB2Mtv1Lo(K9v|h?{BdA;`uQh7Jqz?beHGx{ri7=UbWI$`8BQzy~^?SS+;x$ z+(l*I%Qx5Gyg&bkar^Ig-tG05mc3tpyV3acT`h(BbNj9(Ggh5CD0}VZ|BNGV&OB|c zzqGGz{qA>n<~Cl1NcC>1+jj2Rdix8j8J299UwNIEJ(YDrR6pwr#rOa7UrhU-9==QA zxbgNc(TsbxY!+)cm}SY)bL}x>)rpyVw~93cZ)JFM@8WufJq7N%4B5+f=Q_Op$Y48l zX7wgk4fg82TnD20^K%$a9p56yeIa;f>-1xXrJmHD|8C7ZW3FX6hlO5yL-yL^Okbva zw!fdt>?hy9T_Rwa-J;AxA=<;{QQ!@`t%vY}c=XE5#T4`T}>81DVuYQZy@GHFk zD<1V-_6D2f8T(S#1vWAvd2grY|D8Is?o!ylwX0i9=GpwSzP{`H0kc!C51Q|^iT_Sl zaQ*kU>Yw4|UF#2oPGYXh=li8!IW2zHmOu9v-YMJmPMZD+-9h_5qp1%=UBX(n8j$Ut Lu6{1-oD!M<%eIf| literal 0 HcmV?d00001 diff --git a/test/fixtures/test_table_3_3_3.png b/test/fixtures/test_table_3_3_3.png new file mode 100644 index 0000000000000000000000000000000000000000..35a0059e8650c39d1b3d4ad1a52267eb53549c22 GIT binary patch literal 834 zcmeAS@N?(olHy`uVBq!ia0y~yU<5K5893O0R7}x|GzJD{PEQxdkcv5PZ|%+!4HRK} z(BbOxMu+9#sqzcw&ZU3g6cKDz@X}e6bIPHTn$ew%+Bx;K0{#REy!$ulaJZnKBG7 zuSUqq{gk?!SH9_OT?1PY!`9;E5hg?sCm7RWs6&sRG44rrggT#t#pY-a9CaR&F>@7OaG_}@O% zVlXpbrT(k%ztexl4{y?HOJ0<%V#wGnp1}U;X6>?jOwW|h`nle**!K6seXGFzOn<(< z{J-U`U4yK~{rlmJ0VS{1e^svS-#A$?ovmK}-@;l!e*No*_uE|mEfnxGIsA4lgY^{l ziY;7?HWxBe*U5dDxqafinEoBb>sDnwJumI_b^E7o=d1t2*Z*(3wXgnx{!;;q`_iwr z?2MD=JY`&G`5{%eZXNsn%@?D8pO!yyZB^C#mdd0QuX~)z4*}Q$iB} DQtBCp literal 0 HcmV?d00001 diff --git a/test/integration/mapconfig_overviews_adapter.js b/test/integration/mapconfig_overviews_adapter.js index 5ee879c2..079575a7 100644 --- a/test/integration/mapconfig_overviews_adapter.js +++ b/test/integration/mapconfig_overviews_adapter.js @@ -1,12 +1,11 @@ require('../support/test_helper'); var assert = require('assert'); -var _ = require('underscore'); var RedisPool = require('redis-mpool'); var cartodbRedis = require('cartodb-redis'); var PgConnection = require(__dirname + '/../../lib/cartodb/backends/pg_connection'); var PgQueryRunner = require('../../lib/cartodb/backends/pg_query_runner'); -var OverviewsApi = require('../../lib/cartodb/api/overviews_api'); +var OverviewsMetadataApi = require('../../lib/cartodb/api/overviews_metadata_api'); var MapConfigOverviewsAdapter = require('../../lib/cartodb/models/mapconfig_overviews_adapter'); // configure redis pool instance to use in tests @@ -17,10 +16,10 @@ var redisPool = new RedisPool(global.environment.redis); var metadataBackend = cartodbRedis({pool: redisPool}); var pgConnection = new PgConnection(metadataBackend); var pgQueryRunner = new PgQueryRunner(pgConnection); -var overviewsApi = new OverviewsApi(pgQueryRunner); +var overviewsMetadataApi = new OverviewsMetadataApi(pgQueryRunner); -var mapConfigOverviewsAdapter = new MapConfigOverviewsAdapter(overviewsApi); +var mapConfigOverviewsAdapter = new MapConfigOverviewsAdapter(overviewsMetadataApi); describe('MapConfigOverviewsAdapter', function() { @@ -72,20 +71,16 @@ describe('MapConfigOverviewsAdapter', function() { assert.equal(layers[0].options.sql, sql); assert.equal(layers[0].options.cartocss, cartocss); assert.equal(layers[0].options.cartocss_version, cartocss_version); - assert.ok(layers[0].options.overviews); - assert.ok(layers[0].options.overviews.test_table_overviews); - assert.deepEqual(_.keys(layers[0].options.overviews), ['test_table_overviews']); - assert.equal(_.keys(layers[0].options.overviews.test_table_overviews).length, 2); - assert.ok(layers[0].options.overviews.test_table_overviews[1]); - assert.ok(layers[0].options.overviews.test_table_overviews[2]); - assert.equal( - layers[0].options.overviews.test_table_overviews[1].table, - '_vovw_1_test_table_overviews' - ); - assert.equal( - layers[0].options.overviews.test_table_overviews[2].table, - '_vovw_2_test_table_overviews' - ); + assert.ok(layers[0].options.query_rewrite_data); + var expected_data = { + overviews: { + test_table_overviews: { + 1: { table: '_vovw_1_test_table_overviews' }, + 2: { table: '_vovw_2_test_table_overviews' } + } + } + }; + assert.deepEqual(layers[0].options.query_rewrite_data, expected_data); done(); }); }); diff --git a/test/integration/overviews-api.js b/test/integration/overviews-metadata-api.js similarity index 76% rename from test/integration/overviews-api.js rename to test/integration/overviews-metadata-api.js index ff256821..6c054d1e 100644 --- a/test/integration/overviews-api.js +++ b/test/integration/overviews-metadata-api.js @@ -8,12 +8,12 @@ var cartodbRedis = require('cartodb-redis'); var PgConnection = require('../../lib/cartodb/backends/pg_connection'); var PgQueryRunner = require('../../lib/cartodb/backends/pg_query_runner'); var QueryTablesApi = require('../../lib/cartodb/api/query_tables_api'); -var OverviewsApi = require('../../lib/cartodb/api/overviews_api'); +var OverviewsMetadataApi = require('../../lib/cartodb/api/overviews_metadata_api'); -describe('OverviewsApi', function() { +describe('OverviewsMetadataApi', function() { - var queryTablesApi, overviewsApi; + var queryTablesApi, overviewsMetadataApi; before(function() { var redisPool = new RedisPool(global.environment.redis); @@ -21,12 +21,12 @@ describe('OverviewsApi', function() { var pgConnection = new PgConnection(metadataBackend); var pgQueryRunner = new PgQueryRunner(pgConnection); queryTablesApi = new QueryTablesApi(pgQueryRunner); - overviewsApi = new OverviewsApi(pgQueryRunner); + overviewsMetadataApi = new OverviewsMetadataApi(pgQueryRunner); }); it('should return an empty relation for tables that have no overviews', function(done) { var query = 'select * from test_table'; - overviewsApi.getOverviewsMetadata('localhost', query, function(err, result) { + overviewsMetadataApi.getOverviewsMetadata('localhost', query, function(err, result) { assert.ok(!err, err); assert.deepEqual(result, {}); @@ -37,7 +37,7 @@ describe('OverviewsApi', function() { it('should return overviews metadata', function(done) { var query = 'select * from test_table_overviews'; - overviewsApi.getOverviewsMetadata('localhost', query, function(err, result) { + overviewsMetadataApi.getOverviewsMetadata('localhost', query, function(err, result) { assert.ok(!err, err); assert.deepEqual(result, { diff --git a/test/support/sql/CDB_Overviews.sql b/test/support/sql/CDB_Overviews.sql index fb927938..654bd9d2 100644 --- a/test/support/sql/CDB_Overviews.sql +++ b/test/support/sql/CDB_Overviews.sql @@ -16,6 +16,31 @@ $$ LANGUAGE PLPGSQL; CREATE OR REPLACE FUNCTION CDB_ZoomFromScale(scaleDenominator numeric) RETURNS int AS $$ BEGIN - RETURN 0; + CASE + WHEN scaleDenominator > 500000000 THEN RETURN 0; + WHEN scaleDenominator <= 500000000 AND scaleDenominator > 200000000 THEN RETURN 1; + WHEN scaleDenominator <= 200000000 AND scaleDenominator > 100000000 THEN RETURN 2; + WHEN scaleDenominator <= 100000000 AND scaleDenominator > 50000000 THEN RETURN 3; + WHEN scaleDenominator <= 50000000 AND scaleDenominator > 25000000 THEN RETURN 4; + WHEN scaleDenominator <= 25000000 AND scaleDenominator > 12500000 THEN RETURN 5; + WHEN scaleDenominator <= 12500000 AND scaleDenominator > 6500000 THEN RETURN 6; + WHEN scaleDenominator <= 6500000 AND scaleDenominator > 3000000 THEN RETURN 7; + WHEN scaleDenominator <= 3000000 AND scaleDenominator > 1500000 THEN RETURN 8; + WHEN scaleDenominator <= 1500000 AND scaleDenominator > 750000 THEN RETURN 9; + WHEN scaleDenominator <= 750000 AND scaleDenominator > 400000 THEN RETURN 10; + WHEN scaleDenominator <= 400000 AND scaleDenominator > 200000 THEN RETURN 11; + WHEN scaleDenominator <= 200000 AND scaleDenominator > 100000 THEN RETURN 12; + WHEN scaleDenominator <= 100000 AND scaleDenominator > 50000 THEN RETURN 13; + WHEN scaleDenominator <= 50000 AND scaleDenominator > 25000 THEN RETURN 14; + WHEN scaleDenominator <= 25000 AND scaleDenominator > 12500 THEN RETURN 15; + WHEN scaleDenominator <= 12500 AND scaleDenominator > 5000 THEN RETURN 16; + WHEN scaleDenominator <= 5000 AND scaleDenominator > 2500 THEN RETURN 17; + WHEN scaleDenominator <= 2500 AND scaleDenominator > 1500 THEN RETURN 18; + WHEN scaleDenominator <= 1500 AND scaleDenominator > 750 THEN RETURN 19; + WHEN scaleDenominator <= 750 AND scaleDenominator > 500 THEN RETURN 20; + WHEN scaleDenominator <= 500 AND scaleDenominator > 250 THEN RETURN 21; + WHEN scaleDenominator <= 250 AND scaleDenominator > 100 THEN RETURN 22; + WHEN scaleDenominator <= 100 THEN RETURN 23; + END CASE; END $$ LANGUAGE plpgsql IMMUTABLE; diff --git a/test/support/sql/windshaft.test.sql b/test/support/sql/windshaft.test.sql index 66a3de69..8a01903b 100644 --- a/test/support/sql/windshaft.test.sql +++ b/test/support/sql/windshaft.test.sql @@ -325,3 +325,10 @@ CREATE TABLE _vovw_2_test_table_overviews ( GRANT ALL ON TABLE _vovw_2_test_table_overviews TO :TESTUSER; GRANT SELECT ON TABLE _vovw_2_test_table_overviews TO :PUBLICUSER; + +INSERT INTO _vovw_2_test_table_overviews VALUES +('2011-09-21 14:02:21.358706', '2011-09-21 14:02:21.314252', 1, 'Hawai', 'Calle de Pérez Galdós 9, Madrid, Spain', '0101000020E610000000000000000020C00000000000004440', '0101000020110F000076491621312319C122D4663F1DCC5241'), +('2011-09-21 14:02:21.358706', '2011-09-21 14:02:21.319101', 2, 'El Estocolmo', 'Calle de la Palma 72, Madrid, Spain', '0101000020E610000000000000009431C026043C75E7224340', '0101000020110F0000C4356B29423319C15DD1092DADCC5241'); + +INSERT INTO _vovw_1_test_table_overviews VALUES +('2011-09-21 14:02:21.358706', '2011-09-21 14:02:21.314252', 1, 'Hawai', 'Calle de Pérez Galdós 9, Madrid, Spain', '0101000020E610000000000000000020C00000000000004440', '0101000020110F000076491621312319C122D4663F1DCC5241'); diff --git a/test/unit/cartodb/overviews_query_rewriter.js b/test/unit/cartodb/overviews_query_rewriter.js new file mode 100644 index 00000000..9adbe031 --- /dev/null +++ b/test/unit/cartodb/overviews_query_rewriter.js @@ -0,0 +1,429 @@ +require('../../support/test_helper'); + +var assert = require('assert'); +var OverviewsQueryRewriter = require('../../../lib/cartodb/utils/overviews_query_rewriter'); +var overviewsQueryRewriter = new OverviewsQueryRewriter({ + zoom_level: 'ZoomLevel()' +}); + +function normalize_whitespace(txt) { + return txt.replace(/\s+/g, " ").trim(); +} + +// compare SQL statements ignoring whitespace +function assertSameSql(sql1, sql2) { + assert.equal(normalize_whitespace(sql1), normalize_whitespace(sql2)); +} + +describe('Overviews query rewriter', function() { + + it('does not alter queries if no overviews data is present', function(done){ + var sql = "SELECT * FROM table1"; + var overviews_sql = overviewsQueryRewriter.query(sql); + assert.equal(overviews_sql, sql); + overviews_sql = overviewsQueryRewriter.query(sql, {}); + assert.equal(overviews_sql, sql); + overviews_sql = overviewsQueryRewriter.query(sql, { overviews: {} }); + assert.equal(overviews_sql, sql); + done(); + }); + + + it('does not alter queries which don\'t use overviews', function(done){ + var sql = "SELECT * FROM table1"; + var data = { + overviews: { + table2: { + 0: { table: 'table2_ov0' }, + 1: { table: 'table2_ov1' }, + 4: { table: 'table2_ov4' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + done(); + }); + + // jshint multistr:true + + it('generates query with single overview layer for level 0', function(done){ + var sql = "SELECT * FROM table1"; + var data = { + overviews: { + table1: { + 0: { table: 'table1_ov0' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM table1_ov0, _vovw_scale WHERE _vovw_z = 0\ + UNION ALL\ + SELECT * FROM table1, _vovw_scale WHERE _vovw_z > 0\ + )\ + SELECT * FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query with single overview layer for level >0', function(done){ + var sql = "SELECT * FROM table1"; + var data = { + overviews: { + table1: { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM table1_ov2, _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM table1, _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT * FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query with multiple overview layers for all levels up to N', function(done){ + var sql = "SELECT * FROM table1"; + var data = { + overviews: { + table1: { + 0: { table: 'table1_ov0' }, + 1: { table: 'table1_ov1' }, + 2: { table: 'table1_ov2' }, + 3: { table: 'table1_ov3' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM table1_ov0, _vovw_scale WHERE _vovw_z = 0\ + UNION ALL\ + SELECT * FROM table1_ov1, _vovw_scale WHERE _vovw_z = 1\ + UNION ALL\ + SELECT * FROM table1_ov2, _vovw_scale WHERE _vovw_z = 2\ + UNION ALL\ + SELECT * FROM table1_ov3, _vovw_scale WHERE _vovw_z = 3\ + UNION ALL\ + SELECT * FROM table1, _vovw_scale WHERE _vovw_z > 3\ + )\ + SELECT * FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query with multiple overview layers for random levels', function(done){ + var sql = "SELECT * FROM table1"; + var data = { + overviews: { + table1: { + 0: { table: 'table1_ov0' }, + 1: { table: 'table1_ov1' }, + 6: { table: 'table1_ov6' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM table1_ov0, _vovw_scale WHERE _vovw_z = 0\ + UNION ALL\ + SELECT * FROM table1_ov1, _vovw_scale WHERE _vovw_z = 1\ + UNION ALL\ + SELECT * FROM table1_ov6, _vovw_scale WHERE _vovw_z > 1 AND _vovw_z <= 6\ + UNION ALL\ + SELECT * FROM table1, _vovw_scale WHERE _vovw_z > 6\ + )\ + SELECT * FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query for a table with explicit schema', function(done){ + var sql = "SELECT * FROM public.table1"; + var data = { + overviews: { + 'public.table1': { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM public.table1_ov2, _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM public.table1, _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT * FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query for a table with explicit schema in the overviews info', function(done){ + var sql = "SELECT * FROM public.table1"; + var data = { + overviews: { + 'public.table1': { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM public.table1_ov2, _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM public.table1, _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT * FROM _vovw_table1\ + "; + + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query for a table that needs quoting with explicit schema', function(done){ + var sql = "SELECT * FROM public.\"table 1\""; + var data = { + overviews: { + 'public."table 1"': { + 2: { table: '"table 1_ov2"' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + \"_vovw_table 1\" AS (\ + SELECT * FROM public.\"table 1_ov2\", _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM public.\"table 1\", _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT * FROM \"_vovw_table 1\"\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query for a table with explicit schema that needs quoting', function(done){ + var sql = "SELECT * FROM \"user-1\".table1"; + var data = { + overviews: { + '"user-1".table1': { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM \"user-1\".table1_ov2, _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM \"user-1\".table1, _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT * FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + + it('generates query for a table with explicit schema both needing quoting', function(done){ + var sql = "SELECT * FROM \"user-1\".\"table 1\""; + var data = { + overviews: { + '"user-1"."table 1"': { + 2: { table: '"table 1_ov2"' } + + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + \"_vovw_table 1\" AS (\ + SELECT * FROM \"user-1\".\"table 1_ov2\", _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM \"user-1\".\"table 1\", _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT * FROM \"_vovw_table 1\"\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + + it('generates query using overviews for queries with selected columns', function(done){ + var sql = "SELECT column1, column2, column3 FROM table1"; + var data = { + overviews: { + table1: { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM table1_ov2, _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM table1, _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT column1, column2, column3 FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query using overviews for queries with selected columns and all columns', function(done){ + var sql = "SELECT table1.*, column1, column2, column3 FROM table1"; + var data = { + overviews: { + table1: { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM table1_ov2, _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM table1, _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT _vovw_table1.*, column1, column2, column3 FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query using overviews for queries with a semicolon', function(done){ + var sql = "SELECT table1.*, column1, column2, column3 FROM table1;"; + var data = { + overviews: { + table1: { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM table1_ov2, _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM table1, _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT _vovw_table1.*, column1, column2, column3 FROM _vovw_table1;\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('generates query using overviews for queries with extra whitespace', function(done){ + var sql = " SELECT table1.* , column1,column2, column3 FROM table1 "; + var data = { + overviews: { + table1: { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + var expected_sql = "\ + WITH\ + _vovw_scale AS ( SELECT ZoomLevel() AS _vovw_z ),\ + _vovw_table1 AS (\ + SELECT * FROM table1_ov2, _vovw_scale WHERE _vovw_z <= 2\ + UNION ALL\ + SELECT * FROM table1, _vovw_scale WHERE _vovw_z > 2\ + )\ + SELECT _vovw_table1.* , column1,column2, column3 FROM _vovw_table1\ + "; + assertSameSql(overviews_sql, expected_sql); + done(); + }); + + it('does not alter queries which have not the simple supported form', function(done){ + var sql = "SELECT * FROM table1 WHERE column1='x'"; + var data = { + overviews: { + table1: { + 2: { table: 'table1_ov2' } + } + } + }; + var overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + + sql = "SELECT * FROM table1 JOIN table2 ON (table1.col1=table2.col1)"; + overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + + sql = "SELECT a+b AS c FROM table1"; + overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + + sql = "SELECT f(a) AS b FROM table1"; + overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + + sql = "SELECT * FROM table1 AS x"; + overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + + sql = "WITH a AS (1) SELECT * FROM table1"; + overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + + sql = "SELECT * FROM table1 WHERE a=1"; + overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + + sql = "\ + SELECT table1.* FROM table1 \ + JOIN areas ON ST_Intersects(table1.the_geom, areas.the_geom) \ + WHERE areas.name='A' \ + "; + overviews_sql = overviewsQueryRewriter.query(sql, data); + assert.equal(overviews_sql, sql); + + done(); + }); + + +}); diff --git a/test/unit/cartodb/table_name_parser.js b/test/unit/cartodb/table_name_parser.js new file mode 100644 index 00000000..c08ebd8a --- /dev/null +++ b/test/unit/cartodb/table_name_parser.js @@ -0,0 +1,60 @@ +require('../../support/test_helper'); + +var assert = require('assert'); +var TableNameParser = require('../../../lib/cartodb/utils/table_name_parser'); + +describe('TableNameParser', function() { + + it('parses table names with scheme and quotes', function(done){ + + var test_cases = [ + ['xyz', { schema: null, table: 'xyz' }], + ['"xyz"', { schema: null, table: 'xyz' }], + ['"xy z"', { schema: null, table: 'xy z' }], + ['"xy.z"', { schema: null, table: 'xy.z' }], + ['"x.y.z"', { schema: null, table: 'x.y.z' }], + ['abc.xyz', { schema: 'abc', table: 'xyz' }], + ['"abc".xyz', { schema: 'abc', table: 'xyz' }], + ['abc."xyz"', { schema: 'abc', table: 'xyz' }], + ['"abc"."xyz"', { schema: 'abc', table: 'xyz' }], + ['"a bc"."x yz"', { schema: 'a bc', table: 'x yz' }], + ['"a bc".xyz', { schema: 'a bc', table: 'xyz' }], + ['"a.bc".xyz', { schema: 'a.bc', table: 'xyz' }], + ['"a.b.c".xyz', { schema: 'a.b.c', table: 'xyz' }], + ['"a.b.c.".xyz', { schema: 'a.b.c.', table: 'xyz' }], + ['"a""bc".xyz', { schema: 'a"bc', table: 'xyz' }], + ['"a""bc"."x""yz"', { schema: 'a"bc', table: 'x"yz' }], + ]; + + test_cases.forEach(function(test_case) { + var table_name = test_case[0]; + var expected_result = test_case[1]; + var result = TableNameParser.parse(table_name); + assert.deepEqual(result, expected_result); + }); + done(); + }); + + it('quotes identifiers that need quoting', function(done){ + assert.equal(TableNameParser.quote('x yz'), '"x yz"'); + assert.equal(TableNameParser.quote('x-yz'), '"x-yz"'); + assert.equal(TableNameParser.quote('x.yz'), '"x.yz"'); + done(); + }); + + it('doubles quotes', function(done){ + assert.equal(TableNameParser.quote('x"yz'), '"x""yz"'); + assert.equal(TableNameParser.quote('x"y"z'), '"x""y""z"'); + assert.equal(TableNameParser.quote('x""y"z'), '"x""""y""z"'); + assert.equal(TableNameParser.quote('x "yz'), '"x ""yz"'); + assert.equal(TableNameParser.quote('x"y-y"z'), '"x""y-y""z"'); + done(); + }); + + it('does not quote identifiers that don\'t need to be quoted', function(done){ + assert.equal(TableNameParser.quote('xyz'), 'xyz'); + assert.equal(TableNameParser.quote('x_z123'), 'x_z123'); + done(); + }); + +}); From 56095926e0d5f7723d8f342275ad36ac748a5c51 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 5 Feb 2016 08:23:02 +0100 Subject: [PATCH 40/43] Remove CartCSS handling from QueryRewriter QueryRewriter doesn't require a style method anymore --- lib/cartodb/utils/overviews_query_rewriter.js | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/lib/cartodb/utils/overviews_query_rewriter.js b/lib/cartodb/utils/overviews_query_rewriter.js index 0758fa60..0b6c592a 100644 --- a/lib/cartodb/utils/overviews_query_rewriter.js +++ b/lib/cartodb/utils/overviews_query_rewriter.js @@ -171,21 +171,6 @@ OverviewsQueryRewriter.prototype.query = function(query, data) { return overviews_query(query, overviews, zoom_level_expression); }; -OverviewsQueryRewriter.prototype.style = function(cartocss, cartocss_version, data) { - var overviews = this.overviews_metadata(data); - if ( !overviews ) { - return cartocss; - } - for ( var table in overviews ) { - if (overviews.hasOwnProperty(table)) { - // No modification of the CartoCSS is currently performed - // var table_view = overviews_view_name(table); - // cartocss = replace_table_in_style(cartocss, table, table_view); - } - } - return cartocss; -}; - OverviewsQueryRewriter.prototype.is_supported_query = function(sql) { return !!sql.match( /^\s*SELECT\s+[\*\.a-z0-9_,\s]+?\s+FROM\s+((\"[^"]+\"|[a-z0-9_]+)\.)?(\"[^"]+\"|[a-z0-9_]+)\s*;?\s*$/i From bbb8841f5af52338430345ad027bb60cd5da1341 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 9 Feb 2016 17:31:10 +0100 Subject: [PATCH 41/43] Upgrade Windshaft to 1.9.0 This version supports the current (provisional) QueryRewriter interface --- npm-shrinkwrap.json | 15 +++++++-------- package.json | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index b545d61d..ab6927dd 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1,6 +1,6 @@ { "name": "windshaft-cartodb", - "version": "2.22.0", + "version": "2.22.1", "dependencies": { "body-parser": { "version": "1.14.2", @@ -85,7 +85,7 @@ }, "mime-types": { "version": "2.1.9", - "from": "mime-types@>=2.1.2 <2.2.0", + "from": "mime-types@>=2.1.9 <2.2.0", "resolved": "https://registry.npmjs.org/mime-types/-/mime-types-2.1.9.tgz", "dependencies": { "mime-db": { @@ -193,7 +193,7 @@ "dependencies": { "mime-types": { "version": "2.1.9", - "from": "mime-types@>=2.1.9 <2.2.0", + "from": "mime-types@>=2.1.6 <2.2.0", "resolved": "https://registry.npmjs.org/mime-types/-/mime-types-2.1.9.tgz", "dependencies": { "mime-db": { @@ -382,7 +382,7 @@ }, "mime-types": { "version": "2.1.9", - "from": "mime-types@>=2.1.9 <2.2.0", + "from": "mime-types@>=2.1.6 <2.2.0", "resolved": "https://registry.npmjs.org/mime-types/-/mime-types-2.1.9.tgz", "dependencies": { "mime-db": { @@ -536,7 +536,7 @@ }, "inherits": { "version": "2.0.1", - "from": "inherits@>=2.0.0 <3.0.0", + "from": "inherits@>=2.0.1 <2.1.0", "resolved": "https://registry.npmjs.org/inherits/-/inherits-2.0.1.tgz" }, "isarray": { @@ -830,9 +830,8 @@ "resolved": "https://registry.npmjs.org/underscore/-/underscore-1.6.0.tgz" }, "windshaft": { - "version": "1.8.3", - "from": "windshaft@1.8.3", - "resolved": "https://registry.npmjs.org/windshaft/-/windshaft-1.8.3.tgz", + "version": "1.9.0", + "from": "windshaft@1.9.0", "dependencies": { "mapnik": { "version": "1.4.15-cdb6", diff --git a/package.json b/package.json index bb9c996a..f036ad3a 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,7 @@ "node-statsd": "~0.0.7", "underscore" : "~1.6.0", "dot": "~1.0.2", - "windshaft": "1.8.3", + "windshaft": "1.9.0", "step": "~0.0.6", "queue-async": "~1.0.7", "request": "~2.62.0", From 4f8534afb35d73089f76c231339af0d9caaef153 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 10 Feb 2016 12:16:37 +0100 Subject: [PATCH 42/43] Fix: accept empty layers in the MapConfigOverviewsAdapter --- lib/cartodb/models/mapconfig_overviews_adapter.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index 72a29485..df81dfee 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -10,8 +10,8 @@ module.exports = MapConfigOverviewsAdapter; MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callback) { var self = this; - if (!layers) { - return callback(null); + if (!layers || layers.length === 0) { + return callback(null, layers); } var augmentLayersQueue = queue(layers.length); @@ -39,7 +39,7 @@ MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callb } if (!layers || layers.length === 0) { - return callback(new Error('Missing layers array from layergroup config')); + return callback(new Error('XX Missing layers array from layergroup config')); } return callback(null, layers); From cd2bc319d8553fe962e478da3d77d807887da68b Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 10 Feb 2016 12:27:39 +0100 Subject: [PATCH 43/43] Fix: bad error message --- lib/cartodb/models/mapconfig_overviews_adapter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/mapconfig_overviews_adapter.js b/lib/cartodb/models/mapconfig_overviews_adapter.js index df81dfee..70a02dc5 100644 --- a/lib/cartodb/models/mapconfig_overviews_adapter.js +++ b/lib/cartodb/models/mapconfig_overviews_adapter.js @@ -39,7 +39,7 @@ MapConfigOverviewsAdapter.prototype.getLayers = function(username, layers, callb } if (!layers || layers.length === 0) { - return callback(new Error('XX Missing layers array from layergroup config')); + return callback(new Error('Missing layers array from layergroup config')); } return callback(null, layers);