From 1d199f8713c8d34ed574d272ed65213c0222277f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 30 Jul 2018 15:19:53 +0200 Subject: [PATCH 01/12] Remove step in method --- lib/cartodb/backends/dataview.js | 79 ++++++++++++++++---------------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 4eebb2a2..4a79612f 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -21,51 +21,52 @@ function DataviewBackend(analysisBackend) { module.exports = DataviewBackend; DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, params, callback) { + const dataviewName = params.dataviewName; - var dataviewName = params.dataviewName; - step( - function getMapConfig() { - mapConfigProvider.getMapConfig(this); - }, - function runDataviewQuery(err, mapConfig) { - assert.ifError(err); + mapConfigProvider.getMapConfig(function (err, mapConfig) { + if (err) { + return callback(err); + } - var dataviewDefinition = getDataviewDefinition(mapConfig.obj(), dataviewName); - if (!dataviewDefinition) { - throw new Error("Dataview '" + dataviewName + "' does not exists"); - } + var dataviewDefinition = getDataviewDefinition(mapConfig.obj(), dataviewName); + if (!dataviewDefinition) { + throw new Error("Dataview '" + dataviewName + "' does not exists"); + } - var ownFilter = +params.own_filter; - var noFilters = +params.no_filters; - if (Number.isFinite(ownFilter) && Number.isFinite(noFilters)) { - err = new Error(); - err.message = 'Both own_filter and no_filters cannot be sent in the same request'; - err.type = 'dataview'; - err.http_status = 400; + var ownFilter = +params.own_filter; + var noFilters = +params.no_filters; + if (Number.isFinite(ownFilter) && Number.isFinite(noFilters)) { + err = new Error(); + err.message = 'Both own_filter and no_filters cannot be sent in the same request'; + err.type = 'dataview'; + err.http_status = 400; + return callback(err); + } + + var pg = new PSQL(dbParamsFromReqParams(params)); + + var query = getDataviewQuery(dataviewDefinition, ownFilter, noFilters); + if (params.bbox) { + var bboxFilter = new BBoxFilter({column: 'the_geom_webmercator', srid: 3857}, {bbox: params.bbox}); + query = bboxFilter.sql(query); + } + + var queryRewriteData = getQueryRewriteData(mapConfig, dataviewDefinition, params); + + var dataviewFactory = DataviewFactoryWithOverviews.getFactory( + overviewsQueryRewriter, queryRewriteData, { bbox: params.bbox } + ); + + var dataview = dataviewFactory.getDataview(query, dataviewDefinition); + + dataview.getResult(pg, getOverrideParams(params, !!ownFilter), function (err, dataview) { + if (err) { return callback(err); } - var pg = new PSQL(dbParamsFromReqParams(params)); - - var query = getDataviewQuery(dataviewDefinition, ownFilter, noFilters); - if (params.bbox) { - var bboxFilter = new BBoxFilter({column: 'the_geom_webmercator', srid: 3857}, {bbox: params.bbox}); - query = bboxFilter.sql(query); - } - - var queryRewriteData = getQueryRewriteData(mapConfig, dataviewDefinition, params); - - var dataviewFactory = DataviewFactoryWithOverviews.getFactory( - overviewsQueryRewriter, queryRewriteData, { bbox: params.bbox } - ); - - var dataview = dataviewFactory.getDataview(query, dataviewDefinition); - dataview.getResult(pg, getOverrideParams(params, !!ownFilter), this); - }, - function returnCallback(err, result) { - return callback(err, result); - } - ); + return callback(null, dataview); + }); + }); }; function getDataviewQuery(dataviewDefinition, ownFilter, noFilters) { From 94b7353fbf20553633a19b1ab2597d3c77a8f14d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 30 Jul 2018 15:52:04 +0200 Subject: [PATCH 02/12] Fix uncaught exception --- lib/cartodb/backends/dataview.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 4a79612f..c62d6bb7 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -59,7 +59,15 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param var dataview = dataviewFactory.getDataview(query, dataviewDefinition); - dataview.getResult(pg, getOverrideParams(params, !!ownFilter), function (err, dataview) { + let overrideParams; + + try { + overrideParams = getOverrideParams(params, !!ownFilter); + } catch (error) { + return callback(error); + } + + dataview.getResult(pg, overrideParams, function (err, dataview) { if (err) { return callback(err); } From 6be1a77a29e01f4e17496166d6098e6b2ce175e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 30 Jul 2018 15:57:47 +0200 Subject: [PATCH 03/12] Use callback to return the error --- lib/cartodb/backends/dataview.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index c62d6bb7..b4eee7fa 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -30,7 +30,7 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param var dataviewDefinition = getDataviewDefinition(mapConfig.obj(), dataviewName); if (!dataviewDefinition) { - throw new Error("Dataview '" + dataviewName + "' does not exists"); + return callback(new Error("Dataview '" + dataviewName + "' does not exists")); } var ownFilter = +params.own_filter; From cca0848e6d0ef457a58aa25f0beef9699fb82fa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 30 Jul 2018 15:59:43 +0200 Subject: [PATCH 04/12] Improve error --- lib/cartodb/backends/dataview.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index b4eee7fa..1be165e8 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -30,7 +30,10 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param var dataviewDefinition = getDataviewDefinition(mapConfig.obj(), dataviewName); if (!dataviewDefinition) { - return callback(new Error("Dataview '" + dataviewName + "' does not exists")); + const error = new Error(`Dataview '${dataviewName}' does not exists`); + error.type = 'dataview'; + error.http_status = 400; + return callback(error); } var ownFilter = +params.own_filter; From 13075460acbb362bce53e8f8fba86144d7e9f545 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 30 Jul 2018 16:00:30 +0200 Subject: [PATCH 05/12] Do not override incoming arguments --- lib/cartodb/backends/dataview.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 1be165e8..25684711 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -39,10 +39,9 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param var ownFilter = +params.own_filter; var noFilters = +params.no_filters; if (Number.isFinite(ownFilter) && Number.isFinite(noFilters)) { - err = new Error(); - err.message = 'Both own_filter and no_filters cannot be sent in the same request'; - err.type = 'dataview'; - err.http_status = 400; + const error = new Error('Both own_filter and no_filters cannot be sent in the same request'); + error.type = 'dataview'; + error.http_status = 400; return callback(err); } From 230b1bb3db020642d41c2b403bc6ab494504c345 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 31 Jul 2018 11:59:39 +0200 Subject: [PATCH 06/12] Remove step .searchDataview() --- lib/cartodb/backends/dataview.js | 79 ++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 25684711..1abf488c 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -1,7 +1,5 @@ -var assert = require('assert'); var _ = require('underscore'); var PSQL = require('cartodb-psql'); -var step = require('step'); var BBoxFilter = require('../models/filter/bbox'); var DataviewFactory = require('../models/dataview/factory'); var DataviewFactoryWithOverviews = require('../models/dataview/overviews/factory'); @@ -140,39 +138,52 @@ function getOverrideParams(params, ownFilter) { } DataviewBackend.prototype.search = function (mapConfigProvider, user, dataviewName, params, callback) { - step( - function getMapConfig() { - mapConfigProvider.getMapConfig(this); - }, - function runDataviewSearchQuery(err, mapConfig) { - assert.ifError(err); - - var dataviewDefinition = getDataviewDefinition(mapConfig.obj(), dataviewName); - if (!dataviewDefinition) { - throw new Error("Dataview '" + dataviewName + "' does not exists"); - } - - var pg = new PSQL(dbParamsFromReqParams(params)); - - var ownFilter = +params.own_filter; - ownFilter = !!ownFilter; - - var query = (ownFilter) ? dataviewDefinition.sql.own_filter_on : dataviewDefinition.sql.own_filter_off; - - if (params.bbox) { - var bboxFilter = new BBoxFilter({column: 'the_geom', srid: 4326}, {bbox: params.bbox}); - query = bboxFilter.sql(query); - } - - var userQuery = params.q; - - var dataview = DataviewFactory.getDataview(query, dataviewDefinition); - dataview.search(pg, userQuery, this); - }, - function returnCallback(err, result) { - return callback(err, result); + mapConfigProvider.getMapConfig(function (err, mapConfig) { + if (err) { + return callback(err); } - ); + + var dataviewDefinition = getDataviewDefinition(mapConfig.obj(), dataviewName); + if (!dataviewDefinition) { + const error = new Error(`Dataview '${dataviewName}' does not exists`); + error.type = 'dataview'; + error.http_status = 400; + return callback(error); + } + + var pg; + + try { + pg = new PSQL(dbParamsFromReqParams(params)); + } catch (error) { + return callback(error); + } + + var ownFilter = +params.own_filter; + ownFilter = !!ownFilter; + + var query = (ownFilter) ? dataviewDefinition.sql.own_filter_on : dataviewDefinition.sql.own_filter_off; + + if (params.bbox) { + try { + var bboxFilter = new BBoxFilter({ column: 'the_geom', srid: 4326 }, { bbox: params.bbox }); + query = bboxFilter.sql(query); + } catch (error) { + return callback(error); + } + } + + var userQuery = params.q; + + var dataview = DataviewFactory.getDataview(query, dataviewDefinition); + dataview.search(pg, userQuery, function (err, result) { + if (err) { + return callback(err); + } + + return callback(null, result); + }); + }); }; function getDataviewDefinition(mapConfig, dataviewName) { From 70ac0587dba800b219920a26a9bd83f4d1950ee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 31 Jul 2018 12:15:20 +0200 Subject: [PATCH 07/12] Missing error --- lib/cartodb/backends/dataview.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 1abf488c..fe549092 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -40,7 +40,7 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param const error = new Error('Both own_filter and no_filters cannot be sent in the same request'); error.type = 'dataview'; error.http_status = 400; - return callback(err); + return callback(error); } var pg = new PSQL(dbParamsFromReqParams(params)); From 18603ad24f5c0bd636fa5f0eb350f526fde8f018 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 31 Jul 2018 12:43:54 +0200 Subject: [PATCH 08/12] Reduce cyclomatic complexity --- lib/cartodb/backends/dataview.js | 34 +++++++++++++++++--------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index fe549092..1b13a930 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -152,30 +152,18 @@ DataviewBackend.prototype.search = function (mapConfigProvider, user, dataviewNa } var pg; + var query; + var dataview; try { pg = new PSQL(dbParamsFromReqParams(params)); + query = getQueryWithFilters(dataviewDefinition, params); + dataview = DataviewFactory.getDataview(query, dataviewDefinition); } catch (error) { return callback(error); } - var ownFilter = +params.own_filter; - ownFilter = !!ownFilter; - - var query = (ownFilter) ? dataviewDefinition.sql.own_filter_on : dataviewDefinition.sql.own_filter_off; - - if (params.bbox) { - try { - var bboxFilter = new BBoxFilter({ column: 'the_geom', srid: 4326 }, { bbox: params.bbox }); - query = bboxFilter.sql(query); - } catch (error) { - return callback(error); - } - } - var userQuery = params.q; - - var dataview = DataviewFactory.getDataview(query, dataviewDefinition); dataview.search(pg, userQuery, function (err, result) { if (err) { return callback(err); @@ -186,6 +174,20 @@ DataviewBackend.prototype.search = function (mapConfigProvider, user, dataviewNa }); }; +function getQueryWithFilters (dataviewDefinition, params) { + var ownFilter = +params.own_filter; + ownFilter = !!ownFilter; + + var query = (ownFilter) ? dataviewDefinition.sql.own_filter_on : dataviewDefinition.sql.own_filter_off; + + if (params.bbox) { + var bboxFilter = new BBoxFilter({ column: 'the_geom', srid: 4326 }, { bbox: params.bbox }); + query = bboxFilter.sql(query); + } + + return query; +} + function getDataviewDefinition(mapConfig, dataviewName) { var dataviews = mapConfig.dataviews || {}; return dataviews[dataviewName]; From 9124a26a45d7553c5fcde54197b5ee8db8205c52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 31 Jul 2018 12:47:03 +0200 Subject: [PATCH 09/12] Move veriable declaration --- lib/cartodb/backends/dataview.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 1b13a930..87605ce6 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -154,6 +154,7 @@ DataviewBackend.prototype.search = function (mapConfigProvider, user, dataviewNa var pg; var query; var dataview; + var userQuery = params.q; try { pg = new PSQL(dbParamsFromReqParams(params)); @@ -163,7 +164,6 @@ DataviewBackend.prototype.search = function (mapConfigProvider, user, dataviewNa return callback(error); } - var userQuery = params.q; dataview.search(pg, userQuery, function (err, result) { if (err) { return callback(err); From d4de54f2921f5cc1b474fd58a64878ad8cb943e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 31 Jul 2018 13:26:38 +0200 Subject: [PATCH 10/12] Extract get query with filters --- lib/cartodb/backends/dataview.js | 38 +++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 87605ce6..1c7a02f5 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -34,9 +34,7 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param return callback(error); } - var ownFilter = +params.own_filter; - var noFilters = +params.no_filters; - if (Number.isFinite(ownFilter) && Number.isFinite(noFilters)) { + if (!validFilterParams(params)) { const error = new Error('Both own_filter and no_filters cannot be sent in the same request'); error.type = 'dataview'; error.http_status = 400; @@ -44,15 +42,8 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param } var pg = new PSQL(dbParamsFromReqParams(params)); - - var query = getDataviewQuery(dataviewDefinition, ownFilter, noFilters); - if (params.bbox) { - var bboxFilter = new BBoxFilter({column: 'the_geom_webmercator', srid: 3857}, {bbox: params.bbox}); - query = bboxFilter.sql(query); - } - + var query = getQueryWithFilters(dataviewDefinition, params); var queryRewriteData = getQueryRewriteData(mapConfig, dataviewDefinition, params); - var dataviewFactory = DataviewFactoryWithOverviews.getFactory( overviewsQueryRewriter, queryRewriteData, { bbox: params.bbox } ); @@ -62,6 +53,7 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param let overrideParams; try { + var ownFilter = +params.own_filter; overrideParams = getOverrideParams(params, !!ownFilter); } catch (error) { return callback(error); @@ -77,6 +69,26 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param }); }; +function validFilterParams (params) { + var ownFilter = +params.own_filter; + var noFilters = +params.no_filters; + + return !(Number.isFinite(ownFilter) && Number.isFinite(noFilters)); +} + +function getQueryWithFilters (dataviewDefinition, params) { + var ownFilter = +params.own_filter; + var noFilters = +params.no_filters; + var query = getDataviewQuery(dataviewDefinition, ownFilter, noFilters); + + if (params.bbox) { + var bboxFilter = new BBoxFilter({column: 'the_geom_webmercator', srid: 3857}, {bbox: params.bbox}); + query = bboxFilter.sql(query); + } + + return query; +} + function getDataviewQuery(dataviewDefinition, ownFilter, noFilters) { if (noFilters) { return dataviewDefinition.sql.no_filters; @@ -158,7 +170,7 @@ DataviewBackend.prototype.search = function (mapConfigProvider, user, dataviewNa try { pg = new PSQL(dbParamsFromReqParams(params)); - query = getQueryWithFilters(dataviewDefinition, params); + query = getQueryWithOwnFilters(dataviewDefinition, params); dataview = DataviewFactory.getDataview(query, dataviewDefinition); } catch (error) { return callback(error); @@ -174,7 +186,7 @@ DataviewBackend.prototype.search = function (mapConfigProvider, user, dataviewNa }); }; -function getQueryWithFilters (dataviewDefinition, params) { +function getQueryWithOwnFilters (dataviewDefinition, params) { var ownFilter = +params.own_filter; ownFilter = !!ownFilter; From ec0e90e8ce1b3d4347cfcd66080a8e3e35bc25a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 31 Jul 2018 13:33:33 +0200 Subject: [PATCH 11/12] Avoid uncaught exceptions --- lib/cartodb/backends/dataview.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 1c7a02f5..bf0b31bf 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -41,18 +41,18 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param return callback(error); } - var pg = new PSQL(dbParamsFromReqParams(params)); - var query = getQueryWithFilters(dataviewDefinition, params); - var queryRewriteData = getQueryRewriteData(mapConfig, dataviewDefinition, params); - var dataviewFactory = DataviewFactoryWithOverviews.getFactory( - overviewsQueryRewriter, queryRewriteData, { bbox: params.bbox } - ); - - var dataview = dataviewFactory.getDataview(query, dataviewDefinition); - - let overrideParams; + var pg; + var overrideParams; + var dataview; try { + pg = new PSQL(dbParamsFromReqParams(params)); + var query = getQueryWithFilters(dataviewDefinition, params); + var queryRewriteData = getQueryRewriteData(mapConfig, dataviewDefinition, params); + var dataviewFactory = DataviewFactoryWithOverviews.getFactory(overviewsQueryRewriter, queryRewriteData, { + bbox: params.bbox + }); + dataview = dataviewFactory.getDataview(query, dataviewDefinition); var ownFilter = +params.own_filter; overrideParams = getOverrideParams(params, !!ownFilter); } catch (error) { From 3a3baf3c853f4229dd5527e0aca93a52b3ff2a23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 31 Jul 2018 15:41:29 +0200 Subject: [PATCH 12/12] Rename variable --- lib/cartodb/backends/dataview.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index bf0b31bf..542bbb4e 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -59,12 +59,12 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param return callback(error); } - dataview.getResult(pg, overrideParams, function (err, dataview) { + dataview.getResult(pg, overrideParams, function (err, dataviewResult) { if (err) { return callback(err); } - return callback(null, dataview); + return callback(null, dataviewResult); }); }); };