From ebae218219b183bed635c67561518309499f4d71 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 5 Jun 2015 13:37:49 -0400 Subject: [PATCH 1/5] Use windshaft branch with unified error reponses format --- npm-shrinkwrap.json | 22 ++++++++++++++++------ package.json | 2 +- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 73339ccd..3e94d8aa 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -314,7 +314,7 @@ "dependencies": { "ansi-regex": { "version": "1.1.1", - "from": "ansi-regex@1.1.1", + "from": "ansi-regex@^1.1.0", "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-1.1.1.tgz" } } @@ -379,7 +379,7 @@ } }, "log4js": { - "version": "0.6.21", + "version": "0.6.25", "from": "https://github.com/CartoDB/log4js-node/tarball/cdb", "resolved": "https://github.com/CartoDB/log4js-node/tarball/cdb", "dependencies": { @@ -413,6 +413,16 @@ "from": "inherits@~2.0.1" } } + }, + "semver": { + "version": "4.3.6", + "from": "semver@~4.3.3", + "resolved": "https://registry.npmjs.org/semver/-/semver-4.3.6.tgz" + }, + "underscore": { + "version": "1.8.2", + "from": "underscore@1.8.2", + "resolved": "https://registry.npmjs.org/underscore/-/underscore-1.8.2.tgz" } } }, @@ -476,9 +486,9 @@ "resolved": "https://registry.npmjs.org/underscore/-/underscore-1.6.0.tgz" }, "windshaft": { - "version": "0.44.1", - "from": "windshaft@0.44.1", - "resolved": "https://registry.npmjs.org/windshaft/-/windshaft-0.44.1.tgz", + "version": "0.45.0", + "from": "https://github.com/CartoDB/Windshaft/tarball/issue-351", + "resolved": "https://github.com/CartoDB/Windshaft/tarball/issue-351", "dependencies": { "chronograph": { "version": "0.1.0", @@ -2329,7 +2339,7 @@ }, "mime": { "version": "1.2.11", - "from": "mime@~1.2.9", + "from": "mime@~1.2.11", "resolved": "https://registry.npmjs.org/mime/-/mime-1.2.11.tgz" } } diff --git a/package.json b/package.json index 16647e98..837300aa 100644 --- a/package.json +++ b/package.json @@ -24,7 +24,7 @@ "dependencies": { "underscore" : "~1.6.0", "dot": "~1.0.2", - "windshaft": "0.44.1", + "windshaft": "https://github.com/CartoDB/Windshaft/tarball/issue-351", "step": "~0.0.5", "queue-async": "~1.0.7", "request": "~2.9.203", From 68c70effec7cfde52e4be9413321bef908fedd6a Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 5 Jun 2015 13:38:38 -0400 Subject: [PATCH 2/5] Named maps returning `errors=>Array` instead of `error=>String` --- lib/cartodb/controllers/named_maps.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index 1ea98843..b0ba0278 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -315,23 +315,22 @@ NamedMapsController.prototype.instantiateTemplate = function(req, res, template_ }, function prepareResponse(err, layergroup) { if ( err ) { - throw err; + return callback(err, { errors: [''+err] }); } var tplhash = self.templateMaps.fingerPrint(template).substring(0,8); layergroup.layergroupid = cdbuser + '@' + tplhash + '@' + layergroup.layergroupid; self.surrogateKeysCache.tag(res, new NamedMapsCacheEntry(cdbuser, template.name)); - return layergroup; - }, - callback + callback(null, layergroup); + } ); }; NamedMapsController.prototype.finish_instantiation = function(err, response, res) { if (err) { var statusCode = 400; - response = { error: ''+err }; + response = { errors: [''+err] }; if ( ! _.isUndefined(err.http_status) ) { statusCode = err.http_status; } @@ -346,7 +345,7 @@ function finishFn(app, res, description, okResponse) { var statusCode = 200; if (err) { statusCode = 400; - response = { error: '' + err }; + response = { errors: ['' + err] }; if ( ! _.isUndefined(err.http_status) ) { statusCode = err.http_status; } From 9bce88f9b156515eac84daf3221c3fb44700a49f Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 5 Jun 2015 13:39:25 -0400 Subject: [PATCH 3/5] Fix tests --- test/acceptance/limits.js | 2 +- test/acceptance/multilayer.js | 2 +- test/acceptance/templates.js | 131 +++++++++++++++++----------------- 3 files changed, 68 insertions(+), 67 deletions(-) diff --git a/test/acceptance/limits.js b/test/acceptance/limits.js index fbe37186..0d393db2 100644 --- a/test/acceptance/limits.js +++ b/test/acceptance/limits.js @@ -174,7 +174,7 @@ describe('render limits', function() { }, function(res) { var parsed = JSON.parse(res.body); - assert.deepEqual(parsed, { error: 'Render timed out' }); + assert.deepEqual(parsed, { errors: ['Render timed out'] }); done(); } ); diff --git a/test/acceptance/multilayer.js b/test/acceptance/multilayer.js index 465f5f79..38256aef 100644 --- a/test/acceptance/multilayer.js +++ b/test/acceptance/multilayer.js @@ -131,7 +131,7 @@ suite(suiteName, function() { }, {}, function(res) { assert.equal(res.statusCode, 403, res.statusCode + ':' + res.body); var parsed = JSON.parse(res.body); - var msg = parsed.error; // TODO: should it be "errors" ? + var msg = parsed.errors[0]; assert.ok(msg.match(/permission denied/i), msg); next(err); }); diff --git a/test/acceptance/templates.js b/test/acceptance/templates.js index 829abd12..dc28681a 100644 --- a/test/acceptance/templates.js +++ b/test/acceptance/templates.js @@ -88,8 +88,8 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 403); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), res.body); - err = parsed.error; + assert.ok(parsed.hasOwnProperty('errors'), res.body); + err = parsed.errors[0]; assert.ok(err.match(/only.*authenticated.*user/i), 'Unexpected error response: ' + err); post_request_1.url += '?api_key=1234'; @@ -113,9 +113,9 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 400, res.body); var parsedBody = JSON.parse(res.body); - assert.ok(parsedBody.hasOwnProperty('error'), res.body); - assert.ok(parsedBody.error.match(/already exists/i), - 'Unexpected error for pre-existing template name: ' + parsedBody.error); + assert.ok(parsedBody.hasOwnProperty('errors'), res.body); + assert.ok(parsedBody.errors[0].match(/already exists/i), + 'Unexpected error for pre-existing template name: ' + parsedBody.errors); return null; }, function finish(err) { @@ -172,10 +172,10 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 400, res.body); var parsedBody = JSON.parse(res.body); - assert.ok(parsedBody.hasOwnProperty('error'), res.body); + assert.ok(parsedBody.hasOwnProperty('errors'), res.body); var re = /invalid.*authentication.*missing/i; - assert.ok(parsedBody.error.match(re), - 'Error for invalid authentication does not match ' + re + ': ' + parsedBody.error); + assert.ok(parsedBody.errors[0].match(re), + 'Error for invalid authentication does not match ' + re + ': ' + parsedBody.errors); return null; }, function postTemplate2(err) @@ -202,10 +202,10 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 400, res.body); var parsedBody = JSON.parse(res.body); - assert.ok(parsedBody.hasOwnProperty('error'), res.body); + assert.ok(parsedBody.hasOwnProperty('errors'), res.body); var re = new RegExp(/invalid.*authentication.*missing/i); - assert.ok(parsedBody.error.match(re), - 'Error for invalid authentication does not match ' + re + ': ' + parsedBody.error); + assert.ok(parsedBody.errors[0].match(re), + 'Error for invalid authentication does not match ' + re + ': ' + parsedBody.errors); return null; }, function postTemplateValid(err) @@ -253,12 +253,12 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 400, res.statusCode + ": " + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); var re = /invalid.*authentication.*missing/i; - assert.ok(parsed.error.match(re), + assert.ok(parsed.errors[0].match(re), 'Error for invalid authentication on PUT does not match ' + - re + ': ' + parsed.error); + re + ': ' + parsed.errors); var del_request = { url: '/api/v1/map/named/' + tpl_id + '?api_key=1234', method: 'DELETE', @@ -425,9 +425,9 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 403, res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), + assert.ok(parsed.hasOwnProperty('errors'), 'Missing error from response: ' + res.body); - err = parsed.error; + err = parsed.errors[0]; assert.ok(err.match(/authenticated user/), err); var next = this; var get_request = { @@ -523,9 +523,9 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 400, res.statusCode + ": " + res.body); var parsedBody = JSON.parse(res.body); - assert.ok(parsedBody.hasOwnProperty('error'), res.body); - assert.ok(parsedBody.error.match(/cannot update name/i), - 'Unexpected error for invalid update: ' + parsedBody.error); + assert.ok(parsedBody.hasOwnProperty('errors'), res.body); + assert.ok(parsedBody.errors[0].match(/cannot update name/i), + 'Unexpected error for invalid update: ' + parsedBody.errors); var put_request = { url: '/api/v1/map/named/unexistent/?api_key=1234', method: 'PUT', @@ -541,9 +541,9 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 400, res.statusCode + ": " + res.body); var parsedBody = JSON.parse(res.body); - assert.ok(parsedBody.hasOwnProperty('error'), res.body); - assert.ok(parsedBody.error.match(/cannot update name/i), - 'Unexpected error for invalid update: ' + parsedBody.error); + assert.ok(parsedBody.hasOwnProperty('errors'), res.body); + assert.ok(parsedBody.errors[0].match(/cannot update name/i), + 'Unexpected error for invalid update: ' + parsedBody.errors); var put_request = { url: '/api/v1/map/named/' + tpl_id + '/?api_key=1234', method: 'PUT', @@ -630,9 +630,9 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 403, res.statusCode + ": " + res.body); var parsedBody = JSON.parse(res.body); - assert.ok(parsedBody.hasOwnProperty('error'), res.body); - assert.ok(parsedBody.error.match(/only.*authenticated.*user/i), - 'Unexpected error for unauthenticated template get: ' + parsedBody.error); + assert.ok(parsedBody.hasOwnProperty('errors'), res.body); + assert.ok(parsedBody.errors[0].match(/only.*authenticated.*user/i), + 'Unexpected error for unauthenticated template get: ' + parsedBody.errors); var get_request = { url: '/api/v1/map/named/' + tpl_id + '?api_key=1234', method: 'GET', @@ -735,10 +735,10 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 403, res.statusCode + ": " + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/only.*authenticated.*user/i), - 'Unexpected error for unauthenticated template get: ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/only.*authenticated.*user/i), + 'Unexpected error for unauthenticated template get: ' + parsed.errors); var del_request = { url: '/api/v1/map/named/' + tpl_id + '?api_key=1234', method: 'DELETE', @@ -767,10 +767,10 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 404, res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/cannot find/i), - 'Unexpected error for missing template: ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/cannot find/i), + 'Unexpected error for missing template: ' + parsed.errors); return null; }, function finish(err) { @@ -863,10 +863,10 @@ describe('template_api', function() { assert.equal(res.statusCode, 403, 'Unexpected success instanciating template with no auth: ' + res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/unauthorized/i), - 'Unexpected error for unauthorized instance : ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/unauthorized/i), + 'Unexpected error for unauthorized instance : ' + parsed.errors); var post_request = { url: '/api/v1/map/named/' + tpl_id + '?auth_token=valid2', method: 'POST', @@ -882,8 +882,8 @@ describe('template_api', function() { if ( err ) throw err; assert.equal(res.statusCode, 404, res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/not found/i), 'Unexpected error for forbidden instance : ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/not found/i), 'Unexpected error for forbidden instance : ' + parsed.errors); var post_request = { url: '/api/v1/map/named/' + tpl_id + '?auth_token=valid2', method: 'POST', @@ -922,10 +922,10 @@ describe('template_api', function() { assert.equal(res.statusCode, 403, 'Fetching tile with no auth: ' + res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/permission denied/i), - 'Unexpected error for unauthorized instance (expected /permission denied/): ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/permission denied/i), + 'Unexpected error for unauthorized instance (expected /permission denied/): ' + parsed.errors); var get_request = { url: '/api/v1/map/' + layergroupid + '/0/0/0.png?auth_token=valid1', method: 'GET', @@ -962,10 +962,10 @@ describe('template_api', function() { assert.equal(res.statusCode, 403, 'Unexpected error for authorized instance: ' + res.statusCode + ' -- ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/cannot use/i), - 'Unexpected error for unauthorized instance (expected /cannot use/): ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/cannot use/i), + 'Unexpected error for unauthorized instance (expected /cannot use/): ' + parsed.errors); return null; }, function deleteTemplate(err) @@ -1089,10 +1089,10 @@ describe('template_api', function() { assert.equal(res.statusCode, 403, 'Unexpected success instanciating template with no auth: ' + res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/unauthorized/i), - 'Unexpected error for unauthorized instance : ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/unauthorized/i), + 'Unexpected error for unauthorized instance : ' + parsed.errors); var post_request = { url: '/api/v1/map/named/' + tpl_id + '?auth_token=valid2', method: 'POST', @@ -1131,10 +1131,11 @@ describe('template_api', function() { assert.equal(res.statusCode, 403, 'Fetching tile with no auth: ' + res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/permission denied/i), - 'Unexpected error for unauthorized instance (expected /permission denied): ' + parsed.error); + console.log(parsed); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/permission denied/i), + 'Unexpected error for unauthorized instance (expected /permission denied): ' + parsed.errors); var get_request = { url: '/api/v1/map/' + layergroupid + ':cb1/0/0/0/0.json.torque?auth_token=valid1', method: 'GET', @@ -1297,10 +1298,10 @@ describe('template_api', function() { assert.equal(res.statusCode, 403, 'Unexpected success instanciating template with no auth: ' + res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/unauthorized/i), - 'Unexpected error for unauthorized instance : ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/unauthorized/i), + 'Unexpected error for unauthorized instance : ' + parsed.errors); var post_request = { url: '/api/v1/map/named/' + tpl_id + '?auth_token=valid2', method: 'POST', @@ -1339,10 +1340,10 @@ describe('template_api', function() { assert.equal(res.statusCode, 403, 'Fetching tile with no auth: ' + res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/permission denied/i), - 'Unexpected error for unauthorized getAttributes (expected /permission denied/): ' + parsed.error); + assert.ok(parsed.hasOwnProperty('errors'), + "Missing 'errors' from response body: " + res.body); + assert.ok(parsed.errors[0].match(/permission denied/i), + 'Unexpected error for unauthorized getAttributes (expected /permission denied/): ' + parsed.errors); var get_request = { url: '/api/v1/map/' + layergroupid + ':cb1/0/attributes/5?auth_token=valid2', method: 'GET', @@ -2072,7 +2073,7 @@ describe('template_api', function() { return done(err); } assert.deepEqual(JSON.parse(res.body), { - error: "Invalid or nonexistent map configuration token '" + nonexistentToken + "'" + errors: ["Invalid or nonexistent map configuration token '" + nonexistentToken + "'"] }); done(); From c5137c9c297788118db7fddc2c92e8fce02e2af1 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 5 Jun 2015 13:41:46 -0400 Subject: [PATCH 4/5] Update news --- NEWS.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS.md b/NEWS.md index 16fbbdea..a5a60af6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,12 @@ Released 2015-mm-dd +Bug fixes: + - Named maps error responses with `{ "errors": ["message"] }` format (#305) + +Announcements: + - Upgrades windshaft + Enhancements: - Named maps names can start with numbers and can contain dashes (-). From a4e303ab63220dc84fc23e961cf13f587fc734a4 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Mon, 8 Jun 2015 10:37:56 -0400 Subject: [PATCH 5/5] Remove console.log from tests --- test/acceptance/templates.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/acceptance/templates.js b/test/acceptance/templates.js index dc28681a..9196c75d 100644 --- a/test/acceptance/templates.js +++ b/test/acceptance/templates.js @@ -1131,7 +1131,6 @@ describe('template_api', function() { assert.equal(res.statusCode, 403, 'Fetching tile with no auth: ' + res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - console.log(parsed); assert.ok(parsed.hasOwnProperty('errors'), "Missing 'errors' from response body: " + res.body); assert.ok(parsed.errors[0].match(/permission denied/i), @@ -1609,7 +1608,6 @@ describe('template_api', function() { function checkInstanciation(err, res) { if ( err ) throw err; - console.log(err, res.body, res.headers); assert.equal(res.statusCode, 200, res.statusCode + ': ' + res.body); // See https://github.com/CartoDB/Windshaft-cartodb/issues/176 helper.checkCache(res);