From db0e063d8c0d64dc6ae5a7c4e9709eda528fed14 Mon Sep 17 00:00:00 2001
From: Diana Shkolnikov <dianashk@gmail.com>
Date: Mon, 31 Aug 2015 00:49:02 -0400
Subject: [PATCH] Refactor controllers to be simple midddleware

Refactor search and doc controllers to allow for post-processing middleware
to handle geojson and response sending. Allows for a much more flexible routing scheme.
---
 app.js                         | 78 ++++++++++++++++++++++------------
 controller/place.js            | 14 +++---
 controller/search.js           | 12 ++----
 middleware/geocodeJSON.js      | 73 +++++++++++++++++++++++++++++++
 middleware/sendJSON.js         |  6 +++
 sanitiser/coarse.js            | 30 -------------
 sanitiser/suggest.js           | 29 -------------
 test/unit/controller/place.js  | 14 ++++--
 test/unit/controller/search.js | 14 ++++--
 test/unit/run.js               |  2 -
 test/unit/sanitiser/coarse.js  | 72 -------------------------------
 11 files changed, 159 insertions(+), 185 deletions(-)
 create mode 100644 middleware/geocodeJSON.js
 create mode 100644 middleware/sendJSON.js
 delete mode 100644 sanitiser/coarse.js
 delete mode 100644 sanitiser/suggest.js
 delete mode 100644 test/unit/sanitiser/coarse.js

diff --git a/app.js b/app.js
index ccdd5116..97ab9192 100644
--- a/app.js
+++ b/app.js
@@ -1,52 +1,76 @@
 
+var Router = require('express').Router;
+
 var app = require('express')();
+
 var peliasConfig = require( 'pelias-config' ).generate().api;
 
+
 if( peliasConfig.accessLog ){
   app.use( require( './middleware/access_log' )( peliasConfig.accessLog ) );
 }
 
-/** ----------------------- middleware ----------------------- **/
+/** ----------------------- pre-processing-middleware ----------------------- **/
 
 app.use( require('./middleware/headers') );
 app.use( require('./middleware/cors') );
 app.use( require('./middleware/jsonp') );
 
-/** ----------------------- sanitisers ----------------------- **/
+/**
+ * Helper function for creating routers
+ *
+ * @param {[{function}]} functions
+ * @returns {express.Router}
+ */
+function createRouter(functions) {
+  var router = Router(); // jshint ignore:line
+  functions.forEach(function (f) {
+    router.use(f);
+  });
+  return router;
+}
+
+var routers = {};
 
-var sanitisers = {};
-sanitisers.place      = require('./sanitiser/place');
-sanitisers.suggest  = require('./sanitiser/suggest');
-sanitisers.search   = require('./sanitiser/search');
-sanitisers.coarse   = require('./sanitiser/coarse');
-sanitisers.reverse  = require('./sanitiser/reverse');
+routers.search = createRouter([
+  require('./sanitiser/search').middleware,
+  require('./controller/search')()
+]);
 
-/** ----------------------- controllers ----------------------- **/
+routers.reverse = createRouter([
+  require('./sanitiser/reverse').middleware,
+  require('./controller/search')(undefined, require('./query/reverse'))
+]);
 
-var controllers     = {};
-controllers.index   = require('./controller/index');
-controllers.place     = require('./controller/place');
-controllers.search  = require('./controller/search');
+routers.place = createRouter([
+  require('./sanitiser/place').middleware,
+  require('./controller/place')()
+]);
+
+routers.index = createRouter([
+  require('./controller/index')()
+]);
 
-/** ----------------------- routes ----------------------- **/
 
 // api root
-app.get( '/', controllers.index() );
+app.get( '/', routers.index );
+
+app.get( '/place', routers.place );
+
+app.get( '/autocomplete', routers.search );
+
+app.get( '/search', routers.search);
+app.post( '/search', routers.search);
+
+app.get( '/reverse', routers.reverse );
 
-// place API
-app.get( '/place', sanitisers.place.middleware, controllers.place() );
 
-// suggest APIs
-app.get( '/suggest', sanitisers.search.middleware, controllers.search() );
-app.get( '/suggest/nearby', sanitisers.suggest.middleware, controllers.search() );
-app.get( '/suggest/coarse', sanitisers.coarse.middleware, controllers.search() );
+/** -------------------- post-processing-middleware ------------------**/
 
-// search APIs
-app.get( '/search', sanitisers.search.middleware, controllers.search() );
-app.get( '/search/coarse', sanitisers.coarse.middleware, controllers.search() );
+// TODO: name mapping for admin values (admin0 => country, etc)
+app.use(require('./middleware/geocodeJSON')());
+app.use(require('./middleware/sendJSON'));
 
-// reverse API
-app.get( '/reverse', sanitisers.reverse.middleware, controllers.search(undefined, require('./query/reverse')) );
 
 /** ----------------------- error middleware ----------------------- **/
 
@@ -54,4 +78,4 @@ app.use( require('./middleware/404') );
 app.use( require('./middleware/408') );
 app.use( require('./middleware/500') );
 
-module.exports = app;
+module.exports = app;
\ No newline at end of file
diff --git a/controller/place.js b/controller/place.js
index 42cb0069..5d3a2402 100644
--- a/controller/place.js
+++ b/controller/place.js
@@ -1,5 +1,4 @@
 var service = { mget: require('../service/mget') };
-var geojsonify = require('../helper/geojsonify').search;
 
 function setup( backend ){
   // allow overriding of dependencies
@@ -14,18 +13,15 @@ function setup( backend ){
       };
     });
 
-    service.mget( backend, query, function( err, docs ){
+    service.mget( backend, query, function( err, docs ) {
       // error handler
       if( err ){ return next( err ); }
 
-      // convert docs to geojson
-      var geojson = geojsonify( docs, req.clean );
-
-      // response envelope
-      geojson.date = new Date().getTime();
+      req.results = {
+        data: docs
+      };
 
-      // respond
-      return res.status(200).json( geojson );
+      next();
     });
   }
 
diff --git a/controller/search.js b/controller/search.js
index 8c9f7970..f38091cd 100644
--- a/controller/search.js
+++ b/controller/search.js
@@ -1,6 +1,5 @@
 
 var service = { search: require('../service/search') };
-var geojsonify = require('../helper/geojsonify').search;
 
 function setup( backend, query ){
 
@@ -32,14 +31,11 @@ function setup( backend, query ){
       // error handler
       if( err ){ return next( err ); }
 
-      // convert docs to geojson
-      var geojson = geojsonify( docs, req.clean );
+      req.results = {
+        data: docs
+      };
 
-      // response envelope
-      geojson.date = new Date().getTime();
-
-      // respond
-      return res.status(200).json( geojson );
+      next();
     });
 
   }
diff --git a/middleware/geocodeJSON.js b/middleware/geocodeJSON.js
new file mode 100644
index 00000000..a5e136b0
--- /dev/null
+++ b/middleware/geocodeJSON.js
@@ -0,0 +1,73 @@
+var extend = require('extend');
+var geojsonify = require('../helper/geojsonify').search;
+
+function setup(peliasConfig) {
+
+  peliasConfig = peliasConfig || require( 'pelias-config' ).generate().api;
+  
+  function middleware(req, res, next) {
+    return convertToGeocodeJSON(peliasConfig, req, next);
+  }
+
+  return middleware;
+}
+
+function convertToGeocodeJSON(peliasConfig, req, next) {
+
+  req.results.geojson = { geocoding: {} };
+
+  // REQUIRED. A semver.org compliant version number. Describes the version of
+  // the GeocodeJSON spec that is implemented by this instance.
+  req.results.geojson.geocoding.version = '0.1';
+
+  // OPTIONAL. Default: null. The licence of the data. In case of multiple sources,
+  // and then multiple licences, can be an object with one key by source.
+  // Can be a freeform text property describing the licensing details.
+  // Can be a URI on the server, which outlines licensing details.
+  req.results.geojson.geocoding.license = peliasConfig.host + '/license'; // TODO: add to config
+
+  // OPTIONAL. Default: null. The attribution of the data. In case of multiple sources,
+  // and then multiple attributions, can be an object with one key by source.
+  // Can be a URI on the server, which outlines attribution details.
+  req.results.geojson.geocoding.attribution = peliasConfig.host + '/attribution'; // TODO: add to config
+
+  // OPTIONAL. Default: null. The query that has been issued to trigger the
+  // search.
+  // Freeform object.
+  // This is the equivalent of how the engine interpreted the incoming request.
+  // Helpful for debugging and understanding how the input impacts results.
+  req.results.geojson.geocoding.query = req.clean;
+
+  // OPTIONAL. Warnings and errors.
+  addMessages(req.results, 'warnings', req.results.geojson.geocoding);
+  addMessages(req.results, 'errors', req.results.geojson.geocoding);
+
+  // OPTIONAL
+  // Freeform
+  addEngine(peliasConfig, req.results.geojson.geocoding);
+
+  // response envelope
+  req.results.geojson.geocoding.timestamp = new Date().getTime();
+
+  // convert docs to geojson and merge with geocoding block
+  extend(req.results.geojson, geojsonify(req.results.data, req.clean));
+
+  next();
+}
+
+function addMessages(results, msgType, geocoding) {
+  if (results.hasOwnProperty(msgType)) {
+    geocoding.messages = geocoding.messages || {};
+    geocoding.messages[msgType] = results[msgType];
+  }
+}
+
+function addEngine(peliasConfig, geocoding) {
+  geocoding.engine = {
+    name: 'Pelias',
+    author: 'Mapzen',
+    version: peliasConfig.version // TODO: add to config
+  };
+}
+
+module.exports = setup;
diff --git a/middleware/sendJSON.js b/middleware/sendJSON.js
new file mode 100644
index 00000000..8ab0bc88
--- /dev/null
+++ b/middleware/sendJSON.js
@@ -0,0 +1,6 @@
+function sendJSONResponse(req, res) {
+  // respond
+  return res.status(200).json(req.results.geojson);
+}
+
+module.exports = sendJSONResponse;
\ No newline at end of file
diff --git a/sanitiser/coarse.js b/sanitiser/coarse.js
deleted file mode 100644
index 8bd6a940..00000000
--- a/sanitiser/coarse.js
+++ /dev/null
@@ -1,30 +0,0 @@
-
-var _sanitize = require('../sanitiser/_sanitize'),
-    sanitizers = {
-      input: require('../sanitiser/_input'),
-      size: require('../sanitiser/_size'),
-      layers: function( req ) {
-        req.query.layers = 'admin';
-        var layers = require('../sanitiser/_layers');
-        return layers(req);
-      },
-      latlonzoom: require('../sanitiser/_geo'),
-      details: require('../sanitiser/_details')
-    };
-
-var sanitize = function(req, cb) { _sanitize(req, sanitizers, cb); };
-
-// export sanitize for testing
-module.exports.sanitize = sanitize;
-
-// middleware
-module.exports.middleware = function( req, res, next ){
-  sanitize( req, function( err, clean ){
-    if( err ){
-      res.status(400); // 400 Bad Request
-      return next(err);
-    }
-    req.clean = clean;
-    next();
-  });
-};
diff --git a/sanitiser/suggest.js b/sanitiser/suggest.js
deleted file mode 100644
index 3741f3c4..00000000
--- a/sanitiser/suggest.js
+++ /dev/null
@@ -1,29 +0,0 @@
-
-var _sanitize = require('../sanitiser/_sanitize'),
-    sanitizers = {
-      input: require('../sanitiser/_input'),
-      size: require('../sanitiser/_size'),
-      layers: require('../sanitiser/_layers'),
-      details: require('../sanitiser/_details'),
-      latlonzoom: function( req ) {
-        var geo = require('../sanitiser/_geo');
-        return geo(req, true);
-      }
-    };
-
-var sanitize = function(req, cb) { _sanitize(req, sanitizers, cb); };
-
-// export sanitize for testing
-module.exports.sanitize = sanitize;
-
-// middleware
-module.exports.middleware = function( req, res, next ){
-  sanitize( req, function( err, clean ){
-    if( err ){
-      res.status(400); // 400 Bad Request
-      return next(err);
-    }
-    req.clean = clean;
-    next();
-  });
-};
diff --git a/test/unit/controller/place.js b/test/unit/controller/place.js
index 9fe9115a..9c93e2d8 100644
--- a/test/unit/controller/place.js
+++ b/test/unit/controller/place.js
@@ -55,10 +55,13 @@ module.exports.tests.functional_success = function(test, common) {
         t.equal(json.type, 'FeatureCollection', 'valid geojson');
         t.true(Array.isArray(json.features), 'features is array');
         t.deepEqual(json.features, expected, 'values correctly mapped');
-        t.end();
       }
     };
-    controller( { clean: { ids: [ {'id' : 123, 'type': 'a' } ] } }, res );
+    var next = function next() {
+      t.equal(arguments.length, 0, 'next was called without error');
+      t.end();
+    };
+    controller( { clean: { ids: [ {'id' : 123, 'type': 'a' } ] } }, res, next );
   });
 
   var detailed_expectation = [{
@@ -109,10 +112,13 @@ module.exports.tests.functional_success = function(test, common) {
         t.equal(json.type, 'FeatureCollection', 'valid geojson');
         t.true(Array.isArray(json.features), 'features is array');
         t.deepEqual(json.features, detailed_expectation, 'values correctly mapped along with details');
-        t.end();
       }
     };
-    controller( { clean: { ids: [ {'id' : 123, 'type': 'a' } ], details: true } }, res );
+    var next = function next() {
+      t.equal(arguments.length, 0, 'next was called without error');
+      t.end();
+    };
+    controller( { clean: { ids: [ {'id' : 123, 'type': 'a' } ], details: true } }, res, next );
   });
 };
 
diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js
index 11b7515d..135393d9 100644
--- a/test/unit/controller/search.js
+++ b/test/unit/controller/search.js
@@ -57,10 +57,13 @@ module.exports.tests.functional_success = function(test, common) {
         t.equal(json.type, 'FeatureCollection', 'valid geojson');
         t.true(Array.isArray(json.features), 'features is array');
         t.deepEqual(json.features, expected, 'values correctly mapped');
-        t.end();
       }
     };
-    controller( { clean: { a: 'b' } }, res );
+    var next = function next() {
+      t.equal(arguments.length, 0, 'next was called without error');
+      t.end();
+    };
+    controller( { clean: { a: 'b' } }, res, next );
   });
 
   var detailed_expectation = [{
@@ -111,10 +114,13 @@ module.exports.tests.functional_success = function(test, common) {
         t.equal(json.type, 'FeatureCollection', 'valid geojson');
         t.true(Array.isArray(json.features), 'features is array');
         t.deepEqual(json.features, detailed_expectation, 'values with details correctly mapped');
-        t.end();
       }
     };
-    controller( { clean: { a: 'b', details: true } }, res );
+    var next = function next() {
+      t.equal(arguments.length, 0, 'next was called without error');
+      t.end();
+    };
+    controller( { clean: { a: 'b', details: true } }, res, next );
   });
 };
 
diff --git a/test/unit/run.js b/test/unit/run.js
index 96e146cf..619da765 100644
--- a/test/unit/run.js
+++ b/test/unit/run.js
@@ -8,11 +8,9 @@ var tests = [
   require('./controller/search'),
   require('./service/mget'),
   require('./service/search'),
-  require('./sanitiser/suggest'),
   require('./sanitiser/search'),
   require('./sanitiser/reverse'),
   require('./sanitiser/place'),
-  require('./sanitiser/coarse'),
   require('./query/indeces'),
   require('./query/sort'),
   require('./query/search'),
diff --git a/test/unit/sanitiser/coarse.js b/test/unit/sanitiser/coarse.js
deleted file mode 100644
index 13eca7c2..00000000
--- a/test/unit/sanitiser/coarse.js
+++ /dev/null
@@ -1,72 +0,0 @@
-
-var coarse  = require('../../../sanitiser/coarse'),
-    _sanitize    = coarse.sanitize,
-    middleware   = coarse.middleware,
-    valid_layers = [ 'admin0', 'admin1', 'admin2', 'neighborhood', 'locality', 'local_admin' ],
-    defaultClean = require('../sanitiser/_input').defaultClean,    
-    sanitize = function(query, cb) { _sanitize({'query':query}, cb); };
-
-module.exports.tests = {};
-
-module.exports.tests.interface = function(test, common) {
-  test('sanitize interface', function(t) {
-    t.equal(typeof sanitize, 'function', 'sanitize is a function');
-    t.equal(sanitize.length, 2, 'sanitize interface');
-    t.end();
-  });
-  test('middleware interface', function(t) {
-    t.equal(typeof middleware, 'function', 'middleware is a function');
-    t.equal(middleware.length, 3, 'sanitizee has a valid middleware');
-    t.end();
-  });
-};
-
-module.exports.tests.layers = function(test, common) {
-  test('valid layers', function(t) {
-    sanitize({ input: 'test', lat: 0, lon: 0 }, function( err, clean ){
-      t.equal(err, undefined, 'no error');
-      t.deepEqual(clean.layers, valid_layers, 'layers set correctly');
-    });
-    t.end();
-  });
-};
-
-module.exports.tests.middleware_failure = function(test, common) {
-  test('middleware failure', function(t) {
-    var res = { status: function( code ){
-      t.equal(code, 400, 'status set');
-    }};
-    var next = function( message ){
-      var defaultError = 'invalid param \'input\': text length, must be >0';
-      t.equal(message, defaultError);
-      t.end();
-    };
-    middleware( {}, res, next );
-  });
-};
-
-module.exports.tests.middleware_success = function(test, common) {
-  test('middleware success', function(t) {
-    var req = { query: { input: 'test', lat: 0, lon: 0 }};
-    var clean = defaultClean;
-    clean.layers = valid_layers;
-
-    var next = function( message ){
-      t.equal(message, undefined, 'no error message set');
-      t.deepEqual(req.clean, clean);
-      t.end();
-    };
-    middleware( req, undefined, next );
-  });
-};
-
-module.exports.all = function (tape, common) {
-
-  function test(name, testFunction) {
-    return tape('SANTIZE /coarse ' + name, testFunction);
-  }
-
-  for( var testCase in module.exports.tests ){
-    module.exports.tests[testCase](test, common);
-  }
-};
\ No newline at end of file