From 6ada8ba6a2864cbe29e1040e37e16fd721b30d6a Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 21 Mar 2018 17:01:32 +0100 Subject: [PATCH 01/17] Implement aggregation filters --- .../aggregation/aggregation-mapconfig.js | 26 +- .../models/aggregation/aggregation-query.js | 129 ++++- .../aggregation/aggregation-validator.js | 32 ++ test/acceptance/aggregation.js | 504 ++++++++++++++++++ 4 files changed, 680 insertions(+), 11 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-mapconfig.js b/lib/cartodb/models/aggregation/aggregation-mapconfig.js index 50e4dcfe..9306518c 100644 --- a/lib/cartodb/models/aggregation/aggregation-mapconfig.js +++ b/lib/cartodb/models/aggregation/aggregation-mapconfig.js @@ -4,7 +4,8 @@ const aggregationValidator = require('./aggregation-validator'); const { createPositiveNumberValidator, createIncludesValueValidator, - createAggregationColumnsValidator + createAggregationColumnsValidator, + createAggregationFiltersValidator } = aggregationValidator; const SubstitutionTokens = require('../../utils/substitution-tokens'); @@ -43,6 +44,18 @@ module.exports = class AggregationMapConfig extends MapConfig { ]; } + static get FILTER_PARAMETERS () { + return [ + // TODO: valid combinations of parameters: + // * Except for less/greater params, only one parameter allowed per filter. + // * Any less parameter can be combined with one of the greater paramters. (to define a range) + 'less_than', 'less_than_or_equal_to', + 'greater_than', 'greater_than_or_equal_to', + 'equal', 'not_equal', + 'between', 'in', 'not_in' + ]; + } + static supportsGeometryType(geometryType) { return AggregationMapConfig.SUPPORTED_GEOMETRY_TYPES.includes(geometryType); } @@ -58,11 +71,15 @@ module.exports = class AggregationMapConfig extends MapConfig { const positiveNumberValidator = createPositiveNumberValidator(this); const includesValidPlacementsValidator = createIncludesValueValidator(this, AggregationMapConfig.PLACEMENTS); const aggregationColumnsValidator = createAggregationColumnsValidator(this, AggregationMapConfig.AGGREGATIONS); + const aggregationFiltersValidator = createAggregationFiltersValidator( + this, AggregationMapConfig.FILTER_PARAMETERS + ); validate('resolution', positiveNumberValidator); validate('placement', includesValidPlacementsValidator); validate('threshold', positiveNumberValidator); validate('columns', aggregationColumnsValidator); + validate('filters', aggregationFiltersValidator); this.user = user; this.pgConnection = connection; @@ -77,7 +94,8 @@ module.exports = class AggregationMapConfig extends MapConfig { threshold = AggregationMapConfig.THRESHOLD, placement, columns = {}, - dimensions = {} + dimensions = {}, + filters = {} } = this.getAggregation(index); return aggregationQuery({ @@ -87,6 +105,7 @@ module.exports = class AggregationMapConfig extends MapConfig { placement, columns, dimensions, + filters, isDefaultAggregation: this._isDefaultLayerAggregation(index) }); } @@ -220,7 +239,8 @@ module.exports = class AggregationMapConfig extends MapConfig { _isDefaultAggregation (aggregation) { return aggregation.placement === undefined && aggregation.columns === undefined && - this._isEmptyParameter(aggregation.dimensions); + this._isEmptyParameter(aggregation.dimensions) && + this._isEmptyParameter(aggregation.filters); } _isEmptyParameter(parameter) { diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 16e897ab..bfd2def6 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -42,7 +42,8 @@ const queryForOptions = (options) => templateForOptions(options)({ sourceQuery: options.query, res: 256/options.resolution, columns: options.columns, - dimensions: options.dimensions + dimensions: options.dimensions, + filters: options.filters }); module.exports = queryForOptions; @@ -93,20 +94,23 @@ const aggregateColumnNames = (ctx, table) => { return sep(Object.keys(columns)); }; +const aggregateExpression = (column_name, column_parameters) => { + const aggregate_function = column_parameters.aggregate_function || 'count'; + const aggregate_definition = SUPPORTED_AGGREGATE_FUNCTIONS[aggregate_function]; + if (!aggregate_definition) { + throw new Error("Invalid Aggregate function: '" + aggregate_function + "'"); + } + return aggregate_definition.sql(column_name, column_parameters); +}; + const aggregateColumnDefs = ctx => { let columns = aggregateColumns(ctx); return sep(Object.keys(columns).map(column_name => { - const aggregate_function = columns[column_name].aggregate_function || 'count'; - const aggregate_definition = SUPPORTED_AGGREGATE_FUNCTIONS[aggregate_function]; - if (!aggregate_definition) { - throw new Error("Invalid Aggregate function: '" + aggregate_function + "'"); - } - const aggregate_expression = aggregate_definition.sql(column_name, columns[column_name]); + const aggregate_expression = aggregateExpression(column_name, columns[column_name]); return `${aggregate_expression} AS ${column_name}`; })); }; - const aggregateDimensions = ctx => ctx.dimensions || {}; const dimensionNames = (ctx, table) => { @@ -127,6 +131,111 @@ const dimensionDefs = ctx => { })); }; +const aggregateFilters = ctx => ctx.filters || {}; + +const filterConditionSQL = (expr, filter) => { + // TODO: validate filter parameters (e.g. cannot have both greater_than and greater_than or equal to) + + if (filter) { + if (!Array.isArray(filter)) { + filter = [filter]; + } + if (filter.length > 0) { + return filter.map(f => filterSingleConditionSQL(expr, f)).join(' OR '); + } + } +}; + +const filterSingleConditionSQL = (expr, filter) => { + let cond; + Object.keys(FILTERS).some(f => { + cond = FILTERS[f](expr, filter); + return cond; + }); + return cond; +}; + +const sqlQ = (value) => { + if (isFinite(value)) { + return String(value); + } + return `'${value}'`; // TODO: escape single quotes! (by doubling them) +}; + +/* jshint eqeqeq: false */ +/* x != null is used to check for both null and undefined; triple !== wouldn't do the trick */ + +const FILTERS = { + between: (expr, filter) => { + const lo = filter.greater_than_or_equal_to, hi = filter.less_than_or_equal_to; + if (lo != null && hi != null) { + return `(${expr} BETWEEN ${sqlQ(lo)} AND ${sqlQ(hi)})`; + } + }, + in: (expr, filter) => { + if (filter.in != null) { + return `(${expr} IN (${filter.in.map(v => sqlQ(v)).join(',')}))`; + } + }, + notin: (expr, filter) => { + if (filter.not_in != null) { + return `(${expr} NOT IN (${filter.not_in.map(v => sqlQ(v)).join(',')}))`; + } + }, + equal: (expr, filter) => { + if (filter.equal != null) { + return `(${expr} = ${sqlQ(filter.equal)})`; + } + }, + not_equal: (expr, filter) => { + if (filter.not_equal != null) { + return `(${expr} <> ${sqlQ(filter.not_equal)})`; + } + }, + range: (expr, filter) => { + let conds = []; + if (filter.greater_than_or_equal_to != null) { + conds.push(`(${expr} >= ${sqlQ(filter.greater_than_or_equal_to)})`); + } + if (filter.greater_than != null) { + conds.push(`(${expr} > ${sqlQ(filter.greater_than)})`); + } + if (filter.less_than_or_equal_to != null) { + conds.push(`(${expr} <= ${sqlQ(filter.less_than_or_equal_to)})`); + } + if (filter.less_than != null) { + conds.push(`(${expr} < ${sqlQ(filter.less_than)})`); + } + if (conds.length > 0) { + return conds.join(' AND '); + } + } +}; + +const filterConditions = ctx => { + let columns = aggregateColumns(ctx); + let dimensions = aggregateDimensions(ctx); + let filters = aggregateFilters(ctx); + return Object.keys(filters).map(filtered_column => { + let filtered_expr; + if (columns[filtered_column]) { + filtered_expr = aggregateExpression(filtered_column, columns[filtered_column]); + } + else if (dimensions[filtered_column]) { + filtered_expr = dimensions[filtered_column]; + } + if (!filtered_expr) { + throw new Error("Invalid filtered column: '" + filtered_column + "'"); + } + return filterConditionSQL(filtered_expr, filters[filtered_column]); + }).join(' AND '); +}; + +const havingClause = ctx => { + let cond = filterConditions(ctx); + return cond ? `HAVING ${cond}` : ''; +}; + // SQL expression to compute the aggregation resolution (grid cell size). // This is equivalent to `${256/ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!))` // This is defined by the ctx.res parameter, which is the number of grid cells per tile linear dimension @@ -164,6 +273,7 @@ const defaultAggregationQueryTemplate = ctx => ` Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) ${dimensionNames(ctx)} + ${havingClause(ctx)} ) SELECT _cdb_query.* ${aggregateColumnNames(ctx)} @@ -196,6 +306,7 @@ const aggregationQueryTemplates = { Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) ${dimensionNames(ctx)} + ${havingClause(ctx)} `, 'point-grid': ctx => ` @@ -214,6 +325,7 @@ const aggregationQueryTemplates = { FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE the_geom_webmercator && _cdb_params.bbox GROUP BY _cdb_gx, _cdb_gy ${dimensionNames(ctx)} + ${havingClause(ctx)} ) SELECT row_number() over() AS cartodb_id, @@ -241,6 +353,7 @@ const aggregationQueryTemplates = { Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) ${dimensionNames(ctx)} + ${havingClause(ctx)} ) SELECT _cdb_clusters.cartodb_id, diff --git a/lib/cartodb/models/aggregation/aggregation-validator.js b/lib/cartodb/models/aggregation/aggregation-validator.js index d0dc24c2..ce038f05 100644 --- a/lib/cartodb/models/aggregation/aggregation-validator.js +++ b/lib/cartodb/models/aggregation/aggregation-validator.js @@ -42,6 +42,38 @@ module.exports.createAggregationColumnsValidator = function (mapconfig, validAgg }; }; +module.exports.createAggregationFiltersValidator = function (mapconfig, validParameters) { + return function validateAggregationFilters (value, key, index) { + const dims = mapconfig.getAggregation(index).dimensions || {}; + const cols = mapconfig.getAggregation(index).columns || {}; + const validKeys = Object.keys(dims).concat(Object.keys(cols)); + Object.keys(value).forEach((filteredName) => { + // filteredName must be the name of either an aggregated column or a dimension in the same layer + if (!validKeys.includes(filteredName)) { + const message = `Invalid filtered column: ${filteredName}`; + throw createLayerError(message, mapconfig, index); + } + // The filter parameters must be valid + let filters = value[filteredName]; + // a single filter or an array of filters (to be OR-combined) are accepted + if (!Array.isArray(filters)) { + filters = [filters]; + } + filters.forEach(params => { + Object.keys(params).forEach(paramName => { + if (!validParameters.includes(paramName)) { + const message = `Invalid filter parameter name: ${paramName}`; + throw createLayerError(message, mapconfig, index); + } + }); + // TODO: check parameter value (params[paramName]) to be of the correct type + }); + // TODO: if multiple parameters within params check the combination is valid, + // i.e. one of the *less* parameters and one of the *greater* parameters. + }); + }; +}; + function createAggregationColumnNamesValidator(mapconfig) { return function validateAggregationColumnNames (value, key, index) { Object.keys(value).forEach((columnName) => { diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index d8870c8a..db03ad8e 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -1475,6 +1475,510 @@ describe('aggregation', function () { done(); }); }); + + + ['centroid', 'point-sample', 'point-grid'].forEach(placement => { + it(`filters should work for ${placement} placement`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + placement: placement , + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: { + greater_than_or_equal_to: 0 + } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value >= 0); + }); + + done(); + }); + }); + }); + + ['centroid', 'point-sample', 'point-grid'].forEach(placement => { + it(`multiple ORed filters should work for ${placement} placement`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + placement: placement , + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: [ + { greater_than: 0 }, + { less_than: -2 } + ] + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value > 0 || row.properties.value < -2); + }); + + done(); + }); + }); + }); + + ['centroid', 'point-sample', 'point-grid'].forEach(placement => { + it(`multiple ANDed filters should work for ${placement} placement`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + placement: placement , + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + }, + value2: { + aggregate_function: 'sum', + aggregated_column: 'sqrt_value' + } + }, + filters: { + value: { greater_than: 0 }, + value2: { less_than: 9 } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value > 0 && row.properties.value2 < 9); + }); + + done(); + }); + }); + }); + + it(`supports IN filters`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: { in: [1, 3] } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value === 1 || row.properties.value === 3); + }); + + done(); + }); + }); + + it(`supports NOT IN filters`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: { not_in: [1, 3] } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value !== 1 && row.properties.value !== 3); + }); + + done(); + }); + }); + + it(`supports EQUAL filters`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: [{ equal: 1}, { equal: 3}] + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value === 1 || row.properties.value === 3); + }); + + done(); + }); + }); + + it(`supports NOT EQUAL filters`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: { not_equal: 1 } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value !== 1); + }); + + done(); + }); + }); + + it(`supports BETWEEN filters`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: { + greater_than_or_equal_to: -1, + less_than_or_equal_to: 2 + } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value >= -1 || row.properties.value <= 2); + }); + + done(); + }); + }); + + it(`supports RANGE filters`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: { + greater_than: -1, + less_than_or_equal_to: 2 + } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach(row => { + assert.ok(row.properties.value > -1 || row.properties.value <= 2); + }); + + done(); + }); + }); + + it(`invalid filters cause errors`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: { + not_a_valid_parameter: 0 + } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + + const options = { + response: { + status: 400 + } + }; + + this.testClient.getLayergroup(options, (err, body) => { + if (err) { + return done(err); + } + + assert.deepEqual(body, { + errors: [ 'Invalid filter parameter name: not_a_valid_parameter'], + errors_with_context:[{ + type: 'layer', + message: 'Invalid filter parameter name: not_a_valid_parameter', + layer: { + id: "layer0", + index: 0, + type: "mapnik", + } + }] + }); + + done(); + }); + }); + + it(`filters on invalid columns cause errors`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1, + columns: { + value_sum: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + filters: { + value: { + not_a_valid_parameter: 0 + } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + + const options = { + response: { + status: 400 + } + }; + + this.testClient.getLayergroup(options, (err, body) => { + if (err) { + return done(err); + } + + assert.deepEqual(body, { + errors: [ 'Invalid filtered column: value'], + errors_with_context:[{ + type: 'layer', + message: 'Invalid filtered column: value', + layer: { + id: "layer0", + index: 0, + type: "mapnik", + } + }] + }); + + done(); + }); + }); + }); }); }); From ead6fa5f1fdee8f8b5aa88a3ad6faf0e6a35a8f2 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 21 Mar 2018 17:21:05 +0100 Subject: [PATCH 02/17] Document aggregation filters Note that dimension filters remain undocumented --- docs/aggregation.md | 77 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/docs/aggregation.md b/docs/aggregation.md index 2fb361c3..c3e754e1 100644 --- a/docs/aggregation.md +++ b/docs/aggregation.md @@ -185,3 +185,80 @@ This is the minimum number of (estimated) rows in the dataset (query results) fo ] } ``` + +### `filters` + +Aggregated data can be filtered by imposing filtering conditions on the aggregated columns. + +Each condition is represented by one or more parameters: + +* `{ "equal": V }` selects an specific value of the aggregated column. +* `{ "not_equal": V }` selects values different from the one specified. +* `{ "in": [v1, v2, v3] }` selects any value from a list. +* `{ "not_in": [v1, v2, v3] }` selects any value not in a list. +* `{ "less_than": v }` selects values strictly less than the one given. +* `{ "less_than_or_equal_to": v }` selects values less than or equal to the one given. +* `{ "greater_than": v }` selects values strictly greater than the one given. +* `{ "greater_than_or_equal_to": v }` selects values greater than or equal to the one given. + +One of the *less* conditions can be combined with one of the *greater* conditions to select a range of values, for example: +* `{ "greater_than": v1, "less_than": v2 }` +* `{ "greater_than_or_equal_to": v1, "less_than": v2 }` +* `{ "greater_than": v1, "less_than_or_equal_to": v2 }` +* `{ "greater_than_or_equal_to": v1, "less_than_or_equal_to": v2 }` + +For a given column, multiple conditions can be passed in an array; the conditions will logically ORed (any of the conditions have to be verifid for the value to be selected): + +* `"myvalue": [ { "equal": 10 }, { "less_than": 0 }]` will select values of the column `myvalue` which are equal to 10 **or** less than 0. + +In addition, the filters applied to different columns are logically combined with AND (all the conditions have to be satisfied for an element to be selected); for example with the following `filters` parameter we'll select aggregated records which have a `total_value` > 100 **and** a category equal to "a". + +```json +{ + "total_value": { "greater_than": 100 }, + "category": { "equal": "a" } +} +``` + +Note that the filtered columns have to be defined with the `columns` parameter, except for `_cdb_features_count`, which is always implicitly defined and can be filtered too. + +#### Example + +```json +{ + "version": "1.7.0", + "extent": [-20037508.5, -20037508.5, 20037508.5, 20037508.5], + "srid": 3857, + "maxzoom": 18, + "minzoom": 3, + "layers": [ + { + "type": "mapnik", + "options": { + "sql": "select * from table", + "cartocss": "#table { marker-width: [total]; marker-fill: ramp(value, (red, green, blue), jenks); }", + "cartocss_version": "2.3.0", + "aggregation": { + "placement": "centroid", + "columns": { + "total_value": { + "aggregate_function": "sum", + "aggregated_column": "value" + }, + "category": { + "aggregate_function": "mode", + "aggregated_column": "category" + } + }, + "filters" : { + "total_value": { "greater_than": 100 }, + "category": { "equal": "a" } + }, + "resolution": 2, + "threshold": 500000 + } + } + } + ] +} +``` From b9de49d5ab349fde078bcc036cdf6191b9875337 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 21 Mar 2018 17:36:26 +0100 Subject: [PATCH 03/17] Remove superfluous aggregation filter condition The default aggregation doesn't admit filters, so this wasn't necessary. --- lib/cartodb/models/aggregation/aggregation-query.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index bfd2def6..4c89c3e4 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -273,7 +273,6 @@ const defaultAggregationQueryTemplate = ctx => ` Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) ${dimensionNames(ctx)} - ${havingClause(ctx)} ) SELECT _cdb_query.* ${aggregateColumnNames(ctx)} From c1da1a8a169d574e248809d095e67cb5955d7573 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 4 Apr 2018 11:05:03 +0200 Subject: [PATCH 04/17] Use unique cartodb_id in aggregated results See #889 FOr centroid and point-grid the cartodb_id wasn't unique across tiles. --- lib/cartodb/models/aggregation/aggregation-query.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 4c89c3e4..aa417ae0 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -290,7 +290,7 @@ const aggregationQueryTemplates = { !bbox! AS bbox ) SELECT - row_number() over() AS cartodb_id, + MIN(_cdb_query.cartodb_id) AS cartodb_id, ST_SetSRID( ST_MakePoint( AVG(ST_X(_cdb_query.the_geom_webmercator)), @@ -317,6 +317,7 @@ const aggregationQueryTemplates = { ), _cdb_clusters AS ( SELECT + MIN(_cdb_query.cartodb_id) AS cartodb_id, Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy ${dimensionDefs(ctx)} @@ -327,7 +328,7 @@ const aggregationQueryTemplates = { ${havingClause(ctx)} ) SELECT - row_number() over() AS cartodb_id, + _cdb_clusters.cartodb_id AS cartodb_id, ST_SetSRID(ST_MakePoint((_cdb_gx+0.5)*res, (_cdb_gy+0.5)*res), 3857) AS the_geom_webmercator ${dimensionNames(ctx)} ${aggregateColumnNames(ctx)} From 2132960d7c372c3d2bbf0820dd85dc7ceb8b5c98 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 4 Apr 2018 15:25:08 +0200 Subject: [PATCH 05/17] Fix non-default aggregation columns The columns for non-default aggregations were the base columns not the resulting aggregated columns In particular this could cause invalid wrapped SQL code to be passed to ST_AsMVT when the Windshaft pg-mvt renderer was used. --- lib/cartodb/models/aggregation/aggregation-mapconfig.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-mapconfig.js b/lib/cartodb/models/aggregation/aggregation-mapconfig.js index 9306518c..d3666897 100644 --- a/lib/cartodb/models/aggregation/aggregation-mapconfig.js +++ b/lib/cartodb/models/aggregation/aggregation-mapconfig.js @@ -173,16 +173,12 @@ module.exports = class AggregationMapConfig extends MapConfig { let aggregatedColumns = []; if (columns) { - aggregatedColumns = Object.keys(columns) - .map(key => columns[key].aggregated_column) - .filter(aggregatedColumn => typeof aggregatedColumn === 'string'); + aggregatedColumns = Object.keys(columns); } let dimensionsColumns = []; if (dimensions) { - dimensionsColumns = Object.keys(dimensions) - .map(key => dimensions[key]) - .filter(dimension => typeof dimension === 'string'); + dimensionsColumns = Object.keys(dimensions); } return removeDuplicates(aggregatedColumns.concat(dimensionsColumns)); From 071b6816e3afad7420c48155c60e65630ddeab66 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 4 Apr 2018 16:28:07 +0200 Subject: [PATCH 06/17] Fix docs typos in _cdb_feature_count --- docs/MapConfig-Aggregation-extension.md | 2 +- docs/aggregation.md | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/MapConfig-Aggregation-extension.md b/docs/MapConfig-Aggregation-extension.md index 850b179a..bf2172f2 100644 --- a/docs/MapConfig-Aggregation-extension.md +++ b/docs/MapConfig-Aggregation-extension.md @@ -29,7 +29,7 @@ The value of this attribute can be `false` to explicitly disable aggregation for // object, defines the columns of the aggregated datasets. Each property corresponds to a columns name and // should contain an object with two properties: "aggregate_function" (one of "sum", "max", "min", "avg", "mode" or "count"), // and "aggregated_column" (the name of a column of the original layer query or "*") - // A column defined as `"_cdb_features_count": {"aggregate_function": "count", aggregated_column: "*"}` + // A column defined as `"_cdb_feature_count": {"aggregate_function": "count", aggregated_column: "*"}` // is always generated in addition to the defined columns. // The column names `cartodb_id`, `the_geom`, `the_geom_webmercator` and `_cdb_feature_count` cannot be used // for aggregated columns, as they correspond to columns always present in the result. diff --git a/docs/aggregation.md b/docs/aggregation.md index c3e754e1..6920776f 100644 --- a/docs/aggregation.md +++ b/docs/aggregation.md @@ -10,7 +10,7 @@ Aggregation is available only for point geometries. During aggregation the point When no placement or columns are specified a special default aggregation is performed. -This special mode performs only spatial aggregation (using a grid defined by the requested tile and the resolution, parameter, as all the other cases), and returns a _random_ record from each group (grid cell) with all its columns and an additional `_cdb_features_count` with the number of features in the group. +This special mode performs only spatial aggregation (using a grid defined by the requested tile and the resolution, parameter, as all the other cases), and returns a _random_ record from each group (grid cell) with all its columns and an additional `_cdb_feature_count` with the number of features in the group. Regarding the randomness of the sample: currently we use the row with the minimum `cartodb_id` value in each group. @@ -18,7 +18,7 @@ The rationale behind having this special aggregation with all the original colum ### User defined aggregations -When either a explicit placement or columns are requested we no longer use the special, query; we use one determined by the placement (which will default to "centroid"), and it will have as columns only the aggregated columns specified, in addition to `_cdb_features_count`, which is always present. +When either a explicit placement or columns are requested we no longer use the special, query; we use one determined by the placement (which will default to "centroid"), and it will have as columns only the aggregated columns specified, in addition to `_cdb_feature_count`, which is always present. We might decide in the future to allow sampling column values for any of the different placement modes. @@ -220,7 +220,7 @@ In addition, the filters applied to different columns are logically combined wit } ``` -Note that the filtered columns have to be defined with the `columns` parameter, except for `_cdb_features_count`, which is always implicitly defined and can be filtered too. +Note that the filtered columns have to be defined with the `columns` parameter, except for `_cdb_feature_count`, which is always implicitly defined and can be filtered too. #### Example From dc706aeb43c7e88aab1858a5688edec165414499 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 4 Apr 2018 16:29:40 +0200 Subject: [PATCH 07/17] Fix bug with dimension aliases The point-sample aggregation query failed if dimensions had alias different from the base columns --- .../models/aggregation/aggregation-query.js | 2 +- test/acceptance/aggregation.js | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index aa417ae0..cfb9855a 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -358,7 +358,7 @@ const aggregationQueryTemplates = { SELECT _cdb_clusters.cartodb_id, the_geom, the_geom_webmercator - ${dimensionNames(ctx, '_cdb_query')} + ${dimensionNames(ctx, '_cdb_clusters')} ${aggregateColumnNames(ctx, '_cdb_clusters')} FROM _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index db03ad8e..84f4a0fd 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -689,6 +689,45 @@ describe('aggregation', function () { }); }); + ['centroid', 'point-sample', 'point-grid'].forEach(placement => { + it(`dimensions with alias should work for ${placement} placement`, function(done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + placement: placement , + threshold: 1, + dimensions: { + value2: "value" + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach( + feature => assert.equal(typeof feature.properties.value2, 'number') + ); + + done(); + }); + }); + }); + + it(`dimensions should trigger non-default aggregation`, function(done) { this.mapConfig = createVectorMapConfig([ { From 3d36802686aa9d618a92dade88f939b13fa2e01f Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 4 Apr 2018 16:58:11 +0200 Subject: [PATCH 08/17] Tests for columns present in aggregation MVTs --- test/acceptance/aggregation.js | 97 ++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 84f4a0fd..a4cd906d 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -357,6 +357,103 @@ describe('aggregation', function () { }); }); + ['centroid', 'point-sample', 'point-grid'].forEach(placement => { + it('should provide all the requested columns in non-default aggregation ', + function (done) { + const response = { + status: 200, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + placement: placement, + columns: { + 'first_column': { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + dimensions: { + second_column: 'sqrt_value' + }, + threshold: 1 + }, + cartocss: '#layer { marker-width: [first_column]; line-width: [second_column]; }', + cartocss_version: '2.3.0' + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup({ response }, (err, body) => { + if (err) { + return done(err); + } + + + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.png)); + done(); + }); + }); + + it('should provide only the requested columns in non-default aggregation ', + function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + placement: placement, + columns: { + 'first_column': { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + dimensions: { + second_column: 'sqrt_value' + }, + threshold: 1 + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getTile(0, 0, 0, { format: 'mvt' }, function (err, res, mvt) { + if (err) { + return done(err); + } + + const geojsonTile = JSON.parse(mvt.toGeoJSONSync(0)); + let columns = new Set(); + geojsonTile.features.forEach(f => { + Object.keys(f.properties).forEach(p => columns.add(p)); + }); + columns = Array.from(columns); + const expected_columns = [ + '_cdb_feature_count', 'cartodb_id', 'first_column', 'second_column' + ]; + assert.deepEqual(columns.sort(), expected_columns.sort()); + + done(); + }); + }); + }); + it('should skip aggregation to create a layergroup with aggregation defined already', function (done) { const mapConfig = createVectorMapConfig([ { From e8cd6856b52643c145a89e19adcce0dc23d8a96f Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 4 Apr 2018 17:18:56 +0200 Subject: [PATCH 09/17] Add missing aggregation columns to ST_AsMVT Aggregation results always should have the cartodb_id and the feature count --- lib/cartodb/models/aggregation/aggregation-mapconfig.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/models/aggregation/aggregation-mapconfig.js b/lib/cartodb/models/aggregation/aggregation-mapconfig.js index d3666897..548f95d3 100644 --- a/lib/cartodb/models/aggregation/aggregation-mapconfig.js +++ b/lib/cartodb/models/aggregation/aggregation-mapconfig.js @@ -171,6 +171,8 @@ module.exports = class AggregationMapConfig extends MapConfig { _getLayerAggregationRequiredColumns (index) { const { columns, dimensions } = this.getAggregation(index); + let finalColumns = ['cartodb_id', '_cdb_feature_count']; + let aggregatedColumns = []; if (columns) { aggregatedColumns = Object.keys(columns); @@ -181,7 +183,7 @@ module.exports = class AggregationMapConfig extends MapConfig { dimensionsColumns = Object.keys(dimensions); } - return removeDuplicates(aggregatedColumns.concat(dimensionsColumns)); + return removeDuplicates(finalColumns.concat(aggregatedColumns).concat(dimensionsColumns)); } doesLayerReachThreshold(index, featureCount) { From 98cb0878d96c3862e1deee111952cf47bd10b0f5 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 5 Apr 2018 12:12:00 +0200 Subject: [PATCH 10/17] Update NEWS Reflect the changes in #913 --- NEWS.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 588166b9..75756417 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,8 +1,15 @@ # Changelog -## 6.0.1 +## 6.1.0 Released 2018-mm-dd +New features: +- Aggreation filters + +Bug Fixes: +- Non-default aggregation selected the wrong columns (e.g. for vector tiles) +- Aggregation dimensions with alias where broken + ## 6.0.0 Released 2018-03-19 Backward incompatible changes: From 44424583f0c222a876dde7cf00070939bcad7a6e Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 5 Apr 2018 12:12:58 +0200 Subject: [PATCH 11/17] Revert "Use unique cartodb_id in aggregated results" This reverts commit c1da1a8a169d574e248809d095e67cb5955d7573. This is reverted for moving the change out of PR #913 into its own PR for clarity. --- lib/cartodb/models/aggregation/aggregation-query.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index cfb9855a..02211a7d 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -290,7 +290,7 @@ const aggregationQueryTemplates = { !bbox! AS bbox ) SELECT - MIN(_cdb_query.cartodb_id) AS cartodb_id, + row_number() over() AS cartodb_id, ST_SetSRID( ST_MakePoint( AVG(ST_X(_cdb_query.the_geom_webmercator)), @@ -317,7 +317,6 @@ const aggregationQueryTemplates = { ), _cdb_clusters AS ( SELECT - MIN(_cdb_query.cartodb_id) AS cartodb_id, Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy ${dimensionDefs(ctx)} @@ -328,7 +327,7 @@ const aggregationQueryTemplates = { ${havingClause(ctx)} ) SELECT - _cdb_clusters.cartodb_id AS cartodb_id, + row_number() over() AS cartodb_id, ST_SetSRID(ST_MakePoint((_cdb_gx+0.5)*res, (_cdb_gy+0.5)*res), 3857) AS the_geom_webmercator ${dimensionNames(ctx)} ${aggregateColumnNames(ctx)} From f7fad736c355d89ffa299cc3f596c1718abb140e Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 5 Apr 2018 16:01:29 +0200 Subject: [PATCH 12/17] Add test for uniqueness of aggregated cartodb_id --- test/acceptance/aggregation.js | 59 ++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index db03ad8e..e17db2a5 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -1979,6 +1979,65 @@ describe('aggregation', function () { }); }); + + ['default', 'centroid', 'point-sample', 'point-grid'].forEach(placement => { + it(`aggregated ids are unique for ${placement} aggregation`, function (done) { + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 0 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_1, + resolution: 1, + aggregation: { + threshold: 1 + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getTile(1, 0, 1, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + + const tile1 = JSON.parse(mvt.toGeoJSONSync(0)); + + assert.ok(Array.isArray(tile1.features)); + assert.ok(tile1.features.length > 0); + + this.testClient.getTile(1, 1, 0, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + + const tile2 = JSON.parse(mvt.toGeoJSONSync(0)); + + assert.ok(Array.isArray(tile2.features)); + assert.ok(tile2.features.length > 0); + + const tile1Ids = tile1.features.map(f => f.properties.cartodb_id); + const tile2Ids = tile2.features.map(f => f.properties.cartodb_id); + const repeatedIds = tile1Ids.filter(id => tile2Ids.includes(id)); + assert.equal(repeatedIds.length, 0); + + done(); + }); + + }); + }); + }); + + }); }); }); From ffa3a96f1a56d6cacd584bbf1bdd0b89581523cb Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 4 Apr 2018 11:05:03 +0200 Subject: [PATCH 13/17] Use unique cartodb_id in aggregated results See #889 FOr centroid and point-grid the cartodb_id wasn't unique across tiles. --- lib/cartodb/models/aggregation/aggregation-query.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 02211a7d..cfb9855a 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -290,7 +290,7 @@ const aggregationQueryTemplates = { !bbox! AS bbox ) SELECT - row_number() over() AS cartodb_id, + MIN(_cdb_query.cartodb_id) AS cartodb_id, ST_SetSRID( ST_MakePoint( AVG(ST_X(_cdb_query.the_geom_webmercator)), @@ -317,6 +317,7 @@ const aggregationQueryTemplates = { ), _cdb_clusters AS ( SELECT + MIN(_cdb_query.cartodb_id) AS cartodb_id, Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy ${dimensionDefs(ctx)} @@ -327,7 +328,7 @@ const aggregationQueryTemplates = { ${havingClause(ctx)} ) SELECT - row_number() over() AS cartodb_id, + _cdb_clusters.cartodb_id AS cartodb_id, ST_SetSRID(ST_MakePoint((_cdb_gx+0.5)*res, (_cdb_gy+0.5)*res), 3857) AS the_geom_webmercator ${dimensionNames(ctx)} ${aggregateColumnNames(ctx)} From 26c5ff1f9362e0bf19370452ff6898272f39eafd Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 5 Apr 2018 16:36:07 +0200 Subject: [PATCH 14/17] Update news --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 75756417..20b8f06a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -9,6 +9,7 @@ New features: Bug Fixes: - Non-default aggregation selected the wrong columns (e.g. for vector tiles) - Aggregation dimensions with alias where broken +- cartodb_id was not unique accross aggregated vector tiles ## 6.0.0 Released 2018-03-19 From 233f9698f3e51f665eefd755e53f24b0b8ccec9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 6 Apr 2018 12:59:53 +0200 Subject: [PATCH 15/17] fix affectedtables cache --- .../provider/create-layergroup-provider.js | 25 +++++++++++++----- .../mapconfig/provider/map-store-provider.js | 26 ++++++++++++++----- .../mapconfig/provider/named-map-provider.js | 25 +++++++++++++----- 3 files changed, 57 insertions(+), 19 deletions(-) diff --git a/lib/cartodb/models/mapconfig/provider/create-layergroup-provider.js b/lib/cartodb/models/mapconfig/provider/create-layergroup-provider.js index b3b3f191..1f19d395 100644 --- a/lib/cartodb/models/mapconfig/provider/create-layergroup-provider.js +++ b/lib/cartodb/models/mapconfig/provider/create-layergroup-provider.js @@ -58,7 +58,7 @@ CreateLayergroupMapConfigProvider.prototype.filter = MapStoreMapConfigProvider.p CreateLayergroupMapConfigProvider.prototype.createKey = MapStoreMapConfigProvider.prototype.createKey; -CreateLayergroupMapConfigProvider.prototype.getAffectedTables = function (callback) { +CreateLayergroupMapConfigProvider.prototype.createAffectedTables = function (callback) { this.getMapConfig((err, mapConfig) => { if (err) { return callback(err); @@ -67,11 +67,6 @@ CreateLayergroupMapConfigProvider.prototype.getAffectedTables = function (callba const { dbname } = this.params; const token = mapConfig.id(); - if (this.affectedTablesCache.hasAffectedTables(dbname, token)) { - const affectedTables = this.affectedTablesCache.get(dbname, token); - return callback(null, affectedTables); - } - const queries = []; this.mapConfig.getLayers().forEach(layer => { @@ -106,3 +101,21 @@ CreateLayergroupMapConfigProvider.prototype.getAffectedTables = function (callba }); }); }; + +CreateLayergroupMapConfigProvider.prototype.getAffectedTables = function (callback) { + this.getMapConfig((err, mapConfig) => { + if (err) { + return callback(err); + } + + const { dbname } = this.params; + const token = mapConfig.id(); + + if (this.affectedTablesCache.hasAffectedTables(dbname, token)) { + const affectedTables = this.affectedTablesCache.get(dbname, token); + return callback(null, affectedTables); + } + + return this.createAffectedTables(callback); + }); +}; diff --git a/lib/cartodb/models/mapconfig/provider/map-store-provider.js b/lib/cartodb/models/mapconfig/provider/map-store-provider.js index f89a79ce..d9f9da83 100644 --- a/lib/cartodb/models/mapconfig/provider/map-store-provider.js +++ b/lib/cartodb/models/mapconfig/provider/map-store-provider.js @@ -89,7 +89,7 @@ MapStoreMapConfigProvider.prototype.createKey = function(base) { return (base) ? baseKeyTpl(tplValues) : rendererKeyTpl(tplValues); }; -MapStoreMapConfigProvider.prototype.getAffectedTables = function(callback) { +MapStoreMapConfigProvider.prototype.createAffectedTables = function(callback) { this.getMapConfig((err, mapConfig) => { if (err) { return callback(err); @@ -98,12 +98,6 @@ MapStoreMapConfigProvider.prototype.getAffectedTables = function(callback) { const { dbname } = this.params; const token = mapConfig.id(); - if (this.affectedTablesCache.hasAffectedTables(dbname, token)) { - const affectedTables = this.affectedTablesCache.get(dbname, token); - - return callback(null, affectedTables); - } - const queries = []; mapConfig.getLayers().forEach(layer => { @@ -138,3 +132,21 @@ MapStoreMapConfigProvider.prototype.getAffectedTables = function(callback) { }); }); }; + +MapStoreMapConfigProvider.prototype.getAffectedTables = function (callback) { + this.getMapConfig((err, mapConfig) => { + if (err) { + return callback(err); + } + + const { dbname } = this.params; + const token = mapConfig.id(); + + if (this.affectedTablesCache.hasAffectedTables(dbname, token)) { + const affectedTables = this.affectedTablesCache.get(dbname, token); + return callback(null, affectedTables); + } + + return this.createAffectedTables(callback); + }); +}; diff --git a/lib/cartodb/models/mapconfig/provider/named-map-provider.js b/lib/cartodb/models/mapconfig/provider/named-map-provider.js index 50612065..065d20f4 100644 --- a/lib/cartodb/models/mapconfig/provider/named-map-provider.js +++ b/lib/cartodb/models/mapconfig/provider/named-map-provider.js @@ -262,7 +262,7 @@ NamedMapMapConfigProvider.prototype.getTemplateName = function() { return this.templateName; }; -NamedMapMapConfigProvider.prototype.getAffectedTables = function(callback) { +NamedMapMapConfigProvider.prototype.createAffectedTables = function(callback) { this.getMapConfig((err, mapConfig) => { if (err) { return callback(err); @@ -271,11 +271,6 @@ NamedMapMapConfigProvider.prototype.getAffectedTables = function(callback) { const { dbname } = this.rendererParams; const token = mapConfig.id(); - if (this.affectedTablesCache.hasAffectedTables(dbname, token)) { - const affectedTables = this.affectedTablesCache.get(dbname, token); - return callback(null, affectedTables); - } - const queries = []; mapConfig.getLayers().forEach(layer => { @@ -310,3 +305,21 @@ NamedMapMapConfigProvider.prototype.getAffectedTables = function(callback) { }); }); }; + +NamedMapMapConfigProvider.prototype.getAffectedTables = function (callback) { + this.getMapConfig((err, mapConfig) => { + if (err) { + return callback(err); + } + + const { dbname } = this.params; + const token = mapConfig.id(); + + if (this.affectedTablesCache.hasAffectedTables(dbname, token)) { + const affectedTables = this.affectedTablesCache.get(dbname, token); + return callback(null, affectedTables); + } + + return this.createAffectedTables(callback); + }); +}; From c94d78203748cd552b08b2908a543fb582ba150c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 6 Apr 2018 13:00:12 +0200 Subject: [PATCH 16/17] calling to new createAffectedTables --- lib/cartodb/controllers/map.js | 2 +- lib/cartodb/controllers/named_maps.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 9fd67710..004dc0c1 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -422,7 +422,7 @@ function setLastUpdatedTimeToLayergroup () { const { mapConfigProvider, analysesResults } = res.locals; const layergroup = res.body; - mapConfigProvider.getAffectedTables((err, affectedTables) => { + mapConfigProvider.createAffectedTables((err, affectedTables) => { if (err) { return next(err); } diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index 2cb4f609..93e56bbe 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -248,7 +248,7 @@ function getStaticImageOptions ({ tablesExtentApi }) { res.locals.imageOpts = DEFAULT_ZOOM_CENTER; - mapConfigProvider.getAffectedTables((err, affectedTables) => { + mapConfigProvider.createAffectedTables((err, affectedTables) => { if (err) { return next(); } From 25aa967146663679053afd35fb7ba8dee4703d52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 6 Apr 2018 15:26:11 +0200 Subject: [PATCH 17/17] Forbid access to named map admin resources for everyone but master --- lib/cartodb/api/auth_api.js | 2 +- lib/cartodb/controllers/named_maps_admin.js | 11 +++++- test/acceptance/auth/authorization.js | 44 ++++++++++++++++++--- test/support/test-client.js | 30 ++++++++++++++ 4 files changed, 79 insertions(+), 8 deletions(-) diff --git a/lib/cartodb/api/auth_api.js b/lib/cartodb/api/auth_api.js index d27ea3e1..ff42c9df 100644 --- a/lib/cartodb/api/auth_api.js +++ b/lib/cartodb/api/auth_api.js @@ -109,7 +109,7 @@ AuthApi.prototype.authorizedByAPIKey = function(user, res, callback) { return callback(error); } - return callback(null, true); + return callback(null, true, apikey); }); }; diff --git a/lib/cartodb/controllers/named_maps_admin.js b/lib/cartodb/controllers/named_maps_admin.js index afcaa1e3..df3ac766 100644 --- a/lib/cartodb/controllers/named_maps_admin.js +++ b/lib/cartodb/controllers/named_maps_admin.js @@ -102,7 +102,7 @@ function authorizedByAPIKey ({ authApi, action, label }) { return function authorizedByAPIKeyMiddleware (req, res, next) { const { user } = res.locals; - authApi.authorizedByAPIKey(user, res, (err, authenticated) => { + authApi.authorizedByAPIKey(user, res, (err, authenticated, apikey) => { if (err) { return next(err); } @@ -114,6 +114,15 @@ function authorizedByAPIKey ({ authApi, action, label }) { return next(error); } + if (apikey.type !== 'master') { + const error = new Error('Forbidden'); + error.type = 'auth'; + error.subtype = 'api-key-does-not-grant-access'; + error.http_status = 403; + + return next(error); + } + next(); }); }; diff --git a/test/acceptance/auth/authorization.js b/test/acceptance/auth/authorization.js index d7df8b32..02668251 100644 --- a/test/acceptance/auth/authorization.js +++ b/test/acceptance/auth/authorization.js @@ -37,7 +37,7 @@ describe('authorization', function() { }); }); - it('should create and get a named map tile using a regular apikey token', function (done) { + it.skip('should create and get a named map tile using a regular apikey token', function (done) { const apikeyToken = 'regular1'; const mapConfig = { version: '1.7.0', @@ -61,7 +61,7 @@ describe('authorization', function() { testClient.drain(done); }); - }); + }); it('should fail getting a named map tile with default apikey token', function (done) { const apikeyTokenCreate = 'regular1'; @@ -203,7 +203,7 @@ describe('authorization', function() { assert.ok(layergroupResult.hasOwnProperty('errors')); assert.equal(layergroupResult.errors.length, 1); assert.ok(layergroupResult.errors[0].match(/permission denied/), layergroupResult.errors[0]); - + testClient.drain(done); }); }); @@ -257,7 +257,7 @@ describe('authorization', function() { type: 'source', params: { query: 'select * from populated_places_simple_reduced' - } + } } ] }; @@ -354,7 +354,39 @@ describe('authorization', function() { }); }); - it('should create and get a named map tile using a regular apikey token', function (done) { + it('should fail while listing named maps with a regular apikey token', function (done) { + const apikeyToken = 'regular1'; + + const testClient = new TestClient({}, apikeyToken); + + testClient.getNamedMapList({ response: {status: 403 }}, function (err, res, body) { + assert.ifError(err); + + assert.equal(res.statusCode, 403); + + assert.equal(body.errors.length, 1); + assert.ok(body.errors[0].match(/Forbidden/), body.errors[0]); + + testClient.drain(done); + }); + }); + + it('should list named maps with master apikey token', function (done) { + const apikeyToken = 1234; + + const testClient = new TestClient({}, apikeyToken); + + testClient.getNamedMapList({}, function (err, res, body) { + assert.ifError(err); + + assert.equal(res.statusCode, 200); + assert.ok(Array.isArray(body.template_ids)); + + testClient.drain(done); + }); + }); + + it.skip('should create and get a named map tile using a regular apikey token', function (done) { const apikeyToken = 'regular1'; const template = { @@ -391,7 +423,7 @@ describe('authorization', function() { }); }); - it('should fail creating a named map using a regular apikey token and a private table', function (done) { + it.skip('should fail creating a named map using a regular apikey token and a private table', function (done) { const apikeyToken = 'regular1'; const template = { diff --git a/test/support/test-client.js b/test/support/test-client.js index 3e919175..7b068901 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -1254,6 +1254,36 @@ TestClient.prototype.getAnalysesCatalog = function (params, callback) { ); }; +TestClient.prototype.getNamedMapList = function(params, callback) { + const request = { + url: `/api/v1/map/named?${qs.stringify({ api_key: this.apiKey })}`, + method: 'GET', + headers: { + host: 'localhost', + 'Content-Type': 'application/json' + } + }; + + let expectedResponse = { + status: 200, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + if (params.response) { + expectedResponse = Object.assign(expectedResponse, params.response); + } + + assert.response(this.server, request, expectedResponse, (res, err) => { + if (err) { + return callback(err); + } + const body = JSON.parse(res.body); + return callback(null, res, body); + }); +}; + TestClient.prototype.getNamedTile = function (name, z, x, y, format, options, callback) { const { params } = options;