From b828a05b01b80eaa42d79653d439418d444710f6 Mon Sep 17 00:00:00 2001 From: Harish Krishna Date: Thu, 16 Oct 2014 16:10:25 -0400 Subject: [PATCH] simplifying things, DRY - one endpoint 'GET' handles single/multiple requests. plus test coverage --- app.js | 3 -- controller/get.js | 29 ++++++++---- controller/mget.js | 55 ---------------------- sanitiser/_id.js | 62 ++++++++++++++----------- sanitiser/_ids.js | 56 ----------------------- sanitiser/mget.js | 23 ---------- test/unit/sanitiser/get.js | 94 ++++++++++++++++++++++++-------------- 7 files changed, 114 insertions(+), 208 deletions(-) delete mode 100644 controller/mget.js delete mode 100644 sanitiser/_ids.js delete mode 100644 sanitiser/mget.js diff --git a/app.js b/app.js index 1c071cb1..a80a4f82 100644 --- a/app.js +++ b/app.js @@ -12,7 +12,6 @@ app.use( require('./middleware/jsonp') ); var sanitisers = {}; sanitisers.get = require('./sanitiser/get'); -sanitisers.mget = require('./sanitiser/mget'); sanitisers.suggest = require('./sanitiser/suggest'); sanitisers.search = sanitisers.suggest; sanitisers.reverse = require('./sanitiser/reverse'); @@ -22,7 +21,6 @@ sanitisers.reverse = require('./sanitiser/reverse'); var controllers = {}; controllers.index = require('./controller/index'); controllers.get = require('./controller/get'); -controllers.mget = require('./controller/mget'); controllers.suggest = require('./controller/suggest'); controllers.search = require('./controller/search'); @@ -33,7 +31,6 @@ app.get( '/', controllers.index() ); // get doc API app.get( '/get', sanitisers.get.middleware, controllers.get() ); -app.get( '/mget', sanitisers.mget.middleware, controllers.mget() ); // suggest API app.get( '/suggest', sanitisers.suggest.middleware, controllers.suggest() ); diff --git a/controller/get.js b/controller/get.js index 57842b93..b48e6f64 100644 --- a/controller/get.js +++ b/controller/get.js @@ -5,25 +5,36 @@ function setup( backend ){ // allow overriding of dependencies backend = backend || require('../src/backend'); - + backend = new backend(); + function controller( req, res, next ){ + var docs = req.clean.ids.map( function(id) { + return { + _index: 'pelias', + _type: id.type, + _id: id.id + }; + }); + // backend command var cmd = { - index: 'pelias', - type: req.clean.type, - id: req.clean.id + body: { + docs: docs + } }; - // query backend - backend().client.get( cmd, function( err, data ){ - + // query new backend + backend.client.mget( cmd, function( err, data ){ + var docs = []; // handle backend errors if( err ){ return next( err ); } - if( data && data.found && data._source ){ - docs.push(data._source); + if( data && data.docs && Array.isArray(data.docs) && data.docs.length ){ + docs = data.docs.map( function( doc ){ + return doc._source; + }); } // convert docs to geojson diff --git a/controller/mget.js b/controller/mget.js deleted file mode 100644 index fadc4ede..00000000 --- a/controller/mget.js +++ /dev/null @@ -1,55 +0,0 @@ - -var geojsonify = require('../helper/geojsonify').search; - -function setup( backend ){ - - // allow overriding of dependencies - backend = backend || require('../src/backend'); - - function controller( req, res, next ){ - - // backend command - var cmd = req.clean.ids.map( function(id) { - return { - index: 'pelias', - type: id.type, - id: id.id - }; - }); - cmd = { - index: 'pelias', - type: 'geoname', - ids: cmd.map(function(c){ return c.id }) - } - console.log('cmd:') - console.log(cmd) - // query backend - backend().client.mget( cmd, function( err, data ){ - console.log('error:') - console.log(err) - var docs = []; - // handle backend errors - if( err ){ return next( err ); } - - if( data && data.docs && Array.isArray(data.docs) && data.docs.length ){ - docs = data.docs.map( function( doc ){ - return doc._source; - }); - } - - // convert docs to geojson - var geojson = geojsonify( docs ); - - // response envelope - geojson.date = new Date().getTime(); - - // respond - return res.status(200).json( geojson ); - }); - - } - - return controller; -} - -module.exports = setup; diff --git a/sanitiser/_id.js b/sanitiser/_id.js index 52397128..62860243 100644 --- a/sanitiser/_id.js +++ b/sanitiser/_id.js @@ -1,5 +1,5 @@ // validate inputs, convert types and apply defaults -// id generally looks like 'geoname/4163334' (type/id) +// id generally looks like 'geoname:4163334' (type:id) // so, both type and id are required fields. function sanitize( req ){ @@ -7,6 +7,7 @@ function sanitize( req ){ req.clean = req.clean || {}; var params = req.query; var indeces = require('../query/indeces'); + var delim = ':'; // ensure params is a valid object if( Object.prototype.toString.call( params ) !== '[object Object]' ){ @@ -20,38 +21,45 @@ function sanitize( req ){ } }; - // id text - if('string' !== typeof params.id || !params.id.length){ + if(('string' === typeof params.id && !params.id.length) || params.id === undefined){ return errormessage('id'); } - // id format - if(params.id.indexOf('/') === -1) { - return errormessage('id', 'invalid: must be of the format type/id for ex: \'geoname/4163334\''); - } - req.clean.id = params.id; + if( params && params.id && params.id.length ){ + req.clean.ids = []; + params.ids = Array.isArray(params.id) ? params.id : [params.id]; + + for (var i=0; i0') - } - }; - - if( params && params.ids && params.ids.length ){ - req.clean.ids = []; - console.log(params.ids) - params.ids.split(',').forEach( function(param) { - param = param.split('/'); - var type= param[0]; - var id = param[1]; - console.log(param) - // id text - if('string' !== typeof id || !id.length){ - return errormessage('id'); - } - // type text - if('string' !== typeof type || !type.length){ - return errormessage('type'); - } - // type text must be one of the indeces - if(indeces.indexOf(type) == -1){ - return errormessage('type', 'type must be one of these values - [' + indeces.join(", ") + ']'); - } - req.clean.ids.push({ - id: id, - type: type - }); - }); - } - console.log(req.clean) - return { 'error': false }; - -} - -// export function -module.exports = sanitize; \ No newline at end of file diff --git a/sanitiser/mget.js b/sanitiser/mget.js deleted file mode 100644 index b1f6d40f..00000000 --- a/sanitiser/mget.js +++ /dev/null @@ -1,23 +0,0 @@ - -var logger = require('../src/logger'), - _sanitize = require('../sanitiser/_sanitize'), - sanitizers = { - id: require('../sanitiser/_ids') - }; - -var sanitize = function(req, cb) { _sanitize(req, sanitizers, cb); } - -// export sanitize for testing -module.exports.sanitize = sanitize; - -// middleware -module.exports.middleware = function( req, res, next ){ - sanitize( req, function( err, clean ){ - if( err ){ - res.status(400); // 400 Bad Request - return next(err); - } - req.clean = clean; - next(); - }); -}; \ No newline at end of file diff --git a/test/unit/sanitiser/get.js b/test/unit/sanitiser/get.js index 36e777d1..3de73397 100644 --- a/test/unit/sanitiser/get.js +++ b/test/unit/sanitiser/get.js @@ -3,13 +3,19 @@ var get = require('../../../sanitiser/get'), _sanitize = get.sanitize, middleware = get.middleware, indeces = require('../../../query/indeces'), - defaultIdError = 'invalid param \'id\': text length, must be >0', - defaultTypeError = 'invalid param \'type\': text length, must be >0', - defaultFormatError = 'invalid: must be of the format type/id for ex: \'geoname/4163334\'', - defaultError = defaultIdError, - defaultMissingTypeError = 'type must be one of these values - [' + indeces.join(", ") + ']', - defaultClean = { id: '123', type: 'geoname' }, - sanitize = function(query, cb) { _sanitize({'query':query}, cb); } + delimiter = ':', + defaultLengthError = function(input) { return 'invalid param \''+ input + '\': text length, must be >0' }, + defaultFormatError = 'invalid: must be of the format type:id for ex: \'geoname:4163334\'', + defaultError = 'invalid param \'id\': text length, must be >0', + defaultMissingTypeError = function(input) { + var type = input.split(delimiter)[0]; + return type + ' is invalid. It must be one of these values - [' + indeces.join(", ") + ']'}, + defaultClean = { ids: [ { id: '123', type: 'geoname' } ] }, + sanitize = function(query, cb) { _sanitize({'query':query}, cb); }, + inputs = { + valid: [ 'geoname:1', 'osmnode:2', 'admin0:53', 'osmway:44', 'geoname:5' ], + invalid: [ ':', '', '::', 'geoname:', ':234', 'gibberish:23' ] + }; module.exports.tests = {}; @@ -26,37 +32,19 @@ module.exports.tests.interface = function(test, common) { }); }; -module.exports.tests.sanitize_id_and_type = function(test, common) { - var inputs = { - valid: [ - 'geoname/1', - 'osmnode/2', - 'admin0/53', - 'osmway/44', - 'geoname/5' - ], - invalid: [ - '/', - '', - '//', - 'geoname/', - '/234', - 'gibberish/23' - ] - }; - +module.exports.tests.sanitize_id = function(test, common) { test('invalid input', function(t) { inputs.invalid.forEach( function( input ){ sanitize({ id: input }, function( err, clean ){ switch (err) { - case defaultIdError: - t.equal(err, defaultIdError, input + ' is invalid (missing id)'); break; - case defaultTypeError: - t.equal(err, defaultTypeError, input + ' is invalid (missing type)'); break; + case defaultError: + t.equal(err, defaultError, input + ' is invalid input'); break; + case defaultLengthError(input): + t.equal(err, defaultLengthError(input), input + ' is invalid (missing id/type)'); break; case defaultFormatError: t.equal(err, defaultFormatError, input + ' is invalid (invalid format)'); break; - case defaultMissingTypeError: - t.equal(err, defaultMissingTypeError, input + ' is an unknown type'); break; + case defaultMissingTypeError(input): + t.equal(err, defaultMissingTypeError(input), input + ' is an unknown type'); break; default: break; } t.equal(clean, undefined, 'clean not set'); @@ -67,8 +55,8 @@ module.exports.tests.sanitize_id_and_type = function(test, common) { test('valid input', function(t) { inputs.valid.forEach( function( input ){ - var input_parts = input.split('/'); - var expected = { id: input_parts[1], type: input_parts[0] }; + var input_parts = input.split(delimiter); + var expected = { ids: [ { id: input_parts[1], type: input_parts[0] } ] }; sanitize({ id: input }, function( err, clean ){ t.equal(err, undefined, 'no error (' + input + ')' ); t.deepEqual(clean, expected, 'clean set correctly (' + input + ')'); @@ -78,6 +66,42 @@ module.exports.tests.sanitize_id_and_type = function(test, common) { }); }; + +module.exports.tests.sanitize_ids = function(test, common) { + test('invalid input', function(t) { + sanitize({ id: inputs.invalid }, function( err, clean ){ + var input = inputs.invalid[0]; // since it breaks on the first invalid element + switch (err) { + case defaultError: + t.equal(err, defaultError, input + ' is invalid input'); break; + case defaultLengthError(input): + t.equal(err, defaultLengthError(input), input + ' is invalid (missing id/type)'); break; + case defaultFormatError: + t.equal(err, defaultFormatError, input + ' is invalid (invalid format)'); break; + case defaultMissingTypeError(input): + t.equal(err, defaultMissingTypeError(input), input + ' is an unknown type'); break; + default: break; + } + t.equal(clean, undefined, 'clean not set'); + }); + t.end(); + }); + + test('valid input', function(t) { + var expected={}; + expected.ids = []; + inputs.valid.forEach( function( input ){ + var input_parts = input.split(delimiter); + expected.ids.push({ id: input_parts[1], type: input_parts[0] }); + }); + sanitize({ id: inputs.valid }, function( err, clean ){ + t.equal(err, undefined, 'no error' ); + t.deepEqual(clean, expected, 'clean set correctly'); + }); + t.end(); + }); +}; + module.exports.tests.invalid_params = function(test, common) { test('invalid params', function(t) { sanitize( undefined, function( err, clean ){ @@ -102,7 +126,7 @@ module.exports.tests.middleware_failure = function(test, common) { module.exports.tests.middleware_success = function(test, common) { test('middleware success', function(t) { - var req = { query: { id: 'geoname/123' }}; + var req = { query: { id: 'geoname' + delimiter + '123' }}; var next = function( message ){ t.equal(message, undefined, 'no error message set'); t.deepEqual(req.clean, defaultClean);