From ee25585f06b23d3a288d0d46cd14b9454599626f Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Fri, 29 Jun 2018 15:01:12 +0200 Subject: [PATCH 01/14] add new error message --- lib/cartodb/api/middlewares/error-middleware.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/api/middlewares/error-middleware.js b/lib/cartodb/api/middlewares/error-middleware.js index c02974b4..74a7c701 100644 --- a/lib/cartodb/api/middlewares/error-middleware.js +++ b/lib/cartodb/api/middlewares/error-middleware.js @@ -56,16 +56,18 @@ function populateTimeoutErrors (errors) { error.subtype = 'render'; } - if (isDatasourceTimeoutError(error)) { - error.subtype = 'datasource'; - } - if (isTimeoutError(error)) { error.message = 'You are over platform\'s limits. Please contact us to know more details'; error.type = 'limit'; error.http_status = 429; } + if (isDatasourceTimeoutError(error)) { + error.subtype = 'datasource'; + error.message = 'You are over platform\'s limits: SQL query timeout error.' + + ' Refactor your query before running again or contact CARTO support for more details.'; + } + return error; }); } From 23e5cefdf16b0fb259b73f66c5d76bfa10ca28ee Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Mon, 2 Jul 2018 11:29:47 +0200 Subject: [PATCH 02/14] Update NEWS --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index 311a4842..468bd876 100644 --- a/NEWS.md +++ b/NEWS.md @@ -47,6 +47,9 @@ Bug Fixes: - Fix `meta.stats.estimatedFeatureCount` for aggregations and queries with tokens - Static maps filters correctly if `layer` option is passed in the url. +Announcements: + * Improve error message when the DB query is over the user's limits + ## 6.1.0 Released 2018-04-16 From c874a734fd889aebda3c124bd4aa2a9dcce07b49 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Mon, 2 Jul 2018 13:02:36 +0200 Subject: [PATCH 03/14] fix db limit error message set order --- lib/cartodb/api/middlewares/error-middleware.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/api/middlewares/error-middleware.js b/lib/cartodb/api/middlewares/error-middleware.js index 74a7c701..1c022dde 100644 --- a/lib/cartodb/api/middlewares/error-middleware.js +++ b/lib/cartodb/api/middlewares/error-middleware.js @@ -56,13 +56,15 @@ function populateTimeoutErrors (errors) { error.subtype = 'render'; } + const IS_DATASOURCE_TIMEOUT_ERROR = isDatasourceTimeoutError(error); + if (isTimeoutError(error)) { error.message = 'You are over platform\'s limits. Please contact us to know more details'; error.type = 'limit'; error.http_status = 429; } - if (isDatasourceTimeoutError(error)) { + if (IS_DATASOURCE_TIMEOUT_ERROR) { error.subtype = 'datasource'; error.message = 'You are over platform\'s limits: SQL query timeout error.' + ' Refactor your query before running again or contact CARTO support for more details.'; From a8d791e2d3e01bd9eafdcb882c14f1a8591ccb9c Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Mon, 2 Jul 2018 13:03:01 +0200 Subject: [PATCH 04/14] update tests with new error message --- .../acceptance/user-database-timeout-limit.js | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/test/acceptance/user-database-timeout-limit.js b/test/acceptance/user-database-timeout-limit.js index d65f6583..1111adce 100644 --- a/test/acceptance/user-database-timeout-limit.js +++ b/test/acceptance/user-database-timeout-limit.js @@ -68,16 +68,19 @@ const createMapConfig = ({ } }); +const db_limit_error_message = 'You are over platform\'s limits: SQL query timeout error.' + + ' Refactor your query before running again or contact CARTO support for more details.'; + const DATASOURCE_TIMEOUT_ERROR = { - errors: ['You are over platform\'s limits. Please contact us to know more details'], + errors: [db_limit_error_message], errors_with_context: [{ type: 'limit', subtype: 'datasource', - message: 'You are over platform\'s limits. Please contact us to know more details' + message: db_limit_error_message }] }; -describe('user database timeout limit', function () { +describe.only('user database timeout limit', function () { describe('dataview', function () { beforeEach(function (done) { const mapconfig = createMapConfig(); @@ -141,11 +144,11 @@ describe('user database timeout limit', function () { this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { - errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], + errors: [db_limit_error_message ], errors_with_context: [{ type: 'limit', subtype: 'datasource', - message: 'You are over platform\'s limits. Please contact us to know more details', + message: db_limit_error_message, layer: { id: 'layer0', index: 0, type: 'mapnik' } }] }); @@ -363,11 +366,11 @@ describe('user database timeout limit', function () { this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { - errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], + errors: [ db_limit_error_message ], errors_with_context: [{ type: 'limit', subtype: 'datasource', - message: 'You are over platform\'s limits. Please contact us to know more details', + message: db_limit_error_message, layer: { id: 'layer0', index: 0, type: 'mapnik' } }] }); @@ -541,11 +544,11 @@ describe('user database timeout limit', function () { this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { - errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], + errors: [ db_limit_error_message ], errors_with_context: [{ type: 'limit', subtype: 'datasource', - message: 'You are over platform\'s limits. Please contact us to know more details', + message: db_limit_error_message, layer: { id: 'layer0', index: 0, type: 'mapnik' } }] }); @@ -645,11 +648,11 @@ describe('user database timeout limit', function () { this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { - errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], + errors: [db_limit_error_message], errors_with_context: [{ type: 'limit', subtype: 'datasource', - message: 'You are over platform\'s limits. Please contact us to know more details', + message: db_limit_error_message, layer: { id: 'torque-layer0', index: 0, type: 'torque' } }] }); @@ -776,11 +779,11 @@ describe('user database timeout limit', function () { this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { - errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], + errors: [db_limit_error_message], errors_with_context: [{ type: 'limit', subtype: 'datasource', - message: 'You are over platform\'s limits. Please contact us to know more details', + message: db_limit_error_message, layer: { id: 'layer0', index: 0, From 6a6815d89392dec1095d0b27af1bccd1fbb9a6f7 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 3 Jul 2018 14:32:10 +0200 Subject: [PATCH 05/14] update render limits error message --- .../api/middlewares/error-middleware.js | 27 ++++++++++++------- .../acceptance/user-database-timeout-limit.js | 2 +- test/acceptance/user-render-timeout-limit.js | 19 +++++++------ 3 files changed, 30 insertions(+), 18 deletions(-) diff --git a/lib/cartodb/api/middlewares/error-middleware.js b/lib/cartodb/api/middlewares/error-middleware.js index 1c022dde..c1533b43 100644 --- a/lib/cartodb/api/middlewares/error-middleware.js +++ b/lib/cartodb/api/middlewares/error-middleware.js @@ -46,30 +46,39 @@ function isDatasourceTimeoutError (err) { return err.message && err.message.match(/canceling statement due to statement timeout/i); } -function isTimeoutError (err) { - return isRenderTimeoutError(err) || isDatasourceTimeoutError(err); +function isTimeoutError (errorTypes) { + return errorTypes.renderTimeoutError || errorTypes.datasourceTimeoutError; +} + +function getErrorTypes(error) { + return { + renderTimeoutError: isRenderTimeoutError(error), + datasourceTimeoutError: isDatasourceTimeoutError(error), + }; } function populateTimeoutErrors (errors) { return errors.map(function (error) { - if (isRenderTimeoutError(error)) { - error.subtype = 'render'; - } + const errorTypes = getErrorTypes(error); - const IS_DATASOURCE_TIMEOUT_ERROR = isDatasourceTimeoutError(error); - - if (isTimeoutError(error)) { + if (isTimeoutError(errorTypes)) { error.message = 'You are over platform\'s limits. Please contact us to know more details'; error.type = 'limit'; error.http_status = 429; } - if (IS_DATASOURCE_TIMEOUT_ERROR) { + if (errorTypes.datasourceTimeoutError) { error.subtype = 'datasource'; error.message = 'You are over platform\'s limits: SQL query timeout error.' + ' Refactor your query before running again or contact CARTO support for more details.'; } + if (errorTypes.renderTimeoutError) { + error.subtype = 'render'; + error.message = 'You are over platform\'s limits: Render timeout error.' + + ' Contact CARTO support for more details.'; + } + return error; }); } diff --git a/test/acceptance/user-database-timeout-limit.js b/test/acceptance/user-database-timeout-limit.js index 1111adce..611daa6d 100644 --- a/test/acceptance/user-database-timeout-limit.js +++ b/test/acceptance/user-database-timeout-limit.js @@ -80,7 +80,7 @@ const DATASOURCE_TIMEOUT_ERROR = { }] }; -describe.only('user database timeout limit', function () { +describe('user database timeout limit', function () { describe('dataview', function () { beforeEach(function (done) { const mapconfig = createMapConfig(); diff --git a/test/acceptance/user-render-timeout-limit.js b/test/acceptance/user-render-timeout-limit.js index 79822c7b..1a5b7c98 100644 --- a/test/acceptance/user-render-timeout-limit.js +++ b/test/acceptance/user-render-timeout-limit.js @@ -23,6 +23,9 @@ const validationPointSleepSql = ` 2 val `; +const renderTimeoutErrorMessage = 'You are over platform\'s limits: Render timeout error.' + + ' Contact CARTO support for more details.'; + const createMapConfig = ({ version = '1.6.0', type = 'cartodb', @@ -96,11 +99,11 @@ describe('user render timeout limit', function () { assert.ifError(err); assert.deepEqual(timeoutError, { - errors: ["You are over platform\'s limits. Please contact us to know more details"], + errors: [renderTimeoutErrorMessage], errors_with_context: [{ type: 'limit', subtype: 'render', - message: "You are over platform\'s limits. Please contact us to know more details", + message: renderTimeoutErrorMessage, layer: { id: "layer0", index: 0, @@ -188,11 +191,11 @@ describe('user render timeout limit', function () { assert.ifError(err); assert.deepEqual(timeoutError, { - errors: ["You are over platform\'s limits. Please contact us to know more details"], + errors: [renderTimeoutErrorMessage], errors_with_context: [{ type: 'limit', subtype: 'render', - message: "You are over platform\'s limits. Please contact us to know more details" + message: renderTimeoutErrorMessage }] }); @@ -282,11 +285,11 @@ describe('user render timeout limit', function () { assert.ifError(err); assert.deepEqual(tile, { - errors: ['You are over platform\'s limits. Please contact us to know more details'], + errors: [renderTimeoutErrorMessage], errors_with_context: [{ type: 'limit', subtype: 'render', - message: 'You are over platform\'s limits. Please contact us to know more details' + message: renderTimeoutErrorMessage }] }); @@ -384,11 +387,11 @@ describe('user render timeout limit', function () { assert.ifError(err); assert.deepEqual(timeoutError, { - errors: ["You are over platform\'s limits. Please contact us to know more details"], + errors: [renderTimeoutErrorMessage], errors_with_context: [{ type: 'limit', subtype: 'render', - message: "You are over platform\'s limits. Please contact us to know more details" + message: renderTimeoutErrorMessage }] }); From 409a103990dd6ab84ec34dbdd2a151485f16f0fc Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 3 Jul 2018 14:36:29 +0200 Subject: [PATCH 06/14] update rate limit error message --- lib/cartodb/api/middlewares/rate-limit.js | 2 +- test/acceptance/rate-limit.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/api/middlewares/rate-limit.js b/lib/cartodb/api/middlewares/rate-limit.js index 55062d76..a1f54e6e 100644 --- a/lib/cartodb/api/middlewares/rate-limit.js +++ b/lib/cartodb/api/middlewares/rate-limit.js @@ -47,7 +47,7 @@ function rateLimit(userLimitsBackend, endpointGroup = null) { res.set('Retry-After', retry + 1); let rateLimitError = new Error( - 'You are over platform\'s limits. Please contact us to know more details' + 'You are over platform\'s limits: too many requests. Please contact us to know more details' ); rateLimitError.http_status = 429; rateLimitError.type = 'limit'; diff --git a/test/acceptance/rate-limit.test.js b/test/acceptance/rate-limit.test.js index 925d5028..46c854be 100644 --- a/test/acceptance/rate-limit.test.js +++ b/test/acceptance/rate-limit.test.js @@ -144,7 +144,7 @@ function assertRateLimitRequest (status, limit, remaining, reset, retry, done) { assert.ifError(err); } else { assert.ok(err); - assert.equal(err.message, 'You are over platform\'s limits. Please contact us to know more details'); + assert.equal(err.message, 'You are over platform\'s limits: too many requests. Please contact us to know more details'); assert.equal(err.http_status, 429); assert.equal(err.type, 'limit'); assert.equal(err.subtype, 'rate-limit'); From 923b23871f653eb0485e3869376260f88a3e4060 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 3 Jul 2018 14:48:54 +0200 Subject: [PATCH 07/14] Please jshint --- lib/cartodb/api/middlewares/rate-limit.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/api/middlewares/rate-limit.js b/lib/cartodb/api/middlewares/rate-limit.js index a1f54e6e..14452254 100644 --- a/lib/cartodb/api/middlewares/rate-limit.js +++ b/lib/cartodb/api/middlewares/rate-limit.js @@ -47,7 +47,8 @@ function rateLimit(userLimitsBackend, endpointGroup = null) { res.set('Retry-After', retry + 1); let rateLimitError = new Error( - 'You are over platform\'s limits: too many requests. Please contact us to know more details' + 'You are over platform\'s limits: too many requests.' + + ' Please contact us to know more details' ); rateLimitError.http_status = 429; rateLimitError.type = 'limit'; From fdc061b7eede8e108c034eb2f6be073133607be4 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 3 Jul 2018 15:02:58 +0200 Subject: [PATCH 08/14] Please jshint --- test/acceptance/rate-limit.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/acceptance/rate-limit.test.js b/test/acceptance/rate-limit.test.js index 46c854be..38c94c5d 100644 --- a/test/acceptance/rate-limit.test.js +++ b/test/acceptance/rate-limit.test.js @@ -144,7 +144,8 @@ function assertRateLimitRequest (status, limit, remaining, reset, retry, done) { assert.ifError(err); } else { assert.ok(err); - assert.equal(err.message, 'You are over platform\'s limits: too many requests. Please contact us to know more details'); + assert.equal(err.message, 'You are over platform\'s limits: too many requests.' + + ' Please contact us to know more details'); assert.equal(err.http_status, 429); assert.equal(err.type, 'limit'); assert.equal(err.subtype, 'rate-limit'); From 2c334570c3b89848c1ad8745804ad51d0c1b023e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Wed, 4 Jul 2018 17:39:13 +0200 Subject: [PATCH 09/14] run_tests.sh is a *bash* script --- run_tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/run_tests.sh b/run_tests.sh index b409db83..2b6a9523 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash OPT_CREATE_REDIS=yes # create the redis test environment OPT_CREATE_PGSQL=yes # create the PostgreSQL test environment From 6411556a9702a27eec550a47781d3eaea9a597d1 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Wed, 4 Jul 2018 18:33:02 +0200 Subject: [PATCH 10/14] Test for histogram bins beyond limits --- test/acceptance/dataviews/histogram.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/acceptance/dataviews/histogram.js b/test/acceptance/dataviews/histogram.js index 161fdef8..db558087 100644 --- a/test/acceptance/dataviews/histogram.js +++ b/test/acceptance/dataviews/histogram.js @@ -67,6 +67,27 @@ describe('histogram-dataview', function() { ] ); + it('should get bins with min >= start and max <= end', function(done) { + var params = { + bins: 3, + start: 50, + end: 500 + }; + + this.testClient = new TestClient(mapConfig, 1234); + this.testClient.getDataview('pop_max_histogram', params, function(err, dataview) { + assert.ok(!err, err); + + assert.ok(3 === dataview.bins_count, 'Unexpected bin count: ' + dataview.bins_count); + assert.ok(3 === dataview.bins.length, 'Unexpected number of bins: ' + dataview.bins.length); + dataview.bins.forEach(function(bin) { + assert.ok(bin.min >= params.start, 'bin min < start: ' + JSON.stringify(bin)); + assert.ok(bin.max <= params.end, 'bin max > end: ' + JSON.stringify(bin)); + }); + done(); + }); + }); + it('should get bin_width right when max > min in filter', function(done) { var params = { bins: 10, From d937ce498293798ef26f2f62aab261583e41c571 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 5 Jul 2018 11:56:26 +0200 Subject: [PATCH 11/14] Fix the min >= start and max <= end case (WIP) This fixes the 'should get bins with min >= start and max <= end' test case but probably breaks a number of other cases (those with no start and/or no end). --- lib/cartodb/models/dataview/histograms/numeric-histogram.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/dataview/histograms/numeric-histogram.js b/lib/cartodb/models/dataview/histograms/numeric-histogram.js index d3eedad9..11dbdf2b 100644 --- a/lib/cartodb/models/dataview/histograms/numeric-histogram.js +++ b/lib/cartodb/models/dataview/histograms/numeric-histogram.js @@ -135,7 +135,7 @@ SELECT END AS bin FROM ( - ${ctx.query} + SELECT * FROM (${ctx.query}) __ctx_query WHERE ${ctx.column} >= ${ctx.start} AND ${ctx.column} <= ${ctx.end} ) __cdb_filtered_source_query${extra_tables} GROUP BY 10${extra_groupby} ORDER BY 10;`; From a1807fd0c32ba44a69a4fcc75fe4f25f45da4d36 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 5 Jul 2018 12:39:26 +0200 Subject: [PATCH 12/14] A better solution to the start-end problem --- lib/cartodb/models/dataview/histograms/numeric-histogram.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/models/dataview/histograms/numeric-histogram.js b/lib/cartodb/models/dataview/histograms/numeric-histogram.js index 11dbdf2b..2538fa54 100644 --- a/lib/cartodb/models/dataview/histograms/numeric-histogram.js +++ b/lib/cartodb/models/dataview/histograms/numeric-histogram.js @@ -99,6 +99,7 @@ module.exports = class NumericHistogram extends BaseHistogram { var extra_tables = ``; var extra_queries = ``; var extra_groupby = ``; + var extra_filter = ``; if (ctx.start >= ctx.end) { ctx.end = `__cdb_basics.__cdb_max_val`; @@ -106,6 +107,8 @@ module.exports = class NumericHistogram extends BaseHistogram { extra_groupby = `, __cdb_basics.__cdb_max_val, __cdb_basics.__cdb_min_val`; extra_tables = `, __cdb_basics`; extra_queries = `WITH ${irqQueryTpl(ctx)}`; + } else { + extra_filter = `WHERE ${ctx.column} >= ${ctx.start} AND ${ctx.column} <= ${ctx.end}`; } if (ctx.bins <= 0) { @@ -135,7 +138,7 @@ SELECT END AS bin FROM ( - SELECT * FROM (${ctx.query}) __ctx_query WHERE ${ctx.column} >= ${ctx.start} AND ${ctx.column} <= ${ctx.end} + SELECT * FROM (${ctx.query}) __ctx_query${extra_tables} ${extra_filter} ) __cdb_filtered_source_query${extra_tables} GROUP BY 10${extra_groupby} ORDER BY 10;`; From e247e45f9644272c134e64562de1f958a8237601 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 5 Jul 2018 17:21:35 +0200 Subject: [PATCH 13/14] Qualify columns and improve if/else style As suggested by Algunenano: qualify column names with the table/subquery/cte to avoid name clashing, and polish the code style a little. --- .../models/dataview/histograms/numeric-histogram.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/dataview/histograms/numeric-histogram.js b/lib/cartodb/models/dataview/histograms/numeric-histogram.js index 2538fa54..f26a0f80 100644 --- a/lib/cartodb/models/dataview/histograms/numeric-histogram.js +++ b/lib/cartodb/models/dataview/histograms/numeric-histogram.js @@ -101,14 +101,17 @@ module.exports = class NumericHistogram extends BaseHistogram { var extra_groupby = ``; var extra_filter = ``; - if (ctx.start >= ctx.end) { + if (ctx.start < ctx.end) { + extra_filter = ` + WHERE __ctx_query.${ctx.column} >= ${ctx.start} + AND __ctx_query.${ctx.column} <= ${ctx.end} + `; + } else { ctx.end = `__cdb_basics.__cdb_max_val`; ctx.start = `__cdb_basics.__cdb_min_val`; extra_groupby = `, __cdb_basics.__cdb_max_val, __cdb_basics.__cdb_min_val`; extra_tables = `, __cdb_basics`; extra_queries = `WITH ${irqQueryTpl(ctx)}`; - } else { - extra_filter = `WHERE ${ctx.column} >= ${ctx.start} AND ${ctx.column} <= ${ctx.end}`; } if (ctx.bins <= 0) { From b59712ee10035b30922a16c17e2e61ba17539517 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 5 Jul 2018 17:24:29 +0200 Subject: [PATCH 14/14] Update NEWS.md with reference to fix --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 468bd876..edcf57c5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -45,6 +45,7 @@ Bug Fixes: - Static maps fails for unsupported formats - Handling errors extracting the column type on dataviews - Fix `meta.stats.estimatedFeatureCount` for aggregations and queries with tokens +- Fix numeric histogram bounds when `start` and `end` are specified (#991) - Static maps filters correctly if `layer` option is passed in the url. Announcements: