From f7a763b6378c4dc9ce1d334f11582f84ec20e1e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Matall=C3=ADn?= Date: Fri, 7 Jul 2017 17:09:17 +0200 Subject: [PATCH 01/12] widgets: add aggregation for search results --- lib/cartodb/models/dataview/aggregation.js | 3 ++- package.json | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index c15f0506..2fb76600 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -253,6 +253,7 @@ Aggregation.prototype.search = function(psql, userQuery, callback) { var self = this; var _userQuery = psql.escapeLiteral('%' + userQuery + '%'); + var _value = this.aggregation !== 'count' && this.aggregationColumn ? this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; // TODO unfiltered will be wrong as filters are already applied at this point var query = searchQueryTpl({ @@ -265,7 +266,7 @@ Aggregation.prototype.search = function(psql, userQuery, callback) { _searchFiltered: filterCategoriesQueryTpl({ _query: this.query, _column: this.column, - _value: 'count(1)', + _value: _value, _userQuery: _userQuery }) }); diff --git a/package.json b/package.json index c32459f0..f613df30 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,8 @@ "contributors": [ "Simon Tokumine ", "Javi Santana ", - "Sandro Santilli " + "Sandro Santilli ", + "Carlos Matallín " ], "dependencies": { "body-parser": "~1.14.0", From 7fa154c06286d8ae9372f18119a7da6d2fa3b503 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Matall=C3=ADn?= Date: Fri, 7 Jul 2017 17:38:15 +0200 Subject: [PATCH 02/12] widgets: add aggregation for search results, specs --- test/acceptance/widgets/ported/aggregation.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/acceptance/widgets/ported/aggregation.js b/test/acceptance/widgets/ported/aggregation.js index 9329f039..62c998a5 100644 --- a/test/acceptance/widgets/ported/aggregation.js +++ b/test/acceptance/widgets/ported/aggregation.js @@ -322,6 +322,25 @@ describe('widgets', function() { }); }); }); + + [adm0name].forEach(function(userQuery) { + it('should search with sum aggregation: ' + userQuery, function(done) { + this.testClient = new TestClient(aggregationSumMapConfig); + this.testClient.widgetSearch('adm0name', userQuery, function (err, res, searchResult) { + assert.ok(!err, err); + assert.ok(searchResult); + assert.equal(searchResult.type, 'aggregation'); + + assert.equal(searchResult.categories.length, 1); + assert.deepEqual( + searchResult.categories, + [{ category:"Argentina", value:28015640 }] + ); + + done(); + }); + }); + }); }); }); From dd934a39139ea07d76709000b827b99143c38f52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Matall=C3=ADn?= Date: Fri, 7 Jul 2017 17:44:32 +0200 Subject: [PATCH 03/12] linter --- Makefile | 2 +- lib/cartodb/models/dataview/aggregation.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 1913b9e8..a2910a0f 100644 --- a/Makefile +++ b/Makefile @@ -21,7 +21,7 @@ config/environments/test.js: config.status--test TEST_SUITE := $(shell find test/{acceptance,integration,unit} -name "*.js") TEST_SUITE_UNIT := $(shell find test/unit -name "*.js") TEST_SUITE_INTEGRATION := $(shell find test/integration -name "*.js") -TEST_SUITE_ACCEPTANCE := $(shell find test/acceptance -name "*.js") +TEST_SUITE_ACCEPTANCE := $(shell find test/acceptance/widgets/ported -name "*.js") test: config/environments/test.js @echo "***tests***" diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 2fb76600..d7710f4f 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -253,7 +253,8 @@ Aggregation.prototype.search = function(psql, userQuery, callback) { var self = this; var _userQuery = psql.escapeLiteral('%' + userQuery + '%'); - var _value = this.aggregation !== 'count' && this.aggregationColumn ? this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; + var _value = this.aggregation !== 'count' && this.aggregationColumn ? + this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; // TODO unfiltered will be wrong as filters are already applied at this point var query = searchQueryTpl({ From b1c9dd537e0f07486f3d6a6c8d51fca493cc3a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Matall=C3=ADn?= Date: Fri, 7 Jul 2017 17:47:28 +0200 Subject: [PATCH 04/12] Update Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index a2910a0f..1913b9e8 100644 --- a/Makefile +++ b/Makefile @@ -21,7 +21,7 @@ config/environments/test.js: config.status--test TEST_SUITE := $(shell find test/{acceptance,integration,unit} -name "*.js") TEST_SUITE_UNIT := $(shell find test/unit -name "*.js") TEST_SUITE_INTEGRATION := $(shell find test/integration -name "*.js") -TEST_SUITE_ACCEPTANCE := $(shell find test/acceptance/widgets/ported -name "*.js") +TEST_SUITE_ACCEPTANCE := $(shell find test/acceptance -name "*.js") test: config/environments/test.js @echo "***tests***" From e36266a80f7d2736bd94e71f334aeba1c90a777b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 10 Jul 2017 12:29:25 +0200 Subject: [PATCH 05/12] Added test to check all aggregation operations work as expected when searching dataviews --- test/acceptance/dataviews/aggregation.js | 29 ++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/acceptance/dataviews/aggregation.js b/test/acceptance/dataviews/aggregation.js index 259cd2af..c4b40d44 100644 --- a/test/acceptance/dataviews/aggregation.js +++ b/test/acceptance/dataviews/aggregation.js @@ -145,4 +145,33 @@ describe('aggregations happy cases', function() { }); }); }); + + var widgetSearchExpects = { + 'count': [ { category: 'other_a', value: 3 } ], + 'sum': [ { category: 'other_a', value: 6 } ], + 'avg': [ { category: 'other_a', value: 2 } ], + 'max': [ { category: 'other_a', value: 3 } ], + 'min': [ { category: 'other_a', value: 1 } ] + }; + + Object.keys(operations_and_values).forEach(function (operation) { + var description = 'should search OTHER category using "' + operation + '"'; + + it(description, function (done) { + this.testClient = new TestClient(aggregationOperationMapConfig(operation, query_other, 'cat', 'val')); + this.testClient.widgetSearch('cat', 'other_a', function (err, res, searchResult) { + assert.ifError(err); + + assert.ok(searchResult); + assert.equal(searchResult.type, 'aggregation'); + + assert.equal(searchResult.categories.length, 1); + assert.deepEqual( + searchResult.categories, + widgetSearchExpects[operation] + ); + done(); + }); + }); + }); }); From 5b8ecd3df0a2bc46bf8b5619f3552cd8e9361693 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 11 Jul 2017 09:53:12 +0200 Subject: [PATCH 06/12] Release 3.9.6 --- NEWS.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 454b9142..b59924bc 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,7 +1,9 @@ # Changelog ## 3.9.6 -Released 2017-mm-dd +Released 2017-07-11 + + - Dataviews: support for aggregation in search results #708 ## 3.9.5 From a35b1e3e8656e9b354e6f092c1099f8e674fe0ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 11 Jul 2017 09:59:30 +0200 Subject: [PATCH 07/12] Stubs next version --- NEWS.md | 4 ++++ package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index b59924bc..dd6b97f4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,9 @@ # Changelog +## 3.9.7 +Released 2017-mm-dd + + ## 3.9.6 Released 2017-07-11 diff --git a/package.json b/package.json index 4ed05477..5d6a40df 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "private": true, "name": "windshaft-cartodb", - "version": "3.9.6", + "version": "3.9.7", "description": "A map tile server for CartoDB", "keywords": [ "cartodb" From eccc3597aa68113cf9d69c24caac7f63af8a8f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 17 Jul 2017 19:43:59 +0200 Subject: [PATCH 08/12] Respond with 204 when vector tile is empty --- lib/cartodb/controllers/base.js | 4 +++ test/acceptance/mvt.js | 58 +++++++++++++++++++++++++++++++++ test/support/test-client.js | 16 ++++++--- 3 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 test/acceptance/mvt.js diff --git a/lib/cartodb/controllers/base.js b/lib/cartodb/controllers/base.js index ab6587c2..7cdd0713 100644 --- a/lib/cartodb/controllers/base.js +++ b/lib/cartodb/controllers/base.js @@ -201,6 +201,10 @@ BaseController.prototype.sendError = function(req, res, err, label) { var statusCode = findStatusCode(err); + if (err.message === 'Tile does not exist' && req.params.format === 'mvt') { + statusCode = 204; + } + debug('[%s ERROR] -- %d: %s, %s', label, statusCode, err, err.stack); // If a callback was requested, force status to 200 diff --git a/test/acceptance/mvt.js b/test/acceptance/mvt.js new file mode 100644 index 00000000..17653d1d --- /dev/null +++ b/test/acceptance/mvt.js @@ -0,0 +1,58 @@ +require('../support/test_helper'); + +var assert = require('../support/assert'); +var TestClient = require('../support/test-client'); + +function createMapConfig (sql) { + sql = sql || [ + 'select', + ' *', + 'from', + ' populated_places_simple_reduced', + ].join('\n'); + + return { + version: '1.6.0', + layers: [{ + type: "cartodb", + options: { + sql: sql, + cartocss: TestClient.CARTOCSS.POINTS, + cartocss_version: '2.3.0', + interactivity: 'cartodb_id' + } + }] + }; +} + + +describe('mvt', function () { + const testCases = [ + { + desc: 'should get empty mvt with code 204 (no content)', + coords: { z: 0, x: 0, y: 0 }, + format: 'mvt', + mapConfig: createMapConfig('select 1 as cartodb_id, null::geometry as the_geom_webmercator') + } + ]; + + testCases.forEach(function (test) { + it(test.desc, done => { + const testClient = new TestClient(test.mapConfig, 1234); + const { z, x, y } = test.coords; + const options = { + format: test.format, + status: 204 + }; + + testClient.getTile(z, x, y, options, (err, res) => { + assert.ifError(err); + + assert.ifError(err); + assert.equal(res.statusCode, 204); + assert.equal(res.body, ''); + testClient.drain(done); + }); + }); + }); +}); \ No newline at end of file diff --git a/test/support/test-client.js b/test/support/test-client.js index 659d6686..72a6eca9 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -525,7 +525,7 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { }; var expectedResponse = { - status: 200, + status: params.status || 200, headers: { 'Content-Type': 'application/json; charset=utf-8' } @@ -542,7 +542,12 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { if (isMvt) { request.encoding = 'binary'; - expectedResponse.headers['Content-Type'] = 'application/x-protobuf'; + + if (expectedResponse.status === 200) { + expectedResponse.headers['Content-Type'] = 'application/x-protobuf'; + } else if (expectedResponse.status === 204) { + expectedResponse.headers['Content-Type'] = undefined; + } } var isGeojson = format.match(/geojson$/); @@ -561,15 +566,16 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { assert.response(server, request, expectedResponse, function(res, err) { assert.ifError(err); - var obj; if (isPng) { obj = mapnik.Image.fromBytes(new Buffer(res.body, 'binary')); } else if (isMvt) { - obj = new mapnik.VectorTile(z, x, y); - obj.setDataSync(new Buffer(res.body, 'binary')); + if (res.body) { + obj = new mapnik.VectorTile(z, x, y); + obj.setDataSync(new Buffer(res.body, 'binary')); + } } else { obj = JSON.parse(res.body); From ff1399625568432c2da6eb4e5a54ca42ddb53006 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 18 Jul 2017 10:44:27 +0200 Subject: [PATCH 09/12] Add test to check that mvt returns 200 when tile has data --- test/acceptance/mvt.js | 34 +++++++++++++++------------------- test/support/test-client.js | 5 +++++ 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/test/acceptance/mvt.js b/test/acceptance/mvt.js index 17653d1d..db75bb16 100644 --- a/test/acceptance/mvt.js +++ b/test/acceptance/mvt.js @@ -1,16 +1,9 @@ require('../support/test_helper'); -var assert = require('../support/assert'); -var TestClient = require('../support/test-client'); - -function createMapConfig (sql) { - sql = sql || [ - 'select', - ' *', - 'from', - ' populated_places_simple_reduced', - ].join('\n'); +const assert = require('../support/assert'); +const TestClient = require('../support/test-client'); +function createMapConfig (sql = TestClient.SQL.ONE_POINT) { return { version: '1.6.0', layers: [{ @@ -25,14 +18,21 @@ function createMapConfig (sql) { }; } - describe('mvt', function () { const testCases = [ { desc: 'should get empty mvt with code 204 (no content)', coords: { z: 0, x: 0, y: 0 }, format: 'mvt', - mapConfig: createMapConfig('select 1 as cartodb_id, null::geometry as the_geom_webmercator') + status: 204, + mapConfig: createMapConfig(TestClient.SQL.EMPTY) + }, + { + desc: 'should get mvt tile with code 200 (ok)', + coords: { z: 0, x: 0, y: 0 }, + format: 'mvt', + status: 200, + mapConfig: createMapConfig() } ]; @@ -40,17 +40,13 @@ describe('mvt', function () { it(test.desc, done => { const testClient = new TestClient(test.mapConfig, 1234); const { z, x, y } = test.coords; - const options = { - format: test.format, - status: 204 - }; + const { format, status } = test; - testClient.getTile(z, x, y, options, (err, res) => { + testClient.getTile(z, x, y, { format, status }, (err, res) => { assert.ifError(err); assert.ifError(err); - assert.equal(res.statusCode, 204); - assert.equal(res.body, ''); + assert.equal(res.statusCode, test.status); testClient.drain(done); }); }); diff --git a/test/support/test-client.js b/test/support/test-client.js index 72a6eca9..5d0eddff 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -75,6 +75,11 @@ module.exports.CARTOCSS = { ].join('\n') }; +module.exports.SQL = { + EMPTY: 'select 1 as cartodb_id, null::geometry as the_geom_webmercator', + ONE_POINT: 'select 1 as cartodb_id, \'SRID=3857;POINT(0 0)\'::geometry the_geom_webmercator' +} + TestClient.prototype.getWidget = function(widgetName, params, callback) { var self = this; From 0aab434f13fb019e9c23059309fa9941afa6c39f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 18 Jul 2017 10:52:24 +0200 Subject: [PATCH 10/12] Remove duplicated assertion --- test/acceptance/mvt.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/acceptance/mvt.js b/test/acceptance/mvt.js index db75bb16..6510541b 100644 --- a/test/acceptance/mvt.js +++ b/test/acceptance/mvt.js @@ -45,7 +45,6 @@ describe('mvt', function () { testClient.getTile(z, x, y, { format, status }, (err, res) => { assert.ifError(err); - assert.ifError(err); assert.equal(res.statusCode, test.status); testClient.drain(done); }); From 446e2d08020fa51c1077a137b3c3bd6e9d94bf9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 18 Jul 2017 11:05:45 +0200 Subject: [PATCH 11/12] Add empty line at the end of file --- test/acceptance/mvt.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/mvt.js b/test/acceptance/mvt.js index 6510541b..b1ea4455 100644 --- a/test/acceptance/mvt.js +++ b/test/acceptance/mvt.js @@ -50,4 +50,4 @@ describe('mvt', function () { }); }); }); -}); \ No newline at end of file +}); From f306c26da61196b7903f94c4ef93a04a6a7fb69e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 18 Jul 2017 11:08:39 +0200 Subject: [PATCH 12/12] Update News --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index dd6b97f4..fbce5c7d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -3,6 +3,8 @@ ## 3.9.7 Released 2017-mm-dd + - Respond with 204 (No content) when vector tile has no data #712 + ## 3.9.6 Released 2017-07-11