From 3712ca0198feab93c5ecd96f6f8502e0dfa0da23 Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Fri, 14 Jun 2019 16:44:40 -0400
Subject: [PATCH] feat(controller): Remove search_with_ids controller

I was attempting to use the new debug output from
https://github.com/pelias/api/pull/1313 only to realize it was not added
to the `search_with_ids` controller, only the `search` controller.

These two controllers are very similar, so this PR combines them into
one that can be used in all cases.
---
 controller/search.js                    |  24 +-
 controller/search_with_ids.js           | 127 ------
 routes/v1.js                            |   8 +-
 test/unit/controller/search.js          |  11 +-
 test/unit/controller/search_with_ids.js | 559 ------------------------
 test/unit/run.js                        |   1 -
 6 files changed, 27 insertions(+), 703 deletions(-)
 delete mode 100644 controller/search_with_ids.js
 delete mode 100644 test/unit/controller/search_with_ids.js

diff --git a/controller/search.js b/controller/search.js
index fe7e09a18..46e1ed1c0 100644
--- a/controller/search.js
+++ b/controller/search.js
@@ -23,7 +23,10 @@ function setup( apiConfig, esclient, query, should_execute ){
       cleanOutput = logging.removeFields(cleanOutput);
     }
 
-    const renderedQuery = query(req.clean);
+    // rendering a query requires passing the `clean` object, which contains
+    // validated options from query parameters, and the `res` object, since
+    // some queries use the results of previous queries to Placeholder
+    const renderedQuery = query(req.clean, res);
 
     // if there's no query to call ES with, skip the service
     if (_.isUndefined(renderedQuery)) {
@@ -79,10 +82,10 @@ function setup( apiConfig, esclient, query, should_execute ){
         }
 
         // if execution has gotten this far then one of three things happened:
-        // - the request didn't time out
+        // - the request returned a response, either success or error
         // - maxRetries has been hit so we're giving up
         // - another error occurred
-        // in either case, handle the error or results
+        // in any case, handle the error or results
 
         // error handler
         if( err ){
@@ -94,11 +97,17 @@ function setup( apiConfig, esclient, query, should_execute ){
         }
         // set response data
         else {
-          res.data = docs;
-          res.meta = meta || {};
-          // store the query_type for subsequent middleware
-          res.meta.query_type = renderedQuery.type;
+          // because this controller may be called multiple times, there may already
+          // be results.  if there are no results from this ES call, don't overwrite
+          // what's already there from a previous call.
+          if (!_.isEmpty(docs)) {
+            res.data = docs;
+            res.meta = meta || {};
+            // store the query_type for subsequent middleware
+            res.meta.query_type = renderedQuery.type;
+          }
 
+          // put an entry in the debug log no matter the number of results
           debugLog.push(req, {queryType: {
             [renderedQuery.type] : {
               es_took: message.es_took,
@@ -114,7 +123,6 @@ function setup( apiConfig, esclient, query, should_execute ){
       });
       debugLog.stopTimer(req, initialTime);
     });
-
   }
 
   return controller;
diff --git a/controller/search_with_ids.js b/controller/search_with_ids.js
deleted file mode 100644
index 450340475..000000000
--- a/controller/search_with_ids.js
+++ /dev/null
@@ -1,127 +0,0 @@
-const _ = require('lodash');
-
-const searchService = require('../service/search');
-const logger = require('pelias-logger').get('api');
-const logging = require( '../helper/logging' );
-const retry = require('retry');
-const Debug = require('../helper/debug');
-const debugLog = new Debug('controller:search_with_ids');
-
-function isRequestTimeout(err) {
-  return _.get(err, 'status') === 408;
-}
-
-function setup( apiConfig, esclient, query, should_execute ){
-  function controller( req, res, next ){
-    if (!should_execute(req, res)) {
-      return next();
-    }
-
-    const cleanOutput = _.cloneDeep(req.clean);
-    if (logging.isDNT(req)) {
-      logging.removeFields(cleanOutput);
-    }
-
-    const renderedQuery = query(req.clean, res);
-
-    // if there's no query to call ES with, skip the service
-    if (_.isUndefined(renderedQuery)) {
-      debugLog.push(req, `No query to call ES with. Skipping`);
-      return next();
-    }
-
-    // options for retry
-    // maxRetries is from the API config with default of 3
-    // factor of 1 means that each retry attempt will esclient requestTimeout
-    const operationOptions = {
-      retries: _.get(apiConfig, 'requestRetries', 3),
-      factor: 1,
-      minTimeout: _.get(esclient, 'transport.requestTimeout')
-    };
-
-    // setup a new operation
-    const operation = retry.operation(operationOptions);
-
-    // elasticsearch command
-    const cmd = {
-      index: apiConfig.indexName,
-      searchType: 'dfs_query_then_fetch',
-      body: renderedQuery.body
-    };
-
-    logger.debug( '[ES req]', cmd );
-    debugLog.push(req, {ES_req: cmd});
-
-    operation.attempt((currentAttempt) => {
-      const initialTime = debugLog.beginTimer(req, `Attempt ${currentAttempt}`);
-      // query elasticsearch
-      searchService( esclient, cmd, function( err, docs, meta, data ){
-        const message = {
-          controller: 'search_with_ids',
-          queryType: renderedQuery.type,
-          es_hits: _.get(data, 'hits.total'),
-          result_count: _.get(res, 'data', []).length,
-          es_took: _.get(data, 'took', undefined),
-          response_time: _.get(data, 'response_time', undefined),
-          params: req.clean,
-          retries: currentAttempt - 1,
-          text_length: _.get(req, 'clean.text.length', 0)
-        };
-        logger.info('elasticsearch', message);
-
-        // returns true if the operation should be attempted again
-        // (handles bookkeeping of maxRetries)
-        // only consider for status 408 (request timeout)
-        if (isRequestTimeout(err) && operation.retry(err)) {
-          debugLog.stopTimer(req, initialTime, `request timed out on attempt ${currentAttempt}, retrying`);
-          return;
-        }
-
-        // if execution has gotten this far then one of three things happened:
-        // - the request didn't time out
-        // - maxRetries has been hit so we're giving up
-        // - another error occurred
-        // in either case, handle the error or results
-
-        // error handler
-        if( err ){
-          // push err.message or err onto req.errors
-          req.errors.push( _.get(err, 'message', err));
-        }
-        else {
-          // because this is used in response to placeholder, there may already
-          // be results.  if there are no results from this ES call, don't overwrite
-          // what's already there from placeholder.
-          if (!_.isEmpty(docs)) {
-            res.data = docs;
-            res.meta = meta || {};
-            // store the query_type for subsequent middleware
-            res.meta.query_type = renderedQuery.type;
-
-            const messageParts = [
-              '[controller:search]',
-              `[queryType:${renderedQuery.type}]`,
-              `[es_result_count:${docs.length}]`
-            ];
-
-            debugLog.push(req, {queryType: {
-              [renderedQuery.type] : {
-                es_result_count: docs.length
-              }
-            }});
-
-          }
-
-        }
-        logger.debug('[ES response]', docs);
-        next();
-      });
-      debugLog.stopTimer(req, initialTime);
-    });
-
-  }
-
-  return controller;
-}
-
-module.exports = setup;
diff --git a/routes/v1.js b/routes/v1.js
index 9e841f2bc..f21d69f4e 100644
--- a/routes/v1.js
+++ b/routes/v1.js
@@ -33,7 +33,6 @@ var controllers = {
   place: require('../controller/place'),
   placeholder: require('../controller/placeholder'),
   search: require('../controller/search'),
-  search_with_ids: require('../controller/search_with_ids'),
   status: require('../controller/status')
 };
 
@@ -289,11 +288,10 @@ function addRoutes(app, peliasConfig) {
       controllers.libpostal(libpostalService, libpostalShouldExecute),
       controllers.placeholder(placeholderService, geometricFiltersApply, placeholderGeodisambiguationShouldExecute),
       controllers.placeholder(placeholderService, geometricFiltersApply, placeholderIdsLookupShouldExecute),
-      controllers.search_with_ids(peliasConfig.api, esclient, queries.address_using_ids, searchWithIdsShouldExecute),
-      // 3rd parameter is which query module to use, use fallback first, then
-      //  use original search strategy if first query didn't return anything
+      // try 3 different query types: address search using ids, cascading fallback, addressit(very_old_prod)
+      controllers.search(peliasConfig.api, esclient, queries.address_using_ids, searchWithIdsShouldExecute),
       controllers.search(peliasConfig.api, esclient, queries.cascading_fallback, fallbackQueryShouldExecute),
-      sanitizers.defer_to_addressit(shouldDeferToAddressIt),
+      sanitizers.defer_to_addressit(shouldDeferToAddressIt), //run additional sanitizers needed for addressit parser
       controllers.search(peliasConfig.api, esclient, queries.very_old_prod, oldProdQueryShouldExecute),
       postProc.trimByGranularity(),
       postProc.distances('focus.point.'),
diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js
index faee2f70d..c3aa22043 100644
--- a/test/unit/controller/search.js
+++ b/test/unit/controller/search.js
@@ -179,7 +179,7 @@ module.exports.tests.success = function(test, common) {
     })(config, esclient, query, () => { return true; });
 
     const req = { clean: { }, errors: [], warnings: [] };
-    const res = {};
+    const res = { meta: { query_type: 'this is the query type from a previous query'} };
 
     const next = () => {
       t.deepEqual(req, {
@@ -187,8 +187,13 @@ module.exports.tests.success = function(test, common) {
         errors: [],
         warnings: []
       });
-      t.equals(res.data, undefined);
-      t.deepEquals(res.meta, { key: 'value', query_type: 'this is the query type' });
+
+      t.equals(res.data, undefined, 'no data set');
+
+      const expected_meta = {
+        query_type: 'this is the query type from a previous query'
+      };
+      t.deepEquals(res.meta, expected_meta, 'previous query meta information left unchanged');
 
       t.end();
     };
diff --git a/test/unit/controller/search_with_ids.js b/test/unit/controller/search_with_ids.js
deleted file mode 100644
index 83fed4b72..000000000
--- a/test/unit/controller/search_with_ids.js
+++ /dev/null
@@ -1,559 +0,0 @@
-const setup = require('../../../controller/search_with_ids');
-const proxyquire =  require('proxyquire').noCallThru();
-const mocklogger = require('pelias-mock-logger');
-const _ = require('lodash');
-
-module.exports.tests = {};
-
-module.exports.tests.interface = (test, common) => {
-  test('valid interface', (t) => {
-    t.ok(_.isFunction(setup), 'setup is a function');
-    t.ok(_.isFunction(setup()), 'setup returns a controller');
-    t.end();
-  });
-};
-
-module.exports.tests.success = (test, common) => {
-  test('successful request to search service should replace data and meta', (t) => {
-    t.plan(4);
-
-    const logger = mocklogger();
-
-    const config = {
-      indexName: 'indexName value'
-    };
-    const esclient = 'this is the esclient';
-    const query = () => ({
-      body: 'this is the query body',
-      type: 'this is the query type'
-    });
-
-    // a controller that validates the esclient and cmd that was passed to the search service
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': (esclient, cmd, callback) => {
-        t.equal(esclient, 'this is the esclient');
-        t.deepEqual(cmd, {
-          index: 'indexName value',
-          searchType: 'dfs_query_then_fetch',
-          body: 'this is the query body'
-        });
-
-        const docs = [
-          { name: 'replacement result #1'},
-          { name: 'replacement result #2'}
-        ];
-        const meta = { key: 'replacement meta value' };
-
-        callback(undefined, docs, meta);
-      },
-      'pelias-logger': logger
-    })(config, esclient, query, () => true );
-
-    const req = { clean: { }, errors: [], warnings: [] };
-    const res = {
-      data: [
-        { name: 'original result #1'},
-        { name: 'original result #2'}
-      ],
-      meta: {
-        key: 'original meta value'
-      }
-    };
-
-    const next = () => {
-      t.deepEqual(req, {
-        clean: {},
-        errors: [],
-        warnings: []
-      });
-      t.deepEquals(res, {
-        data: [
-          { name: 'replacement result #1'},
-          { name: 'replacement result #2'}
-        ],
-        meta: {
-          key: 'replacement meta value',
-          query_type: 'this is the query type'
-        }
-      });
-
-      t.end();
-    };
-
-    controller(req, res, next);
-
-  });
-
-  test('undefined meta should set empty object into res', (t) => {
-    const logger = mocklogger();
-
-    const config = {
-      indexName: 'indexName value'
-    };
-    const esclient = 'this is the esclient';
-    const query = () => ({
-      body: 'this is the query body',
-      type: 'this is the query type'
-    });
-
-    // a controller that validates the esclient and cmd that was passed to the search service
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': (esclient, cmd, callback) => {
-        const docs = [
-          { name: 'replacement result #1'},
-          { name: 'replacement result #2'}
-        ];
-
-        callback(undefined, docs, undefined);
-      },
-      'pelias-logger': logger
-    })(config, esclient, query, () => true );
-
-    const req = { clean: { }, errors: [], warnings: [] };
-    const res = {
-      data: [
-        { name: 'original result #1'},
-        { name: 'original result #2'}
-      ],
-      meta: {
-        key: 'original meta value'
-      }
-    };
-
-    const next = () => {
-      t.deepEqual(req, {
-        clean: {},
-        errors: [],
-        warnings: []
-      });
-      t.deepEquals(res, {
-        data: [
-          { name: 'replacement result #1'},
-          { name: 'replacement result #2'}
-        ],
-        meta: {
-          query_type: 'this is the query type'
-        }
-      });
-
-      t.end();
-    };
-
-    controller(req, res, next);
-
-  });
-
-  test('undefined docs in response should not overwrite existing results', (t) => {
-    t.plan(1+2); // ensures that search service was called, then req+res+logger tests
-
-    const logger = mocklogger();
-
-    const config = {
-      indexName: 'indexName value'
-    };
-    const esclient = 'this is the esclient';
-    const query = () => ({
-      body: 'this is the query body',
-      type: 'this is the query type'
-    });
-
-    // a controller that validates the esclient and cmd that was passed to the search service
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': (esclient, cmd, callback) => {
-        t.pass('search service was called');
-
-        const meta = { key: 'new value' };
-
-        callback(undefined, undefined, meta);
-      },
-      'pelias-logger': logger
-    })(config, esclient, query, () => true );
-
-    const req = { clean: { }, errors: [], warnings: [] };
-    const res = {
-      data: [
-        { id: 1 },
-        { id: 2 }
-      ],
-      meta: {
-        key: 'value'
-      }
-    };
-
-    const next = () => {
-      t.deepEqual(req, {
-        clean: {},
-        errors: [],
-        warnings: []
-      });
-      t.deepEquals(res, {
-        data: [
-          { id: 1 },
-          { id: 2 }
-        ],
-        meta: { key: 'value' }
-      });
-
-      t.end();
-    };
-
-    controller(req, res, next);
-
-  });
-
-  test('empty docs in response should not overwrite existing results', (t) => {
-    t.plan(3);
-
-    const logger = mocklogger();
-
-    const config = {
-      indexName: 'indexName value'
-    };
-    const esclient = 'this is the esclient';
-    const query = () => ({
-      body: 'this is the query body',
-      type: 'this is the query type'
-    });
-
-    // a controller that validates the esclient and cmd that was passed to the search service
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': (esclient, cmd, callback) => {
-        t.pass('search service was called');
-
-        const meta = { key: 'value' };
-
-        callback(undefined, [], meta);
-      }
-    })(config, esclient, query, () => true );
-
-    const req = { clean: { }, errors: [], warnings: [] };
-    const res = {
-      data: [
-        { name: 'pre-existing result #1' },
-        { name: 'pre-existing result #2' }
-      ],
-      meta: {
-        key: 'value'
-      }
-    };
-
-    const next = () => {
-      t.deepEqual(req, {
-        clean: {},
-        errors: [],
-        warnings: []
-      });
-      t.deepEquals(res, {
-        data: [
-          { name: 'pre-existing result #1' },
-          { name: 'pre-existing result #2' }
-        ],
-        meta: { key: 'value' }
-      });
-
-      t.end();
-    };
-
-    controller(req, res, next);
-
-  });
-
-  test('successful request on retry to search service should log info message', (t) => {
-    t.plan(3+2); // 3 search service calls, 1 req, 1 res
-
-    const logger = mocklogger();
-
-    const config = {
-      indexName: 'indexName value'
-    };
-    const esclient = 'this is the esclient';
-    const query = () => ({
-      body: 'this is the query body',
-      type: 'this is the query type'
-    });
-
-    let searchServiceCallCount = 0;
-
-    const timeoutError = {
-      status: 408,
-      displayName: 'RequestTimeout',
-      message: 'Request Timeout after 17ms'
-    };
-
-    // a controller that validates the esclient and cmd that was passed to the search service
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': (esclient, cmd, callback) => {
-        t.pass('search service was called');
-
-        if (searchServiceCallCount < 2) {
-          // note that the searchService got called
-          searchServiceCallCount++;
-          callback(timeoutError);
-        } else {
-          const docs = [
-            { name: 'replacement result #1'},
-            { name: 'replacement result #2'}
-          ];
-          const meta = { key: 'replacement meta value' };
-
-          callback(undefined, docs, meta);
-        }
-
-      },
-      'pelias-logger': logger
-    })(config, esclient, query, () => true );
-
-    const req = { clean: { }, errors: [], warnings: [] };
-    const res = {
-      data: [
-        { name: 'original result #1'},
-        { name: 'original result #2'}
-      ],
-      meta: {
-        key: 'original meta value'
-      }
-    };
-
-    const next = () => {
-      t.deepEqual(req, {
-        clean: {},
-        errors: [],
-        warnings: []
-      });
-      t.deepEquals(res, {
-        data: [
-          { name: 'replacement result #1'},
-          { name: 'replacement result #2'}
-        ],
-        meta: {
-          key: 'replacement meta value',
-          query_type: 'this is the query type'
-        }
-      });
-
-      t.end();
-    };
-
-    controller(req, res, next);
-
-  });
-
-};
-
-module.exports.tests.service_errors = (test, common) => {
-  //test('default # of request timeout retries should be 3', (t) => {
-    //// test for 1 initial search service, 3 retries, 1 log messages, 1 req, and 1 res
-    //t.plan(1 + 3 + 1 + 2);
-
-    //const logger = mocklogger();
-
-    //const config = {
-      //indexName: 'indexName value'
-    //};
-    //const esclient = 'this is the esclient';
-    //const query = () => ({
-      //body: 'this is the query body',
-    //});
-
-    //const timeoutError = {
-      //status: 408,
-      //displayName: 'RequestTimeout',
-      //message: 'Request Timeout after 17ms'
-    //};
-
-    //// a controller that validates that the search service was called
-    //const controller = proxyquire('../../../controller/search_with_ids', {
-      //'../service/search': (esclient, cmd, callback) => {
-        //// note that the searchService got called
-        //t.pass('search service was called');
-
-        //callback(timeoutError);
-      //},
-      //'pelias-logger': logger
-    //})(config, esclient, query, () => true );
-
-    //const req = { clean: { }, errors: [], warnings: [] };
-    //const res = {};
-
-    //const next = () => {
-      //t.deepEqual(logger.getInfoMessages(), [
-        //'[req] endpoint=undefined {}',
-        //'request timed out on attempt 1, retrying',
-        //'request timed out on attempt 2, retrying',
-        //'request timed out on attempt 3, retrying'
-      //]);
-
-      //t.deepEqual(req, {
-        //clean: {},
-        //errors: [timeoutError.message],
-        //warnings: []
-      //});
-      //t.deepEqual(res, {});
-      //t.end();
-    //};
-
-    //controller(req, res, next);
-
-  //});
-
-  test('explicit apiConfig.requestRetries should retry that many times', (t) => {
-    t.plan(1 + 17); // test for initial search service call and 17 retries
-
-    const config = {
-      indexName: 'indexName value',
-      requestRetries: 17
-    };
-    const esclient = 'this is the esclient';
-    const query = () => ({ });
-
-    const timeoutError = {
-      status: 408,
-      displayName: 'RequestTimeout',
-      message: 'Request Timeout after 17ms'
-    };
-
-    // a controller that validates that the search service was called
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': (esclient, cmd, callback) => {
-        // note that the searchService got called
-        t.pass('search service was called');
-
-        callback(timeoutError);
-      }
-    })(config, esclient, query, () => true );
-
-    const req = { clean: { }, errors: [], warnings: [] };
-    const res = {};
-
-    controller(req, res, () => t.end() );
-
-  });
-
-  test('only status code 408 should be considered a retryable request', (t) => {
-    t.plan(2);
-
-    const config = {
-      indexName: 'indexName value',
-      requestRetries: 17
-    };
-    const esclient = 'this is the esclient';
-    const query = () => ({ });
-
-    const nonTimeoutError = {
-      status: 500,
-      displayName: 'InternalServerError',
-      message: 'an internal server error occurred'
-    };
-
-    // a controller that validates that the search service was called
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': (esclient, cmd, callback) => {
-        // note that the searchService got called
-        t.pass('search service was called');
-
-        callback(nonTimeoutError);
-      }
-    })(config, esclient, query, () => true );
-
-    const req = { clean: { }, errors: [], warnings: [] };
-    const res = {};
-
-    const next = () => {
-      t.deepEqual(req, {
-        clean: {},
-        errors: [nonTimeoutError.message],
-        warnings: []
-      });
-      t.end();
-    };
-
-    controller(req, res, next);
-
-  });
-
-  test('string error should not retry and be logged as-is', (t) => {
-    t.plan(2); // service call + error is in req.errors
-
-    const config = {
-      indexName: 'indexName value'
-    };
-    const esclient = 'this is the esclient';
-    const query = () => ({ });
-
-    // a controller that validates that the search service was called
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': (esclient, cmd, callback) => {
-        // note that the searchService got called
-        t.pass('search service was called');
-
-        callback('this is an error string');
-      }
-    })(config, esclient, query, () => true );
-
-    const req = { clean: { }, errors: [], warnings: [] };
-    const res = {};
-
-    const next = () => {
-      t.deepEqual(req, {
-        clean: {},
-        errors: ['this is an error string'],
-        warnings: []
-      });
-      t.end();
-    };
-
-    controller(req, res, next);
-
-  });
-
-};
-
-module.exports.tests.should_execute = (test, common) => {
-  test('should_execute returning false and empty req.errors should call next', (t) => {
-    const esclient = () => t.fail('esclient should not have been called');
-    const query = () => t.fail('query should not have been called');
-    const should_execute = () => false;
-    const controller = setup( {}, esclient, query, should_execute );
-
-    const req = { };
-    const res = { };
-
-    const next = () => {
-      t.deepEqual(res, { });
-      t.end();
-    };
-    controller(req, res, next);
-
-  });
-
-};
-
-module.exports.tests.undefined_query = (test, common) => {
-  test('query returning undefined should not call service', (t) => {
-    t.plan(0, 'test will fail if search service actually gets called');
-
-    // a function that returns undefined
-    const query = () => undefined;
-
-    const controller = proxyquire('../../../controller/search_with_ids', {
-      '../service/search': () => {
-        t.fail('search service should not have been called');
-      }
-    })(undefined, undefined, query, () => true );
-
-    const next = () => t.end();
-
-    controller({}, {}, next);
-
-  });
-};
-
-module.exports.all = (tape, common) => {
-  function test(name, testFunction) {
-    return tape(`GET /search ${name}`, testFunction);
-  }
-
-  for( const testCase in module.exports.tests ){
-    module.exports.tests[testCase](test, common);
-  }
-};
diff --git a/test/unit/run.js b/test/unit/run.js
index c7761b376..7653bf59b 100644
--- a/test/unit/run.js
+++ b/test/unit/run.js
@@ -18,7 +18,6 @@ var tests = [
   require('./controller/place'),
   require('./controller/placeholder'),
   require('./controller/search'),
-  require('./controller/search_with_ids'),
   require('./controller/predicates/has_parsed_text_properties'),
   require('./controller/predicates/has_request_parameter'),
   require('./controller/predicates/has_response_data'),