added conditional fallback to addressit

controller/predicates/is_addressit_parse.js

@@ -0,0 +1,4 @@
+const _ = require('lodash');
+
+// returns true iff req.clean.parser is addressit
+module.exports = (req, res) => _.get(req, 'clean.parser') === 'addressit';

controller/predicates/is_admin_only_analysis.js

@@ -6,7 +6,7 @@ module.exports = (request, response) => {
   }
 
   // return true only if all non-admin properties of parsed_text are empty
-  return ['number', 'street', 'query', 'category'].every((prop) => {
+  return ['number', 'street', 'query', 'category', 'postalcode'].every((prop) => {
     return _.isEmpty(request.clean.parsed_text[prop]);
   });
 

routes/v1.js

@@ -11,7 +11,7 @@ var sanitizers = {
   autocomplete: require('../sanitizer/autocomplete'),
   place: require('../sanitizer/place'),
   search: require('../sanitizer/search'),
-  search_fallback: require('../sanitizer/search_fallback'),
+  defer_to_addressit: require('../sanitizer/defer_to_addressit'),
   structured_geocoding: require('../sanitizer/structured_geocoding'),
   reverse: require('../sanitizer/reverse'),
   nearby: require('../sanitizer/nearby')
@@ -75,6 +75,7 @@ const isCoarseReverse = require('../controller/predicates/is_coarse_reverse');
 const isAdminOnlyAnalysis = require('../controller/predicates/is_admin_only_analysis');
 const hasResultsAtLayers = require('../controller/predicates/has_results_at_layers');
 const isSingleFieldAnalysis = require('../controller/predicates/is_single_field_analysis');
+const isAddressItParse = require('../controller/predicates/is_addressit_parse');
 
 // shorthand for standard early-exit conditions
 const hasResponseDataOrRequestErrors = any(hasResponseData, hasRequestErrors);
@@ -116,8 +117,7 @@ function addRoutes(app, peliasConfig) {
   const placeholderGeodisambiguationShouldExecute = all(
     not(hasResponseDataOrRequestErrors),
     isPlaceholderServiceEnabled,
-    isAdminOnlyAnalysis,
+    isAdminOnlyAnalysis
-    not(isSingleFieldAnalysis('postalcode'))
   );
 
   // execute placeholder if libpostal identified address parts but ids need to
@@ -129,6 +129,9 @@ function addRoutes(app, peliasConfig) {
     not(hasNumberButNotStreet),
     // don't run placeholder if there's a query or category
     not(hasAnyParsedTextProperty('query', 'category')),
+    // run placeholder if there are any adminareas identified
+    // hasAnyParsedTextProperty('neighbourhood', 'borough', 'city', 'county', 'state', 'country'),
+    // don't run placeholder if only postalcode was identified
     not(isSingleFieldAnalysis('postalcode'))
   );
 
@@ -156,6 +159,18 @@ function addRoutes(app, peliasConfig) {
     not(placeholderShouldHaveExecuted)
   );
 
+  const shouldDeferToAddressIt = all(
+    not(hasRequestErrors),
+    not(hasResponseData),
+    not(placeholderShouldHaveExecuted)
+  );
+
+  const oldProdQueryShouldExecute = all(
+    not(hasRequestErrors),
+    not(hasResponseData),
+    isAddressItParse
+  );
+
   // execute under the following conditions:
   // - there are no errors or data
   // - request is not coarse OR pip service is disabled
@@ -189,11 +204,11 @@ function addRoutes(app, peliasConfig) {
       controllers.placeholder(placeholderService, geometricFiltersApply, placeholderGeodisambiguationShouldExecute),
       controllers.placeholder(placeholderService, geometricFiltersDontApply, placeholderIdsLookupShouldExecute),
       controllers.search_with_ids(peliasConfig.api, esclient, queries.address_using_ids, searchWithIdsShouldExecute),
-      // 3rd parameter is which query module to use, use fallback
+      // 3rd parameter is which query module to use, use fallback first, then
-      //  first, then use original search strategy if first query didn't return anything
+      //  use original search strategy if first query didn't return anything
       controllers.search(peliasConfig.api, esclient, queries.cascading_fallback, fallbackQueryShouldExecute),
-      sanitizers.search_fallback.middleware,
+      sanitizers.defer_to_addressit(shouldDeferToAddressIt),
-      controllers.search(peliasConfig.api, esclient, queries.very_old_prod, fallbackQueryShouldExecute),
+      controllers.search(peliasConfig.api, esclient, queries.very_old_prod, oldProdQueryShouldExecute),
       postProc.trimByGranularity(),
       postProc.distances('focus.point.'),
       postProc.confidenceScores(peliasConfig.api),

sanitizer/_text_addressit.js

@@ -20,6 +20,7 @@ function sanitize( raw, clean ){
 
     // valid text
     clean.text = raw.text;
+    clean.parser = 'addressit';
 
     // remove anything that may have been parsed before
     delete clean.parsed_text;

sanitizer/defer_to_addressit.js

@@ -0,0 +1,32 @@
+const sanitizeAll = require('../sanitizer/sanitizeAll'),
+    sanitizers = {
+      text: require('../sanitizer/_text_addressit')
+    };
+
+const sanitize = function(req, cb) { sanitizeAll(req, sanitizers, cb); };
+const logger = require('pelias-logger').get('api');
+const logging = require( '../helper/logging' );
+
+// middleware
+module.exports = (should_execute) => {
+  return function(req, res, next) {
+    // if res.data already has results then don't call the _text_autocomplete sanitizer
+    // this has been put into place for when the libpostal integration way of querying
+    // ES doesn't return anything and we want to fallback to the old logic
+    if (!should_execute(req, res)) {
+      return next();
+    }
+
+    // log the query that caused a fallback since libpostal+new-queries didn't return anything
+    if (req.path === '/v1/search') {
+      const queryText = logging.isDNT(req) ? '[text removed]' : req.clean.text;
+      logger.info(`fallback queryText: ${queryText}`);
+    }
+
+    sanitize( req, function( err, clean ){
+      next();
+    });
+
+  };
+
+};

sanitizer/search_fallback.js

@@ -1,30 +0,0 @@
-var sanitizeAll = require('../sanitizer/sanitizeAll'),
-    sanitizers = {
-      text: require('../sanitizer/_text_addressit')
-    };
-
-var sanitize = function(req, cb) { sanitizeAll(req, sanitizers, cb); };
-var logger = require('pelias-logger').get('api');
-var logging = require( '../helper/logging' );
-var _ = require('lodash');
-
-// middleware
-module.exports.middleware = function( req, res, next ){
-  // if res.data already has results then don't call the _text_autocomplete sanitizer
-  // this has been put into place for when the libpostal integration way of querying
-  // ES doesn't return anything and we want to fallback to the old logic
-  if (_.get(res, 'data', []).length > 0) {
-    return next();
-  }
-
-  // log the query that caused a fallback since libpostal+new-queries didn't return anything
-  if (req.path === '/v1/search') {
-    const queryText = logging.isDNT(req) ? '[text removed]' : req.clean.text;
-    logger.info(`fallback queryText: ${queryText}`);
-  }
-
-  sanitize( req, function( err, clean ){
-    next();
-  });
-
-};

test/unit/controller/predicates/is_addressit_parse.js

@@ -0,0 +1,73 @@
+'use strict';
+
+const _ = require('lodash');
+const is_addressit_parse = require('../../../../controller/predicates/is_addressit_parse');
+
+module.exports.tests = {};
+
+module.exports.tests.interface = (test, common) => {
+  test('valid interface', t => {
+    t.ok(_.isFunction(is_addressit_parse), 'is_addressit_parse is a function');
+    t.end();
+  });
+};
+
+module.exports.tests.true_conditions = (test, common) => {
+  test('request.clean.parser=addressit should return true', t => {
+    const req = {
+      clean: {
+        parser: 'addressit'
+      }
+    };
+
+    t.ok(is_addressit_parse(req));
+    t.end();
+
+  });
+
+};
+
+module.exports.tests.false_conditions = (test, common) => {
+  test('undefined request should return false', t => {
+    t.notOk(is_addressit_parse(undefined));
+    t.end();
+  });
+
+  test('undefined request.clean should return false', t => {
+    const req = {};
+
+    t.notOk(is_addressit_parse(req));
+    t.end();
+  });
+
+  test('undefined request.clean.parser should return false', t => {
+    const req = {
+      clean: {}
+    };
+
+    t.notOk(is_addressit_parse(req));
+    t.end();
+  });
+
+  test('non-\'addressit\' request.clean.parser should return false', t => {
+    const req = {
+      clean: {
+        parser: 'not addressit'
+      }
+    };
+
+    t.notOk(is_addressit_parse(req));
+    t.end();
+  });
+
+};
+
+module.exports.all = (tape, common) => {
+  function test(name, testFunction) {
+    return tape(`GET /is_addressit_parse ${name}`, testFunction);
+  }
+
+  for( const testCase in module.exports.tests ){
+    module.exports.tests[testCase](test, common);
+  }
+};

test/unit/controller/predicates/is_admin_only_analysis.js

@@ -14,7 +14,7 @@ module.exports.tests.interface = (test, common) => {
 
 module.exports.tests.true_conditions = (test, common) => {
   test('parsed_text with admin-only properties should return true', (t) => {
-    ['neighbourhood', 'borough', 'city', 'county', 'state', 'postalcode', 'country'].forEach((property) => {
+    ['neighbourhood', 'borough', 'city', 'county', 'state', 'country'].forEach((property) => {
       const req = {
         clean: {
           parsed_text: {}
@@ -47,7 +47,7 @@ module.exports.tests.false_conditions = (test, common) => {
   });
 
   test('parsed_text with non-admin properties should return false', (t) => {
-    ['number', 'street', 'query', 'category'].forEach((property) => {
+    ['number', 'street', 'query', 'category', 'postalcode'].forEach((property) => {
       const req = {
         clean: {
           parsed_text: {}

test/unit/run.js

@@ -21,6 +21,7 @@ var tests = [
   require('./controller/predicates/has_response_data'),
   require('./controller/predicates/has_results_at_layers'),
   require('./controller/predicates/has_request_errors'),
+  require('./controller/predicates/is_addressit_parse'),
   require('./controller/predicates/is_admin_only_analysis'),
   require('./controller/predicates/is_coarse_reverse'),
   require('./controller/predicates/is_service_enabled'),
@@ -87,7 +88,7 @@ var tests = [
   require('./sanitizer/reverse'),
   require('./sanitizer/sanitizeAll'),
   require('./sanitizer/search'),
-  require('./sanitizer/search_fallback'),
+  require('./sanitizer/defer_to_addressit'),
   require('./sanitizer/wrap'),
   require('./service/configurations/PlaceHolder'),
   require('./service/configurations/PointInPolygon'),

test/unit/sanitizer/_text_addressit.js

@@ -33,6 +33,7 @@ module.exports.tests.text_parser = function(test, common) {
 
       var expected_clean = {
         text: query.name + ', ' + query.admin_parts,
+        parser: 'addressit',
         parsed_text: {
           name: query.name,
           regions: [ query.name ],
@@ -57,6 +58,7 @@ module.exports.tests.text_parser = function(test, common) {
 
       var expected_clean = {
         text: query.name + ',' + query.admin_parts,
+        parser: 'addressit',
         parsed_text: {
           name: query.name,
           regions: [ query.name ],
@@ -88,6 +90,7 @@ module.exports.tests.text_parser = function(test, common) {
 
       var expected_clean = {
         text: query.name + ', ' + query.admin_parts,
+        parser: 'addressit',
         parsed_text: {
           name: query.name,
           regions: [ query.name, query.admin_parts ],
@@ -111,6 +114,7 @@ module.exports.tests.text_parser = function(test, common) {
 
       var expected_clean = {
         text: query.name + ',' + query.admin_parts,
+        parser: 'addressit',
         parsed_text: {
           name: query.name,
           regions: [ query.name, query.admin_parts ],
@@ -136,6 +140,7 @@ module.exports.tests.text_parser = function(test, common) {
     clean.parsed_text = 'this should be removed';
 
     var expected_clean = {
+      parser: 'addressit',
       text: 'yugolsavia'
     };
 
@@ -155,6 +160,7 @@ module.exports.tests.text_parser = function(test, common) {
     clean.parsed_text = 'this should be removed';
 
     var expected_clean = {
+      parser: 'addressit',
       text: 'small town'
     };
 
@@ -174,6 +180,7 @@ module.exports.tests.text_parser = function(test, common) {
     clean.parsed_text = 'this should be removed';
 
     var expected_clean = {
+      parser: 'addressit',
       text: '123 main'
     };
 
@@ -193,6 +200,7 @@ module.exports.tests.text_parser = function(test, common) {
     clean.parsed_text = 'this should be removed';
 
     var expected_clean = {
+      parser: 'addressit',
       text: 'main 123'
     };
 
@@ -213,6 +221,7 @@ module.exports.tests.text_parser = function(test, common) {
 
     var expected_clean = {
       text: 'main particle new york',
+      parser: 'addressit',
       parsed_text: {
         regions: [ 'main particle' ],
         state: 'NY'
@@ -235,6 +244,7 @@ module.exports.tests.text_parser = function(test, common) {
 
     var expected_clean = {
       text: '123 main st new york ny',
+      parser: 'addressit',
       parsed_text: {
         number: '123',
         street: 'main st',
@@ -259,6 +269,7 @@ module.exports.tests.text_parser = function(test, common) {
 
     var expected_clean = {
       text: '123 main st new york ny 10010',
+      parser: 'addressit',
       parsed_text: {
         number: '123',
         street: 'main st',
@@ -283,6 +294,7 @@ module.exports.tests.text_parser = function(test, common) {
 
     var expected_clean = {
       text: '339 W Main St, Cheshire, 06410',
+      parser: 'addressit',
       parsed_text: {
         name: '339 W Main St',
         number: '339',
@@ -308,6 +320,7 @@ module.exports.tests.text_parser = function(test, common) {
 
     var expected_clean = {
       text: '339 W Main St,Lancaster,PA',
+      parser: 'addressit',
       parsed_text: {
         name: '339 W Main St',
         number: '339',

test/unit/sanitizer/defer_to_addressit.js

@@ -0,0 +1,132 @@
+const proxyquire =  require('proxyquire').noCallThru();
+const mock_logger = require('pelias-mock-logger');
+
+module.exports.tests = {};
+
+module.exports.tests.sanitize = (test, common) => {
+  test('verify that no sanitizers were called when should_execute returns false', (t) => {
+    t.plan(1);
+
+    const logger = mock_logger();
+
+    // rather than re-verify the functionality of all the sanitizers, this test just verifies that they
+    //  were all called correctly
+    const defer_to_addressit = proxyquire('../../../sanitizer/defer_to_addressit', {
+      '../sanitizer/_text_addressit': () => {
+        t.fail('_text_addressit should not have been called');
+      },
+      'pelias-logger': logger
+    })(() => false);
+
+    defer_to_addressit({}, {}, () => {
+      t.equals(logger.getInfoMessages().length, 0);
+      t.end();
+    });
+
+  });
+
+  test('verify that _text_addressit sanitizer was called when should_execute returns true', (t) => {
+    t.plan(2);
+
+    const logger = mock_logger();
+
+    // rather than re-verify the functionality of all the sanitizers, this test just verifies that they
+    //  were all called correctly
+    const defer_to_addressit = proxyquire('../../../sanitizer/defer_to_addressit', {
+      '../sanitizer/_text_addressit': () => {
+        t.pass('_text_addressit should have been called');
+        return { errors: [], warnings: [] };
+      },
+      'pelias-logger': logger,
+      '../helper/logging': {
+        isDNT: () => false
+      }
+    })(() => true);
+
+    const req = {
+      path: '/v1/search',
+      clean: {
+        text: 'this is the query text'
+      }
+    };
+
+    defer_to_addressit(req, {}, () => {
+      t.deepEquals(logger.getInfoMessages(), ['fallback queryText: this is the query text']);
+      t.end();
+    });
+
+  });
+
+  test('query should not be logged if path != \'/v1/search\'', (t) => {
+    t.plan(2);
+
+    const logger = mock_logger();
+
+    // rather than re-verify the functionality of all the sanitizers, this test just verifies that they
+    //  were all called correctly
+    const defer_to_addressit = proxyquire('../../../sanitizer/defer_to_addressit', {
+      '../sanitizer/_text_addressit': () => {
+        t.pass('_text_addressit should have been called');
+        return { errors: [], warnings: [] };
+      },
+      'pelias-logger': logger
+    })(() => true);
+
+    const req = {
+      path: 'not /v1/search',
+      clean: {
+        text: 'this is the query text'
+      }
+    };
+
+    defer_to_addressit(req, {}, () => {
+      t.deepEquals(logger.getInfoMessages(), []);
+      t.end();
+    });
+
+  });
+
+  test('query should be logged as [text removed] if private', (t) => {
+    t.plan(2);
+
+    const logger = mock_logger();
+
+    // rather than re-verify the functionality of all the sanitizers, this test just verifies that they
+    //  were all called correctly
+    const defer_to_addressit = proxyquire('../../../sanitizer/defer_to_addressit', {
+      '../sanitizer/_text_addressit': () => {
+        t.pass('_text_addressit should have been called');
+        return { errors: [], warnings: [] };
+      },
+      'pelias-logger': logger,
+      '../helper/logging': {
+        isDNT: () => true
+      }
+    })(() => true);
+
+    const req = {
+      path: '/v1/search',
+      clean: {
+        text: 'this is the query text'
+      }
+    };
+
+    defer_to_addressit(req, {}, () => {
+      t.deepEquals(logger.getInfoMessages(), ['fallback queryText: [text removed]']);
+      t.end();
+    });
+
+  });
+
+};
+
+module.exports.all = function (tape, common) {
+
+  function test(name, testFunction) {
+    return tape(`SANITIZE /defer_to_addressit ${name}`, testFunction);
+  }
+
+  for( var testCase in module.exports.tests ){
+    module.exports.tests[testCase](test, common);
+  }
+};

test/unit/sanitizer/search_fallback.js

@@ -1,185 +0,0 @@
-var proxyquire =  require('proxyquire').noCallThru();
-
-module.exports.tests = {};
-
-module.exports.tests.sanitize = function(test, common) {
-  test('verify that all sanitizers were called as expected when `res` is undefined', function(t) {
-    var called_sanitizers = [];
-
-    // rather than re-verify the functionality of all the sanitizers, this test just verifies that they
-    //  were all called correctly
-    var search = proxyquire('../../../sanitizer/search_fallback', {
-      '../sanitizer/_text_addressit': function() {
-        called_sanitizers.push('_text_addressit');
-        return { errors: [], warnings: [] };
-      }
-    });
-
-    var expected_sanitizers = [
-      '_text_addressit'
-    ];
-
-    var req = {};
-
-    search.middleware(req, undefined, function(){
-      t.deepEquals(called_sanitizers, expected_sanitizers);
-      t.end();
-    });
-
-  });
-
-  test('verify that all sanitizers were called as expected when `res` has no `data` property', function(t) {
-    var called_sanitizers = [];
-
-    // rather than re-verify the functionality of all the sanitizers, this test just verifies that they
-    //  were all called correctly
-    var search = proxyquire('../../../sanitizer/search_fallback', {
-      '../sanitizer/_text_addressit': function() {
-        called_sanitizers.push('_text_addressit');
-        return { errors: [], warnings: [] };
-      }
-    });
-
-    var expected_sanitizers = [
-      '_text_addressit'
-    ];
-
-    var req = {};
-    var res = {};
-
-    search.middleware(req, res, function(){
-      t.deepEquals(called_sanitizers, expected_sanitizers);
-      t.end();
-    });
-
-  });
-
-  test('verify that all sanitizers were called as expected when res.data is empty', function(t) {
-    var called_sanitizers = [];
-
-    // rather than re-verify the functionality of all the sanitizers, this test just verifies that they
-    //  were all called correctly
-    var search = proxyquire('../../../sanitizer/search_fallback', {
-      '../sanitizer/_text_addressit': function() {
-        called_sanitizers.push('_text_addressit');
-        return { errors: [], warnings: [] };
-      }
-    });
-
-    var expected_sanitizers = [
-      '_text_addressit'
-    ];
-
-    var req = {};
-    var res = {
-      data: []
-    };
-
-    search.middleware(req, res, function(){
-      t.deepEquals(called_sanitizers, expected_sanitizers);
-      t.end();
-    });
-
-  });
-
-  test('non-empty res.data should not call the _text_autocomplete sanitizer', function(t) {
-    var called_sanitizers = [];
-
-    // rather than re-verify the functionality of all the sanitizers, this test just verifies that they
-    //  were all called correctly
-    var search = proxyquire('../../../sanitizer/search_fallback', {
-      '../sanitizer/_text_autocomplete': function() {
-        throw new Error('_text_autocomplete sanitizer should not have been called');
-      }
-    });
-
-    var expected_sanitizers = [];
-
-    var req = {};
-    var res = {
-      data: [{}]
-    };
-
-    search.middleware(req, res, function(){
-      t.deepEquals(called_sanitizers, expected_sanitizers);
-      t.end();
-    });
-
-  });
-
-  test('req.clean.text should be logged when isDNT=false', (t) => {
-    const infoLog = [];
-
-    const search = proxyquire('../../../sanitizer/search_fallback', {
-      'pelias-logger': {
-        get: () => {
-          return {
-            info: (msg) => {
-              infoLog.push(msg);
-            }
-          };
-        }
-      },
-      '../helper/logging': {
-        isDNT: () => { return false; }
-      }
-    });
-
-    const req = {
-      path: '/v1/search',
-      clean: {
-        text: 'this is the query text'
-      }
-    };
-
-    search.middleware(req, undefined, () => {
-      t.deepEquals(infoLog, [`fallback queryText: ${req.clean.text}`]);
-      t.end();
-    });
-
-  });
-
-  test('req.clean.text should not be logged when isDNT=true', (t) => {
-    const infoLog = [];
-
-    const search = proxyquire('../../../sanitizer/search_fallback', {
-      'pelias-logger': {
-        get: () => {
-          return {
-            info: (msg) => {
-              infoLog.push(msg);
-            }
-          };
-        }
-      },
-      '../helper/logging': {
-        isDNT: () => { return true; }
-      }
-    });
-
-    const req = {
-      path: '/v1/search',
-      clean: {
-        text: 'this is the query text'
-      }
-    };
-
-    search.middleware(req, undefined, () => {
-      t.deepEquals(infoLog, ['fallback queryText: [text removed]']);
-      t.end();
-    });
-
-  });
-
-};
-
-module.exports.all = function (tape, common) {
-
-  function test(name, testFunction) {
-    return tape('SANITIZE /search_fallback ' + name, testFunction);
-  }
-
-  for( var testCase in module.exports.tests ){
-    module.exports.tests[testCase](test, common);
-  }
-};