Merge branch 'master' into unit-tests-for-type-mapping-helper

controller/place.js

@@ -19,13 +19,8 @@ function setup( backend ){
         /*
          * some gids aren't resolvable to a single type (ex: osmnode and osmway
          * both have source osm and layer venue), so expect an array of
-         * possible values. It's important to use `type` here instead of
+         * possible values. */
-         * `_type`, as the former actually queries against the type, and thus
+        _type: id.types,
-         * can accept multiple match values.  `_type`, on the other hand,
-         * simply changes the actual URL of the query sent to Elasticsearch to
-         * contain a type, which obviously can only take a single type.
-         */
-        type: id.types,
         _id: id.id
       };
     });

query/defaults.js

@@ -39,35 +39,35 @@ module.exports = extend( false, peliasQuery.defaults, {
 
   'address:housenumber:analyzer': 'standard',
   'address:housenumber:field': 'address.number',
-  'address:housenumber:boost': 1,
+  'address:housenumber:boost': 2,
 
   'address:street:analyzer': 'standard',
   'address:street:field': 'address.street',
-  'address:street:boost': 1,
+  'address:street:boost': 5,
 
   'address:postcode:analyzer': 'standard',
   'address:postcode:field': 'address.zip',
-  'address:postcode:boost': 1,
+  'address:postcode:boost': 3,
 
   'admin:alpha3:analyzer': 'standard',
   'admin:alpha3:field': 'alpha3',
-  'admin:alpha3:boost': 1,
+  'admin:alpha3:boost': 5,
 
   'admin:admin0:analyzer': 'peliasAdmin',
   'admin:admin0:field': 'admin0',
-  'admin:admin0:boost': 1,
+  'admin:admin0:boost': 4,
 
   'admin:admin1:analyzer': 'peliasAdmin',
   'admin:admin1:field': 'admin1',
-  'admin:admin1:boost': 1,
+  'admin:admin1:boost': 3,
 
   'admin:admin1_abbr:analyzer': 'peliasAdmin',
   'admin:admin1_abbr:field': 'admin1_abbr',
-  'admin:admin1_abbr:boost': 1,
+  'admin:admin1_abbr:boost': 3,
 
   'admin:admin2:analyzer': 'peliasAdmin',
   'admin:admin2:field': 'admin2',
-  'admin:admin2:boost': 1,
+  'admin:admin2:boost': 2,
 
   'admin:local_admin:analyzer': 'peliasAdmin',
   'admin:local_admin:field': 'local_admin',

sanitiser/_geo_common.js

@@ -1,7 +1,8 @@
 /**
  * helper sanitiser methods for geo parameters
  */
-var util = require('util'),
+var groups = require('./_groups'),
+    util = require('util'),
     check = require('check-types'),
     _ = require('lodash');
 
@@ -14,37 +15,29 @@ var util = require('util'),
  * @param {bool} bbox_is_required
  */
 function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) {
-
+  // calculate full property names from the key_prefix
-  // the names we use to define the corners of the rect
+  var properties = [ 'min_lat', 'max_lat', 'min_lon', 'max_lon' ].map(function(prop) {
-  var mandatoryProps = [ 'min_lat', 'max_lat', 'min_lon', 'max_lon' ];
+    return key_prefix + '.' + prop;
-
-  // count up how many fields the user actually specified
-  var totalFieldsSpecified = 0;
-  mandatoryProps.forEach( function( prop ){
-    if( raw.hasOwnProperty( key_prefix + '.' + prop ) ){
-      totalFieldsSpecified++;
-    }
   });
 
-  // all fields specified
+  // sanitize the rect property group, this throws an exception if
-  if( 4 === totalFieldsSpecified ) {
+  // the group is not complete
-    // reuse the coord sanitizer and set required:true so we get a fatal error if
+  var bbox_present;
-    // any one of the corners is not specified.
+  if (bbox_is_required) {
-    sanitize_coord( key_prefix + '.min_lat', clean, raw[ key_prefix + '.min_lat' ], true );
+    bbox_present = groups.required(raw, properties);
-    sanitize_coord( key_prefix + '.max_lat', clean, raw[ key_prefix + '.max_lat' ], true );
+  } else {
-    sanitize_coord( key_prefix + '.min_lon', clean, raw[ key_prefix + '.min_lon' ], true );
+    bbox_present = groups.optional(raw, properties);
-    sanitize_coord( key_prefix + '.max_lon', clean, raw[ key_prefix + '.max_lon' ], true );
-  }
-  // fields only partially specified
-  else if( totalFieldsSpecified > 0 ){
-    var format1 = 'missing rect param \'%s\' requires all of: \'%s\' to be present';
-    throw new Error( util.format( format1, key_prefix, mandatoryProps.join('\',\'') ) );
-  }
-  // fields required, eg. ( totalFieldsSpecified === 0 && bbox_is_required === true )
-  else if( bbox_is_required ){
-    var format2 = 'missing rect param \'%s\' requires all of: \'%s\' to be present';
-    throw new Error( util.format( format2, key_prefix, mandatoryProps.join('\',\'') ) );
   }
+
+  // don't bother checking individual elements if bbox is not required
+  // and not present
+  if (!bbox_present) { return; }
+
+  // check each property individually. now that it is known a bbox is present,
+  // all properties must exist, so pass the true flag for coord_is_required
+  properties.forEach(function(prop) {
+    sanitize_coord(prop, clean, raw[prop], true);
+  });
 }
 
 /**
@@ -56,43 +49,13 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) {
  * @param {bool} circle_is_required
  */
 function sanitize_circle( key_prefix, clean, raw, circle_is_required ) {
-
+  // sanitize both a point and a radius if radius is present
-  // the names we use to define the centroid
+  // otherwise just sanittize the point
-  var mandatoryProps = [ 'lat', 'lon' ];
+  if( check.assigned( raw[ key_prefix + '.radius' ] ) ){
-
+    sanitize_coord( key_prefix + '.radius', clean, raw[ key_prefix + '.radius' ], true );
-  // count up how many fields the user actually specified
+    sanitize_point( key_prefix, clean, raw, true);
-  var totalFieldsSpecified = 0;
+  } else {
-  mandatoryProps.forEach( function( prop ){
+    sanitize_point( key_prefix, clean, raw, circle_is_required);
-    if( raw.hasOwnProperty( key_prefix + '.' + prop ) ){
-      totalFieldsSpecified++;
-    }
-  });
-
-  // all fields specified
-  if( 2 === totalFieldsSpecified ) {
-    // reuse the coord sanitizer and set required:true so we get a fatal error if
-    // any one of the coords is not specified.
-    sanitize_coord( key_prefix + '.lat', clean, raw[ key_prefix + '.lat' ], true );
-    sanitize_coord( key_prefix + '.lon', clean, raw[ key_prefix + '.lon' ], true );
-
-    if( check.assigned( raw[ key_prefix + '.radius' ] ) ){
-      sanitize_coord( key_prefix + '.radius', clean, raw[ key_prefix + '.radius' ], true );
-    }
-  }
-  // fields only partially specified
-  else if( totalFieldsSpecified > 0 ){
-    var format1 = 'missing circle param \'%s\' requires all of: \'%s\' to be present';
-    throw new Error( util.format( format1, key_prefix, mandatoryProps.join('\',\'') ) );
-  }
-  // radius was specified without lat or lon
-  else if( raw.hasOwnProperty( key_prefix + '.radius' ) ){
-    var format2 = 'missing circle param \'%s\' requires all of: \'%s\' to be present';
-    throw new Error( util.format( format2, key_prefix, mandatoryProps.join('\',\'') ) );
-  }
-  // fields required, eg. ( totalFieldsSpecified === 0 && bbox_is_required === true )
-  else if( circle_is_required ){
-    var format3 = 'missing circle param \'%s\' requires all of: \'%s\' to be present';
-    throw new Error( util.format( format3, key_prefix, mandatoryProps.join('\',\'') ) );
   }
 }
 
@@ -105,35 +68,29 @@ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) {
  * @param {bool} point_is_required
  */
 function sanitize_point( key_prefix, clean, raw, point_is_required ) {
-
+  // calculate full property names from the key_prefix
-  // the names we use to define the point
+  var properties = [ 'lat', 'lon'].map(function(prop) {
-  var mandatoryProps = [ 'lat', 'lon' ];
+    return key_prefix + '.' + prop;
-
-  // count up how many fields the user actually specified
-  var totalFieldsSpecified = 0;
-  mandatoryProps.forEach( function( prop ){
-    if( raw.hasOwnProperty( key_prefix + '.' + prop ) ){
-      totalFieldsSpecified++;
-    }
   });
 
-  // all fields specified
+  // sanitize the rect property group, this throws an exception if
-  if( 2 === totalFieldsSpecified ) {
+  // the group is not complete
-    // reuse the coord sanitizer and set required:true so we get a fatal error if
+  var point_present;
-    // any one of the coords is not specified.
+  if (point_is_required) {
-    sanitize_coord( key_prefix + '.lat', clean, raw[ key_prefix + '.lat' ], true );
+    point_present = groups.required(raw, properties);
-    sanitize_coord( key_prefix + '.lon', clean, raw[ key_prefix + '.lon' ], true );
+  } else {
-  }
+    point_present = groups.optional(raw, properties);
-  // fields only partially specified
-  else if( totalFieldsSpecified > 0 ){
-    var format1 = 'missing point param \'%s\' requires all of: \'%s\' to be present';
-    throw new Error( util.format( format1, key_prefix, mandatoryProps.join('\',\'') ) );
-  }
-  // fields required, eg. ( totalFieldsSpecified === 0 && bbox_is_required === true )
-  else if( point_is_required ){
-    var format2 = 'missing point param \'%s\' requires all of: \'%s\' to be present';
-    throw new Error( util.format( format2, key_prefix, mandatoryProps.join('\',\'') ) );
   }
+
+  // don't bother checking individual elements if point is not required
+  // and not present
+  if (!point_present) { return; }
+
+  // check each property individually. now that it is known a bbox is present,
+  // all properties must exist, so pass the true flag for coord_is_required
+  properties.forEach(function(prop) {
+    sanitize_coord(prop, clean, raw[prop], true);
+  });
 }
 
 /**

sanitiser/_groups.js

@@ -0,0 +1,57 @@
+var _ = require('lodash');
+
+/*
+ * Specify an object and array of keys to check.
+ * An error will be thrown if at least one, but not all of them are specified
+ *
+ * @param {object} - the object
+ * @param {array} - keys to check
+ *
+ * returns true if all are present, false if none are present, throws an exception otherwise
+ */
+function optional_group(object, keys) {
+  var contained_in_object = _.contains.bind(null, Object.keys(object));
+
+  if (keys.every(contained_in_object)) {
+    return true;
+  } else if (!keys.some(contained_in_object)) {
+    return false;
+  } else {
+    throw new Error(error_message(keys));
+  }
+}
+
+/*
+ * Specify an object and array of keys to check.
+ * An error will be thrown if any of the keys are missing from the object
+ */
+function required_group(object, keys) {
+  var contained_in_object = _.contains.bind(null, Object.keys(object));
+
+  if (keys.every(contained_in_object)) {
+    return true;
+  } else {
+    throw new Error(error_message(keys));
+  }
+}
+
+/*
+ * Create a friendly error message from a list of keys
+ */
+function error_message(keys) {
+  var start = 'parameters ';
+
+  var listStart = keys.slice(0, -1).join(', ');
+  var listEnd = ' and ' + keys[keys.length - 1];
+
+  var adjective = (keys.length > 2) ? 'all' : 'both';
+
+  var end = ' must ' + adjective + ' be specified';
+
+  return start + listStart + listEnd + end;
+}
+
+module.exports = {
+  optional: optional_group,
+  required: required_group
+};

test/unit/controller/place.js

@@ -41,7 +41,7 @@ module.exports.tests.functional_success = function(test, common) {
 
   test('functional success', function(t) {
     var backend = mockBackend( 'client/mget/ok/1', function( cmd ){
-      t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', type: [ 'a' ] } ] } }, 'correct backend command');
+      t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', _type: [ 'a' ] } ] } }, 'correct backend command');
     });
     var controller = setup( backend );
     var res = {
@@ -70,7 +70,7 @@ module.exports.tests.functional_success = function(test, common) {
 module.exports.tests.functional_failure = function(test, common) {
   test('functional failure', function(t) {
     var backend = mockBackend( 'client/mget/fail/1', function( cmd ){
-      t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', type: [ 'b' ] } ] } }, 'correct backend command');
+      t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', _type: [ 'b' ] } ] } }, 'correct backend command');
     });
     var controller = setup( backend );
     var req = { clean: { ids: [ {'id' : 123, types: [ 'b' ] } ] }, errors: [], warnings: [] };

test/unit/run.js

@@ -6,34 +6,35 @@ var tests = [
   require('./controller/index'),
   require('./controller/place'),
   require('./controller/search'),
-  require('./service/mget'),
-  require('./service/search'),
-  require('./sanitiser/_ids'),
-  require('./sanitiser/_flag_bool'),
-  require('./sanitiser/autocomplete'),
-  require('./sanitiser/_sources'),
-  require('./sanitiser/_boundary_country'),
-  require('./sanitiser/search'),
-  require('./sanitiser/_layers'),
-  require('./sanitiser/reverse'),
-  require('./sanitiser/place'),
-  require('./query/search'),
-  require('./query/autocomplete'),
-  require('./query/reverse'),
-  require('./query/defaults'),
-  require('./helper/query_parser'),
   require('./helper/geojsonify'),
-  require('./helper/labelSchema'),
   require('./helper/labelGenerator'),
-  require('./helper/types'),
+  require('./helper/labelSchema'),
+  require('./helper/query_parser'),
   require('./helper/type_mapping'),
-  require('./sanitiser/_geo_common'),
+  require('./helper/types'),
-  require('./middleware/distance'),
-  require('./middleware/confidenceScoreReverse'),
   require('./middleware/confidenceScore'),
-  require('./sanitiser/_size'),
+  require('./middleware/confidenceScoreReverse'),
-  require('./sanitiser/_single_scalar_parameters'),
+  require('./middleware/distance'),
+  require('./query/autocomplete'),
+  require('./query/defaults'),
+  require('./query/reverse'),
+  require('./query/search'),
+  require('./sanitiser/_boundary_country'),
+  require('./sanitiser/_flag_bool'),
+  require('./sanitiser/_geo_common'),
   require('./sanitiser/_geo_reverse'),
+  require('./sanitiser/_groups'),
+  require('./sanitiser/_ids'),
+  require('./sanitiser/_layers'),
+  require('./sanitiser/_single_scalar_parameters'),
+  require('./sanitiser/_size'),
+  require('./sanitiser/_sources'),
+  require('./sanitiser/autocomplete'),
+  require('./sanitiser/place'),
+  require('./sanitiser/reverse'),
+  require('./sanitiser/search'),
+  require('./service/mget'),
+  require('./service/search'),
 ];
 
 tests.map(function(t) {

test/unit/sanitiser/_groups.js

@@ -0,0 +1,75 @@
+var groups = require('../../../sanitiser/_groups');
+
+module.exports.tests = {};
+
+module.exports.tests.optional_group = function(test, common) {
+  test('optional group none present ', function(t) {
+    var object = {};
+    t.doesNotThrow(function() {
+      var present = groups.optional(object, [ 'a', 'b' ]);
+      t.equal(present, false, 'group reported not present');
+    });
+    t.end();
+  });
+
+  test('optional group all present ', function(t) {
+    var object = { 'a': 5, 'b': 9 };
+    t.doesNotThrow(function() {
+      var present = groups.optional(object, [ 'a', 'b' ]);
+      t.equal(present, true, 'group reported present');
+    });
+    t.end();
+  });
+
+  test('optional group some present ', function(t) {
+    var object = { 'b': 9 };
+    t.throws(function() {
+      groups.optional(object, [ 'a', 'b' ]);
+    }, new RegExp('parameters a and b must both be specified'));
+    t.end();
+  });
+
+  test('optional group some present (larger group) ', function(t) {
+    var object = { 'b': 9, 'd': 5 };
+    t.throws(function() {
+      groups.optional(object, [ 'a', 'b', 'c', 'd', 'e' ]);
+    }, new RegExp('parameters a, b, c, d and e must all be specified'));
+    t.end();
+  });
+};
+
+module.exports.tests.required_group = function(test, common) {
+  test('required group none present ', function(t) {
+    var object = {};
+    t.throws(function() {
+      groups.required(object, [ 'a', 'b' ]);
+    }, new RegExp('parameters a and b must both be specified'));
+    t.end();
+  });
+
+  test('required group all present ', function(t) {
+    var object = { 'a': 5, 'b': 9 };
+    t.doesNotThrow(function() {
+      groups.required(object, [ 'a', 'b' ]);
+    });
+    t.end();
+  });
+
+  test('required group some present ', function(t) {
+    var object = { 'b': 9 };
+    t.throws(function() {
+      groups.required(object, [ 'a', 'b' ]);
+    }, new RegExp('parameters a and b must both be specified'));
+    t.end();
+  });
+};
+
+module.exports.all = function (tape, common) {
+  function test(name, testFunction) {
+    return tape('SANTIZE _groups ' + name, testFunction);
+  }
+
+  for( var testCase in module.exports.tests ){
+    module.exports.tests[testCase](test, common);
+  }
+};

test/unit/sanitiser/search.js

@@ -167,7 +167,7 @@ module.exports.tests.sanitize_optional_geo = function(test, common) {
     var req = { query: { text: 'test', 'focus.point.lon': 0 } };
     sanitize(req, function(){
       var expected_lon = 0;
-      t.equal(req.errors[0], 'missing point param \'focus.point\' requires all of: \'lat\',\'lon\' to be present');
+      t.equal(req.errors[0], 'parameters focus.point.lat and focus.point.lon must both be specified');
       t.equal(req.clean['focus.point.lat'], undefined);
       t.equal(req.clean['focus.point.lon'], undefined);
     });
@@ -177,7 +177,7 @@ module.exports.tests.sanitize_optional_geo = function(test, common) {
     var req = { query: { text: 'test', 'focus.point.lat': 0 } };
     sanitize(req, function(){
       var expected_lat = 0;
-      t.equal(req.errors[0], 'missing point param \'focus.point\' requires all of: \'lat\',\'lon\' to be present');
+      t.equal(req.errors[0], 'parameters focus.point.lat and focus.point.lon must both be specified');
       t.equal(req.clean['focus.point.lat'], undefined);
       t.equal(req.clean['focus.point.lon'], undefined);
     });