diff --git a/NEWS.md b/NEWS.md index 0e0c2b9c..f0f42253 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,10 @@ 1.22.1 - 2015-mm-dd ------------------- +Bug fixes: + + * Close stream responses on error (#219) + Enhancements: * Format files split into pg and ogr directories diff --git a/app/models/formats/pg/geojson.js b/app/models/formats/pg/geojson.js index cdaf9c29..5dda9d36 100644 --- a/app/models/formats/pg/geojson.js +++ b/app/models/formats/pg/geojson.js @@ -1,6 +1,7 @@ var _ = require('underscore'); var pg = require('./../pg'); +var PgErrorHandler = require('../../../postgresql/error_handler'); function GeoJsonFormat() { this.buffer = ''; @@ -54,7 +55,7 @@ GeoJsonFormat.prototype.handleQueryRow = function(row) { }; GeoJsonFormat.prototype.handleQueryEnd = function(/*result*/) { - if (this.error) { + if (this.error && !this._streamingStarted) { this.callback(this.error); return; } @@ -67,7 +68,15 @@ GeoJsonFormat.prototype.handleQueryEnd = function(/*result*/) { this.startStreaming(); } - this.buffer += ']}'; // end of features + this.buffer += ']'; // end of features + + if (this.error) { + var pgErrorHandler = new PgErrorHandler(this.error); + this.buffer += ',"error":' + JSON.stringify([pgErrorHandler.getMessage()]); + } + + this.buffer += '}'; // end of root object + if (this.opts.callback) { this.buffer += ')'; } diff --git a/app/models/formats/pg/svg.js b/app/models/formats/pg/svg.js index 3bedb7c6..489a8a45 100644 --- a/app/models/formats/pg/svg.js +++ b/app/models/formats/pg/svg.js @@ -139,7 +139,7 @@ SvgFormat.prototype.handleQueryRow = function(row) { }; SvgFormat.prototype.handleQueryEnd = function() { - if ( this.error ) { + if ( this.error && !this._streamingStarted) { this.callback(this.error); return; } diff --git a/test/acceptance/app.test.js b/test/acceptance/app.test.js index d8c9d01b..78644c90 100644 --- a/test/acceptance/app.test.js +++ b/test/acceptance/app.test.js @@ -1458,29 +1458,6 @@ it('GET with callback must return 200 status error even if it is an error', func }); }); - it('stream response is closed on error and error message is part of the response', function(done){ - assert.response( - app, - { - url: "/api/v1/sql?" + querystring.stringify({ - q: "SELECT 100/(cartodb_id - 3) cdb_ratio FROM untitle_table_4" - }), - headers: {host: 'vizzuality.cartodb.com'}, - method: 'GET' - }, - { - status: 200 - }, - function(res) { - var parsedBody = JSON.parse(res.body); - assert.equal(parsedBody.rows.length, 2); - assert.deepEqual(parsedBody.fields, {"cdb_ratio": {"type": "number"}}); - assert.deepEqual(parsedBody.error, ["division by zero"]); - done(); - } - ); - }); - it('too large rows get into error log', function(done){ var dbMaxRowSize = global.settings.db_max_row_size; diff --git a/test/acceptance/export/svg.js b/test/acceptance/export/svg.js index 8cce3460..4937232f 100644 --- a/test/acceptance/export/svg.js +++ b/test/acceptance/export/svg.js @@ -170,6 +170,31 @@ it('SVG format with missing "the_geom" field', function(done){ }); }); + it('should close on error and error must be the only key in the body', function(done) { + assert.response( + app, + { + url: "/api/v1/sql?" + querystring.stringify({ + q: "SELECT the_geom, 100/(cartodb_id - 3) cdb_ratio FROM untitle_table_4", + format: 'svg' + }), + headers: { + host: 'vizzuality.cartodb.com' + }, + method: 'GET' + }, + { + status: 400 + }, + function(res) { + var parsedBody = JSON.parse(res.body); + assert.deepEqual(Object.keys(parsedBody), ['error']); + assert.deepEqual(parsedBody.error, ["division by zero"]); + done(); + } + ); + }); + }); diff --git a/test/acceptance/export/topojson.js b/test/acceptance/export/topojson.js index e996b051..29acb880 100644 --- a/test/acceptance/export/topojson.js +++ b/test/acceptance/export/topojson.js @@ -239,4 +239,31 @@ it('null geometries', function(done){ ); }); + + it('should close on error and error must be the only key in the body', function(done) { + assert.response( + app, + { + url: "/api/v1/sql?" + querystring.stringify({ + q: "SELECT the_geom, 100/(cartodb_id - 3) cdb_ratio FROM untitle_table_4", + format: 'topojson' + }), + headers: { + host: 'vizzuality.cartodb.com' + }, + method: 'GET' + }, + { + status: 400 + }, + function(res) { + var parsedBody = JSON.parse(res.body); + assert.deepEqual(Object.keys(parsedBody), ['error']); + assert.deepEqual(parsedBody.error, ["division by zero"]); + done(); + } + ); + }); + + }); diff --git a/test/acceptance/stream-responses.js b/test/acceptance/stream-responses.js new file mode 100644 index 00000000..a2c03c80 --- /dev/null +++ b/test/acceptance/stream-responses.js @@ -0,0 +1,71 @@ +require('../helper'); + +var app = require(global.settings.app_root + '/app/controllers/app')(); +var assert = require('../support/assert'); +var querystring = require('querystring'); + +describe('stream-responses', function() { + + function createFailingQueryRequest(format) { + var params = { + q: "SELECT the_geom, 100/(cartodb_id - 3) cdb_ratio FROM untitle_table_4" + }; + + if (format) { + params.format = format; + } + + return { + url: "/api/v1/sql?" + querystring.stringify(params), + headers: { + host: 'vizzuality.cartodb.com' + }, + method: 'GET' + }; + } + + var okResponse = { + status: 200 + }; + + describe('format-json', function() { + + it('should close on error and error message must be part of the response', function(done) { + assert.response( + app, + createFailingQueryRequest(), + okResponse, + function(res) { + var parsedBody = JSON.parse(res.body); + assert.equal(parsedBody.rows.length, 2); + assert.deepEqual(parsedBody.fields, { + the_geom: { type: "geometry" }, + cdb_ratio: { type: "number" } + }); + assert.deepEqual(parsedBody.error, ["division by zero"]); + done(); + } + ); + }); + + }); + + describe('format-geojson', function() { + + it('should close on error and error message must be part of the response', function(done) { + assert.response( + app, + createFailingQueryRequest('geojson'), + okResponse, + function(res) { + var parsedBody = JSON.parse(res.body); + assert.equal(parsedBody.features.length, 2); + assert.deepEqual(parsedBody.error, ["division by zero"]); + done(); + } + ); + }); + + }); + +});