Refactor geojsonify code and add ability to pass in clean query params

helper/geojsonify.js

@@ -1,68 +1,18 @@
 
-var GeoJSON = require('geojson'),
+var GeoJSON = require('geojson');
-    extent = require('geojson-extent'),
+var extent = require('geojson-extent');
-    labelGenerator = require('./labelGenerator'),
+var labelGenerator = require('./labelGenerator');
-    logger = require('pelias-logger').get('api'),
+var logger = require('pelias-logger').get('api');
-    type_mapping = require('./type_mapping'),
+var type_mapping = require('./type_mapping');
-    Document = require('pelias-model').Document,
+var _ = require('lodash');
-    _ = require('lodash');
+var addDetails = require('./geojsonify_place_details');
+var addMetaData = require('./geojsonify_meta_data');
 
-// Properties to be copied
+function geojsonifyPlaces( params, docs ){
-// If a property is identified as a single string, assume it should be presented as a string in response
-// If something other than string is desired, use the following structure: { name: 'category', type: 'array' }
-var DETAILS_PROPS = [
-  'housenumber',
-  'street',
-  'postalcode',
-  { name: 'confidence', type: 'default' },
-  'distance',
-  'country',
-  'country_gid',
-  'country_a',
-  'macroregion',
-  'macroregion_gid',
-  'macroregion_a',
-  'region',
-  'region_gid',
-  'region_a',
-  'macrocounty',
-  'macrocounty_gid',
-  'macrocounty_a',
-  'county',
-  'county_gid',
-  'county_a',
-  'localadmin',
-  'localadmin_gid',
-  'localadmin_a',
-  'locality',
-  'locality_gid',
-  'locality_a',
-  'borough',
-  'borough_gid',
-  'borough_a',
-  'neighbourhood',
-  'neighbourhood_gid',
-  { name: 'bounding_box', type: 'default' },
-  { name: 'category', type: 'array' }
-];
-
-function lookupSource(src) {
-  return src.source;
-}
-
-function lookupSourceId(src) {
-  return src.source_id;
-}
-
-function lookupLayer(src) {
-  return src.layer;
-}
-
-function geojsonifyPlaces( docs ){
 
   // flatten & expand data for geojson conversion
   var geodata = docs
-    .map(geojsonifyPlace)
+    .map(geojsonifyPlace.bind(null, params))
     .filter( function( doc ){
       return !!doc;
     });
@@ -85,7 +35,7 @@ function geojsonifyPlaces( docs ){
   return geojson;
 }
 
-function geojsonifyPlace(place) {
+function geojsonifyPlace(params, place) {
 
   // something went very wrong
   if( !place || !place.hasOwnProperty( 'center_point' ) ) {
@@ -95,7 +45,8 @@ function geojsonifyPlace(place) {
   var output = {};
 
   addMetaData(place, output);
-  addDetails(place, output);
+  addName(place, output);
+  addDetails(params, place, output);
   addLabel(place, output);
 
 
@@ -108,17 +59,15 @@ function geojsonifyPlace(place) {
 }
 
 /**
- * Add details properties
+ * Validate and add name property
  *
  * @param {object} src
  * @param {object} dst
  */
-function addDetails(src, dst) {
+function addName(src, dst) {
   // map name
   if( !src.name || !src.name.default ) { return warning(src); }
   dst.name = src.name.default;
-
-  copyProperties(src, DETAILS_PROPS, dst);
 }
 
 /**
@@ -208,104 +157,6 @@ function computeBBox(geojson, geojsonExtentPoints) {
   }
 }
 
-/**
- * Copy specified properties from source to dest.
- * Ignore missing properties.
- *
- * @param {object} source
- * @param {[]} props
- * @param {object} dst
- */
-function copyProperties( source, props, dst ) {
-  props.forEach( function ( prop ) {
-
-    var property = {
-      name: prop.name || prop,
-      type: prop.type || 'string'
-    };
-
-    var value = null;
-    if ( source.hasOwnProperty( property.name ) ) {
-
-      switch (property.type) {
-        case 'string':
-          value = getStringValue(source[property.name]);
-          break;
-        case 'array':
-          value = getArrayValue(source[property.name]);
-          break;
-        // default behavior is to copy property exactly as is
-        default:
-          value = source[property.name];
-      }
-
-      if (_.isNumber(value) || (value && !_.isEmpty(value))) {
-        dst[property.name] = value;
-      }
-    }
-  });
-}
-
-function getStringValue(property) {
-  // isEmpty check works for all types of values: strings, arrays, objects
-  if (_.isEmpty(property)) {
-    return '';
-  }
-
-  if (_.isString(property)) {
-    return property;
-  }
-
-  // array value, take first item in array (at this time only used for admin values)
-  if (_.isArray(property)) {
-    return property[0];
-  }
-
-  return _.toString(property);
-}
-
-
-function getArrayValue(property) {
-  // isEmpty check works for all types of values: strings, arrays, objects
-  if (_.isEmpty(property)) {
-    return '';
-  }
-
-  if (_.isArray(property)) {
-    return property;
-  }
-
-  return [property];
-}
-
-/**
- * Create a gid from a document
- * @TODO modify all importers to create separate source and layer fields to remove mapping
- *
- * @param {object} src
- */
-function makeGid(src) {
-  var doc = new Document(lookupSource(src), lookupLayer(src), src._id);
-  return doc.getGid();
-}
-
-/**
- * Determine and set place id, type, and source
- *
- * @param {object} src
- * @param {object} dst
- */
-function addMetaData(src, dst) {
-  dst.id = src._id;
-  dst.gid = makeGid(src);
-  dst.layer = lookupLayer(src);
-  dst.source = lookupSource(src);
-  dst.source_id = lookupSourceId(src);
-  if (src.hasOwnProperty('bounding_box')) {
-    dst.bounding_box = src.bounding_box;
-  }
-}
-
 /**
  * emit a warning if the doc format is invalid
  *
@@ -317,4 +168,4 @@ function warning( doc ) {
 }
 
 
-module.exports.search = geojsonifyPlaces;
+module.exports = geojsonifyPlaces;

helper/geojsonify_meta_data.js

@@ -0,0 +1,42 @@
+var Document = require('pelias-model').Document;
+
+/**
+ * Determine and set place id, type, and source
+ *
+ * @param {object} src
+ * @param {object} dst
+ */
+function addMetaData(src, dst) {
+  dst.id = src._id;
+  dst.gid = makeGid(src);
+  dst.layer = lookupLayer(src);
+  dst.source = lookupSource(src);
+  dst.source_id = lookupSourceId(src);
+  if (src.hasOwnProperty('bounding_box')) {
+    dst.bounding_box = src.bounding_box;
+  }
+}
+
+/**
+ * Create a gid from a document
+ *
+ * @param {object} src
+ */
+function makeGid(src) {
+  var doc = new Document(lookupSource(src), lookupLayer(src), src._id);
+  return doc.getGid();
+}
+
+function lookupSource(src) {
+  return src.source;
+}
+
+function lookupSourceId(src) {
+  return src.source_id;
+}
+
+function lookupLayer(src) {
+  return src.layer;
+}
+
+module.exports = addMetaData;

helper/geojsonify_place_details.js

@@ -0,0 +1,133 @@
+var _ = require('lodash');
+
+// Properties to be copied
+// If a property is identified as a single string, assume it should be presented as a string in response
+// If something other than string is desired, use the following structure: { name: 'category', type: 'array' }
+var DETAILS_PROPS = [
+  { name: 'housenumber',       type: 'string' },
+  { name: 'street',            type: 'string' },
+  { name: 'postalcode',        type: 'string' },
+  { name: 'confidence',        type: 'default' },
+  { name: 'distance',          type: 'default' },
+  { name: 'country',           type: 'string' },
+  { name: 'country_gid',       type: 'string' },
+  { name: 'country_a',         type: 'string' },
+  { name: 'macroregion',       type: 'string' },
+  { name: 'macroregion_gid',   type: 'string' },
+  { name: 'macroregion_a',     type: 'string' },
+  { name: 'region',            type: 'string' },
+  { name: 'region_gid',        type: 'string' },
+  { name: 'region_a',          type: 'string' },
+  { name: 'macrocounty',       type: 'string' },
+  { name: 'macrocounty_gid',   type: 'string' },
+  { name: 'macrocounty_a',     type: 'string' },
+  { name: 'county',            type: 'string' },
+  { name: 'county_gid',        type: 'string' },
+  { name: 'county_a',          type: 'string' },
+  { name: 'localadmin',        type: 'string' },
+  { name: 'localadmin_gid',    type: 'string' },
+  { name: 'localadmin_a',      type: 'string' },
+  { name: 'locality',          type: 'string' },
+  { name: 'locality_gid',      type: 'string' },
+  { name: 'locality_a',        type: 'string' },
+  { name: 'borough',           type: 'string' },
+  { name: 'borough_gid',       type: 'string' },
+  { name: 'borough_a',         type: 'string' },
+  { name: 'neighbourhood',     type: 'string' },
+  { name: 'neighbourhood_gid', type: 'string' },
+  { name: 'bounding_box',      type: 'default' },
+  { name: 'category',          type: 'array',     condition: checkCategoryParam }
+];
+
+function checkCategoryParam(params) {
+  return _.isObject(params) && params.hasOwnProperty('categories');
+}
+
+/**
+ * Add details properties
+ *
+ * @param {object} params clean query params
+ * @param {object} src
+ * @param {object} dst
+ */
+function addDetails(params, src, dst) {
+  copyProperties(params, src, DETAILS_PROPS, dst);
+}
+
+/**
+ * Copy specified properties from source to dest.
+ * Ignore missing properties.
+ *
+ * @param {object} params clean query params
+ * @param {object} source
+ * @param {[]} props
+ * @param {object} dst
+ */
+function copyProperties( params, source, props, dst ) {
+  props.forEach( function ( prop ) {
+
+    // if condition isn't met, just return without setting the property
+    if (_.isFunction(prop.condition) && !prop.condition(params)) {
+      return;
+    }
+
+    var property = {
+      name: prop.name || prop,
+      type: prop.type || 'default'
+    };
+
+    var value = null;
+    if ( source.hasOwnProperty( property.name ) ) {
+
+      switch (property.type) {
+        case 'string':
+          value = getStringValue(source[property.name]);
+          break;
+        case 'array':
+          value = getArrayValue(source[property.name]);
+          break;
+        // default behavior is to copy property exactly as is
+        default:
+          value = source[property.name];
+      }
+
+      if (_.isNumber(value) || (value && !_.isEmpty(value))) {
+        dst[property.name] = value;
+      }
+    }
+  });
+}
+
+function getStringValue(property) {
+  // isEmpty check works for all types of values: strings, arrays, objects
+  if (_.isEmpty(property)) {
+    return '';
+  }
+
+  if (_.isString(property)) {
+    return property;
+  }
+
+  // array value, take first item in array (at this time only used for admin values)
+  if (_.isArray(property)) {
+    return property[0];
+  }
+
+  return _.toString(property);
+}
+
+
+function getArrayValue(property) {
+  // isEmpty check works for all types of values: strings, arrays, objects
+  if (_.isEmpty(property)) {
+    return '';
+  }
+
+  if (_.isArray(property)) {
+    return property;
+  }
+
+  return [property];
+}
+
+module.exports = addDetails;

middleware/geocodeJSON.js

@@ -1,6 +1,6 @@
 var url = require('url');
 var extend = require('extend');
-var geojsonify = require('../helper/geojsonify').search;
+var geojsonify = require('../helper/geojsonify');
 
 /**
  * Returns a middleware function that converts elasticsearch
@@ -72,7 +72,7 @@ function convertToGeocodeJSON(req, res, next, opts) {
   res.body.geocoding.timestamp = new Date().getTime();
 
   // convert docs to geojson and merge with geocoding block
-  extend(res.body, geojsonify(res.data || []));
+  extend(res.body, geojsonify(req.clean, res.data || []));
 
   next();
 }

sanitiser/nearby.js

@@ -0,0 +1,29 @@
+
+var type_mapping = require('../helper/type_mapping');
+var sanitizeAll = require('../sanitiser/sanitizeAll'),
+    sanitizers = {
+      quattroshapes_deprecation: require('../sanitiser/_deprecate_quattroshapes'),
+      singleScalarParameters: require('../sanitiser/_single_scalar_parameters'),
+      layers: require('../sanitiser/_targets')('layers', type_mapping.layer_mapping),
+      sources: require('../sanitiser/_targets')('sources', type_mapping.source_mapping),
+      // depends on the layers and sources sanitisers, must be run after them
+      sources_and_layers: require('../sanitiser/_sources_and_layers'),
+      size: require('../sanitiser/_size')(/* use defaults*/),
+      private: require('../sanitiser/_flag_bool')('private', false),
+      geo_reverse: require('../sanitiser/_geo_reverse'),
+      boundary_country: require('../sanitiser/_boundary_country'),
+      categories: require('../sanitiser/_categories')
+    };
+
+var sanitize = function(req, cb) { sanitizeAll(req, sanitizers, cb); };
+
+// export sanitize for testing
+module.exports.sanitize = sanitize;
+module.exports.sanitiser_list = sanitizers;
+
+// middleware
+module.exports.middleware = function( req, res, next ){
+  sanitize( req, function( err, clean ){
+    next();
+  });
+};

test/unit/helper/geojsonify.js

@@ -4,9 +4,9 @@ var geojsonify = require('../../../helper/geojsonify');
 module.exports.tests = {};
 
 module.exports.tests.interface = function(test, common) {
-  test('valid interface .search()', function(t) {
+  test('valid interface', function(t) {
-    t.equal(typeof geojsonify.search, 'function', 'search is a function');
+    t.equal(typeof geojsonify, 'function', 'search is a function');
-    t.equal(geojsonify.search.length, 1, 'accepts x arguments');
+    t.equal(geojsonify.length, 2, 'accepts x arguments');
     t.end();
   });
 };
@@ -30,14 +30,14 @@ module.exports.tests.earth = function(test, common) {
 
   test('earth', function(t) {
     t.doesNotThrow(function(){
-      geojsonify.search( earth );
+      geojsonify( {}, earth );
     });
     t.end();
   });
 
 };
 
-module.exports.tests.search = function(test, common) {
+module.exports.tests.geojsonify = function(test, common) {
 
   var input = [
     {
@@ -222,8 +222,8 @@ module.exports.tests.search = function(test, common) {
     ]
   };
 
-  test('geojsonify.search(doc)', function(t) {
+  test('geojsonify(doc)', function(t) {
-    var json = geojsonify.search( input );
+    var json = geojsonify( {categories: 'foo'}, input );
 
     t.deepEqual(json, expected, 'all docs mapped');
     t.end();
@@ -370,7 +370,67 @@ module.exports.tests.search = function(test, common) {
       ]
     };
 
-    var json = geojsonify.search( input );
+    var json = geojsonify( {categories: 'foo'}, input );
+
+    t.deepEqual(json, expected, 'all wanted properties exposed');
+    t.end();
+  });
+};
+
+module.exports.tests.categories = function (test, common) {
+  test('only set category if categories filter was used', function (t) {
+    var input = [
+      {
+        '_id': '85816607',
+        'bounding_box': {
+          'min_lat': 40.6514712164,
+          'max_lat': 40.6737320588,
+          'min_lon': -73.8967895508,
+          'max_lon': -73.8665771484
+        },
+        'source': 'whosonfirst',
+        'layer': 'neighbourhood',
+        'center_point': {
+          'lon': -73.881319,
+          'lat': 40.663303
+        },
+        'name': {
+          'default': 'East New York'
+        },
+        'source_id': '85816607',
+        'category': ['government']
+      }
+    ];
+
+    var expected = {
+      'type': 'FeatureCollection',
+      'bbox': [-73.8967895508, 40.6514712164, -73.8665771484, 40.6737320588],
+      'features': [
+        {
+          'type': 'Feature',
+          'properties': {
+            'id': '85816607',
+            'gid': 'whosonfirst:neighbourhood:85816607',
+            'layer': 'neighbourhood',
+            'source': 'whosonfirst',
+            'source_id': '85816607',
+            'name': 'East New York',
+            'category': ['government'],
+            'label': 'East New York'
+          },
+          'bbox': [-73.8967895508,40.6514712164,-73.8665771484,40.6737320588],
+          'geometry': {
+            'type': 'Point',
+            'coordinates': [
+              -73.881319,
+              40.663303
+            ]
+          }
+        }
+      ]
+    };
+
+    var json = geojsonify( {categories: 'foo'}, input );
 
     t.deepEqual(json, expected, 'all wanted properties exposed');
     t.end();

test/unit/sanitiser/nearby.js

@@ -0,0 +1,204 @@
+
+// @todo: refactor this test, it's pretty messy, brittle and hard to follow
+
+var reverse  = require('../../../sanitiser/reverse'),
+    sanitize = reverse.sanitize,
+    middleware = reverse.middleware,
+    defaults = require('../../../query/reverse_defaults'),
+    defaultError = 'missing param \'lat\'',
+    defaultClean =  { 'point.lat': 0,
+                      'point.lon': 0,
+                      'boundary.circle.lat': 0,
+                      'boundary.circle.lon': 0,
+                      'boundary.circle.radius': parseFloat(defaults['boundary:circle:radius']),
+                      size: 10,
+                      private: false
+                    };
+
+// these are the default values you would expect when no input params are specified.
+// @todo: why is this different from $defaultClean?
+var emptyClean = { private: false, size: 10 };
+
+module.exports.tests = {};
+
+module.exports.tests.interface = function(test, common) {
+  test('sanitize interface', function(t) {
+    t.equal(typeof sanitize, 'function', 'sanitize is a function');
+    t.equal(sanitize.length, 2, 'sanitize interface');
+    t.end();
+  });
+  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.end();
+  });
+};
+
+module.exports.tests.sanitisers = function(test, common) {
+  test('check sanitiser list', function (t) {
+    var expected = ['quattroshapes_deprecation', 'singleScalarParameters', 'layers',
+      'sources', 'sources_and_layers', 'size', 'private', 'geo_reverse', 'boundary_country', 'categories'];
+    t.deepEqual(Object.keys(reverse.sanitiser_list), expected);
+    t.end();
+  });
+};
+
+module.exports.tests.sanitize_lat = function(test, common) {
+  var lats = {
+    invalid: [],
+    valid: [ 0, 45, 90, -0, '0', '45', '90', -181, -120, -91, 91, 120, 181 ],
+    missing: ['', undefined, null]
+  };
+  test('invalid lat', function(t) {
+    lats.invalid.forEach( function( lat ){
+      var req = { query: { 'point.lat': lat, 'point.lon': 0 } };
+      sanitize(req, function(){
+        t.equal(req.errors[0], 'invalid param \'point.lat\': must be >-90 and <90', lat + ' is an invalid latitude');
+        t.deepEqual(req.clean, emptyClean, 'clean only has default values set');
+      });
+    });
+    t.end();
+  });
+  test('valid lat', function(t) {
+    lats.valid.forEach( function( lat ){
+      var req = { query: { 'point.lat': lat, 'point.lon': 0 } };
+      sanitize(req, function(){
+        var expected_lat = parseFloat( lat );
+        t.deepEqual(req.errors, [], 'no errors');
+        t.equal(req.clean['point.lat'], expected_lat, 'clean set correctly (' + lat + ')');
+      });
+    });
+    t.end();
+  });
+  test('missing lat', function(t) {
+    lats.missing.forEach( function( lat ){
+      var req = { query: { 'point.lat': lat, 'point.lon': 0 } };
+      sanitize(req, function(){
+        t.equal(req.errors[0], 'missing param \'point.lat\'', 'latitude is a required field');
+        t.deepEqual(req.clean, emptyClean, 'clean only has default values set');
+      });
+    });
+    t.end();
+  });
+};
+
+module.exports.tests.sanitize_lon = function(test, common) {
+  var lons = {
+    valid: [ -360, -181, 181, -180, -1, -0, 0, 45, 90, '-180', '0', '180' ],
+    missing: ['', undefined, null]
+  };
+  test('valid lon', function(t) {
+    lons.valid.forEach( function( lon ){
+      var req = { query: { 'point.lat': 0, 'point.lon': lon } };
+      sanitize(req, function(){
+        var expected_lon = parseFloat( lon );
+        t.deepEqual(req.errors, [], 'no errors');
+        t.equal(req.clean['point.lon'], expected_lon, 'clean set correctly (' + lon + ')');
+      });
+    });
+    t.end();
+  });
+  test('missing lon', function(t) {
+    lons.missing.forEach( function( lon ){
+      var req = { query: { 'point.lat': 0, 'point.lon': lon } };
+
+      // @todo: why is lat set?
+      var expected = { 'point.lat': 0, private: false, size: 10 };
+      sanitize(req, function(){
+        t.equal(req.errors[0], 'missing param \'point.lon\'', 'longitude is a required field');
+        t.deepEqual(req.clean, expected, 'clean only has default values set');
+      });
+    });
+    t.end();
+  });
+};
+
+module.exports.tests.sanitize_size = function(test, common) {
+  test('invalid size value', function(t) {
+    var req = { query: { size: 'a', 'point.lat': 0, 'point.lon': 0 } };
+    sanitize(req, function(){
+      t.equal(req.clean.size, 10, 'default size set');
+      t.end();
+    });
+  });
+  test('below min size value', function(t) {
+    var req = { query: { size: -100, 'point.lat': 0, 'point.lon': 0 } };
+    sanitize(req, function(){
+      t.equal(req.clean.size, 1, 'min size set');
+      t.end();
+    });
+  });
+  test('above max size value', function(t) {
+    var req = { query: { size: 9999, 'point.lat': 0, 'point.lon': 0 } };
+    sanitize(req, function(){
+      t.equal(req.clean.size, 40, 'max size set');
+      t.end();
+    });
+  });
+};
+
+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: { 'point.lat': 0, 'point.lon': 0, 'private': value } };
+      sanitize(req, function(){
+        t.equal(req.clean.private, false, 'default private set (to false)');
+        t.end();
+      });
+    });
+  });
+
+  var valid_values = ['true', true, 1, '1'];
+  valid_values.forEach(function(value) {
+    test('valid private param ' + value, function(t) {
+      var req = { query: { 'point.lat': 0, 'point.lon': 0, 'private': value } };
+      sanitize(req, function(){
+        t.equal(req.clean.private, true, 'private set to true');
+        t.end();
+      });
+    });
+  });
+
+  var valid_false_values = ['false', false, 0];
+  valid_false_values.forEach(function(value) {
+    test('test setting false explicitly ' + value, function(t) {
+      var req = { query: { 'point.lat': 0, 'point.lon': 0, 'private': value } };
+      sanitize(req, function(){
+        t.equal(req.clean.private, false, 'private set to false');
+        t.end();
+      });
+    });
+  });
+
+  test('test default behavior', function(t) {
+    var req = { query: { 'point.lat': 0, 'point.lon': 0 } };
+    sanitize(req, function(){
+      t.equal(req.clean.private, false, 'private set to false');
+      t.end();
+    });
+  });
+};
+
+module.exports.tests.middleware_success = function(test, common) {
+  test('middleware success', function(t) {
+    var req = { query: { 'point.lat': 0, 'point.lon': 0 }};
+    var next = function(){
+      t.deepEqual(req.errors, [], 'no error message set');
+      t.deepEqual(req.clean, defaultClean);
+      t.end();
+    };
+    middleware( req, undefined, next );
+  });
+};
+
+module.exports.all = function (tape, common) {
+
+  function test(name, testFunction) {
+    return tape('SANTIZE /reverse ' + name, testFunction);
+  }
+
+  for( var testCase in module.exports.tests ){
+    module.exports.tests[testCase](test, common);
+  }
+};