From f82e4031802742419bdd071eb09a4219b476b6f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 25 Feb 2019 19:40:18 +0100 Subject: [PATCH 01/16] Implement query to get features of a cluster for an aggregated map from @jgoizueta's cluster query --- .../models/aggregation/aggregation-query.js | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 1bddf02d..f74df93a 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -448,3 +448,31 @@ const aggregationQueryTemplates = { module.exports.SUPPORTED_PLACEMENTS = Object.keys(aggregationQueryTemplates); module.exports.GEOMETRY_COLUMN = 'the_geom_webmercator'; + +const clusterFeaturesQuery = ctx => ` + WITH + _cdb_params AS ( + SELECT + ${gridResolution(ctx)} AS res + ), + _cell AS ( + SELECT + ST_MakeEnvelope( + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)*_cdb_params.res, + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)*_cdb_params.res, + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res + 1)*_cdb_params.res, + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res + 1)*_cdb_params.res, + 3857 + ) AS bbox + FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + WHERE _cdb_query.cartodb_id = ${ctx.id} + ) + SELECT _cdb_query.* FROM _cell, (${ctx.sourceQuery}) _cdb_query + WHERE ST_Intersects(_cdb_query.the_geom_webmercator, _cell.bbox) +`; + +module.exports.featuresQuery = (id, options) => clusterFeaturesQuery({ + id, + sourceQuery: options.query, + res: 256/options.resolution +}); From c3df075d91973929650172bffb3ba9845cf5040c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 26 Feb 2019 19:19:44 +0100 Subject: [PATCH 02/16] Draft --- lib/cartodb/api/api-router.js | 6 +- ...lustered-features-layergroup-controller.js | 91 ++++++++++ lib/cartodb/api/map/map-router.js | 15 +- lib/cartodb/backends/cluster.js | 168 ++++++++++++++++++ test/acceptance/cluster.js | 49 +++++ test/support/test-client.js | 100 +++++++++++ 6 files changed, 427 insertions(+), 2 deletions(-) create mode 100644 lib/cartodb/api/map/clustered-features-layergroup-controller.js create mode 100644 lib/cartodb/backends/cluster.js create mode 100644 test/acceptance/cluster.js diff --git a/lib/cartodb/api/api-router.js b/lib/cartodb/api/api-router.js index fafc90b3..af82a3b8 100644 --- a/lib/cartodb/api/api-router.js +++ b/lib/cartodb/api/api-router.js @@ -21,6 +21,8 @@ const OverviewsMetadataBackend = require('../backends/overviews-metadata'); const FilterStatsApi = require('../backends/filter-stats'); const TablesExtentBackend = require('../backends/tables-extent'); +const ClusterBackend = require('../backends/cluster'); + const LayergroupAffectedTablesCache = require('../cache/layergroup_affected_tables'); const SurrogateKeysCache = require('../cache/surrogate_keys_cache'); const VarnishHttpCacheBackend = require('../cache/backend/varnish_http'); @@ -110,6 +112,7 @@ module.exports = class ApiRouter { const analysisBackend = new AnalysisBackend(metadataBackend, serverOptions.analysis); const dataviewBackend = new DataviewBackend(analysisBackend); const statsBackend = new StatsBackend(); + const clusterBackend = new ClusterBackend(); const userLimitsBackend = new UserLimitsBackend(metadataBackend, { limits: { @@ -179,7 +182,8 @@ module.exports = class ApiRouter { statsBackend, layergroupMetadata, namedMapProviderCache, - tablesExtentBackend + tablesExtentBackend, + clusterBackend }; this.mapRouter = new MapRouter({ collaborators }); diff --git a/lib/cartodb/api/map/clustered-features-layergroup-controller.js b/lib/cartodb/api/map/clustered-features-layergroup-controller.js new file mode 100644 index 00000000..2d83fd4d --- /dev/null +++ b/lib/cartodb/api/map/clustered-features-layergroup-controller.js @@ -0,0 +1,91 @@ +'use strict'; + +const layergroupToken = require('../middlewares/layergroup-token'); +const cleanUpQueryParams = require('../middlewares/clean-up-query-params'); +const credentials = require('../middlewares/credentials'); +const dbConnSetup = require('../middlewares/db-conn-setup'); +const authorize = require('../middlewares/authorize'); +const rateLimit = require('../middlewares/rate-limit'); +const { RATE_LIMIT_ENDPOINTS_GROUPS } = rateLimit; +const createMapStoreMapConfigProvider = require('../middlewares/map-store-map-config-provider'); +const cacheControlHeader = require('../middlewares/cache-control-header'); +const cacheChannelHeader = require('../middlewares/cache-channel-header'); +const surrogateKeyHeader = require('../middlewares/surrogate-key-header'); +const lastModifiedHeader = require('../middlewares/last-modified-header'); + +module.exports = class AggregatedFeaturesLayergroupController { + constructor ( + clusterBackend, + pgConnection, + mapStore, + userLimitsBackend, + layergroupAffectedTablesCache, + authBackend, + surrogateKeysCache + ) { + this.clusterBackend = clusterBackend; + this.pgConnection = pgConnection; + this.mapStore = mapStore; + this.userLimitsBackend = userLimitsBackend; + this.layergroupAffectedTablesCache = layergroupAffectedTablesCache; + this.authBackend = authBackend; + this.surrogateKeysCache = surrogateKeysCache; + } + + register (mapRouter) { + mapRouter.get('/:token/:layer/cluster/:clusterId', this.middlewares()); + } + + middlewares () { + return [ + layergroupToken(), + credentials(), + authorize(this.authBackend), + dbConnSetup(this.pgConnection), + rateLimit(this.userLimitsBackend, RATE_LIMIT_ENDPOINTS_GROUPS.ATTRIBUTES), + cleanUpQueryParams(), + createMapStoreMapConfigProvider( + this.mapStore, + this.userLimitsBackend, + this.pgConnection, + this.layergroupAffectedTablesCache + ), + getClusteredFeatures(this.clusterBackend), + cacheControlHeader(), + cacheChannelHeader(), + surrogateKeyHeader({ surrogateKeysCache: this.surrogateKeysCache }), + lastModifiedHeader() + ]; + } +}; + +function getClusteredFeatures (clusterBackend) { + return function getFeatureAttributesMiddleware (req, res, next) { + req.profiler.start('windshaft.maplayer_cluster_features'); + + const { mapConfigProvider } = res.locals; + const { token } = res.locals; + const { dbuser, dbname, dbpassword, dbhost, dbport } = res.locals; + const { layer, clusterId } = req.params; + + const params = { + token, + dbuser, dbname, dbpassword, dbhost, dbport, + layer, clusterId + }; + + clusterBackend.getClusterFeatures(mapConfigProvider, params, (err, features, stats = {}) => { + req.profiler.add(stats); + + if (err) { + err.label = 'GET CLUSTERED FEATURES'; + return next(err); + } + + res.statusCode = 200; + res.body = features; + + next(); + }); + }; +} diff --git a/lib/cartodb/api/map/map-router.js b/lib/cartodb/api/map/map-router.js index dac55670..8fb7b8b2 100644 --- a/lib/cartodb/api/map/map-router.js +++ b/lib/cartodb/api/map/map-router.js @@ -10,6 +10,7 @@ const TileLayergroupController = require('./tile-layergroup-controller'); const AnonymousMapController = require('./anonymous-map-controller'); const PreviewTemplateController = require('./preview-template-controller'); const AnalysesCatalogController = require('./analyses-catalog-controller'); +const ClusteredFeaturesLayergroupController = require('./clustered-features-layergroup-controller'); module.exports = class MapRouter { constructor ({ collaborators }) { @@ -32,7 +33,8 @@ module.exports = class MapRouter { statsBackend, layergroupMetadata, namedMapProviderCache, - tablesExtentBackend + tablesExtentBackend, + clusterBackend } = collaborators; this.analysisLayergroupController = new AnalysisLayergroupController( @@ -112,6 +114,16 @@ module.exports = class MapRouter { authBackend, userLimitsBackend ); + + this.clusteredFeaturesLayergroupController = new ClusteredFeaturesLayergroupController( + clusterBackend, + pgConnection, + mapStore, + userLimitsBackend, + layergroupAffectedTablesCache, + authBackend, + surrogateKeysCache + ); } register (apiRouter, mapPaths) { @@ -125,6 +137,7 @@ module.exports = class MapRouter { this.anonymousMapController.register(mapRouter); this.previewTemplateController.register(mapRouter); this.analysesController.register(mapRouter); + this.clusteredFeaturesLayergroupController.register(mapRouter); mapPaths.forEach(path => apiRouter.use(path, mapRouter)); } diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js new file mode 100644 index 00000000..a76bba29 --- /dev/null +++ b/lib/cartodb/backends/cluster.js @@ -0,0 +1,168 @@ +'use strict'; + +const PSQL = require('cartodb-psql'); +const dbParamsFromReqParams = require('../utils/database-params'); +const debug = require('debug')('backend:cluster'); + +module.exports = class ClusterBackend { + getClusterFeatures (mapConfigProvider, params, callback) { + mapConfigProvider.getMapConfig((err, mapConfig) => { + if (err) { + return callback(err); + } + + // if (!mapConfig.isAggregationLayer(params.layer)) { + // const error = new Error(`Map ${params.token} has no aggregation defined for layer ${params.layer}`); + // return callback(error); + // } + + const layer = mapConfig.getLayer(params.layer); + + let pg; + try { + pg = new PSQL(dbParamsFromReqParams(params)); + } catch (error) { + return callback(error); + } + + const query = layer.options.sql; + const resolution = layer.options.aggregation.resolution || 1; + + getColumnsName(pg, query, (err, columns) => { + if (err) { + return callback(err); + } + + const { clusterId } = params; + + getClusterFeatures(pg, clusterId, columns, query, resolution, (err, features) => { + if (err) { + return callback(err); + } + + return callback(null, features); + }); + }); + }); + } +}; + +const SKIP_COLUMNS = { + 'the_geom': true, + 'the_geom_webmercator': true +}; + +function getColumnsName (pg, query, callback) { + const sql = replaceTokens(limitedQuery({ + query: query + })); + + debug(sql); + + pg.query(sql, function (err, resultSet) { + if (err) { + return callback(err); + } + + const fields = resultSet.fields || []; + const columnNames = fields.map(field => field.name) + .filter(columnName => !SKIP_COLUMNS[columnName]); + + return callback(null, columnNames); + }, true); +} + +function getClusterFeatures (pg, clusterId, columns, query, resolution, callback) { + const sql = replaceTokens(clusterFeaturesQuery({ + id: clusterId, + query: query, + res: resolution, + columns: columns + })); + + debug(sql); + + pg.query(sql, (err, data) => { + if (err) { + return callback(err); + } + + return callback(null, data); + } , true); // use read-only transaction +} + +const SUBSTITUTION_TOKENS = { + bbox: /!bbox!/g, + scale_denominator: /!scale_denominator!/g, + pixel_width: /!pixel_width!/g, + pixel_height: /!pixel_height!/g, + var_zoom: /@zoom/g, + var_bbox: /@bbox/g, + var_x: /@x/g, + var_y: /@y/g, +}; + +function replaceTokens(sql, replaceValues) { + if (!sql) { + return sql; + } + + replaceValues = replaceValues || { + bbox: 'ST_MakeEnvelope(0,0,0,0)', + scale_denominator: '0', + pixel_width: '1', + pixel_height: '1', + var_zoom: '0', + var_bbox: '[0,0,0,0]', + var_x: '0', + var_y: '0' + }; + + Object.keys(replaceValues).forEach(function(token) { + if (SUBSTITUTION_TOKENS[token]) { + sql = sql.replace(SUBSTITUTION_TOKENS[token], replaceValues[token]); + } + }); + + return sql; +} + +const limitedQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_schema LIMIT 0`; +// const nonGeomsQuery = ctx => `SELECT ${ctx.columns.join(', ')} FROM (${ctx.query}) __cdb_non_geoms_query`; +const clusterFeaturesQuery = ctx => ` + WITH + _cdb_params AS ( + SELECT + ${gridResolution(ctx)} AS res + ), + _cell AS ( + SELECT + ST_MakeEnvelope( + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)*_cdb_params.res, + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)*_cdb_params.res, + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res + 1)*_cdb_params.res, + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res + 1)*_cdb_params.res, + 3857 + ) AS bbox + FROM (${ctx.query}) _cdb_query, _cdb_params + WHERE _cdb_query.cartodb_id = ${ctx.id} + ) + SELECT ${ctx.columns.join(', ')} FROM ( + SELECT _cdb_query.* FROM _cell, (${ctx.query}) _cdb_query + WHERE ST_Intersects(_cdb_query.the_geom_webmercator, _cell.bbox) + ) __cdb_non_geoms_query +`; + +// SQL expression to compute the aggregation resolution (grid cell size). +// This is defined by the ctx.res parameter, which is the number of grid cells per tile linear dimension +// (i.e. each tile is divided into ctx.res*ctx.res cells). +// We limit the the minimum resolution to avoid division by zero problems. The limit used is +// the pixel size of zoom level 30 (i.e. 1/2*(30+8) of the full earth web-mercator extent), which is about 0.15 mm. +// Computing this using !scale_denominator!, !pixel_width! or !pixel_height! produces +// inaccurate results due to rounding present in those values. +const gridResolution = ctx => { + const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38); + const pixelSize = 'CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!))'; + debug(ctx); + return `GREATEST(${256/ctx.res}*${pixelSize}, ${minimumResolution})::double precision`; +}; diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js new file mode 100644 index 00000000..0cb3f1a7 --- /dev/null +++ b/test/acceptance/cluster.js @@ -0,0 +1,49 @@ +'use strict'; + +require('../support/test_helper'); + +// const assert = require('../support/assert'); +const TestClient = require('../support/test-client'); + +const POINTS_SQL_1 = ` + select + x + 4 as cartodb_id, + st_setsrid(st_makepoint(x*10, x*10), 4326) as the_geom, + st_transform(st_setsrid(st_makepoint(x*10, x*10), 4326), 3857) as the_geom_webmercator, + x as value + from generate_series(-3, 3) x +`; + +const defaultLayers = [{ + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: true + } +}]; + +function createVectorMapConfig (layers = defaultLayers) { + return { + version: '1.8.0', + layers: layers + }; +} + +describe('cluster', function () { + it.only('should get aggregated features of an aggregated map', function (done) { + const mapConfig = createVectorMapConfig(); + const testClient = new TestClient(mapConfig); + const clusterId = 1; + const layerId = 0; + const params = {}; + + testClient.getClusterFeatures(clusterId, layerId, params, (err, body) => { + if (err) { + return done(err); + } + + console.log('>>>>>>>>>>>>', body.rows); + testClient.drain(done); + }); + }); +}); diff --git a/test/support/test-client.js b/test/support/test-client.js index 77363b01..9eb340cb 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -620,6 +620,106 @@ TestClient.prototype.getFeatureAttributes = function(featureId, layerId, params, ); }; +TestClient.prototype.getClusterFeatures = function(clusterId, layerId, params, callback) { + var self = this; + + if (!callback) { + callback = params; + params = {}; + } + + var extraParams = {}; + + if (this.apiKey) { + extraParams.api_key = this.apiKey; + } + + // if (params && params.filters) { + // extraParams.filters = JSON.stringify(params.filters); + // } + + var url = '/api/v1/map'; + if (Object.keys(extraParams).length > 0) { + url += '?' + qs.stringify(extraParams); + } + + var expectedResponse = params.response || { + status: 200, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + step( + function createLayergroup() { + var next = this; + assert.response(self.server, + { + url: url, + method: 'POST', + headers: { + host: 'localhost', + 'Content-Type': 'application/json' + }, + data: JSON.stringify(self.mapConfig) + }, + { + status: 200, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }, + function(res, err) { + if (err) { + return next(err); + } + + var parsedBody = JSON.parse(res.body); + + if (parsedBody.layergroupid) { + self.keysToDelete['map_cfg|' + LayergroupToken.parse(parsedBody.layergroupid).token] = 0; + self.keysToDelete['user:localhost:mapviews:global'] = 5; + } + + return next(null, parsedBody.layergroupid); + } + ); + }, + function getCLusterFeatures(err, layergroupId) { + assert.ifError(err); + + var next = this; + + url = '/api/v1/map/' + layergroupId + '/' + layerId + '/cluster/' + clusterId; + + assert.response(self.server, + { + url: url, + method: 'GET', + headers: { + host: 'localhost' + } + }, + expectedResponse, + function(res, err) { + if (err) { + return next(err); + } + + next(null, JSON.parse(res.body)); + } + ); + }, + function finish(err, attributes) { + if (err) { + return callback(err); + } + + return callback(null, attributes); + } + ); +}; + TestClient.prototype.getTile = function(z, x, y, params, callback) { var self = this; From b87298bad90f1cd475c245385156217c694032e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 27 Feb 2019 12:42:54 +0100 Subject: [PATCH 03/16] Fix resolution definition and use zoom as path param --- ...lustered-features-layergroup-controller.js | 6 ++-- lib/cartodb/backends/cluster.js | 36 ++++++++++--------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/lib/cartodb/api/map/clustered-features-layergroup-controller.js b/lib/cartodb/api/map/clustered-features-layergroup-controller.js index 2d83fd4d..b84f0bdf 100644 --- a/lib/cartodb/api/map/clustered-features-layergroup-controller.js +++ b/lib/cartodb/api/map/clustered-features-layergroup-controller.js @@ -33,7 +33,7 @@ module.exports = class AggregatedFeaturesLayergroupController { } register (mapRouter) { - mapRouter.get('/:token/:layer/cluster/:clusterId', this.middlewares()); + mapRouter.get('/:token/:layer/:z/cluster/:clusterId', this.middlewares()); } middlewares () { @@ -66,12 +66,12 @@ function getClusteredFeatures (clusterBackend) { const { mapConfigProvider } = res.locals; const { token } = res.locals; const { dbuser, dbname, dbpassword, dbhost, dbport } = res.locals; - const { layer, clusterId } = req.params; + const { layer, z: zoom, clusterId } = req.params; const params = { token, dbuser, dbname, dbpassword, dbhost, dbport, - layer, clusterId + layer, zoom, clusterId }; clusterBackend.getClusterFeatures(mapConfigProvider, params, (err, features, stats = {}) => { diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index a76bba29..2e470596 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -25,7 +25,7 @@ module.exports = class ClusterBackend { return callback(error); } - const query = layer.options.sql; + const query = layer.options.sql_raw; const resolution = layer.options.aggregation.resolution || 1; getColumnsName(pg, query, (err, columns) => { @@ -33,9 +33,9 @@ module.exports = class ClusterBackend { return callback(err); } - const { clusterId } = params; + const { zoom, clusterId } = params; - getClusterFeatures(pg, clusterId, columns, query, resolution, (err, features) => { + getClusterFeatures(pg, zoom, clusterId, columns, query, resolution, (err, features) => { if (err) { return callback(err); } @@ -57,7 +57,7 @@ function getColumnsName (pg, query, callback) { query: query })); - debug(sql); + debug('> getColumnsName:', sql); pg.query(sql, function (err, resultSet) { if (err) { @@ -72,15 +72,16 @@ function getColumnsName (pg, query, callback) { }, true); } -function getClusterFeatures (pg, clusterId, columns, query, resolution, callback) { +function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, callback) { const sql = replaceTokens(clusterFeaturesQuery({ + zoom: zoom, id: clusterId, query: query, - res: resolution, + res: 256/resolution, columns: columns })); - debug(sql); + debug('> getClusterFeatures:', sql); pg.query(sql, (err, data) => { if (err) { @@ -108,10 +109,10 @@ function replaceTokens(sql, replaceValues) { } replaceValues = replaceValues || { - bbox: 'ST_MakeEnvelope(0,0,0,0)', - scale_denominator: '0', - pixel_width: '1', - pixel_height: '1', + bbox: 'ST_MakeEnvelope(-20037508.34,-20037508.34,20037508.34,20037508.34,3857)', + scale_denominator: '500000001', + pixel_width: '156412', + pixel_height: '156412', var_zoom: '0', var_bbox: '[0,0,0,0]', var_x: '0', @@ -128,7 +129,6 @@ function replaceTokens(sql, replaceValues) { } const limitedQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_schema LIMIT 0`; -// const nonGeomsQuery = ctx => `SELECT ${ctx.columns.join(', ')} FROM (${ctx.query}) __cdb_non_geoms_query`; const clusterFeaturesQuery = ctx => ` WITH _cdb_params AS ( @@ -147,9 +147,12 @@ const clusterFeaturesQuery = ctx => ` FROM (${ctx.query}) _cdb_query, _cdb_params WHERE _cdb_query.cartodb_id = ${ctx.id} ) - SELECT ${ctx.columns.join(', ')} FROM ( - SELECT _cdb_query.* FROM _cell, (${ctx.query}) _cdb_query - WHERE ST_Intersects(_cdb_query.the_geom_webmercator, _cell.bbox) + SELECT + ${ctx.columns.join(', ')} + FROM ( + SELECT _cdb_query.* + FROM _cell, (${ctx.query}) _cdb_query + WHERE ST_Intersects(_cdb_query.the_geom_webmercator, _cell.bbox) ) __cdb_non_geoms_query `; @@ -162,7 +165,6 @@ const clusterFeaturesQuery = ctx => ` // inaccurate results due to rounding present in those values. const gridResolution = ctx => { const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38); - const pixelSize = 'CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!))'; - debug(ctx); + const pixelSize = `CDB_XYZ_Resolution(${ctx.zoom})`; return `GREATEST(${256/ctx.res}*${pixelSize}, ${minimumResolution})::double precision`; }; From 561be2e5e80de4ff11c092f60204153b0a818c7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 27 Feb 2019 12:43:26 +0100 Subject: [PATCH 04/16] Add tests --- test/acceptance/cluster.js | 116 +++++++++++++++++++++++++++++++----- test/support/test-client.js | 6 +- 2 files changed, 105 insertions(+), 17 deletions(-) diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index 0cb3f1a7..c62e87c8 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -2,7 +2,7 @@ require('../support/test_helper'); -// const assert = require('../support/assert'); +const assert = require('../support/assert'); const TestClient = require('../support/test-client'); const POINTS_SQL_1 = ` @@ -18,7 +18,9 @@ const defaultLayers = [{ type: 'cartodb', options: { sql: POINTS_SQL_1, - aggregation: true + aggregation: { + threshold: 1 + } } }]; @@ -30,20 +32,106 @@ function createVectorMapConfig (layers = defaultLayers) { } describe('cluster', function () { - it.only('should get aggregated features of an aggregated map', function (done) { - const mapConfig = createVectorMapConfig(); - const testClient = new TestClient(mapConfig); - const clusterId = 1; - const layerId = 0; - const params = {}; - - testClient.getClusterFeatures(clusterId, layerId, params, (err, body) => { - if (err) { - return done(err); + describe('resolution = 1', function () { + const suite = [ + { + cartodb_id: 1, + expected: [ { cartodb_id: 1, value: -3 } ] + }, + { + cartodb_id: 2, + expected: [ { cartodb_id: 2, value: -2 } ] + }, + { + cartodb_id: 3, + expected: [ { cartodb_id: 3, value: -1 } ] + }, + { + cartodb_id: 4, + expected: [ { cartodb_id: 4, value: 0 } ] + }, + { + cartodb_id: 5, + expected: [ { cartodb_id: 5, value: 1 } ] + }, + { + cartodb_id: 6, + expected: [ { cartodb_id: 6, value: 2 } ] } + ]; - console.log('>>>>>>>>>>>>', body.rows); - testClient.drain(done); + suite.forEach(({ cartodb_id, expected }) => { + it(`should get just one disaggregated feature: cartodb_id = ${cartodb_id}`, function (done) { + const mapConfig = createVectorMapConfig(); + const testClient = new TestClient(mapConfig); + const zoom = 0; + const clusterId = cartodb_id; + const layerId = 0; + const params = {}; + + testClient.getClusterFeatures(zoom, clusterId, layerId, params, (err, body) => { + if (err) { + return done(err); + } + + assert.deepStrictEqual(body.rows, expected); + testClient.drain(done); + }); + }); + }); + }); + + describe('resolution = 50', function () { + const suite = [ + { + cartodb_id: 1, + resolution: 50, + expected: [ + { cartodb_id: 1, value: -3 }, + { cartodb_id: 2, value: -2 }, + { cartodb_id: 3, value: -1 }, + { cartodb_id: 4, value: 0 }, + ] + }, + { + cartodb_id: 5, + resolution: 50, + expected: [ + { cartodb_id: 5, value: 1 }, + { cartodb_id: 6, value: 2 }, + { cartodb_id: 7, value: 3 } + ] + } + ]; + + suite.forEach(({ cartodb_id, resolution, expected }) => { + it(`should get just one disaggregated feature: cartodb_id = ${cartodb_id}`, function (done) { + const mapConfig = createVectorMapConfig([{ + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + resolution: resolution + } + } + }]); + + const testClient = new TestClient(mapConfig); + const zoom = 0; + const clusterId = cartodb_id; + const layerId = 0; + const params = {}; + + testClient.getClusterFeatures(zoom, clusterId, layerId, params, (err, body) => { + if (err) { + return done(err); + } + + assert.deepStrictEqual(body.rows, expected); + testClient.drain(done); + }); + }); }); }); }); diff --git a/test/support/test-client.js b/test/support/test-client.js index 9eb340cb..b1e4a211 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -620,7 +620,7 @@ TestClient.prototype.getFeatureAttributes = function(featureId, layerId, params, ); }; -TestClient.prototype.getClusterFeatures = function(clusterId, layerId, params, callback) { +TestClient.prototype.getClusterFeatures = function (zoom, clusterId, layerId, params, callback) { var self = this; if (!callback) { @@ -685,12 +685,12 @@ TestClient.prototype.getClusterFeatures = function(clusterId, layerId, params, c } ); }, - function getCLusterFeatures(err, layergroupId) { + function getCLusterFeatures (err, layergroupId) { assert.ifError(err); var next = this; - url = '/api/v1/map/' + layergroupId + '/' + layerId + '/cluster/' + clusterId; + url = '/api/v1/map/' + layergroupId + '/' + layerId + '/' + zoom + '/cluster/' + clusterId; assert.response(self.server, { From 6531770e48d51c6c1ee0acac67c945466dc3b39b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 27 Feb 2019 12:47:20 +0100 Subject: [PATCH 05/16] Remove subtitution tokens --- lib/cartodb/backends/cluster.js | 44 +++------------------------------ 1 file changed, 4 insertions(+), 40 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 2e470596..3779fdf7 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -53,9 +53,9 @@ const SKIP_COLUMNS = { }; function getColumnsName (pg, query, callback) { - const sql = replaceTokens(limitedQuery({ + const sql = limitedQuery({ query: query - })); + }); debug('> getColumnsName:', sql); @@ -73,13 +73,13 @@ function getColumnsName (pg, query, callback) { } function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, callback) { - const sql = replaceTokens(clusterFeaturesQuery({ + const sql = clusterFeaturesQuery({ zoom: zoom, id: clusterId, query: query, res: 256/resolution, columns: columns - })); + }); debug('> getClusterFeatures:', sql); @@ -92,42 +92,6 @@ function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, ca } , true); // use read-only transaction } -const SUBSTITUTION_TOKENS = { - bbox: /!bbox!/g, - scale_denominator: /!scale_denominator!/g, - pixel_width: /!pixel_width!/g, - pixel_height: /!pixel_height!/g, - var_zoom: /@zoom/g, - var_bbox: /@bbox/g, - var_x: /@x/g, - var_y: /@y/g, -}; - -function replaceTokens(sql, replaceValues) { - if (!sql) { - return sql; - } - - replaceValues = replaceValues || { - bbox: 'ST_MakeEnvelope(-20037508.34,-20037508.34,20037508.34,20037508.34,3857)', - scale_denominator: '500000001', - pixel_width: '156412', - pixel_height: '156412', - var_zoom: '0', - var_bbox: '[0,0,0,0]', - var_x: '0', - var_y: '0' - }; - - Object.keys(replaceValues).forEach(function(token) { - if (SUBSTITUTION_TOKENS[token]) { - sql = sql.replace(SUBSTITUTION_TOKENS[token], replaceValues[token]); - } - }); - - return sql; -} - const limitedQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_schema LIMIT 0`; const clusterFeaturesQuery = ctx => ` WITH From 32938eeab79a23663d9c96c925d7a9b65e768d55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 27 Feb 2019 18:54:21 +0100 Subject: [PATCH 06/16] Validated only aggregated layers can be requested by the new endpoint --- ...lustered-features-layergroup-controller.js | 4 +- lib/cartodb/backends/cluster.js | 20 +- test/acceptance/cluster.js | 235 ++++++++++++++---- 3 files changed, 200 insertions(+), 59 deletions(-) diff --git a/lib/cartodb/api/map/clustered-features-layergroup-controller.js b/lib/cartodb/api/map/clustered-features-layergroup-controller.js index b84f0bdf..e83a865d 100644 --- a/lib/cartodb/api/map/clustered-features-layergroup-controller.js +++ b/lib/cartodb/api/map/clustered-features-layergroup-controller.js @@ -64,12 +64,12 @@ function getClusteredFeatures (clusterBackend) { req.profiler.start('windshaft.maplayer_cluster_features'); const { mapConfigProvider } = res.locals; - const { token } = res.locals; + const { user, token } = res.locals; const { dbuser, dbname, dbpassword, dbhost, dbport } = res.locals; const { layer, z: zoom, clusterId } = req.params; const params = { - token, + user, token, dbuser, dbname, dbpassword, dbhost, dbport, layer, zoom, clusterId }; diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 3779fdf7..baa48523 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -3,21 +3,15 @@ const PSQL = require('cartodb-psql'); const dbParamsFromReqParams = require('../utils/database-params'); const debug = require('debug')('backend:cluster'); +const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfig'); module.exports = class ClusterBackend { getClusterFeatures (mapConfigProvider, params, callback) { - mapConfigProvider.getMapConfig((err, mapConfig) => { + mapConfigProvider.getMapConfig((err, _mapConfig) => { if (err) { return callback(err); } - // if (!mapConfig.isAggregationLayer(params.layer)) { - // const error = new Error(`Map ${params.token} has no aggregation defined for layer ${params.layer}`); - // return callback(error); - // } - - const layer = mapConfig.getLayer(params.layer); - let pg; try { pg = new PSQL(dbParamsFromReqParams(params)); @@ -25,6 +19,16 @@ module.exports = class ClusterBackend { return callback(error); } + const { user, token, layer: layerIndex } = params; + const mapConfig = new AggregationMapConfig(user, _mapConfig.obj(), pg); + + if (!mapConfig.isAggregationLayer(layerIndex)) { + const error = new Error(`Map ${token} has no aggregation defined for layer ${layerIndex}`); + debug(error); + return callback(error); + } + + const layer = mapConfig.getLayer(layerIndex); const query = layer.options.sql_raw; const resolution = layer.options.aggregation.resolution || 1; diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index c62e87c8..82ec5ff8 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -32,58 +32,133 @@ function createVectorMapConfig (layers = defaultLayers) { } describe('cluster', function () { - describe('resolution = 1', function () { - const suite = [ - { - cartodb_id: 1, - expected: [ { cartodb_id: 1, value: -3 } ] - }, - { - cartodb_id: 2, - expected: [ { cartodb_id: 2, value: -2 } ] - }, - { - cartodb_id: 3, - expected: [ { cartodb_id: 3, value: -1 } ] - }, - { - cartodb_id: 4, - expected: [ { cartodb_id: 4, value: 0 } ] - }, - { - cartodb_id: 5, - expected: [ { cartodb_id: 5, value: 1 } ] - }, - { - cartodb_id: 6, - expected: [ { cartodb_id: 6, value: 2 } ] - } - ]; + describe('map-config w/o aggregation', function () { + it('should return error while fetching disaggregated features', function (done) { + const mapConfig = createVectorMapConfig([{ + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + cartocss: TestClient.CARTOCSS.POINTS, + cartocss_version: '2.3.0' + } + }]); + const testClient = new TestClient(mapConfig); + const zoom = 0; + const cartodb_id = 1; + const layerId = 0; + const params = { + response: { + status: 400 + } + }; - suite.forEach(({ cartodb_id, expected }) => { - it(`should get just one disaggregated feature: cartodb_id = ${cartodb_id}`, function (done) { - const mapConfig = createVectorMapConfig(); - const testClient = new TestClient(mapConfig); - const zoom = 0; - const clusterId = cartodb_id; - const layerId = 0; - const params = {}; + testClient.getClusterFeatures(zoom, cartodb_id, layerId, params, (err, body) => { + if (err) { + return done(err); + } - testClient.getClusterFeatures(zoom, clusterId, layerId, params, (err, body) => { - if (err) { - return done(err); - } - - assert.deepStrictEqual(body.rows, expected); - testClient.drain(done); + assert.deepStrictEqual(body, { + errors:[ 'Map d725a568ab961af8197d311eececb83a has no aggregation defined for layer 0' ], + errors_with_context:[ + { + type: 'unknown', + message: 'Map d725a568ab961af8197d311eececb83a has no aggregation defined for layer 0' + } + ] }); + testClient.drain(done); }); }); }); - describe('resolution = 50', function () { + describe('map-config with aggregation', function () { const suite = [ { + zoom: 0, + cartodb_id: 1, + resolution: 0.5, + expected: [ { cartodb_id: 1, value: -3 } ] + }, + { + zoom: 0, + cartodb_id: 2, + resolution: 0.5, + expected: [ { cartodb_id: 2, value: -2 } ] + }, + { + zoom: 0, + cartodb_id: 3, + resolution: 0.5, + expected: [ { cartodb_id: 3, value: -1 } ] + }, + { + zoom: 0, + cartodb_id: 4, + resolution: 0.5, + expected: [ { cartodb_id: 4, value: 0 } ] + }, + { + zoom: 0, + cartodb_id: 5, + resolution: 0.5, + expected: [ { cartodb_id: 5, value: 1 } ] + }, + { + zoom: 0, + cartodb_id: 6, + resolution: 0.5, + expected: [ { cartodb_id: 6, value: 2 } ] + }, + { + zoom: 0, + cartodb_id: 7, + resolution: 0.5, + expected: [ { cartodb_id: 7, value: 3 } ] + }, + { + zoom: 0, + cartodb_id: 1, + resolution: 1, + expected: [ { cartodb_id: 1, value: -3 } ] + }, + { + zoom: 0, + cartodb_id: 2, + resolution: 1, + expected: [ { cartodb_id: 2, value: -2 } ] + }, + { + zoom: 0, + cartodb_id: 3, + resolution: 1, + expected: [ { cartodb_id: 3, value: -1 } ] + }, + { + zoom: 0, + cartodb_id: 4, + resolution: 1, + expected: [ { cartodb_id: 4, value: 0 } ] + }, + { + zoom: 0, + cartodb_id: 5, + resolution: 1, + expected: [ { cartodb_id: 5, value: 1 } ] + }, + { + zoom: 0, + cartodb_id: 6, + resolution: 1, + expected: [ { cartodb_id: 6, value: 2 } ] + }, + { + zoom: 0, + cartodb_id: 7, + resolution: 1, + expected: [ { cartodb_id: 7, value: 3 } ] + }, + { + zoom: 0, cartodb_id: 1, resolution: 50, expected: [ @@ -94,6 +169,70 @@ describe('cluster', function () { ] }, { + zoom: 0, + cartodb_id: 5, + resolution: 50, + expected: [ + { cartodb_id: 5, value: 1 }, + { cartodb_id: 6, value: 2 }, + { cartodb_id: 7, value: 3 } + ] + }, + { + zoom: 1, + cartodb_id: 1, + resolution: 1, + expected: [ { cartodb_id: 1, value: -3 } ] + }, + { + zoom: 1, + cartodb_id: 2, + resolution: 1, + expected: [ { cartodb_id: 2, value: -2 } ] + }, + { + zoom: 1, + cartodb_id: 3, + resolution: 1, + expected: [ { cartodb_id: 3, value: -1 } ] + }, + { + zoom: 1, + cartodb_id: 4, + resolution: 1, + expected: [ { cartodb_id: 4, value: 0 } ] + }, + { + zoom: 1, + cartodb_id: 5, + resolution: 1, + expected: [ { cartodb_id: 5, value: 1 } ] + }, + { + zoom: 1, + cartodb_id: 6, + resolution: 1, + expected: [ { cartodb_id: 6, value: 2 } ] + }, + { + zoom: 1, + cartodb_id: 7, + resolution: 1, + expected: [ { cartodb_id: 7, value: 3 } ] + }, + { + zoom: 1, + cartodb_id: 1, + resolution: 50, + expected: [ + { cartodb_id: 1, value: -3 }, + { cartodb_id: 2, value: -2 }, + { cartodb_id: 3, value: -1 }, + { cartodb_id: 4, value: 0 }, + ] + }, + { + zoom: 1, cartodb_id: 5, resolution: 50, expected: [ @@ -104,8 +243,9 @@ describe('cluster', function () { } ]; - suite.forEach(({ cartodb_id, resolution, expected }) => { - it(`should get just one disaggregated feature: cartodb_id = ${cartodb_id}`, function (done) { + suite.forEach(({ zoom, cartodb_id, resolution, expected }) => { + const description = `should get features for z: ${zoom} cartodb_id: ${cartodb_id}, res: ${resolution}`; + it(description, function (done) { const mapConfig = createVectorMapConfig([{ type: 'cartodb', options: { @@ -116,14 +256,11 @@ describe('cluster', function () { } } }]); - const testClient = new TestClient(mapConfig); - const zoom = 0; - const clusterId = cartodb_id; const layerId = 0; const params = {}; - testClient.getClusterFeatures(zoom, clusterId, layerId, params, (err, body) => { + testClient.getClusterFeatures(zoom, cartodb_id, layerId, params, (err, body) => { if (err) { return done(err); } From 92e62069d4bda3705373d9234341f78b3a49d7c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 28 Feb 2019 12:13:15 +0100 Subject: [PATCH 07/16] Improve error handling --- lib/cartodb/backends/cluster.js | 13 +++++++-- test/acceptance/cluster.js | 52 +++++++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index baa48523..2b93f2ee 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -22,13 +22,22 @@ module.exports = class ClusterBackend { const { user, token, layer: layerIndex } = params; const mapConfig = new AggregationMapConfig(user, _mapConfig.obj(), pg); - if (!mapConfig.isAggregationLayer(layerIndex)) { + const layer = mapConfig.getLayer(layerIndex); + + if (layer.options.aggregation === false || !mapConfig.isAggregationLayer(layerIndex)) { const error = new Error(`Map ${token} has no aggregation defined for layer ${layerIndex}`); + error.http_status = 400; + error.type = 'layer'; + error.subtype = 'aggregation'; + error.layer = { + index: layerIndex, + type: layer.type + }; + debug(error); return callback(error); } - const layer = mapConfig.getLayer(layerIndex); const query = layer.options.sql_raw; const resolution = layer.options.aggregation.resolution || 1; diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index 82ec5ff8..a31af1b7 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -61,11 +61,59 @@ describe('cluster', function () { errors:[ 'Map d725a568ab961af8197d311eececb83a has no aggregation defined for layer 0' ], errors_with_context:[ { - type: 'unknown', - message: 'Map d725a568ab961af8197d311eececb83a has no aggregation defined for layer 0' + layer: { + index: '0', + type: 'cartodb' + }, + message: 'Map d725a568ab961af8197d311eececb83a has no aggregation defined for layer 0', + subtype: 'aggregation', + type: 'layer' } ] }); + + testClient.drain(done); + }); + }); + + it('with aggregation disabled should return error while fetching disaggregated features', function (done) { + const mapConfig = createVectorMapConfig([{ + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: false + } + }]); + const testClient = new TestClient(mapConfig); + const zoom = 0; + const cartodb_id = 1; + const layerId = 0; + const params = { + response: { + status: 400 + } + }; + + testClient.getClusterFeatures(zoom, cartodb_id, layerId, params, (err, body) => { + if (err) { + return done(err); + } + + assert.deepStrictEqual(body, { + errors:[ 'Map 3a09728f8c08444820336ea9983ce92b has no aggregation defined for layer 0' ], + errors_with_context:[ + { + layer: { + index: '0', + type: 'cartodb' + }, + message: 'Map 3a09728f8c08444820336ea9983ce92b has no aggregation defined for layer 0', + subtype: 'aggregation', + type: 'layer' + } + ] + }); + testClient.drain(done); }); }); From 77d5d8ebd414bf33e47224f6bd6cdef6da280e46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 11:21:18 +0100 Subject: [PATCH 08/16] Be able to aggregate by a field --- ...lustered-features-layergroup-controller.js | 6 +- lib/cartodb/backends/cluster.js | 34 +++- test/acceptance/cluster.js | 187 ++++++++++++++---- test/support/test-client.js | 7 +- 4 files changed, 181 insertions(+), 53 deletions(-) diff --git a/lib/cartodb/api/map/clustered-features-layergroup-controller.js b/lib/cartodb/api/map/clustered-features-layergroup-controller.js index e83a865d..9779a09a 100644 --- a/lib/cartodb/api/map/clustered-features-layergroup-controller.js +++ b/lib/cartodb/api/map/clustered-features-layergroup-controller.js @@ -43,7 +43,7 @@ module.exports = class AggregatedFeaturesLayergroupController { authorize(this.authBackend), dbConnSetup(this.pgConnection), rateLimit(this.userLimitsBackend, RATE_LIMIT_ENDPOINTS_GROUPS.ATTRIBUTES), - cleanUpQueryParams(), + cleanUpQueryParams([ 'aggregation' ]), createMapStoreMapConfigProvider( this.mapStore, this.userLimitsBackend, @@ -67,11 +67,13 @@ function getClusteredFeatures (clusterBackend) { const { user, token } = res.locals; const { dbuser, dbname, dbpassword, dbhost, dbport } = res.locals; const { layer, z: zoom, clusterId } = req.params; + const { aggregation } = req.query; const params = { user, token, dbuser, dbname, dbpassword, dbhost, dbport, - layer, zoom, clusterId + layer, zoom, clusterId, + aggregation }; clusterBackend.getClusterFeatures(mapConfigProvider, params, (err, features, stats = {}) => { diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 2b93f2ee..cba5b80e 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -38,6 +38,7 @@ module.exports = class ClusterBackend { return callback(error); } + const { aggregation } = params; const query = layer.options.sql_raw; const resolution = layer.options.aggregation.resolution || 1; @@ -48,7 +49,7 @@ module.exports = class ClusterBackend { const { zoom, clusterId } = params; - getClusterFeatures(pg, zoom, clusterId, columns, query, resolution, (err, features) => { + getClusterFeatures(pg, zoom, clusterId, columns, query, resolution, aggregation, (err, features) => { if (err) { return callback(err); } @@ -85,8 +86,8 @@ function getColumnsName (pg, query, callback) { }, true); } -function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, callback) { - const sql = clusterFeaturesQuery({ +function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, aggregation, callback) { + let sql = clusterFeaturesQuery({ zoom: zoom, id: clusterId, query: query, @@ -94,6 +95,17 @@ function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, ca columns: columns }); + if (aggregation !== undefined) { + aggregation = JSON.parse(aggregation); + const { columns = [], expresions = [] } = aggregation; + + sql = aggregationQuery({ + columns, + query: sql, + expresions + }); + } + debug('> getClusterFeatures:', sql); pg.query(sql, (err, data) => { @@ -133,15 +145,17 @@ const clusterFeaturesQuery = ctx => ` ) __cdb_non_geoms_query `; -// SQL expression to compute the aggregation resolution (grid cell size). -// This is defined by the ctx.res parameter, which is the number of grid cells per tile linear dimension -// (i.e. each tile is divided into ctx.res*ctx.res cells). -// We limit the the minimum resolution to avoid division by zero problems. The limit used is -// the pixel size of zoom level 30 (i.e. 1/2*(30+8) of the full earth web-mercator extent), which is about 0.15 mm. -// Computing this using !scale_denominator!, !pixel_width! or !pixel_height! produces -// inaccurate results due to rounding present in those values. const gridResolution = ctx => { const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38); const pixelSize = `CDB_XYZ_Resolution(${ctx.zoom})`; return `GREATEST(${256/ctx.res}*${pixelSize}, ${minimumResolution})::double precision`; }; + +const aggregationQuery = ctx => ` + SELECT + count(1) as _cdb_feature_count + ${ctx.columns.length ? `,${ctx.columns.join(', ')}` : ''} + ${ctx.expresions.length ? `,${ctx.expresions.join(', ')}` : ''} + FROM (${ctx.query}) __cdb_aggregation + ${ctx.columns.length ? `GROUP BY ${ctx.columns.join(', ')}` : ''} +`; diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index a31af1b7..8b6ee5d7 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -10,7 +10,11 @@ const POINTS_SQL_1 = ` x + 4 as cartodb_id, st_setsrid(st_makepoint(x*10, x*10), 4326) as the_geom, st_transform(st_setsrid(st_makepoint(x*10, x*10), 4326), 3857) as the_geom_webmercator, - x as value + x as value, + CASE + WHEN x % 2 = 0 THEN 'even' + ELSE 'odd' + END AS type from generate_series(-3, 3) x `; @@ -58,14 +62,14 @@ describe('cluster', function () { } assert.deepStrictEqual(body, { - errors:[ 'Map d725a568ab961af8197d311eececb83a has no aggregation defined for layer 0' ], + errors:[ 'Map c502fc8fc1cb0d5e412db3deabffeee5 has no aggregation defined for layer 0' ], errors_with_context:[ { layer: { index: '0', type: 'cartodb' }, - message: 'Map d725a568ab961af8197d311eececb83a has no aggregation defined for layer 0', + message: 'Map c502fc8fc1cb0d5e412db3deabffeee5 has no aggregation defined for layer 0', subtype: 'aggregation', type: 'layer' } @@ -100,14 +104,14 @@ describe('cluster', function () { } assert.deepStrictEqual(body, { - errors:[ 'Map 3a09728f8c08444820336ea9983ce92b has no aggregation defined for layer 0' ], + errors:[ 'Map 18792467ae296929d04e32dfe7f81a80 has no aggregation defined for layer 0' ], errors_with_context:[ { layer: { index: '0', type: 'cartodb' }, - message: 'Map 3a09728f8c08444820336ea9983ce92b has no aggregation defined for layer 0', + message: 'Map 18792467ae296929d04e32dfe7f81a80 has no aggregation defined for layer 0', subtype: 'aggregation', type: 'layer' } @@ -125,95 +129,95 @@ describe('cluster', function () { zoom: 0, cartodb_id: 1, resolution: 0.5, - expected: [ { cartodb_id: 1, value: -3 } ] + expected: [ { cartodb_id: 1, value: -3, type: 'odd' } ] }, { zoom: 0, cartodb_id: 2, resolution: 0.5, - expected: [ { cartodb_id: 2, value: -2 } ] + expected: [ { cartodb_id: 2, value: -2, type: 'even' } ] }, { zoom: 0, cartodb_id: 3, resolution: 0.5, - expected: [ { cartodb_id: 3, value: -1 } ] + expected: [ { cartodb_id: 3, value: -1, type: 'odd' } ] }, { zoom: 0, cartodb_id: 4, resolution: 0.5, - expected: [ { cartodb_id: 4, value: 0 } ] + expected: [ { cartodb_id: 4, value: 0, type: 'even' } ] }, { zoom: 0, cartodb_id: 5, resolution: 0.5, - expected: [ { cartodb_id: 5, value: 1 } ] + expected: [ { cartodb_id: 5, value: 1, type: 'odd' } ] }, { zoom: 0, cartodb_id: 6, resolution: 0.5, - expected: [ { cartodb_id: 6, value: 2 } ] + expected: [ { cartodb_id: 6, value: 2, type: 'even' } ] }, { zoom: 0, cartodb_id: 7, resolution: 0.5, - expected: [ { cartodb_id: 7, value: 3 } ] + expected: [ { cartodb_id: 7, value: 3, type: 'odd' } ] }, { zoom: 0, cartodb_id: 1, resolution: 1, - expected: [ { cartodb_id: 1, value: -3 } ] + expected: [ { cartodb_id: 1, value: -3, type: 'odd' } ] }, { zoom: 0, cartodb_id: 2, resolution: 1, - expected: [ { cartodb_id: 2, value: -2 } ] + expected: [ { cartodb_id: 2, value: -2, type: 'even' } ] }, { zoom: 0, cartodb_id: 3, resolution: 1, - expected: [ { cartodb_id: 3, value: -1 } ] + expected: [ { cartodb_id: 3, value: -1, type: 'odd' } ] }, { zoom: 0, cartodb_id: 4, resolution: 1, - expected: [ { cartodb_id: 4, value: 0 } ] + expected: [ { cartodb_id: 4, value: 0, type: 'even' } ] }, { zoom: 0, cartodb_id: 5, resolution: 1, - expected: [ { cartodb_id: 5, value: 1 } ] + expected: [ { cartodb_id: 5, value: 1, type: 'odd' } ] }, { zoom: 0, cartodb_id: 6, resolution: 1, - expected: [ { cartodb_id: 6, value: 2 } ] + expected: [ { cartodb_id: 6, value: 2, type: 'even' } ] }, { zoom: 0, cartodb_id: 7, resolution: 1, - expected: [ { cartodb_id: 7, value: 3 } ] + expected: [ { cartodb_id: 7, value: 3, type: 'odd' } ] }, { zoom: 0, cartodb_id: 1, resolution: 50, expected: [ - { cartodb_id: 1, value: -3 }, - { cartodb_id: 2, value: -2 }, - { cartodb_id: 3, value: -1 }, - { cartodb_id: 4, value: 0 }, + { cartodb_id: 1, value: -3, type: 'odd' }, + { cartodb_id: 2, value: -2, type: 'even' }, + { cartodb_id: 3, value: -1, type: 'odd' }, + { cartodb_id: 4, value: 0, type: 'even' }, ] }, { @@ -221,62 +225,62 @@ describe('cluster', function () { cartodb_id: 5, resolution: 50, expected: [ - { cartodb_id: 5, value: 1 }, - { cartodb_id: 6, value: 2 }, - { cartodb_id: 7, value: 3 } + { cartodb_id: 5, value: 1, type: 'odd' }, + { cartodb_id: 6, value: 2, type: 'even' }, + { cartodb_id: 7, value: 3, type: 'odd' } ] }, { zoom: 1, cartodb_id: 1, resolution: 1, - expected: [ { cartodb_id: 1, value: -3 } ] + expected: [ { cartodb_id: 1, value: -3, type: 'odd' } ] }, { zoom: 1, cartodb_id: 2, resolution: 1, - expected: [ { cartodb_id: 2, value: -2 } ] + expected: [ { cartodb_id: 2, value: -2, type: 'even' } ] }, { zoom: 1, cartodb_id: 3, resolution: 1, - expected: [ { cartodb_id: 3, value: -1 } ] + expected: [ { cartodb_id: 3, value: -1, type: 'odd' } ] }, { zoom: 1, cartodb_id: 4, resolution: 1, - expected: [ { cartodb_id: 4, value: 0 } ] + expected: [ { cartodb_id: 4, value: 0, type: 'even' } ] }, { zoom: 1, cartodb_id: 5, resolution: 1, - expected: [ { cartodb_id: 5, value: 1 } ] + expected: [ { cartodb_id: 5, value: 1, type: 'odd' } ] }, { zoom: 1, cartodb_id: 6, resolution: 1, - expected: [ { cartodb_id: 6, value: 2 } ] + expected: [ { cartodb_id: 6, value: 2, type: 'even' } ] }, { zoom: 1, cartodb_id: 7, resolution: 1, - expected: [ { cartodb_id: 7, value: 3 } ] + expected: [ { cartodb_id: 7, value: 3, type: 'odd' } ] }, { zoom: 1, cartodb_id: 1, resolution: 50, expected: [ - { cartodb_id: 1, value: -3 }, - { cartodb_id: 2, value: -2 }, - { cartodb_id: 3, value: -1 }, - { cartodb_id: 4, value: 0 }, + { cartodb_id: 1, value: -3, type: 'odd' }, + { cartodb_id: 2, value: -2, type: 'even'}, + { cartodb_id: 3, value: -1, type: 'odd' }, + { cartodb_id: 4, value: 0, type: 'even' }, ] }, { @@ -284,9 +288,9 @@ describe('cluster', function () { cartodb_id: 5, resolution: 50, expected: [ - { cartodb_id: 5, value: 1 }, - { cartodb_id: 6, value: 2 }, - { cartodb_id: 7, value: 3 } + { cartodb_id: 5, value: 1, type: 'odd' }, + { cartodb_id: 6, value: 2, type: 'even' }, + { cartodb_id: 7, value: 3, type: 'odd' } ] } ]; @@ -319,4 +323,107 @@ describe('cluster', function () { }); }); }); + + describe('map-config w/o aggregation', function () { + const suite = [ + { + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: ['type'] }, + expected: [ { _cdb_feature_count: 1, type: 'odd' } ] + }, + { + zoom: 0, + cartodb_id: 2, + resolution: 1, + aggregation: { columns: ['type'] }, + expected: [ { _cdb_feature_count: 1, type: 'even' } ] + }, + { + zoom: 0, + cartodb_id: 3, + resolution: 1, + aggregation: { columns: ['type'] }, + expected: [ { _cdb_feature_count: 1, type: 'odd' } ] + }, + { + zoom: 0, + cartodb_id: 4, + resolution: 1, + aggregation: { columns: ['type'] }, + expected: [ { _cdb_feature_count: 1, type: 'even' } ] + }, + { + zoom: 0, + cartodb_id: 5, + resolution: 1, + aggregation: { columns: ['type'] }, + expected: [ { _cdb_feature_count: 1, type: 'odd' } ] + }, + { + zoom: 0, + cartodb_id: 6, + resolution: 1, + aggregation: { columns: ['type'] }, + expected: [ { _cdb_feature_count: 1, type: 'even' } ] + }, + { + zoom: 0, + cartodb_id: 7, + resolution: 1, + aggregation: { columns: ['type'] }, + expected: [ { _cdb_feature_count: 1, type: 'odd' } ] + }, + { + zoom: 0, + cartodb_id: 1, + resolution: 50, + aggregation: { columns: ['type'] }, + expected: [ + { _cdb_feature_count: 2, type: 'even' }, + { _cdb_feature_count: 2, type: 'odd' } + ] + }, + { + zoom: 0, + cartodb_id: 5, + resolution: 50, + aggregation: { columns: ['type'] }, + expected: [ + { _cdb_feature_count: 1, type: 'even' }, + { _cdb_feature_count: 2, type: 'odd' } + ] + } + ]; + + + suite.forEach(({ zoom, cartodb_id, resolution, aggregation, expected }) => { + it('should return features aggregated by type', function (done) { + const mapConfig = createVectorMapConfig([{ + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + resolution + } + } + }]); + const testClient = new TestClient(mapConfig); + const layerId = 0; + const params = { aggregation }; + + testClient.getClusterFeatures(zoom, cartodb_id, layerId, params, (err, body) => { + if (err) { + return done(err); + } + + assert.deepStrictEqual(body.rows, expected); + + testClient.drain(done); + }); + }); + }); + }); }); diff --git a/test/support/test-client.js b/test/support/test-client.js index b1e4a211..5d88f6e0 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -690,7 +690,12 @@ TestClient.prototype.getClusterFeatures = function (zoom, clusterId, layerId, pa var next = this; - url = '/api/v1/map/' + layergroupId + '/' + layerId + '/' + zoom + '/cluster/' + clusterId; + let queryParams = ''; + if (params.aggregation) { + queryParams = qs.stringify({ aggregation: JSON.stringify(params.aggregation) }); + } + + url = `/api/v1/map/${layergroupId}/${layerId}/${zoom}/cluster/${clusterId}?${queryParams}`; assert.response(self.server, { From 6dadb1bf6fa58f36b39e3b48475a4ff20e6a1808 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 15:17:22 +0100 Subject: [PATCH 09/16] Validate aggregation input --- lib/cartodb/backends/cluster.js | 63 ++++++++++++- test/acceptance/cluster.js | 159 +++++++++++++++++++++++++++++++- 2 files changed, 215 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index cba5b80e..671684e4 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -38,7 +38,61 @@ module.exports = class ClusterBackend { return callback(error); } - const { aggregation } = params; + let { aggregation } = params; + + if ( aggregation !== undefined) { + try { + aggregation = JSON.parse(aggregation); + } catch (err) { + const error = new Error(`Invalid aggregation input, should be a a valid JSON`); + error.http_status = 400; + error.type = 'layer'; + error.subtype = 'aggregation'; + error.layer = { + index: layerIndex, + type: layer.type + }; + + return callback(error); + } + + const { columns, expressions } = aggregation; + + if (!Array.isArray(columns) || !columns.length) { + const error = new Error( + `Invalid aggregation input, columns should be and array of column names` + ); + error.http_status = 400; + error.type = 'layer'; + error.subtype = 'aggregation'; + error.layer = { + index: layerIndex, + type: layer.type + }; + + return callback(error); + } + + if (expressions !== undefined) { + if (expressions === null || + Array.isArray(expressions) || + ['string', 'number', 'boolean'].includes(typeof expressions)) { + const error = new Error( + `Invalid aggregation input, expressions should be and object with expressions` + ); + error.http_status = 400; + error.type = 'layer'; + error.subtype = 'aggregation'; + error.layer = { + index: layerIndex, + type: layer.type + }; + + return callback(error); + } + } + } + const query = layer.options.sql_raw; const resolution = layer.options.aggregation.resolution || 1; @@ -96,13 +150,12 @@ function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, ag }); if (aggregation !== undefined) { - aggregation = JSON.parse(aggregation); - const { columns = [], expresions = [] } = aggregation; + const { columns = [], expressions = [] } = aggregation; sql = aggregationQuery({ columns, query: sql, - expresions + expressions }); } @@ -155,7 +208,7 @@ const aggregationQuery = ctx => ` SELECT count(1) as _cdb_feature_count ${ctx.columns.length ? `,${ctx.columns.join(', ')}` : ''} - ${ctx.expresions.length ? `,${ctx.expresions.join(', ')}` : ''} + ${ctx.expressions.length ? `,${ctx.expressions.join(', ')}` : ''} FROM (${ctx.query}) __cdb_aggregation ${ctx.columns.length ? `GROUP BY ${ctx.columns.join(', ')}` : ''} `; diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index 8b6ee5d7..5e856da2 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -324,7 +324,7 @@ describe('cluster', function () { }); }); - describe('map-config w/o aggregation', function () { + describe('with aggregation', function () { const suite = [ { zoom: 0, @@ -397,7 +397,6 @@ describe('cluster', function () { } ]; - suite.forEach(({ zoom, cartodb_id, resolution, aggregation, expected }) => { it('should return features aggregated by type', function (done) { const mapConfig = createVectorMapConfig([{ @@ -426,4 +425,160 @@ describe('cluster', function () { }); }); }); + + describe('invalid aggregation', function () { + const expectedColumnsError = { + errors:[ 'Invalid aggregation input, columns should be and array of column names' ], + errors_with_context:[ + { + layer: { + index: '0', + type: 'cartodb' + }, + message: 'Invalid aggregation input, columns should be and array of column names', + subtype: 'aggregation', + type: 'layer' + } + ] + }; + + const expectedExpressionsError = { + errors:[ 'Invalid aggregation input, expressions should be and object with expressions' ], + errors_with_context:[ + { + layer: { + index: '0', + type: 'cartodb' + }, + message: 'Invalid aggregation input, expressions should be and object with expressions', + subtype: 'aggregation', + type: 'layer' + } + ] + }; + + const suite = [ + { + description: 'empty aggregation object should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: {}, + expected: expectedColumnsError + }, + { + description: 'empty aggregation array should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: [], + expected: expectedColumnsError + }, + { + description: 'aggregation as string should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: 'wadus', + expected: expectedColumnsError + }, + { + description: 'empty columns array should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: [] }, + expected: expectedColumnsError + }, + { + description: 'empty columns object should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: {} }, + expected: expectedColumnsError + }, + { + description: 'columns as string should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: 'wadus' }, + expected: expectedColumnsError + }, + { + description: 'columns as null should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: null }, + expected: expectedColumnsError + }, + { + description: 'empty expressions array should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: [ 'type' ], expressions: [] }, + expected: expectedExpressionsError + }, + { + description: 'empty expressions number should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: [ 'type' ], expressions: 1 }, + expected: expectedExpressionsError + }, + { + description: 'expressions as string should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: [ 'type' ], expressions: 'wadus' }, + expected: expectedExpressionsError + }, + { + description: 'expressions as null should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { columns: [ 'type' ], expressions: null }, + expected: expectedExpressionsError + } + ]; + + suite.forEach(({ description, zoom, cartodb_id, resolution, aggregation, expected }) => { + it(description, function (done) { + const mapConfig = createVectorMapConfig([{ + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + resolution + } + } + }]); + const testClient = new TestClient(mapConfig); + const layerId = 0; + const params = { + response: { + status: 400 + }, + aggregation + }; + + testClient.getClusterFeatures(zoom, cartodb_id, layerId, params, (err, body) => { + if (err) { + return done(err); + } + + assert.deepStrictEqual(body, expected); + + testClient.drain(done); + }); + }); + }); + }); }); From 57a229655c816d6c25735c5b2b77fde6ff1427b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 15:45:38 +0100 Subject: [PATCH 10/16] Implement aggregation functions --- lib/cartodb/backends/cluster.js | 12 ++- test/acceptance/cluster.js | 145 +++++++++++++++++++++++++++++++- 2 files changed, 151 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 671684e4..c9108da9 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -78,7 +78,7 @@ module.exports = class ClusterBackend { Array.isArray(expressions) || ['string', 'number', 'boolean'].includes(typeof expressions)) { const error = new Error( - `Invalid aggregation input, expressions should be and object with expressions` + `Invalid aggregation input, expressions should be and object with valid functions` ); error.http_status = 400; error.type = 'layer'; @@ -150,12 +150,16 @@ function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, ag }); if (aggregation !== undefined) { - const { columns = [], expressions = [] } = aggregation; + let { columns = [], expressions = [] } = aggregation; + + if (expressions) { + expressions = Object.entries(expressions).map(entries =>`${entries[1].aggregated_function}(${entries[1].aggregated_column}) AS ${entries[0]}`); + } sql = aggregationQuery({ columns, - query: sql, - expressions + expressions, + query: sql }); } diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index 5e856da2..881dbf8e 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -394,6 +394,147 @@ describe('cluster', function () { { _cdb_feature_count: 1, type: 'even' }, { _cdb_feature_count: 2, type: 'odd' } ] + }, + { + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ { _cdb_feature_count: 1, type: 'odd', max_value: -3 } ] + }, + { + zoom: 0, + cartodb_id: 2, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ { _cdb_feature_count: 1, type: 'even', max_value: -2 } ] + }, + { + zoom: 0, + cartodb_id: 3, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ { _cdb_feature_count: 1, type: 'odd', max_value: -1 } ] + }, + { + zoom: 0, + cartodb_id: 4, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ { _cdb_feature_count: 1, type: 'even', max_value: 0 } ] + }, + { + zoom: 0, + cartodb_id: 5, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ { _cdb_feature_count: 1, type: 'odd', max_value: 1 } ] + }, + { + zoom: 0, + cartodb_id: 6, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ { _cdb_feature_count: 1, type: 'even', max_value: 2 } ] + }, + { + zoom: 0, + cartodb_id: 7, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ { _cdb_feature_count: 1, type: 'odd', max_value: 3 } ] + }, + { + zoom: 0, + cartodb_id: 1, + resolution: 50, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ + { _cdb_feature_count: 2, type: 'even', max_value: 0 }, + { _cdb_feature_count: 2, type: 'odd', max_value: -1 } + ] + }, + { + zoom: 0, + cartodb_id: 5, + resolution: 50, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregated_function: 'max', + aggregated_column: 'value', + } + } + }, + expected: [ + { _cdb_feature_count: 1, type: 'even', max_value: 2 }, + { _cdb_feature_count: 2, type: 'odd', max_value: 3 } + ] } ]; @@ -443,14 +584,14 @@ describe('cluster', function () { }; const expectedExpressionsError = { - errors:[ 'Invalid aggregation input, expressions should be and object with expressions' ], + errors:[ 'Invalid aggregation input, expressions should be and object with valid functions' ], errors_with_context:[ { layer: { index: '0', type: 'cartodb' }, - message: 'Invalid aggregation input, expressions should be and object with expressions', + message: 'Invalid aggregation input, expressions should be and object with valid functions', subtype: 'aggregation', type: 'layer' } From f9e5d9d0a976771ca448aaae78e182a4b4289d6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 17:02:06 +0100 Subject: [PATCH 11/16] Validate aggregation expresions --- lib/cartodb/backends/cluster.js | 33 +++++++- test/acceptance/cluster.js | 142 ++++++++++++++++++++++++++++---- 2 files changed, 160 insertions(+), 15 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index c9108da9..650b42ce 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -90,6 +90,36 @@ module.exports = class ClusterBackend { return callback(error); } + + for (const [columnName, exp] of Object.entries(expressions)) { + const { aggregate_function, aggregated_column } = exp; + + if (typeof aggregated_column !== 'string') { + const error = new Error(`Invalid aggregation input, aggregated column should be an string`); + error.http_status = 400; + error.type = 'layer'; + error.subtype = 'aggregation'; + error.layer = { + index: layerIndex, + type: layer.type + }; + + return callback(error); + } + + if (typeof aggregate_function !== 'string') { + const error = new Error(`Invalid aggregation input, aggregate function should be an string`); + error.http_status = 400; + error.type = 'layer'; + error.subtype = 'aggregation'; + error.layer = { + index: layerIndex, + type: layer.type + }; + + return callback(error); + } + } } } @@ -153,7 +183,8 @@ function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, ag let { columns = [], expressions = [] } = aggregation; if (expressions) { - expressions = Object.entries(expressions).map(entries =>`${entries[1].aggregated_function}(${entries[1].aggregated_column}) AS ${entries[0]}`); + expressions = Object.entries(expressions) + .map(([columnName, exp]) => `${exp.aggregate_function}(${exp.aggregated_column}) AS ${columnName}`); } sql = aggregationQuery({ diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index 881dbf8e..87ff63f5 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -403,7 +403,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -418,7 +418,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -433,7 +433,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -448,7 +448,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -463,7 +463,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -478,7 +478,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -493,7 +493,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -508,7 +508,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -526,7 +526,7 @@ describe('cluster', function () { columns: [ 'type' ], expressions: { max_value: { - aggregated_function: 'max', + aggregate_function: 'max', aggregated_column: 'value', } } @@ -567,7 +567,7 @@ describe('cluster', function () { }); }); - describe('invalid aggregation', function () { + describe.only('invalid aggregation', function () { const expectedColumnsError = { errors:[ 'Invalid aggregation input, columns should be and array of column names' ], errors_with_context:[ @@ -598,6 +598,57 @@ describe('cluster', function () { ] }; + const invalidFunctionExpressionsError = { + errors:[ 'function wadus(integer) does not exist' ], + errors_with_context:[ + { + message: 'function wadus(integer) does not exist', + type: 'unknown' + } + ] + }; + + const invalidColumnExpressionsError = { + errors:[ 'column \"wadus\" does not exist' ], + errors_with_context:[ + { + message: 'column \"wadus\" does not exist', + type: 'unknown' + } + ] + }; + + + const expectedAggregatedColumnError = { + errors:[ 'Invalid aggregation input, aggregated column should be an string' ], + errors_with_context:[ + { + layer: { + index: '0', + type: 'cartodb' + }, + message: 'Invalid aggregation input, aggregated column should be an string', + subtype: 'aggregation', + type: 'layer' + } + ] + }; + + const expectedAggregateFunctionError = { + errors:[ 'Invalid aggregation input, aggregate function should be an string' ], + errors_with_context:[ + { + layer: { + index: '0', + type: 'cartodb' + }, + message: 'Invalid aggregation input, aggregate function should be an string', + subtype: 'aggregation', + type: 'layer' + } + ] + }; + const suite = [ { description: 'empty aggregation object should respond with error', @@ -686,10 +737,75 @@ describe('cluster', function () { resolution: 1, aggregation: { columns: [ 'type' ], expressions: null }, expected: expectedExpressionsError + }, + { + description: 'invalid aggregation function should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregate_function: 'wadus', + aggregated_column: 'value' + } + } + }, + expected: invalidFunctionExpressionsError + }, + { + description: 'invalid aggregation column should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregate_function: 'max', + aggregated_column: 'wadus' + } + } + }, + status: 404, + expected: invalidColumnExpressionsError + }, + { + description: 'aggregated column as non string should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregate_function: 'max', + aggregated_column: 1 + } + } + }, + expected: expectedAggregatedColumnError + }, + { + description: 'aggregate function as non string should respond with error', + zoom: 0, + cartodb_id: 1, + resolution: 1, + aggregation: { + columns: [ 'type' ], + expressions: { + max_value: { + aggregate_function: 1, + aggregated_column: 'value' + } + } + }, + expected: expectedAggregateFunctionError } ]; - suite.forEach(({ description, zoom, cartodb_id, resolution, aggregation, expected }) => { + suite.forEach(({ description, zoom, cartodb_id, resolution, aggregation, expected, status = 400 }) => { it(description, function (done) { const mapConfig = createVectorMapConfig([{ type: 'cartodb', @@ -704,9 +820,7 @@ describe('cluster', function () { const testClient = new TestClient(mapConfig); const layerId = 0; const params = { - response: { - status: 400 - }, + response: { status }, aggregation }; From dcf147cdfbc38ac9efd20c1f95e4457793badd1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 17:05:35 +0100 Subject: [PATCH 12/16] linter --- lib/cartodb/backends/cluster.js | 9 +++++---- test/acceptance/cluster.js | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index 650b42ce..c90f176d 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -6,6 +6,7 @@ const debug = require('debug')('backend:cluster'); const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfig'); module.exports = class ClusterBackend { + // jshint maxcomplexity: 17 getClusterFeatures (mapConfigProvider, params, callback) { mapConfigProvider.getMapConfig((err, _mapConfig) => { if (err) { @@ -91,9 +92,7 @@ module.exports = class ClusterBackend { return callback(error); } - for (const [columnName, exp] of Object.entries(expressions)) { - const { aggregate_function, aggregated_column } = exp; - + for (const { aggregate_function, aggregated_column } of Object.values(expressions)) { if (typeof aggregated_column !== 'string') { const error = new Error(`Invalid aggregation input, aggregated column should be an string`); error.http_status = 400; @@ -108,7 +107,9 @@ module.exports = class ClusterBackend { } if (typeof aggregate_function !== 'string') { - const error = new Error(`Invalid aggregation input, aggregate function should be an string`); + const error = new Error( + `Invalid aggregation input, aggregate function should be an string` + ); error.http_status = 400; error.type = 'layer'; error.subtype = 'aggregation'; diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index 87ff63f5..a535f6c3 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -567,7 +567,7 @@ describe('cluster', function () { }); }); - describe.only('invalid aggregation', function () { + describe('invalid aggregation', function () { const expectedColumnsError = { errors:[ 'Invalid aggregation input, columns should be and array of column names' ], errors_with_context:[ From a412e37a32e71432af76e07d334a6a34021488b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 17:21:55 +0100 Subject: [PATCH 13/16] Improve test descriptions --- test/acceptance/cluster.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/acceptance/cluster.js b/test/acceptance/cluster.js index a535f6c3..632b6e8a 100644 --- a/test/acceptance/cluster.js +++ b/test/acceptance/cluster.js @@ -36,7 +36,7 @@ function createVectorMapConfig (layers = defaultLayers) { } describe('cluster', function () { - describe('map-config w/o aggregation', function () { + describe('w/o aggregation', function () { it('should return error while fetching disaggregated features', function (done) { const mapConfig = createVectorMapConfig([{ type: 'cartodb', @@ -123,7 +123,7 @@ describe('cluster', function () { }); }); - describe('map-config with aggregation', function () { + describe('fetch features within a cluster grid', function () { const suite = [ { zoom: 0, @@ -324,7 +324,7 @@ describe('cluster', function () { }); }); - describe('with aggregation', function () { + describe('valid aggregation input', function () { const suite = [ { zoom: 0, @@ -567,7 +567,7 @@ describe('cluster', function () { }); }); - describe('invalid aggregation', function () { + describe('invalid aggregation input', function () { const expectedColumnsError = { errors:[ 'Invalid aggregation input, columns should be and array of column names' ], errors_with_context:[ From f5fb60aa560afaf92869c2af21498ec0313f368f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 17:34:32 +0100 Subject: [PATCH 14/16] Filter output --- .../api/map/clustered-features-layergroup-controller.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/api/map/clustered-features-layergroup-controller.js b/lib/cartodb/api/map/clustered-features-layergroup-controller.js index 9779a09a..48e56b32 100644 --- a/lib/cartodb/api/map/clustered-features-layergroup-controller.js +++ b/lib/cartodb/api/map/clustered-features-layergroup-controller.js @@ -85,7 +85,8 @@ function getClusteredFeatures (clusterBackend) { } res.statusCode = 200; - res.body = features; + const { rows, fields } = features; + res.body = { rows, fields }; next(); }); From 2a0287b35820c60df8f339f91752af7d9d704e49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 17:49:26 +0100 Subject: [PATCH 15/16] Add todos --- lib/cartodb/api/map/clustered-features-layergroup-controller.js | 1 + lib/cartodb/backends/cluster.js | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/cartodb/api/map/clustered-features-layergroup-controller.js b/lib/cartodb/api/map/clustered-features-layergroup-controller.js index 48e56b32..5c17c5b1 100644 --- a/lib/cartodb/api/map/clustered-features-layergroup-controller.js +++ b/lib/cartodb/api/map/clustered-features-layergroup-controller.js @@ -42,6 +42,7 @@ module.exports = class AggregatedFeaturesLayergroupController { credentials(), authorize(this.authBackend), dbConnSetup(this.pgConnection), + // TODO: create its rate limit rateLimit(this.userLimitsBackend, RATE_LIMIT_ENDPOINTS_GROUPS.ATTRIBUTES), cleanUpQueryParams([ 'aggregation' ]), createMapStoreMapConfigProvider( diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js index c90f176d..c9776eba 100644 --- a/lib/cartodb/backends/cluster.js +++ b/lib/cartodb/backends/cluster.js @@ -6,6 +6,7 @@ const debug = require('debug')('backend:cluster'); const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfig'); module.exports = class ClusterBackend { + // TODO: reduce complexity // jshint maxcomplexity: 17 getClusterFeatures (mapConfigProvider, params, callback) { mapConfigProvider.getMapConfig((err, _mapConfig) => { From 47321aebc45a141f71d06707d1ec9b9ae7a384d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Mar 2019 17:53:15 +0100 Subject: [PATCH 16/16] Updates NEWS and next release version --- NEWS.md | 5 ++++- package-lock.json | 2 +- package.json | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/NEWS.md b/NEWS.md index 1e592442..605a9751 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,8 +1,11 @@ # Changelog -## 7.0.1 +## 7.1.0 Released 2019-mm-dd +Announcements: +- Experimental support for listing features in a grid when the map uses the dynamic agregation. + ## 7.0.0 Released 2019-02-22 diff --git a/package-lock.json b/package-lock.json index fb770817..94641675 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "windshaft-cartodb", - "version": "7.0.1", + "version": "7.1.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index f2ce3a36..b62b2e6e 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "private": true, "name": "windshaft-cartodb", - "version": "7.0.1", + "version": "7.1.0", "description": "A map tile server for CartoDB", "keywords": [ "cartodb"