From 372090b5fa941e92b4395e42901aa93315453c30 Mon Sep 17 00:00:00 2001
From: Stephen Hess <stephen.k.hess@gmail.com>
Date: Wed, 19 Jul 2017 12:00:46 -0400
Subject: [PATCH 1/4] accept req AND res, pass both to getHeaders/getParameters

---
 service.js      |  24 ++++---
 test/service.js | 164 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 160 insertions(+), 28 deletions(-)

diff --git a/service.js b/service.js
index 4d419df..c860f3d 100644
--- a/service.js
+++ b/service.js
@@ -10,8 +10,8 @@ function isDoNotTrack(headers) {
 }
 
 // superagent doesn't exposed the assembled GET request, so synthesize it
-function synthesizeUrl(serviceConfig, req) {
-  const parameters = _.map(serviceConfig.getParameters(req), (value, key) => {
+function synthesizeUrl(serviceConfig, req, res) {
+  const parameters = _.map(serviceConfig.getParameters(req, res), (value, key) => {
     return `${key}=${value}`;
   }).join('&');
 
@@ -33,7 +33,11 @@ module.exports = function setup(serviceConfig) {
   if (!serviceConfig.isEnabled()) {
     logger.warn(`${serviceConfig.getName()} service disabled`);
 
-    return (req, callback) => {
+    return (req, res, callback) => {
+      // only req was passed in so treat res as callback
+      if (_.isUndefined(callback)) {
+        callback = res;
+      }
       // respond with an error to any call
       callback(`${serviceConfig.getName()} service disabled`);
     };
@@ -41,8 +45,14 @@ module.exports = function setup(serviceConfig) {
   }
 
   logger.info(`using ${serviceConfig.getName()} service at ${serviceConfig.getBaseUrl()}`);
-  return (req, callback) => {
-    const headers = serviceConfig.getHeaders(req) || {};
+  return (req, res, callback) => {
+    // only req was passed in so treat res as callback
+    if (_.isUndefined(callback)) {
+      callback = res;
+      res = undefined;
+    }
+
+    const headers = serviceConfig.getHeaders(req, res) || {};
 
     // save off do_not_track value for later check
     const do_not_track = isDoNotTrack(req.headers);
@@ -52,7 +62,7 @@ module.exports = function setup(serviceConfig) {
       logger.debug(`${serviceConfig.getName()}: ${serviceConfig.getBaseUrl()}`);
 
     } else {
-      logger.debug(`${serviceConfig.getName()}: ${synthesizeUrl(serviceConfig, req)}`);
+      logger.debug(`${serviceConfig.getName()}: ${synthesizeUrl(serviceConfig, req, res)}`);
 
     }
 
@@ -62,7 +72,7 @@ module.exports = function setup(serviceConfig) {
       .timeout(serviceConfig.getTimeout())
       .retry(serviceConfig.getRetries())
       .accept('json')
-      .query(serviceConfig.getParameters(req))
+      .query(serviceConfig.getParameters(req, res))
       .on('error', (err) => {
         if (err.status) {
           // first handle case where a non-200 was returned
diff --git a/test/service.js b/test/service.js
index 1b8faf1..d15f042 100644
--- a/test/service.js
+++ b/test/service.js
@@ -3,6 +3,7 @@
 const proxyquire = require('proxyquire').noCallThru();
 const express = require('express');
 const tape = require('tape');
+const _ = require('lodash');
 
 const ServiceConfiguration = require('../ServiceConfiguration');
 
@@ -65,7 +66,7 @@ tape('request logging', (test) => {
 
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, (err, results) => {
+    service({}, {}, (err, results) => {
       t.ok(logger.isDebugMessage(`foo: http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value`));
       t.end();
 
@@ -113,7 +114,7 @@ tape('request logging', (test) => {
 
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service(req, (err, results) => {
+    service(req, {}, (err, results) => {
       t.ok(logger.isDebugMessage(`foo: http://localhost:${port}/`));
       t.end();
 
@@ -125,7 +126,7 @@ tape('request logging', (test) => {
 
 });
 
-tape('do-nothing service tests', (test) => {
+tape('service disabled tests', (test) => {
   test.test('undefined config.url should return service that logs that config.name service is not available', (t) => {
     const logger = require('pelias-mock-logger')();
 
@@ -141,7 +142,7 @@ tape('do-nothing service tests', (test) => {
 
     t.ok(logger.isWarnMessage(/^foo service disabled$/));
 
-    service({}, (err) => {
+    service({}, {}, (err) => {
       t.equals(err, 'foo service disabled');
       t.end();
     });
@@ -175,7 +176,7 @@ tape('failure conditions tests', (test) => {
 
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, (err, results) => {
+    service({}, {}, (err, results) => {
       t.equals(err.code, 'ECONNREFUSED');
       t.notOk(results);
       t.ok(logger.isErrorMessage(new RegExp(`^http://localhost:${port}/built_url: .*ECONNREFUSED`)),
@@ -217,8 +218,9 @@ tape('failure conditions tests', (test) => {
         dnt: 1
       }
     };
+    const res = {};
 
-    service(req, (err, results) => {
+    service(req, res, (err, results) => {
       t.equals(err.code, 'ECONNREFUSED');
       t.notOk(results);
       t.ok(logger.isErrorMessage(new RegExp(`^http://localhost:${port}/ \\[do_not_track\\]: .*ECONNREFUSED`)),
@@ -268,7 +270,7 @@ tape('failure conditions tests', (test) => {
 
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, (err, results) => {
+    service({}, {}, (err, results) => {
       t.equals(err, `http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
         'returned status 400: a bad request was made');
       t.notOk(results);
@@ -324,8 +326,9 @@ tape('failure conditions tests', (test) => {
         dnt: 1
       }
     };
+    const res = {};
 
-    service(req, (err, results) => {
+    service(req, res, (err, results) => {
       t.equals(err, `http://localhost:${port}/ [do_not_track] returned status 400: a bad request was made`);
       t.notOk(results);
       t.ok(logger.isErrorMessage(`http://localhost:${port}/ [do_not_track] ` +
@@ -375,7 +378,7 @@ tape('failure conditions tests', (test) => {
 
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, (err, results) => {
+    service({}, {}, (err, results) => {
       t.equals(err, `http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
         `could not parse response: this is not parseable as JSON`);
       t.notOk(results, 'should return undefined');
@@ -431,8 +434,9 @@ tape('failure conditions tests', (test) => {
         dnt: 1
       }
     };
+    const res = {};
 
-    service(req, (err, results) => {
+    service(req, res, (err, results) => {
       t.equals(err, `http://localhost:${port}/ [do_not_track] ` +
         `could not parse response: this is not parseable as JSON`);
       t.notOk(results, 'should return undefined');
@@ -483,7 +487,7 @@ tape('failure conditions tests', (test) => {
 
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, (err, results) => {
+    service({}, {}, (err, results) => {
       t.equals(err, `http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
         'returned status 503: request timeout');
       t.notOk(results);
@@ -506,8 +510,17 @@ tape('success conditions tests', (test) => {
     webServer.get('/some_endpoint', (req, res, next) => {
       t.notOk(req.headers.hasOwnProperty('dnt'), 'dnt header should not have been passed');
 
-      t.equals(req.headers.header1, 'header1 value', 'all headers should have been passed');
-      t.deepEquals(req.query, { param1: 'param1 value', param2: 'param2 value' });
+      t.equals(req.headers.req_header1, 'req_header1 value');
+      t.equals(req.headers.req_header2, 'req_header2 value');
+      t.equals(req.headers.res_header1, 'res_header1 value');
+      t.equals(req.headers.res_header2, 'res_header2 value');
+
+      t.deepEquals(req.query, {
+        req_param1: 'req_param1 value',
+        req_param2: 'req_param2 value',
+        res_param1: 'res_param1 value',
+        res_param2: 'res_param2 value'
+      });
 
       res.status(200).json([1, 2, 3]);
     });
@@ -522,13 +535,16 @@ tape('success conditions tests', (test) => {
         super('foo', { url: `http://localhost:${port}` } );
       }
       getUrl(req) {
-        return `http://localhost:${port}/some_endpoint`;
+        // pull endpoint from req to show that req was passed
+        return `http://localhost:${port}/${req.endpoint}`;
       }
-      getParameters(req) {
-        return { param1: 'param1 value', param2: 'param2 value' };
+      getParameters(req, res) {
+        // combine req and res to show that both were passed
+        return _.extend({}, req.params, res.params);
       }
-      getHeaders(req) {
-        return { header1: 'header1 value' };
+      getHeaders(req, res) {
+        // combine req and res to show that both were passed
+        return _.extend({}, req.headers, res.headers);
       }
     };
 
@@ -538,7 +554,35 @@ tape('success conditions tests', (test) => {
 
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, (err, results) => {
+    // setup non-empty req and res so their contents can be used later by MockServiceConfig
+    const req = {
+      // referenced in getUrl
+      endpoint: 'some_endpoint',
+      // referenced in getParameters
+      params: {
+        req_param1: 'req_param1 value',
+        req_param2: 'req_param2 value'
+      },
+      // referenced in getHeaders
+      headers: {
+        req_header1: 'req_header1 value',
+        req_header2: 'req_header2 value'
+      }
+    };
+    const res = {
+      // referenced in getParameters
+      params: {
+        res_param1: 'res_param1 value',
+        res_param2: 'res_param2 value'
+      },
+      // referenced in getHeaders
+      headers: {
+        res_header1: 'res_header1 value',
+        res_header2: 'res_header2 value'
+      }
+    };
+
+    service(req, res, (err, results) => {
       t.notOk(err, 'should be no error');
       t.deepEquals(results, [1, 2, 3]);
       t.notOk(logger.hasErrorMessages());
@@ -591,8 +635,9 @@ tape('success conditions tests', (test) => {
         dnt: 1
       }
     };
+    const res = {};
 
-    service(req, (err, results) => {
+    service(req, res, (err, results) => {
       t.notOk(err, 'should be no error');
       t.deepEquals(results, [1, 2, 3]);
       t.notOk(logger.hasErrorMessages());
@@ -648,7 +693,7 @@ tape('success conditions tests', (test) => {
 
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}/`)));
 
-    service({}, (err, results) => {
+    service({}, {}, (err, results) => {
       t.notOk(err, 'should be no error');
       t.deepEquals(results, [1, 2, 3]);
       t.notOk(logger.hasErrorMessages());
@@ -662,3 +707,80 @@ tape('success conditions tests', (test) => {
   });
 
 });
+
+tape('callback-as-2nd-parameter tests', (test) => {
+  test.test('service disabled: 2nd parameter should be treated as callback when 3rd parameter is undefined', (t) => {
+    const logger = require('pelias-mock-logger')();
+
+    const MockServiceConfig = class extends ServiceConfiguration {
+      constructor(o) {
+        super('foo', { } );
+      }
+    };
+
+    const service = proxyquire('../service', {
+      'pelias-logger': logger
+    })(new MockServiceConfig());
+
+    t.ok(logger.isWarnMessage(/^foo service disabled$/));
+
+    service({}, (err) => {
+      t.equals(err, 'foo service disabled');
+      t.end();
+    });
+
+  });
+
+  test.test('service enabled: 2nd parameter should be treated as callback when 3rd parameter is undefined', (t) => {
+    const webServer = express();
+    webServer.get('/some_endpoint', (req, res, next) => {
+      t.notOk(req.headers.hasOwnProperty('dnt'), 'dnt header should not have been passed');
+
+      t.equals(req.headers.header1, 'header1 value', 'all headers should have been passed');
+      t.deepEquals(req.query, { param1: 'param1 value', param2: 'param2 value' });
+
+      res.status(200).json([1, 2, 3]);
+    });
+
+    const server = webServer.listen();
+    const port = server.address().port;
+
+    const logger = require('pelias-mock-logger')();
+
+    const MockServiceConfig = class extends ServiceConfiguration {
+      constructor(o) {
+        super('foo', { url: `http://localhost:${port}` } );
+      }
+      getUrl(req) {
+        t.deepEquals(req, { key: 'value' });
+        return `http://localhost:${port}/some_endpoint`;
+      }
+      getParameters(req, res) {
+        t.equals(res, undefined, 'should have defined res as undefined in 2-parameter call');
+        return { param1: 'param1 value', param2: 'param2 value' };
+      }
+      getHeaders(req, res) {
+        t.equals(res, undefined, 'should have defined res as undefined in 2-parameter call');
+        return { header1: 'header1 value' };
+      }
+    };
+
+    const service = proxyquire('../service', {
+      'pelias-logger': logger
+    })(new MockServiceConfig());
+
+    t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
+
+    service({ key: 'value' }, (err, results) => {
+      t.notOk(err, 'should be no error');
+      t.deepEquals(results, [1, 2, 3]);
+      t.notOk(logger.hasErrorMessages());
+      t.end();
+
+      server.close();
+
+    });
+
+  });
+
+});

From 873f56f876f6c567c0a9ef0288e60976bf6c72ca Mon Sep 17 00:00:00 2001
From: Stephen Hess <stephen.k.hess@gmail.com>
Date: Wed, 19 Jul 2017 17:20:11 -0400
Subject: [PATCH 2/4] pass res to synthesizeUrl for debug purposes

---
 service.js      | 10 ++---
 test/service.js | 97 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 85 insertions(+), 22 deletions(-)

diff --git a/service.js b/service.js
index c860f3d..b5b5835 100644
--- a/service.js
+++ b/service.js
@@ -80,8 +80,8 @@ module.exports = function setup(serviceConfig) {
             logger.error(`${serviceConfig.getBaseUrl()} [do_not_track] returned status ${err.status}: ${err.response.text}`);
             return callback(`${serviceConfig.getBaseUrl()} [do_not_track] returned status ${err.status}: ${err.response.text}`);
           } else {
-            logger.error(`${synthesizeUrl(serviceConfig, req)} returned status ${err.status}: ${err.response.text}`);
-            return callback(`${synthesizeUrl(serviceConfig, req)} returned status ${err.status}: ${err.response.text}`);
+            logger.error(`${synthesizeUrl(serviceConfig, req, res)} returned status ${err.status}: ${err.response.text}`);
+            return callback(`${synthesizeUrl(serviceConfig, req, res)} returned status ${err.status}: ${err.response.text}`);
           }
 
         }
@@ -91,7 +91,7 @@ module.exports = function setup(serviceConfig) {
           logger.error(`${serviceConfig.getBaseUrl()} [do_not_track]: ${JSON.stringify(err)}`);
           return callback(err);
         } else {
-          logger.error(`${serviceConfig.getUrl(req)}: ${JSON.stringify(err)}`);
+          logger.error(`${synthesizeUrl(serviceConfig, req, res)}: ${JSON.stringify(err)}`);
           return callback(err);
         }
 
@@ -111,8 +111,8 @@ module.exports = function setup(serviceConfig) {
           logger.error(`${serviceConfig.getBaseUrl()} [do_not_track] could not parse response: ${response.text}`);
           return callback(`${serviceConfig.getBaseUrl()} [do_not_track] could not parse response: ${response.text}`);
         } else {
-          logger.error(`${synthesizeUrl(serviceConfig, req)} could not parse response: ${response.text}`);
-          return callback(`${synthesizeUrl(serviceConfig, req)} could not parse response: ${response.text}`);
+          logger.error(`${synthesizeUrl(serviceConfig, req, res)} could not parse response: ${response.text}`);
+          return callback(`${synthesizeUrl(serviceConfig, req, res)} could not parse response: ${response.text}`);
         }
 
       });
diff --git a/test/service.js b/test/service.js
index d15f042..9ba05a2 100644
--- a/test/service.js
+++ b/test/service.js
@@ -166,7 +166,11 @@ tape('failure conditions tests', (test) => {
         super('foo', { url: `http://localhost:${port}` } );
       }
       getUrl(req) {
-        return `http://localhost:${port}/built_url`;
+        return `http://localhost:${port}/some_endpoint`;
+      }
+      getParameters(req) {
+        // combine req and res to show that both were passed
+        return _.extend({}, req.params, res.params);
       }
     };
 
@@ -174,12 +178,26 @@ tape('failure conditions tests', (test) => {
       'pelias-logger': logger
     })(new MockServiceConfig());
 
+    // setup non-empty req and res so their contents can be used later by MockServiceConfig
+    const req = {
+      // referenced in getParameters
+      params: {
+        req_param: 'req_param value'
+      }
+    };
+    const res = {
+      // referenced in getParameters
+      params: {
+        res_param: 'res_param value'
+      }
+    };
+
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, {}, (err, results) => {
+    service(req, res, (err, results) => {
       t.equals(err.code, 'ECONNREFUSED');
       t.notOk(results);
-      t.ok(logger.isErrorMessage(new RegExp(`^http://localhost:${port}/built_url: .*ECONNREFUSED`)),
+      t.ok(logger.isErrorMessage(new RegExp(`^http://localhost:${port}/some_endpoint\\?req_param=req_param%20value&res_param=res_param%20value: .*ECONNREFUSED`)),
         'there should be a connection refused error message');
       t.end();
 
@@ -239,7 +257,7 @@ tape('failure conditions tests', (test) => {
       t.notOk(req.headers.hasOwnProperty('dnt'), 'dnt header should not have been passed');
 
       t.equals(req.headers.header1, 'header1 value', 'all headers should have been passed');
-      t.deepEquals(req.query, { param1: 'param1 value', param2: 'param2 value' });
+      t.deepEquals(req.query, { req_param: 'req_param value', res_param: 'res_param value' });
 
       res.status(400).send('a bad request was made');
     });
@@ -257,7 +275,8 @@ tape('failure conditions tests', (test) => {
         return `http://localhost:${port}/some_endpoint`;
       }
       getParameters(req) {
-        return { param1: 'param1 value', param2: 'param2 value' };
+        // combine req and res to show that both were passed
+        return _.extend({}, req.params, res.params);
       }
       getHeaders(req) {
         return { header1: 'header1 value' };
@@ -268,13 +287,27 @@ tape('failure conditions tests', (test) => {
       'pelias-logger': logger
     })(new MockServiceConfig());
 
+    // setup non-empty req and res so their contents can be used later by MockServiceConfig
+    const req = {
+      // referenced in getParameters
+      params: {
+        req_param: 'req_param value'
+      }
+    };
+    const res = {
+      // referenced in getParameters
+      params: {
+        res_param: 'res_param value'
+      }
+    };
+
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, {}, (err, results) => {
-      t.equals(err, `http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
+    service(req, res, (err, results) => {
+      t.equals(err, `http://localhost:${port}/some_endpoint?req_param=req_param%20value&res_param=res_param%20value ` +
         'returned status 400: a bad request was made');
       t.notOk(results);
-      t.ok(logger.isErrorMessage(`http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
+      t.ok(logger.isErrorMessage(`http://localhost:${port}/some_endpoint?req_param=req_param%20value&res_param=res_param%20value ` +
         `returned status 400: a bad request was made`));
       t.end();
 
@@ -347,7 +380,7 @@ tape('failure conditions tests', (test) => {
       t.notOk(req.headers.hasOwnProperty('dnt'), 'dnt header should not have been passed');
 
       t.equals(req.headers.header1, 'header1 value', 'all headers should have been passed');
-      t.deepEquals(req.query, { param1: 'param1 value', param2: 'param2 value' });
+      t.deepEquals(req.query, { req_param: 'req_param value', res_param: 'res_param value' });
 
       res.set('Content-Type', 'text/plain').status(200).send('this is not parseable as JSON');
     });
@@ -365,7 +398,8 @@ tape('failure conditions tests', (test) => {
         return `http://localhost:${port}/some_endpoint`;
       }
       getParameters(req) {
-        return { param1: 'param1 value', param2: 'param2 value' };
+        // combine req and res to show that both were passed
+        return _.extend({}, req.params, res.params);
       }
       getHeaders(req) {
         return { header1: 'header1 value' };
@@ -376,13 +410,27 @@ tape('failure conditions tests', (test) => {
       'pelias-logger': logger
     })(new MockServiceConfig());
 
+    // setup non-empty req and res so their contents can be used later by MockServiceConfig
+    const req = {
+      // referenced in getParameters
+      params: {
+        req_param: 'req_param value'
+      }
+    };
+    const res = {
+      // referenced in getParameters
+      params: {
+        res_param: 'res_param value'
+      }
+    };
+
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, {}, (err, results) => {
-      t.equals(err, `http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
+    service(req, res, (err, results) => {
+      t.equals(err, `http://localhost:${port}/some_endpoint?req_param=req_param%20value&res_param=res_param%20value ` +
         `could not parse response: this is not parseable as JSON`);
       t.notOk(results, 'should return undefined');
-      t.ok(logger.isErrorMessage(`http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
+      t.ok(logger.isErrorMessage(`http://localhost:${port}/some_endpoint?req_param=req_param%20value&res_param=res_param%20value ` +
         `could not parse response: this is not parseable as JSON`));
       t.end();
 
@@ -471,7 +519,8 @@ tape('failure conditions tests', (test) => {
         return `http://localhost:${port}/some_endpoint`;
       }
       getParameters(req) {
-        return { param1: 'param1 value', param2: 'param2 value' };
+        // combine req and res to show that both were passed
+        return _.extend({}, req.params, res.params);
       }
       getHeaders(req) {
         return { header1: 'header1 value' };
@@ -485,13 +534,27 @@ tape('failure conditions tests', (test) => {
       'pelias-logger': logger
     })(new MockServiceConfig());
 
+    // setup non-empty req and res so their contents can be used later by MockServiceConfig
+    const req = {
+      // referenced in getParameters
+      params: {
+        req_param: 'req_param value'
+      }
+    };
+    const res = {
+      // referenced in getParameters
+      params: {
+        res_param: 'res_param value'
+      }
+    };
+
     t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`)));
 
-    service({}, {}, (err, results) => {
-      t.equals(err, `http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
+    service(req, res, (err, results) => {
+      t.equals(err, `http://localhost:${port}/some_endpoint?req_param=req_param%20value&res_param=res_param%20value ` +
         'returned status 503: request timeout');
       t.notOk(results);
-      t.ok(logger.isErrorMessage(`http://localhost:${port}/some_endpoint?param1=param1%20value&param2=param2%20value ` +
+      t.ok(logger.isErrorMessage(`http://localhost:${port}/some_endpoint?req_param=req_param%20value&res_param=res_param%20value ` +
         `returned status 503: request timeout`));
       t.equals(requestCount, 2);
       t.end();

From 32772adaf9fac41a1b488596f7c47bdb0e3d8d36 Mon Sep 17 00:00:00 2001
From: Stephen Hess <stephen.k.hess@gmail.com>
Date: Thu, 27 Jul 2017 10:01:49 -0400
Subject: [PATCH 3/4] save off url for logging to avoid recomputation

---
 service.js | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/service.js b/service.js
index b5b5835..e790756 100644
--- a/service.js
+++ b/service.js
@@ -1,3 +1,5 @@
+'use strict';
+
 const request = require('superagent');
 const _ = require('lodash');
 
@@ -56,13 +58,16 @@ module.exports = function setup(serviceConfig) {
 
     // save off do_not_track value for later check
     const do_not_track = isDoNotTrack(req.headers);
+    let url_for_logging;
 
     if (do_not_track) {
       headers.dnt = '1';
-      logger.debug(`${serviceConfig.getName()}: ${serviceConfig.getBaseUrl()}`);
+      url_for_logging = serviceConfig.getBaseUrl();
+      logger.debug(`${serviceConfig.getName()}: ${url_for_logging}`);
 
     } else {
-      logger.debug(`${serviceConfig.getName()}: ${synthesizeUrl(serviceConfig, req, res)}`);
+      url_for_logging = synthesizeUrl(serviceConfig, req, res);
+      logger.debug(`${serviceConfig.getName()}: ${url_for_logging}`);
 
     }
 
@@ -77,21 +82,21 @@ module.exports = function setup(serviceConfig) {
         if (err.status) {
           // first handle case where a non-200 was returned
           if (do_not_track) {
-            logger.error(`${serviceConfig.getBaseUrl()} [do_not_track] returned status ${err.status}: ${err.response.text}`);
-            return callback(`${serviceConfig.getBaseUrl()} [do_not_track] returned status ${err.status}: ${err.response.text}`);
+            logger.error(`${url_for_logging} [do_not_track] returned status ${err.status}: ${err.response.text}`);
+            return callback(`${url_for_logging} [do_not_track] returned status ${err.status}: ${err.response.text}`);
           } else {
-            logger.error(`${synthesizeUrl(serviceConfig, req, res)} returned status ${err.status}: ${err.response.text}`);
-            return callback(`${synthesizeUrl(serviceConfig, req, res)} returned status ${err.status}: ${err.response.text}`);
+            logger.error(`${url_for_logging} returned status ${err.status}: ${err.response.text}`);
+            return callback(`${url_for_logging} returned status ${err.status}: ${err.response.text}`);
           }
 
         }
 
         // handle case that something catastrophic happened while contacting the server
         if (do_not_track) {
-          logger.error(`${serviceConfig.getBaseUrl()} [do_not_track]: ${JSON.stringify(err)}`);
+          logger.error(`${url_for_logging} [do_not_track]: ${JSON.stringify(err)}`);
           return callback(err);
         } else {
-          logger.error(`${synthesizeUrl(serviceConfig, req, res)}: ${JSON.stringify(err)}`);
+          logger.error(`${url_for_logging}: ${JSON.stringify(err)}`);
           return callback(err);
         }
 
@@ -108,11 +113,11 @@ module.exports = function setup(serviceConfig) {
         }
 
         if (do_not_track) {
-          logger.error(`${serviceConfig.getBaseUrl()} [do_not_track] could not parse response: ${response.text}`);
-          return callback(`${serviceConfig.getBaseUrl()} [do_not_track] could not parse response: ${response.text}`);
+          logger.error(`${url_for_logging} [do_not_track] could not parse response: ${response.text}`);
+          return callback(`${url_for_logging} [do_not_track] could not parse response: ${response.text}`);
         } else {
-          logger.error(`${synthesizeUrl(serviceConfig, req, res)} could not parse response: ${response.text}`);
-          return callback(`${synthesizeUrl(serviceConfig, req, res)} could not parse response: ${response.text}`);
+          logger.error(`${url_for_logging} could not parse response: ${response.text}`);
+          return callback(`${url_for_logging} could not parse response: ${response.text}`);
         }
 
       });

From e19c8d14fc246aa6f162b09c27d5835f31d2915e Mon Sep 17 00:00:00 2001
From: Stephen Hess <stephen.k.hess@gmail.com>
Date: Thu, 27 Jul 2017 14:05:31 -0400
Subject: [PATCH 4/4] reduced 2 debug lines to 1

---
 service.js | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/service.js b/service.js
index e790756..1e2abaa 100644
--- a/service.js
+++ b/service.js
@@ -63,14 +63,12 @@ module.exports = function setup(serviceConfig) {
     if (do_not_track) {
       headers.dnt = '1';
       url_for_logging = serviceConfig.getBaseUrl();
-      logger.debug(`${serviceConfig.getName()}: ${url_for_logging}`);
-
     } else {
       url_for_logging = synthesizeUrl(serviceConfig, req, res);
-      logger.debug(`${serviceConfig.getName()}: ${url_for_logging}`);
-
     }
 
+    logger.debug(`${serviceConfig.getName()}: ${url_for_logging}`);
+
     request
       .get(serviceConfig.getUrl(req))
       .set(headers)