Merge pull request #904 from pelias/cleanup-reverse-artifacts

Cleanup reverse artifacts

controller/coarse_reverse.js

@@ -103,6 +103,11 @@ function setup(service, should_execute) {
       return next();
     }
 
+    // return a warning to the caller that boundary.circle.radius will be ignored
+    if (!_.isUndefined(req.clean['boundary.circle.radius'])) {
+      req.warnings.push('boundary.circle.radius is not applicable for coarse reverse');
+    }
+
     // because coarse reverse is called when non-coarse reverse didn't return
     //  anything, treat requested layers as if it didn't contain non-coarse layers
     const effective_layers = getEffectiveLayers(req.clean.layers);

query/reverse_defaults.js

@@ -14,7 +14,6 @@ module.exports = _.merge({}, peliasQuery.defaults, {
   'sort:distance:distance_type': 'plane',
 
   'boundary:circle:radius': '1km',
-  'boundary:circle:radius:coarse': '500km',
   'boundary:circle:distance_type': 'plane',
   'boundary:circle:optimize_bbox': 'indexed',
 

sanitizer/_geo_reverse.js

@@ -4,6 +4,8 @@ var defaults = require('../query/reverse_defaults');
 var LAT_LON_IS_REQUIRED = true,
     CIRCLE_IS_REQUIRED = false;
 
+const non_coarse_layers = ['venue', 'address', 'street'];
+
 // validate inputs, convert types and apply defaults
 module.exports = function sanitize( raw, clean ){
 
@@ -29,12 +31,7 @@ module.exports = function sanitize( raw, clean ){
 
     // if no radius was passed, set the default
     if ( _.isUndefined( raw['boundary.circle.radius'] ) ) {
-      // the default is small unless layers other than venue or address were explicitly specified
+      raw['boundary.circle.radius'] = defaults['boundary:circle:radius'];
-      if (clean.layers && clean.layers.length > 0 && !_.includes(clean.layers, 'venue') && !_.includes(clean.layers, 'address')) {
-        raw['boundary.circle.radius'] = defaults['boundary:circle:radius:coarse'];
-      } else {
-        raw['boundary.circle.radius'] = defaults['boundary:circle:radius'];
-      }
     }
 
     // santize the boundary.circle

sanitizer/_geonames_deprecation.js

@@ -0,0 +1,25 @@
+const _ = require('lodash');
+
+/**
+with the release of coarse reverse as a separate thing and ES reverse only
+handling venues, addresses, and streets, geonames make no sense in the reverse context
+**/
+
+function sanitize( raw, clean, opts ) {
+  // error & warning messages
+  const messages = { errors: [], warnings: [] };
+
+  if (_.isEqual(clean.sources, ['geonames']) || _.isEqual(clean.sources, ['gn'])) {
+    messages.errors.push('/reverse does not support geonames');
+
+  } else if (_.includes(clean.sources, 'geonames') || _.includes(clean.sources, 'gn')) {
+    clean.sources = _.without(clean.sources, 'geonames', 'gn');
+    messages.warnings.push('/reverse does not support geonames');
+
+  }
+
+  return messages;
+  
+}
+
+module.exports = sanitize;

sanitizer/reverse.js

@@ -8,6 +8,7 @@ var sanitizeAll = require('../sanitizer/sanitizeAll'),
       sources: require('../sanitizer/_targets')('sources', type_mapping.source_mapping),
       // depends on the layers and sources sanitizers, must be run after them
       sources_and_layers: require('../sanitizer/_sources_and_layers'),
+      geonames_deprecation: require('../sanitizer/_geonames_deprecation'),
       size: require('../sanitizer/_size')(/* use defaults*/),
       private: require('../sanitizer/_flag_bool')('private', false),
       geo_reverse: require('../sanitizer/_geo_reverse'),

test/ciao/reverse/boundary_circle_valid_radius_coarse.coffee

@@ -0,0 +1,39 @@
+
+#> bounding circle
+path: '/v1/reverse?layers=coarse&point.lat=40.744243&point.lon=-73.990342&boundary.circle.radius=999.9'
+
+#? 200 ok
+response.statusCode.should.be.equal 200
+response.should.have.header 'charset', 'utf8'
+response.should.have.header 'content-type', 'application/json; charset=utf-8'
+
+#? valid geocoding block
+should.exist json.geocoding
+should.exist json.geocoding.version
+should.exist json.geocoding.attribution
+should.exist json.geocoding.query
+should.exist json.geocoding.engine
+should.exist json.geocoding.engine.name
+should.exist json.geocoding.engine.author
+should.exist json.geocoding.engine.version
+should.exist json.geocoding.timestamp
+
+#? valid geojson
+json.type.should.be.equal 'FeatureCollection'
+json.features.should.be.instanceof Array
+
+#? expected errors
+should.not.exist json.geocoding.errors
+
+#? expected warnings
+should.exist json.geocoding.warnings
+json.geocoding.warnings.should.eql [ 'boundary.circle.radius is not applicable for coarse reverse' ]
+
+#? inputs
+json.geocoding.query['size'].should.eql 10
+json.geocoding.query['layers'].should.eql 'coarse'
+json.geocoding.query['point.lat'].should.eql 40.744243
+json.geocoding.query['point.lon'].should.eql -73.990342
+json.geocoding.query['boundary.circle.lat'].should.eql 40.744243
+json.geocoding.query['boundary.circle.lon'].should.eql -73.990342
+json.geocoding.query['boundary.circle.radius'].should.eql 999.9

test/unit/controller/coarse_reverse.js

@@ -80,6 +80,80 @@ module.exports.tests.error_conditions = (test, common) => {
 
 };
 
+module.exports.tests.boundary_circle_radius_warnings = (test, common) => {
+  test('defined clean[boundary.circle.radius] should add a warning', (t) => {
+    const service = (req, callback) => {
+      callback(undefined, {});
+    };
+
+    const logger = require('pelias-mock-logger')();
+
+    const controller = proxyquire('../../../controller/coarse_reverse', {
+      'pelias-logger': logger
+    })(service, _.constant(true));
+
+    const req = {
+      warnings: [],
+      clean: {
+        'boundary.circle.radius': 17
+      }
+    };
+
+    const res = { };
+
+    const next = () => {};
+
+    controller(req, res, next);
+
+    const expected = {
+      meta: {},
+      data: []
+    };
+
+    t.deepEquals(req.warnings, ['boundary.circle.radius is not applicable for coarse reverse']);
+    t.deepEquals(res, expected);
+    t.notOk(logger.hasErrorMessages());
+    t.end();
+
+  });
+
+  test('defined clean[boundary.circle.radius] should add a warning', (t) => {
+    const service = (req, callback) => {
+      callback(undefined, {});
+    };
+
+    const logger = require('pelias-mock-logger')();
+
+    const controller = proxyquire('../../../controller/coarse_reverse', {
+      'pelias-logger': logger
+    })(service, _.constant(true));
+
+    const req = {
+      warnings: [],
+      clean: {}
+    };
+
+    const res = { };
+
+    // verify that next was called
+    const next = () => { };
+
+    controller(req, res, next);
+
+    const expected = {
+      meta: {},
+      data: []
+    };
+
+    t.deepEquals(req.warnings, []);
+    t.deepEquals(res, expected);
+    t.notOk(logger.hasErrorMessages());
+    t.end();
+
+  });
+
+};
+
 module.exports.tests.success_conditions = (test, common) => {
   test('service returning results should use first entry for each layer', (t) => {
     t.plan(4);

test/unit/fixture/reverse_null_island.js

@@ -5,7 +5,7 @@ module.exports = {
     'bool': {
       'filter': [{
         'geo_distance': {
-          'distance': '500km',
+          'distance': '3km',
           'distance_type': 'plane',
           'optimize_bbox': 'indexed',
           'center_point': {

test/unit/fixture/reverse_standard.js

@@ -5,7 +5,7 @@ module.exports = {
     'bool': {
       'filter': [{
         'geo_distance': {
-          'distance': '500km',
+          'distance': '3km',
           'distance_type': 'plane',
           'optimize_bbox': 'indexed',
           'center_point': {

test/unit/fixture/reverse_with_boundary_country.js

@@ -15,7 +15,7 @@ module.exports = {
       ],
       'filter': [{
         'geo_distance': {
-          'distance': '500km',
+          'distance': '3km',
           'distance_type': 'plane',
           'optimize_bbox': 'indexed',
           'center_point': {

test/unit/fixture/reverse_with_layer_filtering.js

@@ -6,7 +6,7 @@ module.exports = {
       'filter': [
         {
           'geo_distance': {
-            'distance': '500km',
+            'distance': '3km',
             'distance_type': 'plane',
             'optimize_bbox': 'indexed',
             'center_point': {

test/unit/fixture/reverse_with_layer_filtering_non_coarse_subset.js

@@ -6,7 +6,7 @@ module.exports = {
       'filter': [
         {
           'geo_distance': {
-            'distance': '500km',
+            'distance': '3km',
             'distance_type': 'plane',
             'optimize_bbox': 'indexed',
             'center_point': {

test/unit/fixture/reverse_with_source_filtering.js

@@ -6,7 +6,7 @@ module.exports = {
       'filter': [
         {
           'geo_distance': {
-            'distance': '500km',
+            'distance': '3km',
             'distance_type': 'plane',
             'optimize_bbox': 'indexed',
             'center_point': {

test/unit/query/reverse.js

@@ -16,7 +16,7 @@ module.exports.tests.query = function(test, common) {
       'point.lon': -82.50622,
       'boundary.circle.lat': 29.49136,
       'boundary.circle.lon': -82.50622,
-      'boundary.circle.radius': 500
+      'boundary.circle.radius': 3
     });
 
     var compiled = JSON.parse( JSON.stringify( query ) );
@@ -33,7 +33,7 @@ module.exports.tests.query = function(test, common) {
       'point.lon': 0,
       'boundary.circle.lat': 0,
       'boundary.circle.lon': 0,
-      'boundary.circle.radius': 500
+      'boundary.circle.radius': 3
     });
 
     var compiled = JSON.parse( JSON.stringify( query ) );
@@ -67,7 +67,7 @@ module.exports.tests.query = function(test, common) {
       'point.lon': -82.50622,
       'boundary.circle.lat': 111,
       'boundary.circle.lon': 333,
-      'boundary.circle.radius': 500
+      'boundary.circle.radius': 3
     };
     var query = generate(clean);
     var compiled = JSON.parse( JSON.stringify( query ) );
@@ -102,7 +102,7 @@ module.exports.tests.query = function(test, common) {
       'point.lon': -82.50622,
       'boundary.circle.lat': 29.49136,
       'boundary.circle.lon': -82.50622,
-      'boundary.circle.radius': 500,
+      'boundary.circle.radius': 3,
       'boundary.country': 'ABC'
     });
 
@@ -120,7 +120,7 @@ module.exports.tests.query = function(test, common) {
       'point.lon': -82.50622,
       'boundary.circle.lat': 29.49136,
       'boundary.circle.lon': -82.50622,
-      'boundary.circle.radius': 500,
+      'boundary.circle.radius': 3,
       'sources': ['test']
     });
 
@@ -138,7 +138,7 @@ module.exports.tests.query = function(test, common) {
       'point.lon': -82.50622,
       'boundary.circle.lat': 29.49136,
       'boundary.circle.lon': -82.50622,
-      'boundary.circle.radius': 500,
+      'boundary.circle.radius': 3,
       // only venue, address, and street layers should be retained
       'layers': ['neighbourhood', 'venue', 'locality', 'address', 'region', 'street', 'country']
     });
@@ -158,7 +158,7 @@ module.exports.tests.query = function(test, common) {
       'point.lon': -82.50622,
       'boundary.circle.lat': 29.49136,
       'boundary.circle.lon': -82.50622,
-      'boundary.circle.radius': 500,
+      'boundary.circle.radius': 3,
       // only venue, address, and street layers should be retained
       'layers': ['neighbourhood', 'venue', 'street', 'locality']
     });

test/unit/run.js

@@ -56,6 +56,7 @@ var tests = [
   require('./query/text_parser'),
   require('./sanitizer/_boundary_country'),
   require('./sanitizer/_flag_bool'),
+  require('./sanitizer/_geonames_deprecation'),
   require('./sanitizer/_geonames_warnings'),
   require('./sanitizer/_geo_common'),
   require('./sanitizer/_geo_reverse'),

test/unit/sanitizer/_geo_reverse.js

@@ -1,17 +1,19 @@
-var sanitize = require('../../../sanitizer/_geo_reverse');
+'use strict';
-var defaults = require('../../../query/reverse_defaults');
+
+const sanitize = require('../../../sanitizer/_geo_reverse');
+const defaults = require('../../../query/reverse_defaults');
 
 module.exports.tests = {};
 
-module.exports.tests.sanitize_boundary_country = function(test, common) {
+module.exports.tests.warning_situations = (test, common) => {
-  test('raw with boundary.circle.lat should add warning about ignored boundary.circle', function(t) {
+  test('raw with boundary.circle.lat should add warning about ignored boundary.circle', (t) => {
-    var raw = {
+    const raw = {
       'point.lat': '12.121212',
       'point.lon': '21.212121',
       'boundary.circle.lat': '13.131313'
     };
-    var clean = {};
+    const clean = {};
-    var errorsAndWarnings = sanitize(raw, clean);
+    const errorsAndWarnings = sanitize(raw, clean);
 
     t.equals(clean['boundary.circle.lat'], 12.121212, 'should be set to point.lat');
     t.deepEquals(errorsAndWarnings, {
@@ -19,16 +21,17 @@ module.exports.tests.sanitize_boundary_country = function(test, common) {
       warnings: ['boundary.circle.lat/boundary.circle.lon are currently unsupported']
     }, 'no warnings/errors');
     t.end();
+
   });
 
-  test('raw with boundary.circle.lon should add warning about ignored boundary.circle', function(t) {
+  test('raw with boundary.circle.lon should add warning about ignored boundary.circle', (t) => {
-    var raw = {
+    const raw = {
       'point.lat': '12.121212',
       'point.lon': '21.212121',
       'boundary.circle.lon': '31.313131'
     };
-    var clean = {};
+    const clean = {};
-    var errorsAndWarnings = sanitize(raw, clean);
+    const errorsAndWarnings = sanitize(raw, clean);
 
     t.equals(clean['boundary.circle.lon'], 21.212121, 'should be set to point.lon');
     t.deepEquals(errorsAndWarnings, {
@@ -36,16 +39,17 @@ module.exports.tests.sanitize_boundary_country = function(test, common) {
       warnings: ['boundary.circle.lat/boundary.circle.lon are currently unsupported']
     }, 'no warnings/errors');
     t.end();
+
   });
 
-  test('raw with boundary.circle.radius shouldn\'t add warning about ignored boundary.circle', function(t) {
+  test('raw with boundary.circle.radius shouldn\'t add warning about ignored boundary.circle', (t) => {
-    var raw = {
+    const raw = {
       'point.lat': '12.121212',
       'point.lon': '21.212121',
       'boundary.circle.radius': '17'
     };
-    var clean = {};
+    const clean = {};
-    var errorsAndWarnings = sanitize(raw, clean);
+    const errorsAndWarnings = sanitize(raw, clean);
 
     // t.equals(clean['boundary.circle.radius'], 12.121212, 'should be set to point.lat')
     t.deepEquals(errorsAndWarnings, {
@@ -53,85 +57,61 @@ module.exports.tests.sanitize_boundary_country = function(test, common) {
       warnings: []
     }, 'no warnings/errors');
     t.end();
+
   });
 
-  test('boundary.circle.lat/lon should be overridden with point.lat/lon', function(t) {
+};
-    var raw = {
+
+module.exports.tests.success_conditions = (test, common) => {
+  test('boundary.circle.radius specified in request should override default', (t) => {
+    const raw = {
       'point.lat': '12.121212',
       'point.lon': '21.212121',
-      'boundary.circle.lat': '13.131313',
+      'boundary.circle.radius': '3248732857km' // this will never be the default
-      'boundary.circle.lon': '31.313131'
     };
-    var clean = {};
+    const clean = {};
-    var errorsAndWarnings = sanitize(raw, clean);
+    const errorsAndWarnings = sanitize(raw, clean);
 
-    t.equals(raw['boundary.circle.lat'], 12.121212, 'should be set to point.lat');
+    t.equals(raw['boundary.circle.lat'], 12.121212);
-    t.equals(raw['boundary.circle.lon'], 21.212121, 'should be set to point.lon');
+    t.equals(raw['boundary.circle.lon'], 21.212121);
-    t.equals(clean['boundary.circle.lat'], 12.121212, 'should be set to point.lat');
+    t.equals(raw['boundary.circle.radius'], '3248732857km');
-    t.equals(clean['boundary.circle.lon'], 21.212121, 'should be set to point.lon');
+    t.equals(clean['boundary.circle.lat'], 12.121212);
+    t.equals(clean['boundary.circle.lon'], 21.212121);
+    t.equals(clean['boundary.circle.radius'], 3248732857.0);
+
+    t.deepEquals(errorsAndWarnings, { errors: [], warnings: [] });
     t.end();
+
   });
 
-  test('no boundary.circle.radius and no layers supplied should be set to default', function(t) {
+  test('boundary.circle.radius not specified should use default', (t) => {
-    var raw = {
+    const raw = {
       'point.lat': '12.121212',
       'point.lon': '21.212121'
     };
-    var clean = {};
+    const clean = {};
-    var errorsAndWarnings = sanitize(raw, clean);
+    const errorsAndWarnings = sanitize(raw, clean);
 
+    t.equals(raw['boundary.circle.lat'], 12.121212);
+    t.equals(raw['boundary.circle.lon'], 21.212121);
     t.equals(raw['boundary.circle.radius'], defaults['boundary:circle:radius'], 'should be from defaults');
+    t.equals(clean['boundary.circle.lat'], 12.121212);
+    t.equals(clean['boundary.circle.lon'], 21.212121);
     t.equals(clean['boundary.circle.radius'], parseFloat(defaults['boundary:circle:radius']), 'should be same as raw');
-    t.end();
-  });
 
-  test('no boundary.circle.radius and coarse layers supplied should be set to coarse default', function(t) {
+    t.deepEquals(errorsAndWarnings, { errors: [], warnings: [] });
-    var raw = {
-      'point.lat': '12.121212',
-      'point.lon': '21.212121'
-    };
-    var clean = { layers: 'coarse' };
-    var errorsAndWarnings = sanitize(raw, clean);
-
-    t.equals(raw['boundary.circle.radius'], defaults['boundary:circle:radius:coarse'], 'should be from defaults');
-    t.equals(clean['boundary.circle.radius'], parseFloat(defaults['boundary:circle:radius:coarse']), 'should be same as raw');
     t.end();
-  });
-
-  test('no boundary.circle.radius and coarse layer supplied should be set to coarse default', function(t) {
-    var raw = {
-      'point.lat': '12.121212',
-      'point.lon': '21.212121'
-    };
-    var clean = { layers: 'locality' };
-    var errorsAndWarnings = sanitize(raw, clean);
 
-    t.equals(raw['boundary.circle.radius'], defaults['boundary:circle:radius:coarse'], 'should be from defaults');
-    t.equals(clean['boundary.circle.radius'], parseFloat(defaults['boundary:circle:radius:coarse']), 'should be same as raw');
-    t.end();
   });
 
-  test('explicit boundary.circle.radius should be used instead of default', function(t) {
-    var raw = {
-      'point.lat': '12.121212',
-      'point.lon': '21.212121',
-      'boundary.circle.radius': '3248732857km' // this will never be the default
-    };
-    var clean = {};
-    var errorsAndWarnings = sanitize(raw, clean);
-
-    t.equals(raw['boundary.circle.radius'], '3248732857km', 'should be parsed float');
-    t.equals(clean['boundary.circle.radius'], 3248732857.0, 'should be copied from raw');
-    t.end();
-  });
 };
 
-module.exports.all = function (tape, common) {
+module.exports.all = (tape, common) => {
   function test(name, testFunction) {
-    return tape('SANTIZE _geo_reverse ' + name, testFunction);
+    return tape(`SANTIZE _geo_reverse ${name}`, testFunction);
   }
 
-  for( var testCase in module.exports.tests ){
+  for( const testCase in module.exports.tests ){
     module.exports.tests[testCase](test, common);
   }
 };

test/unit/sanitizer/_geonames_deprecation.js

@@ -0,0 +1,82 @@
+const geonames_deprecation = require('../../../sanitizer/_geonames_deprecation');
+
+module.exports.tests = {};
+
+module.exports.tests.no_warnings_or_errors_conditions = (test, common) => {
+  test('undefined sources should add neither warnings nor errors', (t) => {
+    const clean = {};
+
+    const messages = geonames_deprecation(undefined, clean);
+
+    t.deepEquals(clean, {});
+    t.deepEquals(messages, { errors: [], warnings: [] });
+    t.end();
+
+  });
+
+  test('geonames/gn not in sources should add neither warnings nor errors', (t) => {
+    const clean = {
+      sources: ['source 1', 'source 2'],
+    };
+
+    const messages = geonames_deprecation(undefined, clean);
+
+    t.deepEquals(clean.sources, ['source 1', 'source 2']);
+    t.deepEquals(messages, { errors: [], warnings: [] });
+    t.end();
+
+  });
+
+};
+
+module.exports.tests.error_conditions = (test, common) => {
+  test('only geonames in sources should not modify clean.sources and add error message', (t) => {
+    ['gn', 'geonames'].forEach((sources) => {
+      const clean = {
+        sources: [sources]
+      };
+
+      const messages = geonames_deprecation(undefined, clean);
+
+      t.deepEquals(clean.sources, [sources]);
+      t.deepEquals(messages.errors, ['/reverse does not support geonames']);
+      t.deepEquals(messages.warnings, []);
+
+    });
+
+    t.end();
+
+  });
+
+};
+
+module.exports.tests.warning_conditions = (test, common) => {
+  test('only geonames in sources should not modify clean.sources and add error message', (t) => {
+    ['gn', 'geonames'].forEach((sources) => {
+      const clean = {
+        sources: ['another source', sources, 'yet another source']
+      };
+
+      const messages = geonames_deprecation(undefined, clean);
+
+      t.deepEquals(clean.sources, ['another source', 'yet another source']);
+      t.deepEquals(messages.errors, []);
+      t.deepEquals(messages.warnings, ['/reverse does not support geonames']);
+
+    });
+
+    t.end();
+
+  });
+
+};
+
+module.exports.all = (tape, common) => {
+  function test(name, testFunction) {
+    return tape(`SANTIZE _geonames_deprecation ${name}`, testFunction);
+  }
+
+  for( var testCase in module.exports.tests ){
+    module.exports.tests[testCase](test, common);
+  }
+};

test/unit/sanitizer/nearby.js

@@ -31,7 +31,8 @@ module.exports.tests.interface = function(test, common) {
 module.exports.tests.sanitizers = function(test, common) {
   test('check sanitizer list', function (t) {
     var expected = ['singleScalarParameters', 'quattroshapes_deprecation', 'layers',
-      'sources', 'sources_and_layers', 'size', 'private', 'geo_reverse', 'boundary_country', 'categories'];
+      'sources', 'sources_and_layers', 'geonames_deprecation', 'size', 'private',
+      'geo_reverse', 'boundary_country', 'categories'];
     t.deepEqual(Object.keys(nearby.sanitizer_list), expected);
     t.end();
   });

test/unit/sanitizer/reverse.js

@@ -37,7 +37,8 @@ module.exports.tests.interface = function(test, common) {
 module.exports.tests.sanitizers = function(test, common) {
   test('check sanitizer list', function (t) {
     var expected = ['singleScalarParameters', 'quattroshapes_deprecation', 'layers',
-      'sources', 'sources_and_layers', 'size', 'private', 'geo_reverse', 'boundary_country'];
+      'sources', 'sources_and_layers', 'geonames_deprecation', 'size', 'private',
+      'geo_reverse', 'boundary_country'];
     t.deepEqual(Object.keys(reverse.sanitizer_list), expected);
     t.end();
   });