From 3e261fb3539c59c70b7fc79b2bdc9e96ed0f0863 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 23 Oct 2018 15:49:34 +0200 Subject: [PATCH 1/6] Going red: fails with 400 due to "Cannot read property 'geom_type' of undefined" --- .../stats/mapnik_stats_layergroup.js | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index dd58fd85..2f4e0289 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -519,6 +519,31 @@ describe('Create mapnik layergroup', function() { }); }); + it(`should not fail "TypeError: ... 'geom_type' of undefined" for empty results`, function(done) { + var testClient = new TestClient({ + version: '1.8.0', + layers: [ + { + type: 'mapnik', + options: { + sql: 'select * from test_table where false', + metadata: { + geometryType: true + } + } + } + ] + }); + + testClient.getLayergroup(function(err, layergroup) { + assert.ifError(err); + assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 0); + assert.equal(layergroup.metadata.layers[0].meta.stats.geometryType, undefined); + testClient.drain(done); + }); + }); + it('should provide a sample as optional metadata', function(done) { var testClient = new TestClient({ version: '1.4.0', From 26e4a052766af50efb57eca941bf8bdb1b44a12f Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 23 Oct 2018 15:50:42 +0200 Subject: [PATCH 2/6] Going green: prevent TypeError for empty tables/results This is the intial step to fix https://github.com/CartoDB/carto-vl/issues/1049. --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 67d00d90..4de960fe 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -71,7 +71,7 @@ function _geometryType(ctx) { const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); const sqlQuery = _getSQL(ctx, sql => queryUtils.getQueryGeometryType(sql, geometryColumn)); return queryUtils.queryPromise(ctx.dbConnection, sqlQuery) - .then(res => ({ geometryType: res.rows[0].geom_type })); + .then(res => ({ geometryType: (res.rows[0] || {}).geom_type })); } return Promise.resolve(); } From 208dbfd951cb8669e84ab656e6e54501569f83f7 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 23 Oct 2018 14:34:17 +0000 Subject: [PATCH 3/6] Output PostgreSQL and PostGIS versions --- .travis.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.travis.yml b/.travis.yml index 1e0952d8..949e9ab6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -60,6 +60,9 @@ jobs: - createuser publicuser - psql -c "CREATE EXTENSION postgis" template_postgis + - psql -c "select version();" template_postgis + - psql -c "select postgis_version();" template_postgis + # install yarn 0.27.5 - curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version 0.27.5 - export PATH="$HOME/.yarn/bin:$PATH" From abd378e5f65986a0c9ef95b4ab50f8219e94c950 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 23 Oct 2018 15:47:48 +0000 Subject: [PATCH 4/6] Run tests based on PostGIS version --- .../stats/mapnik_stats_layergroup.js | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 2f4e0289..368125a6 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -2,14 +2,32 @@ require('../../support/test_helper'); var assert = require('../../support/assert'); var TestClient = require('../../support/test-client'); +const serverOptions = require('../../../lib/cartodb/server_options'); + +const suites = [{ + desc: 'mvt (mapnik)', + usePostGIS: false +}]; + +if (process.env.POSTGIS_VERSION >= '20400') { + suites.push({ + desc: 'mvt (postgis)', + usePostGIS: true + }); +} + +suites.forEach(({desc, usePostGIS}) => { +describe(`[${desc}] Create mapnik layergroup`, function() { + const originalUsePostGIS = serverOptions.renderer.mvt.usePostGIS; -describe('Create mapnik layergroup', function() { before(function() { + serverOptions.renderer.mvt.usePostGIS = usePostGIS; this.layerStatsConfig = global.environment.enabledFeatures.layerStats; global.environment.enabledFeatures.layerStats = true; }); after(function() { + serverOptions.renderer.mvt.usePostGIS = originalUsePostGIS; global.environment.enabledFeatures.layerStats = this.layerStatsConfig; }); @@ -614,3 +632,4 @@ describe('Create mapnik layergroup', function() { }); }); +}); From b13ae62d0f032cbb78ed46becf3db0b8f1a8132e Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Tue, 23 Oct 2018 16:02:37 +0000 Subject: [PATCH 5/6] Do not assert the estimated count as it seems to change over pg versions --- test/acceptance/stats/mapnik_stats_layergroup.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 368125a6..189b42e9 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -556,7 +556,6 @@ describe(`[${desc}] Create mapnik layergroup`, function() { testClient.getLayergroup(function(err, layergroup) { assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); - assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 0); assert.equal(layergroup.metadata.layers[0].meta.stats.geometryType, undefined); testClient.drain(done); }); From 545d387bb41f2d73820c68b0c546b29d14274a55 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Tue, 23 Oct 2018 18:55:31 +0200 Subject: [PATCH 6/6] Fix for non-deterministic test (undefined server) When running tests I got this error: ``` 1) multilayer error cases bogus sql raises 400 status code: TypeError: Cannot read property 'listen' of undefined at Function.assert.response (test/support/assert.js:93:26) at Function.requestLayergroup (test/acceptance/ported/support/test_client.js:79:20) at next (node_modules/step/lib/step.js:51:23) at Step (node_modules/step/lib/step.js:122:3) at Object.createLayergroup (test/acceptance/ported/support/test_client.js:75:5) at Context. (test/acceptance/ported/multilayer_error_cases.js:304:20) ``` The problem is that `server` is declared but its initialization may depend on the order of execution of suites, which is basically that of the filesystem/checkouts. That is fixed by returning `getServer()`, which seems to be the original intent: return a singleton of `CartodbServer` properly initialized in case it is not overriden through options. --- test/acceptance/ported/support/test_client.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/ported/support/test_client.js b/test/acceptance/ported/support/test_client.js index d3e08328..51caab6d 100644 --- a/test/acceptance/ported/support/test_client.js +++ b/test/acceptance/ported/support/test_client.js @@ -129,7 +129,7 @@ function serverInstance(options) { return otherServer; } - return server; + return getServer(); } function layergroupRequest(layergroupConfig, method, callbackName, extraParams) {