diff --git a/NEWS.md b/NEWS.md index 311a4842..edcf57c5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -45,8 +45,12 @@ 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: + * Improve error message when the DB query is over the user's limits + ## 6.1.0 Released 2018-04-16 diff --git a/lib/cartodb/api/middlewares/error-middleware.js b/lib/cartodb/api/middlewares/error-middleware.js index c02974b4..c1533b43 100644 --- a/lib/cartodb/api/middlewares/error-middleware.js +++ b/lib/cartodb/api/middlewares/error-middleware.js @@ -46,26 +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); - if (isDatasourceTimeoutError(error)) { - error.subtype = 'datasource'; - } - - 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 (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/lib/cartodb/api/middlewares/rate-limit.js b/lib/cartodb/api/middlewares/rate-limit.js index 55062d76..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. 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/lib/cartodb/models/dataview/histograms/numeric-histogram.js b/lib/cartodb/models/dataview/histograms/numeric-histogram.js index d3eedad9..f26a0f80 100644 --- a/lib/cartodb/models/dataview/histograms/numeric-histogram.js +++ b/lib/cartodb/models/dataview/histograms/numeric-histogram.js @@ -99,8 +99,14 @@ module.exports = class NumericHistogram extends BaseHistogram { var extra_tables = ``; var extra_queries = ``; 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`; @@ -135,7 +141,7 @@ SELECT END AS bin FROM ( - ${ctx.query} + SELECT * FROM (${ctx.query}) __ctx_query${extra_tables} ${extra_filter} ) __cdb_filtered_source_query${extra_tables} GROUP BY 10${extra_groupby} ORDER BY 10;`; 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 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, diff --git a/test/acceptance/rate-limit.test.js b/test/acceptance/rate-limit.test.js index 925d5028..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. 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'); diff --git a/test/acceptance/user-database-timeout-limit.js b/test/acceptance/user-database-timeout-limit.js index d65f6583..611daa6d 100644 --- a/test/acceptance/user-database-timeout-limit.js +++ b/test/acceptance/user-database-timeout-limit.js @@ -68,12 +68,15 @@ 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 }] }; @@ -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, 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 }] });