Merge branch '259-rename-place-id-to-ids'

sanitiser/_id.js

@@ -1,88 +0,0 @@
-
-var _ = require('lodash'),
-    check = require('check-types'),
-    types = require('../query/types');
-
-var ID_DELIM = ':';
-
-// validate inputs, convert types and apply defaults
-// id generally looks like 'geoname:4163334' (type:id)
-// so, both type and id are required fields.
-
-function errorMessage(fieldname, message) {
-  return message || 'invalid param \''+ fieldname + '\': text length, must be >0';
-}
-
-function sanitize( raw, clean ){
-
-  // error & warning messages
-  var messages = { errors: [], warnings: [] };
-
-  // 'raw.id' can be an array
-  var rawIds = check.array( raw.id ) ? _.unique( raw.id ) : [];
-
-  // 'raw.id' can be a string
-  if( check.unemptyString( raw.id ) ){
-    rawIds.push( raw.id );
-  }
-
-  // no ids provided
-  if( !rawIds.length ){
-    messages.errors.push( errorMessage('id') );
-  }
-
-  // ensure all elements are valid non-empty strings
-  rawIds = rawIds.filter( function( uc ){
-    if( !check.unemptyString( uc ) ){
-      messages.errors.push( errorMessage('id') );
-      return false;
-    }
-    return true;
-  });
-
-  // init 'clean.ids'
-  clean.ids = [];
-
-  // cycle through raw ids and set those which are valid
-  rawIds.forEach( function( rawId ){
-
-    var param_index = rawId.indexOf(ID_DELIM);
-    var type = rawId.substring(0, param_index );
-    var id   = rawId.substring(param_index + 1);
-
-    // basic format/ presence of ':'
-    if(param_index === -1) {
-      messages.errors.push(
-        errorMessage(null, 'invalid: must be of the format type:id for ex: \'geoname:4163334\'')
-      );
-    }
-
-    // id text
-    else if( !check.unemptyString( id ) ){
-      messages.errors.push( errorMessage( rawId ) );
-    }
-    // type text
-    else if( !check.unemptyString( type ) ){
-      messages.errors.push( errorMessage( rawId ) );
-    }
-    // type text must be one of the types
-    else if( !_.contains( types, type ) ){
-      messages.errors.push(
-        errorMessage('type', type + ' is invalid. It must be one of these values - [' + types.join(', ') + ']')
-      );
-    }
-    // add valid id to 'clean.ids' array
-    else {
-      clean.ids.push({
-        id: id,
-        type: type
-      });
-    }
-
-  });
-
-  return messages;
-}
-
-// export function
-module.exports = sanitize;

sanitiser/_ids.js

@@ -0,0 +1,74 @@
+var _ = require('lodash'),
+    check = require('check-types'),
+    types = require('../query/types');
+
+var ID_DELIM = ':';
+
+// validate inputs, convert types and apply defaults
+// id generally looks like 'geoname:4163334' (type:id)
+// so, both type and id are required fields.
+
+var lengthError = 'invalid param \'ids\': length must be >0';
+
+var formatError = function(input) {
+  return 'id `' + input + 'is invalid: must be of the format type:id for ex: \'geoname:4163334\'';
+};
+
+function sanitize( raw, clean ){
+  // error & warning messages
+  var messages = { errors: [], warnings: [] };
+
+  // 'raw.ids' can be an array if ids is specified multiple times
+  // see https://github.com/pelias/api/issues/272
+  if (check.array( raw.ids )) {
+    messages.errors.push( '`ids` parameter specified multiple times.' );
+    return messages;
+  }
+
+  if (!check.unemptyString( raw.ids )) {
+    messages.errors.push( lengthError);
+    return messages;
+  }
+
+  // split string into array of values
+  var rawIds = raw.ids.split(',');
+
+  // deduplicate
+  rawIds = _.unique(rawIds);
+
+  // ensure all elements are valid non-empty strings
+  if (!rawIds.every(check.unemptyString)) {
+      messages.errors.push( lengthError );
+  }
+
+  // cycle through raw ids and set those which are valid
+  var validIds = rawIds.map( function( rawId ){
+    var param_index = rawId.indexOf(ID_DELIM);
+    var type = rawId.substring(0, param_index );
+    var id   = rawId.substring(param_index + 1);
+
+    // check id format
+    if(!check.contains(rawId, ID_DELIM) || !check.unemptyString( id ) || !check.unemptyString( type )) {
+      messages.errors.push( formatError(rawId) );
+    }
+    // type text must be one of the types
+    else if( !_.contains( types, type ) ){
+      messages.errors.push( type + ' is invalid. It must be one of these values - [' + types.join(', ') + ']' );
+    }
+    else {
+      return {
+        id: id,
+        type: type
+      };
+    }
+  });
+
+  if (validIds.every(check.object)) {
+    clean.ids = validIds;
+  }
+
+  return messages;
+}
+
+// export function
+module.exports = sanitize;

sanitiser/place.js

@@ -1,7 +1,7 @@
 
 var sanitizeAll = require('../sanitiser/sanitizeAll'),
     sanitizers = {
-      id: require('../sanitiser/_id'),
+      ids: require('../sanitiser/_ids'),
       private: require('../sanitiser/_flag_bool')('private', false)
     };
 

test/ciao/place/basic_place.coffee

@@ -1,6 +1,6 @@
 
 #> basic place
-path: '/v1/place?id=geoname:1'
+path: '/v1/place?ids=geoname:1'
 
 #? 200 ok
 response.statusCode.should.be.equal 200

test/ciao/place/missing_id.coffee

@@ -24,11 +24,11 @@ json.features.should.be.instanceof Array
 
 #? expected errors
 should.exist json.geocoding.errors
-json.geocoding.errors.should.eql [ 'invalid param \'id\': text length, must be >0' ]
+json.geocoding.errors.should.eql [ 'invalid param \'ids\': length must be >0' ]
 
 #? expected warnings
 should.not.exist json.geocoding.warnings
 
 #? inputs
-json.geocoding.query['ids'].should.eql []
+should.not.exist json.geocoding.query['ids']
 should.not.exist json.geocoding.query['size']

test/unit/run.js

@@ -8,6 +8,7 @@ var tests = [
   require('./controller/search'),
   require('./service/mget'),
   require('./service/search'),
+  require('./sanitiser/_ids'),
   require('./sanitiser/_flag_bool'),
   require('./sanitiser/autocomplete'),
   require('./sanitiser/_sources'),

test/unit/sanitiser/_ids.js

@@ -0,0 +1,177 @@
+var sanitize = require('../../../sanitiser/_ids');
+
+var delimiter = ':';
+var types = require('../../../query/types');
+var inputs = {
+  valid: [ 'geoname:1', 'osmnode:2', 'admin0:53', 'osmway:44', 'geoname:5' ],
+  invalid: [ ':', '', '::', 'geoname:', ':234', 'gibberish:23' ]
+};
+
+var formatError = function(input) {
+  return 'id `' + input + 'is invalid: must be of the format type:id for ex: \'geoname:4163334\'';
+};
+var lengthError = 'invalid param \'ids\': length must be >0';
+var defaultMissingTypeError = function(input) {
+    var type = input.split(delimiter)[0];
+    return type + ' is invalid. It must be one of these values - [' + types.join(', ') + ']';
+  };
+
+module.exports.tests = {};
+
+module.exports.tests.invalid_ids = function(test, common) {
+  test('invalid id: empty string', function(t) {
+    var raw = { ids: '' };
+    var clean = {};
+
+    var messages = sanitize(raw, clean);
+
+    t.equal(messages.errors[0], lengthError, 'ids length error returned');
+    t.equal(clean.ids, undefined, 'ids unset in clean object');
+    t.end();
+  });
+
+  test('invalid id: single colon', function(t) {
+    var raw = { ids: ':' };
+    var clean = {};
+
+    var messages = sanitize(raw, clean);
+
+    t.equal(messages.errors[0], formatError(':'), 'format error returned');
+    t.equal(clean.ids, undefined, 'ids unset in clean object');
+    t.end();
+  });
+
+  test('invalid id: double colon', function(t) {
+    var raw = { ids: '::' };
+    var clean = {};
+
+    var messages = sanitize(raw, clean);
+
+    t.equal(messages.errors[0], formatError('::'), 'format error returned');
+    t.equal(clean.ids, undefined, 'ids unset in clean object');
+    t.end();
+  });
+
+  test('invalid id: only type section present', function(t) {
+    var raw = { ids: 'geoname:' };
+    var clean = {};
+
+    var messages = sanitize(raw, clean);
+
+    t.equal(messages.errors[0], formatError('geoname:'), 'format error returned');
+    t.equal(clean.ids, undefined, 'ids unset in clean object');
+    t.end();
+  });
+
+  test('invalid id: only type id section present', function(t) {
+    var raw = { ids: ':234' };
+    var clean = {};
+
+    var messages = sanitize(raw, clean);
+
+    t.equal(messages.errors[0], formatError(':234'), 'format error returned');
+    t.equal(clean.ids, undefined, 'ids unset in clean object');
+    t.end();
+  });
+
+  test('invalid id: type name invalid', function(t) {
+    var raw = { ids: 'gibberish:23' };
+    var clean = {};
+
+    var messages = sanitize(raw, clean);
+
+    t.equal(messages.errors[0], defaultMissingTypeError('gibberish:23'), 'format error returned');
+    t.equal(clean.ids, undefined, 'ids unset in clean object');
+    t.end();
+  });
+};
+
+module.exports.tests.valid_ids = function(test, common) {
+  test('ids: valid input', function(t) {
+    inputs.valid.forEach( function( input ){
+      var input_parts = input.split(delimiter);
+      var expected_clean = { ids: [ { id: input_parts[1], type: input_parts[0] } ]};
+      var raw = {  ids: input };
+      var clean = {};
+
+      var messages = sanitize( raw, clean );
+
+      t.deepEqual( messages.errors, [], 'no error (' + input + ')' );
+      t.deepEqual( clean, expected_clean, 'clean set correctly (' + input + ')');
+    });
+    t.end();
+  });
+
+  test('ids: valid input with multiple values' , function(t) {
+    var raw = { ids: inputs.valid.join(',') };
+    var clean = {};
+    var expected_clean={
+      ids: [],
+    };
+    // construct the expected id and type for each valid input
+    inputs.valid.forEach( function( input ){
+      var input_parts = input.split(delimiter);
+      expected_clean.ids.push({ id: input_parts[1], type: input_parts[0] });
+    });
+
+    var messages = sanitize( raw, clean );
+
+    t.deepEqual( messages.errors, [], 'no errors' );
+    t.deepEqual( clean, expected_clean, 'clean set correctly' );
+    t.end();
+  });
+};
+
+module.exports.tests.array_of_ids = function(test, common) {
+  // see https://github.com/pelias/api/issues/272
+  test('array of ids sent by queryparser', function(t) {
+    var raw = { ids: ['geoname:2', 'oswmay:4'] };
+    var clean = {};
+
+    var messages = sanitize( raw, clean);
+
+    t.deepEqual( messages.errors, ['`ids` parameter specified multiple times.'], 'error sent' );
+    t.deepEqual( clean.ids, undefined, 'response is empty due to error' );
+    t.end();
+  });
+};
+
+module.exports.tests.multiple_ids = function(test, common) {
+  test('duplicate ids', function(t) {
+    var expected_clean = { ids: [ { id: '1', type: 'geoname' }, { id: '2', type: 'osmnode' } ] };
+    var raw = { ids: 'geoname:1,osmnode:2' };
+    var clean = {};
+
+    var messages = sanitize( raw, clean);
+
+    t.deepEqual( messages.errors, [], 'no errors' );
+    t.deepEqual( messages.warnings, [], 'no warnings' );
+    t.deepEqual(clean, expected_clean, 'clean set correctly');
+    t.end();
+  });
+};
+
+module.exports.tests.de_dupe = function(test, common) {
+  test('duplicate ids', function(t) {
+    var expected_clean = { ids: [ { id: '1', type: 'geoname' }, { id: '2', type: 'osmnode' } ]};
+    var raw = { ids: 'geoname:1,osmnode:2,geoname:1' };
+    var clean = {};
+
+    var messages = sanitize( raw, clean );
+
+    t.deepEqual( messages.errors, [], 'no errors' );
+    t.deepEqual( messages.warnings, [], 'no warnings' );
+    t.deepEqual(clean, expected_clean, 'clean set correctly');
+    t.end();
+  });
+};
+
+module.exports.all = function (tape, common) {
+  function test(name, testFunction) {
+    return tape('SANTIZE _ids ' + name, testFunction);
+  }
+
+  for( var testCase in module.exports.tests ){
+    module.exports.tests[testCase](test, common);
+  }
+};

test/unit/sanitiser/place.js

@@ -1,26 +1,9 @@
-
-// @todo: refactor this test, it's pretty messy, brittle and hard to follow
-
 var place  = require('../../../sanitiser/place'),
     sanitize = place.sanitize,
     middleware = place.middleware,
-    types = require('../../../query/types'),
+    defaultClean = { ids: [ { id: '123', type: 'geoname' } ], private: false };
-    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 - [' + types.join(', ') + ']'; },
-    defaultClean = { ids: [ { id: '123', type: 'geoname' } ], private: false },
-    inputs = {
-      valid: [ 'geoname:1', 'osmnode:2', 'admin0:53', 'osmway:44', 'geoname:5' ],
-      invalid: [ ':', '', '::', 'geoname:', ':234', 'gibberish:23' ]
-    };
 
 // these are the default values you would expect when no input params are specified.
-var emptyClean = { ids: [], private: false };
-
 module.exports.tests = {};
 
 module.exports.tests.interface = function(test, common) {
@@ -31,81 +14,7 @@ module.exports.tests.interface = function(test, common) {
   });
   test('middleware interface', function(t) {
     t.equal(typeof middleware, 'function', 'middleware is a function');
-    t.equal(middleware.length, 3, 'sanitizee has a valid middleware');
+    t.equal(middleware.length, 3, 'sanitize has a valid middleware');
-    t.end();
-  });
-};
-
-module.exports.tests.sanitize_id = function(test, common) {
-  test('id: invalid input', function(t) {
-    inputs.invalid.forEach( function( input ){
-      var req = { query: { id: input } };
-      sanitize(req, function( ){
-        switch (req.errors[0]) {
-          case defaultError:
-            t.equal(req.errors[0], defaultError, input + ' is invalid input'); break;
-          case defaultLengthError(input):
-            t.equal(req.errors[0], defaultLengthError(input), input + ' is invalid (missing id/type)'); break;
-          case defaultFormatError:
-            t.equal(req.errors[0], defaultFormatError, input + ' is invalid (invalid format)'); break;
-          case defaultMissingTypeError(input):
-            t.equal(req.errors[0], defaultMissingTypeError(input), input + ' is an unknown type'); break;
-          default: break;
-        }
-        t.deepEqual(req.clean, emptyClean, 'clean only has default values set');
-      });
-    });
-    t.end();
-  });
-
-  test('id: valid input', function(t) {
-    inputs.valid.forEach( function( input ){
-      var input_parts = input.split(delimiter);
-      var expected = { ids: [ { id: input_parts[1], type: input_parts[0] } ], private: false };
-      var req = { query: { id: input } };
-      sanitize(req, function(){
-        t.deepEqual( req.errors, [], 'no error (' + input + ')' );
-        t.deepEqual( req.clean, expected, 'clean set correctly (' + input + ')');
-      });
-    });
-    t.end();
-  });
-};
-
-
-module.exports.tests.sanitize_ids = function(test, common) {
-  test('ids: invalid input', function(t) {
-    var req = { query: { id: inputs.invalid } };
-    var expected = [
-      'invalid param \'id\': text length, must be >0',
-      'invalid param \':\': text length, must be >0',
-      'invalid param \'::\': text length, must be >0',
-      'invalid param \'geoname:\': text length, must be >0',
-      'invalid param \':234\': text length, must be >0',
-      'gibberish is invalid. It must be one of these values - ' +
-      '[geoname, osmnode, osmway, admin0, admin1, admin2, neighborhood, ' +
-      'locality, local_admin, osmaddress, openaddresses]'
-    ];
-    sanitize(req, function(){
-      t.deepEqual(req.errors, expected);
-      t.deepEqual(req.clean, emptyClean, 'clean only has default values set');
-    });
-    t.end();
-  });
-
-  test('ids: 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] });
-    });
-    expected.private = false;
-    var req = { query: { id: inputs.valid } };
-    sanitize(req, function(){
-      t.deepEqual( req.errors, [], 'no errors' );
-      t.deepEqual( req.clean, expected, 'clean set correctly' );
-    });
     t.end();
   });
 };
@@ -114,7 +23,7 @@ module.exports.tests.sanitize_private = function(test, common) {
   var invalid_values = [null, -1, 123, NaN, 'abc'];
   invalid_values.forEach(function(value) {
     test('invalid private param ' + value, function(t) {
-      var req = { query: { id:'geoname:123', 'private': value } };
+      var req = { query: { ids:'geoname:123', 'private': value } };
       sanitize(req, function(){
         t.deepEqual( req.errors, [], 'no errors' );
         t.deepEqual( req.warnings, [], 'no warnings' );
@@ -127,7 +36,7 @@ module.exports.tests.sanitize_private = function(test, common) {
   var valid_values = ['true', true, 1];
   valid_values.forEach(function(value) {
     test('valid private param ' + value, function(t) {
-      var req = { query: { id:'geoname:123', 'private': value } };
+      var req = { query: { ids:'geoname:123', 'private': value } };
       sanitize(req, function(){
         t.deepEqual( req.errors, [], 'no errors' );
         t.deepEqual( req.warnings, [], 'no warnings' );
@@ -140,7 +49,7 @@ module.exports.tests.sanitize_private = function(test, common) {
   var valid_false_values = ['false', false, 0];
   valid_false_values.forEach(function(value) {
     test('test setting false explicitly ' + value, function(t) {
-      var req = { query: { id:'geoname:123', 'private': value } };
+      var req = { query: { ids:'geoname:123', 'private': value } };
       sanitize(req, function(){
         t.deepEqual( req.errors, [], 'no errors' );
         t.deepEqual( req.warnings, [], 'no warnings' );
@@ -151,7 +60,7 @@ module.exports.tests.sanitize_private = function(test, common) {
   });
 
   test('test default behavior', function(t) {
-    var req = { query: { id:'geoname:123' } };
+    var req = { query: { ids:'geoname:123' } };
     sanitize(req, function(){
       t.deepEqual( req.errors, [], 'no errors' );
       t.deepEqual( req.warnings, [], 'no warnings' );
@@ -161,24 +70,11 @@ module.exports.tests.sanitize_private = function(test, common) {
   });
 };
 
-module.exports.tests.de_dupe = function(test, common) {
-  var expected = { ids: [ { id: '1', type: 'geoname' }, { id: '2', type: 'osmnode' } ], private: false };
-  var req = { query: { id: ['geoname:1', 'osmnode:2', 'geoname:1'] } };
-  test('duplicate ids', function(t) {
-    sanitize( req, function(){
-      t.deepEqual( req.errors, [], 'no errors' );
-      t.deepEqual( req.warnings, [], 'no warnings' );
-      t.deepEqual(req.clean, expected, 'clean set correctly');
-      t.end();
-    });
-  });
-};
-
 module.exports.tests.invalid_params = function(test, common) {
-  test('invalid params', function(t) {
+  test('no params', function(t) {
     var req = { query: {} };
     sanitize( req, function(){
-      t.equal( req.errors[0], defaultError, 'handle invalid params gracefully');
+      t.equal( req.errors[0], 'invalid param \'ids\': length must be >0', 'error for missing `ids` param');
       t.deepEqual( req.warnings, [], 'no warnings' );
       t.end();
     });
@@ -187,7 +83,7 @@ module.exports.tests.invalid_params = 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: { ids: 'geoname:123' }};
     var next = function(){
       t.deepEqual( req.errors, [], 'no errors' );
       t.deepEqual( req.warnings, [], 'no warnings' );