diff --git a/controller/coarse_reverse.js b/controller/coarse_reverse.js new file mode 100644 index 00000000..dc48d6db --- /dev/null +++ b/controller/coarse_reverse.js @@ -0,0 +1,129 @@ +const logger = require('pelias-logger').get('coarse_reverse'); +const _ = require('lodash'); +const Document = require('pelias-model').Document; + +const granularities = [ + 'neighbourhood', + 'borough', + 'locality', + 'localadmin', + 'county', + 'macrocounty', + 'region', + 'macroregion', + 'dependency', + 'country' +]; + +function getMostGranularLayer(results) { + return granularities.find((granularity) => { + return results.hasOwnProperty(granularity); + }); +} + +function hasResultsAtRequestedLayers(results, layers) { + return _.intersection(layers, Object.keys(results)).length > 0; +} + +function synthesizeDoc(results) { + // now create a model.Document from what's level, using the most granular + // result available as the starting point + // the immediately above cannot be re-used since county may be the most + // granular layer requested but the results may start at region (no county found) + const most_granular_layer = getMostGranularLayer(results); + const id = results[most_granular_layer][0].id; + + const doc = new Document('whosonfirst', most_granular_layer, id.toString()); + doc.setName('default', results[most_granular_layer][0].name); + + if (results[most_granular_layer][0].hasOwnProperty('centroid')) { + doc.setCentroid( results[most_granular_layer][0].centroid ); + } + + if (results[most_granular_layer][0].hasOwnProperty('bounding_box')) { + const parsedBoundingBox = results[most_granular_layer][0].bounding_box.split(',').map(parseFloat); + doc.setBoundingBox({ + upperLeft: { + lat: parsedBoundingBox[3], + lon: parsedBoundingBox[0] + }, + lowerRight: { + lat: parsedBoundingBox[1], + lon: parsedBoundingBox[2] + } + }); + + } + + if (_.has(results, 'country[0].abbr')) { + doc.setAlpha3(results.country[0].abbr); + } + + // assign the administrative hierarchy + Object.keys(results).forEach((layer) => { + if (results[layer][0].hasOwnProperty('abbr')) { + doc.addParent(layer, results[layer][0].name, results[layer][0].id.toString(), results[layer][0].abbr); + } else { + doc.addParent(layer, results[layer][0].name, results[layer][0].id.toString()); + } + + }); + + const esDoc = doc.toESDocument(); + esDoc.data._id = esDoc._id; + esDoc.data._type = esDoc._type; + return esDoc.data; + +} + +function setup(service, should_execute) { + function controller(req, res, next) { + // do not run controller when a request validation error has occurred + if (!should_execute(req, res)) { + return next(); + } + + const centroid = { + lat: req.clean['point.lat'], + lon: req.clean['point.lon'] + }; + + service(centroid, (err, results) => { + // if there's an error, log it and bail + if (err) { + logger.error(err); + return next(); + } + + // find the finest granularity requested + const finest_granularity_requested = granularities.findIndex((granularity) => { + return req.clean.layers.indexOf(granularity) !== -1; + }); + + // now remove everything from the response that is more granular than the + // most granular layer requested. that is, if req.clean.layers=['county'], + // remove neighbourhoods, localities, and localadmins + Object.keys(results).forEach((layer) => { + if (granularities.indexOf(layer) < finest_granularity_requested) { + delete results[layer]; + } + }); + + res.meta = {}; + res.data = []; + // synthesize a doc from results if there's a result at the request layer(s) + if (hasResultsAtRequestedLayers(results, req.clean.layers)) { + res.data.push(synthesizeDoc(results)); + } + + return next(); + + }); + + } + + return controller; + +} + +module.exports = setup; diff --git a/controller/predicates/has_request_errors.js b/controller/predicates/has_request_errors.js new file mode 100644 index 00000000..ccc13361 --- /dev/null +++ b/controller/predicates/has_request_errors.js @@ -0,0 +1,5 @@ +const _ = require('lodash'); + +module.exports = (request, response) => { + return _.get(request, 'errors', []).length > 0; +}; diff --git a/controller/predicates/has_response_data.js b/controller/predicates/has_response_data.js new file mode 100644 index 00000000..a61907d4 --- /dev/null +++ b/controller/predicates/has_response_data.js @@ -0,0 +1,5 @@ +const _ = require('lodash'); + +module.exports = (request, response) => { + return _.get(response, 'data', []).length > 0; +}; diff --git a/controller/predicates/is_coarse_reverse.js b/controller/predicates/is_coarse_reverse.js new file mode 100644 index 00000000..c8a292dd --- /dev/null +++ b/controller/predicates/is_coarse_reverse.js @@ -0,0 +1,9 @@ +const _ = require('lodash'); + +const non_coarse_layers = ['address', 'street', 'venue']; + +module.exports = (req, res) => { + // returns true if layers is undefined, empty, or contains 'address', 'street', or 'venue' + return !_.isEmpty(req.clean.layers) && + _.intersection(req.clean.layers, non_coarse_layers).length === 0; +}; diff --git a/controller/predicates/is_pip_service_enabled.js b/controller/predicates/is_pip_service_enabled.js new file mode 100644 index 00000000..aa787d87 --- /dev/null +++ b/controller/predicates/is_pip_service_enabled.js @@ -0,0 +1,7 @@ +module.exports = (uri) => { + // this predicate relies upon the fact that the schema has already validated + // that api.pipService is a URI-formatted string + return (request, response) => { + return uri !== undefined; + }; +}; diff --git a/controller/search.js b/controller/search.js index 2813bf3c..989060a8 100644 --- a/controller/search.js +++ b/controller/search.js @@ -7,30 +7,13 @@ const logger = require('pelias-logger').get('api'); const logging = require( '../helper/logging' ); const retry = require('retry'); -function requestHasErrors(request) { - return _.get(request, 'errors', []).length > 0; -} - -function responseHasData(response) { - return _.get(response, 'data', []).length > 0; -} - function isRequestTimeout(err) { return _.get(err, 'status') === 408; } -function setup( apiConfig, esclient, query ){ +function setup( apiConfig, esclient, query, should_execute ){ function controller( req, res, next ){ - // do not run controller when a request - // validation error has occurred. - if (requestHasErrors(req)) { - return next(); - } - - // do not run controller if there are already results - // this was added during libpostal integration. if the libpostal parse/query - // doesn't return anything then fallback to old search-engine-y behavior - if (responseHasData(res)) { + if (!should_execute(req, res)) { return next(); } diff --git a/package.json b/package.json index 86e6a7f4..80b46808 100644 --- a/package.json +++ b/package.json @@ -47,7 +47,7 @@ "geojson": "^0.4.0", "geojson-extent": "^0.3.1", "geolib": "^2.0.18", - "iso3166-1": "^0.2.3", + "iso3166-1": "^0.3.0", "joi": "^10.1.0", "lodash": "^4.5.0", "markdown": "0.5.0", @@ -56,11 +56,13 @@ "pelias-config": "2.8.0", "pelias-labels": "1.5.1", "pelias-logger": "0.1.0", + "pelias-mock-logger": "^1.0.1", "pelias-model": "4.5.1", "pelias-query": "8.13.0", "pelias-text-analyzer": "1.7.2", + "predicates": "^1.0.1", "retry": "^0.10.1", - "stats-lite": "2.0.3", + "stats-lite": "^2.0.4", "superagent": "^3.2.1", "through2": "^2.0.3" }, @@ -72,6 +74,7 @@ "nsp": "^2.2.0", "precommit-hook": "^3.0.0", "proxyquire": "^1.7.10", + "request": "^2.79.0", "semantic-release": "^6.3.2", "source-map": "^0.5.6", "tap-dot": "1.0.5", diff --git a/routes/v1.js b/routes/v1.js index 64f820db..60e48172 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -1,6 +1,10 @@ var Router = require('express').Router; var elasticsearch = require('elasticsearch'); +const all = require('predicates').all; +const any = require('predicates').any; +const not = require('predicates').not; + /** ----------------------- sanitizers ----------------------- **/ var sanitizers = { autocomplete: require('../sanitizer/autocomplete'), @@ -20,6 +24,7 @@ var middleware = { /** ----------------------- controllers ----------------------- **/ var controllers = { + coarse_reverse: require('../controller/coarse_reverse'), mdToHTML: require('../controller/markdownToHtml'), place: require('../controller/place'), search: require('../controller/search'), @@ -55,6 +60,14 @@ var postProc = { assignLabels: require('../middleware/assignLabels') }; +// predicates that drive whether controller/search runs +const hasResponseData = require('../controller/predicates/has_response_data'); +const hasRequestErrors = require('../controller/predicates/has_request_errors'); +const isCoarseReverse = require('../controller/predicates/is_coarse_reverse'); + +// shorthand for standard early-exit conditions +const hasResponseDataOrRequestErrors = any(hasResponseData, hasRequestErrors); + /** * Append routes to app * @@ -64,6 +77,24 @@ var postProc = { function addRoutes(app, peliasConfig) { const esclient = elasticsearch.Client(peliasConfig.esclient); + const isPipServiceEnabled = require('../controller/predicates/is_pip_service_enabled')(peliasConfig.api.pipService); + const pipService = require('../service/pointinpolygon')(peliasConfig.api.pipService); + + const coarse_reverse_should_execute = all( + not(hasRequestErrors), isPipServiceEnabled, isCoarseReverse + ); + + // execute under the following conditions: + // - there are no errors or data + // - request is not coarse OR pip service is disabled + const original_reverse_should_execute = all( + not(hasResponseDataOrRequestErrors), + any( + not(isCoarseReverse), + not(isPipServiceEnabled) + ) + ); + var base = '/v1/'; /** ------------------------- routers ------------------------- **/ @@ -80,9 +111,9 @@ function addRoutes(app, peliasConfig) { middleware.calcSize(), // 3rd parameter is which query module to use, use fallback/geodisambiguation // first, then use original search strategy if first query didn't return anything - controllers.search(peliasConfig.api, esclient, queries.libpostal), + controllers.search(peliasConfig.api, esclient, queries.libpostal, not(hasResponseDataOrRequestErrors)), sanitizers.search_fallback.middleware, - controllers.search(peliasConfig.api, esclient, queries.fallback_to_old_prod), + controllers.search(peliasConfig.api, esclient, queries.fallback_to_old_prod, not(hasResponseDataOrRequestErrors)), postProc.trimByGranularity(), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig.api), @@ -101,7 +132,7 @@ function addRoutes(app, peliasConfig) { structured: createRouter([ sanitizers.structured_geocoding.middleware, middleware.calcSize(), - controllers.search(peliasConfig.api, esclient, queries.structured_geocoding), + controllers.search(peliasConfig.api, esclient, queries.structured_geocoding, not(hasResponseDataOrRequestErrors)), postProc.trimByGranularityStructured(), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig.api), @@ -119,7 +150,7 @@ function addRoutes(app, peliasConfig) { ]), autocomplete: createRouter([ sanitizers.autocomplete.middleware, - controllers.search(peliasConfig.api, esclient, queries.autocomplete), + controllers.search(peliasConfig.api, esclient, queries.autocomplete, not(hasResponseDataOrRequestErrors)), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig.api), postProc.dedupe(), @@ -135,7 +166,8 @@ function addRoutes(app, peliasConfig) { reverse: createRouter([ sanitizers.reverse.middleware, middleware.calcSize(), - controllers.search(peliasConfig.api, esclient, queries.reverse), + controllers.coarse_reverse(pipService, coarse_reverse_should_execute), + controllers.search(peliasConfig.api, esclient, queries.reverse, original_reverse_should_execute), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first @@ -153,7 +185,7 @@ function addRoutes(app, peliasConfig) { nearby: createRouter([ sanitizers.nearby.middleware, middleware.calcSize(), - controllers.search(peliasConfig.api, esclient, queries.reverse), + controllers.search(peliasConfig.api, esclient, queries.reverse, not(hasResponseDataOrRequestErrors)), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first diff --git a/schema.js b/schema.js index 61e6c258..78a9141f 100644 --- a/schema.js +++ b/schema.js @@ -25,7 +25,8 @@ module.exports = Joi.object().keys({ requestRetries: Joi.number().integer().min(0), localization: Joi.object().keys({ flipNumberAndStreetCountries: Joi.array().items(Joi.string().regex(/^[A-Z]{3}$/)) - }).unknown(false) + }).unknown(false), + pipService: Joi.string().uri({ scheme: /https?/ }) }).requiredKeys('version', 'indexName', 'host').unknown(true), esclient: Joi.object().keys({ diff --git a/service/pointinpolygon.js b/service/pointinpolygon.js new file mode 100644 index 00000000..21b6fdcf --- /dev/null +++ b/service/pointinpolygon.js @@ -0,0 +1,44 @@ +const logger = require( 'pelias-logger' ).get( 'pointinpolygon' ); +const request = require('request'); +const _ = require('lodash'); + +module.exports = (url) => { + if (!_.isEmpty(url)) { + logger.info(`using point-in-polygon service at ${url}`); + + return function pointinpolygon( centroid, callback ) { + const requestUrl = `${url}/${centroid.lon}/${centroid.lat}`; + + request.get(requestUrl, (err, response, body) => { + if (err) { + logger.error(JSON.stringify(err)); + callback(err); + } + else if (response.statusCode === 200) { + try { + const parsed = JSON.parse(body); + callback(err, parsed); + } + catch (err) { + logger.error(`${requestUrl}: could not parse response body: ${body}`); + callback(`${requestUrl} returned status 200 but with non-JSON response: ${body}`); + } + } + else { + logger.error(`${requestUrl} returned status ${response.statusCode}: ${body}`); + callback(`${requestUrl} returned status ${response.statusCode}: ${body}`); + } + }); + + }; + + } else { + logger.warn('point-in-polygon service disabled'); + + return (centroid, callback) => { + callback(`point-in-polygon service disabled, unable to resolve ${JSON.stringify(centroid)}`); + }; + + } + +}; diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js new file mode 100644 index 00000000..d8071e62 --- /dev/null +++ b/test/unit/controller/coarse_reverse.js @@ -0,0 +1,578 @@ +'use strict'; + +const setup = require('../../../controller/coarse_reverse'); +const proxyquire = require('proxyquire').noCallThru(); + +module.exports.tests = {}; + +module.exports.tests.interface = (test, common) => { + test('valid interface', (t) => { + t.equal(typeof setup, 'function', 'setup is a function'); + t.equal(typeof setup(), 'function', 'setup returns a controller'); + t.end(); + }); +}; + +module.exports.tests.early_exit_conditions = (test, common) => { + test('should_execute returning false should not call service', (t) => { + const service = () => { + throw Error('service should not have been called'); + }; + + const should_execute = () => { return false; }; + const controller = setup(service, should_execute); + + const req = { + clean: { + layers: ['locality'] + }, + errors: ['error'] + }; + + // verify that next was called + let next_was_called = false; + const next = () => { + next_was_called = true; + }; + + // passing res=undefined verifies that it wasn't interacted with + t.doesNotThrow(controller.bind(null, req, undefined, next)); + t.ok(next_was_called); + t.end(); + + }); + +}; + +module.exports.tests.error_conditions = (test, common) => { + test('service error should log and call next', (t) => { + const service = (point, callback) => { + callback('this is an error'); + }; + + const logger = require('pelias-mock-logger')(); + + const should_execute = () => { return true; }; + const controller = proxyquire('../../../controller/coarse_reverse', { + 'pelias-logger': logger + })(service, should_execute); + + const req = { + clean: { + layers: ['locality'], + point: { + lat: 12.121212, + lon: 21.212121 + } + } + }; + + // verify that next was called + let next_was_called = false; + const next = () => { + next_was_called = true; + }; + + // passing res=undefined verifies that it wasn't interacted with + controller(req, undefined, next); + + t.ok(logger.isErrorMessage('this is an error')); + t.ok(next_was_called); + t.end(); + + }); + +}; + +module.exports.tests.success_conditions = (test, common) => { + test('service returning results should use first entry for each layer', (t) => { + const service = (point, callback) => { + const results = { + neighbourhood: [ + { + id: 10, + name: 'neighbourhood name', + abbr: 'neighbourhood abbr', + centroid: { + lat: 12.121212, + lon: 21.212121 + }, + bounding_box: '-76.345902,40.006751,-76.254038,40.072939' + }, + { id: 11, name: 'neighbourhood name 2'} + ], + borough: [ + { id: 20, name: 'borough name', abbr: 'borough abbr'}, + { id: 21, name: 'borough name 2'} + ], + locality: [ + { id: 30, name: 'locality name', abbr: 'locality abbr'}, + { id: 31, name: 'locality name 2'} + ], + localadmin: [ + { id: 40, name: 'localadmin name', abbr: 'localadmin abbr'}, + { id: 41, name: 'localadmin name 2'} + ], + county: [ + { id: 50, name: 'county name', abbr: 'county abbr'}, + { id: 51, name: 'county name 2'} + ], + macrocounty: [ + { id: 60, name: 'macrocounty name', abbr: 'macrocounty abbr'}, + { id: 61, name: 'macrocounty name 2'} + ], + region: [ + { id: 70, name: 'region name', abbr: 'region abbr'}, + { id: 71, name: 'region name 2'} + ], + macroregion: [ + { id: 80, name: 'macroregion name', abbr: 'macroregion abbr'}, + { id: 81, name: 'macroregion name 2'} + ], + dependency: [ + { id: 90, name: 'dependency name', abbr: 'dependency abbr'}, + { id: 91, name: 'dependency name 2'} + ], + country: [ + { id: 100, name: 'country name', abbr: 'xyz'}, + { id: 101, name: 'country name 2'} + ] + }; + + callback(undefined, results); + }; + + const logger = require('pelias-mock-logger')(); + + const should_execute = () => { return true; }; + const controller = proxyquire('../../../controller/coarse_reverse', { + 'pelias-logger': logger + })(service, should_execute); + + const req = { + clean: { + layers: ['neighbourhood'], + point: { + lat: 12.121212, + lon: 21.212121 + } + } + }; + + const res = { }; + + // verify that next was called + let next_was_called = false; + const next = () => { + next_was_called = true; + }; + + controller(req, res, next); + + const expected = { + meta: {}, + data: [ + { + _id: '10', + _type: 'neighbourhood', + layer: 'neighbourhood', + source: 'whosonfirst', + source_id: '10', + name: { + 'default': 'neighbourhood name' + }, + phrase: { + 'default': 'neighbourhood name' + }, + parent: { + neighbourhood: ['neighbourhood name'], + neighbourhood_id: ['10'], + neighbourhood_a: ['neighbourhood abbr'], + borough: ['borough name'], + borough_id: ['20'], + borough_a: ['borough abbr'], + locality: ['locality name'], + locality_id: ['30'], + locality_a: ['locality abbr'], + localadmin: ['localadmin name'], + localadmin_id: ['40'], + localadmin_a: ['localadmin abbr'], + county: ['county name'], + county_id: ['50'], + county_a: ['county abbr'], + macrocounty: ['macrocounty name'], + macrocounty_id: ['60'], + macrocounty_a: ['macrocounty abbr'], + region: ['region name'], + region_id: ['70'], + region_a: ['region abbr'], + macroregion: ['macroregion name'], + macroregion_id: ['80'], + macroregion_a: ['macroregion abbr'], + dependency: ['dependency name'], + dependency_id: ['90'], + dependency_a: ['dependency abbr'], + country: ['country name'], + country_id: ['100'], + country_a: ['xyz'] + }, + alpha3: 'XYZ', + center_point: { + lat: 12.121212, + lon: 21.212121 + }, + bounding_box: '{"min_lat":40.006751,"max_lat":40.072939,"min_lon":-76.345902,"max_lon":-76.254038}' + } + ] + }; + + t.deepEquals(res, expected); + + t.notOk(logger.hasErrorMessages()); + t.ok(next_was_called); + t.end(); + + }); + + test('layers missing from results should be ignored', (t) => { + const service = (point, callback) => { + const results = { + neighbourhood: [ + { + id: 10, + name: 'neighbourhood name', + abbr: 'neighbourhood abbr', + centroid: { + lat: 12.121212, + lon: 21.212121 + }, + bounding_box: '-76.345902,40.006751,-76.254038,40.072939' + } + ] + }; + + callback(undefined, results); + }; + + const logger = require('pelias-mock-logger')(); + + const should_execute = () => { return true; }; + const controller = proxyquire('../../../controller/coarse_reverse', { + 'pelias-logger': logger + })(service, should_execute); + + const req = { + clean: { + layers: ['neighbourhood'], + point: { + lat: 12.121212, + lon: 21.212121 + } + } + }; + + const res = { }; + + // verify that next was called + let next_was_called = false; + const next = () => { + next_was_called = true; + }; + + controller(req, res, next); + + const expected = { + meta: {}, + data: [ + { + _id: '10', + _type: 'neighbourhood', + layer: 'neighbourhood', + source: 'whosonfirst', + source_id: '10', + name: { + 'default': 'neighbourhood name' + }, + phrase: { + 'default': 'neighbourhood name' + }, + parent: { + neighbourhood: ['neighbourhood name'], + neighbourhood_id: ['10'], + neighbourhood_a: ['neighbourhood abbr'] + }, + center_point: { + lat: 12.121212, + lon: 21.212121 + }, + bounding_box: '{"min_lat":40.006751,"max_lat":40.072939,"min_lon":-76.345902,"max_lon":-76.254038}' + } + ] + }; + + t.deepEquals(res, expected); + + t.notOk(logger.hasErrorMessages()); + t.ok(next_was_called); + t.end(); + + }); + + test('most granular layer missing centroid should not set', (t) => { + const service = (point, callback) => { + const results = { + neighbourhood: [ + { + id: 10, + name: 'neighbourhood name', + abbr: 'neighbourhood abbr', + bounding_box: '-76.345902,40.006751,-76.254038,40.072939' + } + ] + }; + + callback(undefined, results); + }; + + const logger = require('pelias-mock-logger')(); + + const should_execute = () => { return true; }; + const controller = proxyquire('../../../controller/coarse_reverse', { + 'pelias-logger': logger + })(service, should_execute); + + const req = { + clean: { + layers: ['neighbourhood'], + point: { + lat: 12.121212, + lon: 21.212121 + } + } + }; + + const res = { }; + + // verify that next was called + let next_was_called = false; + const next = () => { + next_was_called = true; + }; + + controller(req, res, next); + + const expected = { + meta: {}, + data: [ + { + _id: '10', + _type: 'neighbourhood', + layer: 'neighbourhood', + source: 'whosonfirst', + source_id: '10', + name: { + 'default': 'neighbourhood name' + }, + phrase: { + 'default': 'neighbourhood name' + }, + parent: { + neighbourhood: ['neighbourhood name'], + neighbourhood_id: ['10'], + neighbourhood_a: ['neighbourhood abbr'] + }, + bounding_box: '{"min_lat":40.006751,"max_lat":40.072939,"min_lon":-76.345902,"max_lon":-76.254038}' + } + ] + }; + + t.deepEquals(res, expected); + + t.notOk(logger.hasErrorMessages()); + t.ok(next_was_called); + t.end(); + + }); + + test('most granular layer missing bounding_box should not set', (t) => { + const service = (point, callback) => { + const results = { + neighbourhood: [ + { + id: 10, + name: 'neighbourhood name', + abbr: 'neighbourhood abbr', + centroid: { + lat: 12.121212, + lon: 21.212121 + } + } + ] + }; + + callback(undefined, results); + }; + + const logger = require('pelias-mock-logger')(); + + const should_execute = () => { return true; }; + const controller = proxyquire('../../../controller/coarse_reverse', { + 'pelias-logger': logger + })(service, should_execute); + + const req = { + clean: { + layers: ['neighbourhood'], + point: { + lat: 12.121212, + lon: 21.212121 + } + } + }; + + const res = { }; + + // verify that next was called + let next_was_called = false; + const next = () => { + next_was_called = true; + }; + + controller(req, res, next); + + const expected = { + meta: {}, + data: [ + { + _id: '10', + _type: 'neighbourhood', + layer: 'neighbourhood', + source: 'whosonfirst', + source_id: '10', + name: { + 'default': 'neighbourhood name' + }, + phrase: { + 'default': 'neighbourhood name' + }, + parent: { + neighbourhood: ['neighbourhood name'], + neighbourhood_id: ['10'], + neighbourhood_a: ['neighbourhood abbr'] + }, + center_point: { + lat: 12.121212, + lon: 21.212121 + } + } + ] + }; + + t.deepEquals(res, expected); + + t.notOk(logger.hasErrorMessages()); + t.ok(next_was_called); + t.end(); + + }); + +}; + +module.exports.tests.failure_conditions = (test, common) => { + test('service returning 0 results at the requested layer should return nothing', (t) => { + const service = (point, callback) => { + // response without neighbourhood results + const results = { + borough: [ + { id: 20, name: 'borough name', abbr: 'borough abbr'}, + { id: 21, name: 'borough name 2'} + ], + locality: [ + { id: 30, name: 'locality name', abbr: 'locality abbr'}, + { id: 31, name: 'locality name 2'} + ], + localadmin: [ + { id: 40, name: 'localadmin name', abbr: 'localadmin abbr'}, + { id: 41, name: 'localadmin name 2'} + ], + county: [ + { id: 50, name: 'county name', abbr: 'county abbr'}, + { id: 51, name: 'county name 2'} + ], + macrocounty: [ + { id: 60, name: 'macrocounty name', abbr: 'macrocounty abbr'}, + { id: 61, name: 'macrocounty name 2'} + ], + region: [ + { id: 70, name: 'region name', abbr: 'region abbr'}, + { id: 71, name: 'region name 2'} + ], + macroregion: [ + { id: 80, name: 'macroregion name', abbr: 'macroregion abbr'}, + { id: 81, name: 'macroregion name 2'} + ], + dependency: [ + { id: 90, name: 'dependency name', abbr: 'dependency abbr'}, + { id: 91, name: 'dependency name 2'} + ], + country: [ + { id: 100, name: 'country name', abbr: 'xyz'}, + { id: 101, name: 'country name 2'} + ] + }; + + callback(undefined, results); + }; + + const logger = require('pelias-mock-logger')(); + + const should_execute = () => { return true; }; + const controller = proxyquire('../../../controller/coarse_reverse', { + 'pelias-logger': logger + })(service, should_execute); + + const req = { + clean: { + layers: ['neighbourhood'], + point: { + lat: 12.121212, + lon: 21.212121 + } + } + }; + + const res = { }; + + // verify that next was called + let next_was_called = false; + const next = () => { + next_was_called = true; + }; + + controller(req, res, next); + + const expected = { + meta: {}, + data: [] + }; + + t.deepEquals(res, expected); + + t.notOk(logger.hasErrorMessages()); + t.ok(next_was_called); + t.end(); + + }); + +}; + +module.exports.all = (tape, common) => { + + function test(name, testFunction) { + return tape(`GET /coarse_reverse ${name}`, testFunction); + } + + for( const testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/controller/place.js b/test/unit/controller/place.js index 22622e05..25d99463 100644 --- a/test/unit/controller/place.js +++ b/test/unit/controller/place.js @@ -1,6 +1,6 @@ 'use strict'; -const setup = require('../../../controller/search'); +const setup = require('../../../controller/place'); const proxyquire = require('proxyquire').noCallThru(); module.exports.tests = {}; diff --git a/test/unit/controller/predicates/has_request_errors.js b/test/unit/controller/predicates/has_request_errors.js new file mode 100644 index 00000000..bd1868aa --- /dev/null +++ b/test/unit/controller/predicates/has_request_errors.js @@ -0,0 +1,60 @@ +'use strict'; + +const _ = require('lodash'); +const has_request_errors = require('../../../../controller/predicates/has_request_errors'); + +module.exports.tests = {}; + +module.exports.tests.interface = (test, common) => { + test('valid interface', (t) => { + t.equal(typeof has_request_errors, 'function', 'has_request_errors is a function'); + t.end(); + }); +}; + +module.exports.tests.true_conditions = (test, common) => { + test('request with non-empty errors should return true', (t) => { + const req = { + errors: ['error'] + }; + const res = {}; + + t.ok(has_request_errors(req, res)); + t.end(); + + }); + +}; + +module.exports.tests.false_conditions = (test, common) => { + test('response with undefined errors should return false', (t) => { + const req = {}; + const res = {}; + + t.notOk(has_request_errors(req, res)); + t.end(); + + }); + + test('response with empty errors array should return false', (t) => { + const req = { + errors: [] + }; + const res = {}; + + t.notOk(has_request_errors(req, res)); + t.end(); + + }); + +}; + +module.exports.all = (tape, common) => { + function test(name, testFunction) { + return tape(`GET /has_request_errors ${name}`, testFunction); + } + + for( const testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/controller/predicates/has_response_data.js b/test/unit/controller/predicates/has_response_data.js new file mode 100644 index 00000000..f073fc4d --- /dev/null +++ b/test/unit/controller/predicates/has_response_data.js @@ -0,0 +1,60 @@ +'use strict'; + +const _ = require('lodash'); +const has_response_data = require('../../../../controller/predicates/has_response_data'); + +module.exports.tests = {}; + +module.exports.tests.interface = (test, common) => { + test('valid interface', (t) => { + t.equal(typeof has_response_data, 'function', 'has_response_data is a function'); + t.end(); + }); +}; + +module.exports.tests.true_conditions = (test, common) => { + test('response with non-empty data should return true', (t) => { + const req = {}; + const res = { + data: [1] + }; + + t.ok(has_response_data(req, res)); + t.end(); + + }); + +}; + +module.exports.tests.false_conditions = (test, common) => { + test('response with undefined data should return true', (t) => { + const req = {}; + const res = {}; + + t.notOk(has_response_data(req, res)); + t.end(); + + }); + + test('response with empty data array should return true', (t) => { + const req = {}; + const res = { + data: [] + }; + + t.notOk(has_response_data(req, res)); + t.end(); + + }); + +}; + +module.exports.all = (tape, common) => { + function test(name, testFunction) { + return tape(`GET /has_response_data ${name}`, testFunction); + } + + for( const testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/controller/predicates/is_coarse_reverse.js b/test/unit/controller/predicates/is_coarse_reverse.js new file mode 100644 index 00000000..2a38e401 --- /dev/null +++ b/test/unit/controller/predicates/is_coarse_reverse.js @@ -0,0 +1,128 @@ +'use strict'; + +const _ = require('lodash'); +const is_coarse_reverse = require('../../../../controller/predicates/is_coarse_reverse'); + +const coarse_layers = [ + 'continent', + 'country', + 'dependency', + 'macroregion', + 'region', + 'locality', + 'localadmin', + 'macrocounty', + 'county', + 'macrohood', + 'borough', + 'neighbourhood', + 'microhood', + 'disputed' +]; + +module.exports.tests = {}; + +module.exports.tests.interface = (test, common) => { + test('valid interface', (t) => { + t.equal(typeof is_coarse_reverse, 'function', 'is_coarse_reverse is a function'); + t.end(); + }); +}; + +module.exports.tests.false_conditions = (test, common) => { + test('request without layers should return false', (t) => { + const req = { + clean: {} + }; + + t.notOk(is_coarse_reverse(req)); + t.end(); + + }); + + test('request with empty layers should return false', (t) => { + const req = { + clean: { + layers: [] + } + }; + + t.notOk(is_coarse_reverse(req)); + t.end(); + + }); + + test('request with layers just "address" or "venue" return false', (t) => { + ['address', 'street', 'venue'].forEach((non_coarse_layer) => { + const req = { + clean: { + layers: [non_coarse_layer] + } + }; + + t.notOk(is_coarse_reverse(req)); + + }); + + t.end(); + + }); + + test('request with layers containing "address" or "venue" and a coarse layer should return false', (t) => { + ['address', 'street', 'venue'].forEach((non_coarse_layer) => { + const req = { + clean: { + layers: [_.sample(coarse_layers), non_coarse_layer] + } + }; + + t.notOk(is_coarse_reverse(req)); + + }); + + t.end(); + + }); + + test('request with layers containing "address" and "venue" should return false', (t) => { + const req = { + clean: { + layers: ['address', 'venue'] + } + }; + + t.notOk(is_coarse_reverse(req)); + t.end(); + + }); + +}; + +module.exports.tests.true_conditions = (test, common) => { + test('request with non-empty layers and not containing "address" or "venue" should return true', (t) => { + coarse_layers.forEach((coarse_layer) => { + const req = { + clean: { + layers: [coarse_layer] + } + }; + + t.ok(is_coarse_reverse(req)); + + }); + + t.end(); + + }); + +}; + +module.exports.all = (tape, common) => { + function test(name, testFunction) { + return tape(`GET /is_coarse_reverse ${name}`, testFunction); + } + + for( const testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/controller/predicates/is_pip_service_enabled.js b/test/unit/controller/predicates/is_pip_service_enabled.js new file mode 100644 index 00000000..1d3cd63a --- /dev/null +++ b/test/unit/controller/predicates/is_pip_service_enabled.js @@ -0,0 +1,42 @@ +'use strict'; + +const _ = require('lodash'); +const is_pip_service_enabled = require('../../../../controller/predicates/is_pip_service_enabled'); + +module.exports.tests = {}; + +module.exports.tests.interface = (test, common) => { + test('valid interface', (t) => { + t.equal(typeof is_pip_service_enabled, 'function', 'is_pip_service_enabled is a function'); + t.equal(typeof is_pip_service_enabled(), 'function', 'is_pip_service_enabled() is a function'); + t.end(); + }); +}; + +module.exports.tests.true_conditions = (test, common) => { + test('string uri should return true', (t) => { + t.ok(is_pip_service_enabled('pip uri')()); + t.end(); + + }); + +}; + +module.exports.tests.false_conditions = (test, common) => { + test('undefined uri should return false', (t) => { + t.notOk(is_pip_service_enabled()()); + t.end(); + + }); + +}; + +module.exports.all = (tape, common) => { + function test(name, testFunction) { + return tape(`GET /is_pip_service_enabled ${name}`, testFunction); + } + + for( const testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 78e0b2d2..3f80f7d1 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -55,7 +55,7 @@ module.exports.tests.success = function(test, common) { }; } } - })(config, esclient, query); + })(config, esclient, query, () => { return true; }); const req = { clean: { }, errors: [], warnings: [] }; const res = {}; @@ -119,7 +119,7 @@ module.exports.tests.success = function(test, common) { }; } } - })(config, esclient, query); + })(config, esclient, query, () => { return true; }); const req = { clean: { }, errors: [], warnings: [] }; const res = {}; @@ -183,7 +183,7 @@ module.exports.tests.success = function(test, common) { }; } } - })(config, esclient, query); + })(config, esclient, query, () => { return true; }); const req = { clean: { }, errors: [], warnings: [] }; const res = {}; @@ -263,7 +263,7 @@ module.exports.tests.success = function(test, common) { }; } } - })(config, esclient, query); + })(config, esclient, query, () => { return true; }); const req = { clean: { }, errors: [], warnings: [] }; const res = {}; @@ -341,7 +341,7 @@ module.exports.tests.timeout = function(test, common) { }; } } - })(config, esclient, query); + })(config, esclient, query, () => { return true; }); const req = { clean: { }, errors: [], warnings: [] }; const res = {}; @@ -392,7 +392,7 @@ module.exports.tests.timeout = function(test, common) { callback(timeoutError); } - })(config, esclient, query); + })(config, esclient, query, () => { return true; }); const req = { clean: { }, errors: [], warnings: [] }; const res = {}; @@ -432,7 +432,7 @@ module.exports.tests.timeout = function(test, common) { callback(nonTimeoutError); } - })(config, esclient, query); + })(config, esclient, query, () => { return true; }); const req = { clean: { }, errors: [], warnings: [] }; const res = {}; @@ -473,7 +473,7 @@ module.exports.tests.timeout = function(test, common) { callback(stringTypeError); } - })(config, esclient, query); + })(config, esclient, query, () => { return true; }); const req = { clean: { }, errors: [], warnings: [] }; const res = {}; @@ -494,49 +494,21 @@ module.exports.tests.timeout = function(test, common) { }; -module.exports.tests.existing_errors = function(test, common) { - test('req with errors should not call esclient or query', function(t) { +module.exports.tests.should_execute = (test, common) => { + test('should_execute returning false and empty req.errors should call next', (t) => { const esclient = () => { throw new Error('esclient should not have been called'); }; const query = () => { throw new Error('query should not have been called'); }; - const controller = setup( {}, esclient, query ); - - // the existence of `errors` means that a sanitizer detected an error, - // so don't call the esclient - const req = { - errors: ['error'] - }; - const res = { }; - - t.doesNotThrow(() => { - controller(req, res, () => {}); - }); - t.end(); - - }); - -}; - -module.exports.tests.existing_results = function(test, common) { - test('res with existing data should not call esclient or query', function(t) { - const esclient = () => { - throw new Error('esclient should not have been called'); - }; - const query = () => { - throw new Error('query should not have been called'); - }; - const controller = setup( {}, esclient, query ); + const controller = setup( {}, esclient, query, () => { return false; } ); const req = { }; - // the existence of `data` means that there are already results so - // don't call esclient or query - const res = { data: [{}] }; + const res = { }; - const next = function() { - t.deepEqual(res, {data: [{}]}); + const next = () => { + t.deepEqual(res, { }); t.end(); }; controller(req, res, next); @@ -559,7 +531,7 @@ module.exports.tests.undefined_query = function(test, common) { search_service_was_called = true; throw new Error('search service should not have been called'); } - })(undefined, undefined, query); + })(undefined, undefined, query, () => { return true; }); const next = () => { t.notOk(search_service_was_called, 'should have returned before search service was called'); diff --git a/test/unit/run.js b/test/unit/run.js index 71b14e27..a16fb18e 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -11,9 +11,14 @@ var common = { var tests = [ require('./app'), require('./schema'), + require('./controller/coarse_reverse'), require('./controller/index'), require('./controller/place'), require('./controller/search'), + require('./controller/predicates/has_response_data'), + require('./controller/predicates/has_request_errors'), + require('./controller/predicates/is_coarse_reverse'), + require('./controller/predicates/is_pip_service_enabled'), require('./helper/diffPlaces'), require('./helper/geojsonify'), require('./helper/logging'), @@ -73,7 +78,8 @@ var tests = [ require('./sanitizer/wrap'), require('./service/mget'), require('./service/search'), - require('./service/interpolation') + require('./service/interpolation'), + require('./service/pointinpolygon') ]; tests.map(function(t) { diff --git a/test/unit/schema.js b/test/unit/schema.js index e1f9cfd2..6ed11add 100644 --- a/test/unit/schema.js +++ b/test/unit/schema.js @@ -367,6 +367,86 @@ module.exports.tests.api_validation = (test, common) => { }); + test('non-string api.pipService should throw error', (t) => { + [null, 17, {}, [], true].forEach((value) => { + var config = { + api: { + version: 'version value', + indexName: 'index name value', + host: 'host value', + pipService: value + }, + esclient: {} + }; + + t.throws(validate.bind(null, config), /"pipService" must be a string/); + + }); + + t.end(); + + }); + + test('non-URI-formatted api.pipService should throw error', (t) => { + ['this is not a URI'].forEach((value) => { + var config = { + api: { + version: 'version value', + indexName: 'index name value', + host: 'host value', + pipService: value + }, + esclient: {} + }; + + t.throws(validate.bind(null, config), /"pipService" must be a valid uri/); + + }); + + t.end(); + + }); + + test('non-http/https api.pipService should throw error', (t) => { + ['ftp', 'git', 'unknown'].forEach((scheme) => { + var config = { + api: { + version: 'version value', + indexName: 'index name value', + host: 'host value', + pipService: `${scheme}://localhost` + }, + esclient: {} + }; + + t.throws(validate.bind(null, config), /"pipService" must be a valid uri/); + + }); + + t.end(); + + }); + + test('http/https api.pipService should not throw error', (t) => { + ['http', 'https'].forEach((scheme) => { + var config = { + api: { + version: 'version value', + indexName: 'index name value', + host: 'host value', + pipService: `${scheme}://localhost` + }, + esclient: {} + }; + + t.doesNotThrow(validate.bind(null, config), `${scheme} should be allowed`); + + }); + + t.end(); + + }); + }; module.exports.tests.esclient_validation = (test, common) => { diff --git a/test/unit/service/pointinpolygon.js b/test/unit/service/pointinpolygon.js new file mode 100644 index 00000000..ac08b3f7 --- /dev/null +++ b/test/unit/service/pointinpolygon.js @@ -0,0 +1,169 @@ +const proxyquire = require('proxyquire').noCallThru(); + +const setup = require('../../../service/pointinpolygon'); + +module.exports.tests = {}; + +module.exports.tests.interface = (test, common) => { + test('valid interface', (t) => { + const logger = require('pelias-mock-logger')(); + + var service = proxyquire('../../../service/pointinpolygon', { + 'pelias-logger': logger + }); + + t.equal(typeof service, 'function', 'service is a function'); + t.end(); + }); +}; + +module.exports.tests.do_nothing_service = (test, common) => { + test('undefined PiP uri should return service that logs fact that PiP service is not available', (t) => { + const logger = require('pelias-mock-logger')(); + + const service = proxyquire('../../../service/pointinpolygon', { + 'pelias-logger': logger + })(); + + service({ lat: 12.121212, lon: 21.212121 }, (err) => { + t.deepEquals(logger.getWarnMessages(), [ + 'point-in-polygon service disabled' + ]); + t.equals(err, 'point-in-polygon service disabled, unable to resolve {"lat":12.121212,"lon":21.212121}'); + t.end(); + }); + + }); + +}; + +module.exports.tests.success = (test, common) => { + test('lat and lon should be passed to server', (t) => { + const pipServer = require('express')(); + pipServer.get('/:lon/:lat', (req, res, next) => { + t.equals(req.params.lat, '12.121212'); + t.equals(req.params.lon, '21.212121'); + + res.send('{ "field": "value" }'); + }); + + const server = pipServer.listen(); + const port = server.address().port; + + const logger = require('pelias-mock-logger')(); + + const service = proxyquire('../../../service/pointinpolygon', { + 'pelias-logger': logger + })(`http://localhost:${port}`); + + service({ lat: 12.121212, lon: 21.212121}, (err, results) => { + t.notOk(err); + t.deepEquals(results, { field: 'value' }); + + t.ok(logger.isInfoMessage(`using point-in-polygon service at http://localhost:${port}`)); + t.notOk(logger.hasErrorMessages()); + + t.end(); + + server.close(); + + }); + + }); + +}; + +module.exports.tests.failure = (test, common) => { + test('server returning success but non-JSON body should log error and return no results', (t) => { + const pipServer = require('express')(); + pipServer.get('/:lon/:lat', (req, res, next) => { + t.equals(req.params.lat, '12.121212'); + t.equals(req.params.lon, '21.212121'); + + res.send('this is not JSON'); + }); + + const server = pipServer.listen(); + const port = server.address().port; + + const logger = require('pelias-mock-logger')(); + + const service = proxyquire('../../../service/pointinpolygon', { + 'pelias-logger': logger + })(`http://localhost:${port}`); + + service({ lat: 12.121212, lon: 21.212121}, (err, results) => { + t.equals(err, `http://localhost:${port}/21.212121/12.121212 returned status 200 but with non-JSON response: this is not JSON`); + t.notOk(results); + t.ok(logger.isErrorMessage(`http://localhost:${port}/21.212121/12.121212: could not parse response body: this is not JSON`)); + t.end(); + + server.close(); + + }); + + }); + + test('server returning error should log it and return no results', (t) => { + const server = require('express')().listen(); + const port = server.address().port; + + // immediately close the server so to ensure an error response + server.close(); + + const logger = require('pelias-mock-logger')(); + + const service = proxyquire('../../../service/pointinpolygon', { + 'pelias-logger': logger + })(`http://localhost:${port}`); + + service({ lat: 12.121212, lon: 21.212121}, (err, results) => { + t.equals(err.code, 'ECONNREFUSED'); + t.notOk(results); + t.ok(logger.isErrorMessage(/ECONNREFUSED/), 'there should be a connection refused error message'); + t.end(); + + server.close(); + + }); + + }); + + test('non-OK status should log error and return no results', (t) => { + const pipServer = require('express')(); + pipServer.get('/:lat/:lon', (req, res, next) => { + res.status(400).send('a bad request was made'); + }); + + const server = pipServer.listen(); + const port = server.address().port; + + const logger = require('pelias-mock-logger')(); + + const service = proxyquire('../../../service/pointinpolygon', { + 'pelias-logger': logger + })(`http://localhost:${port}`); + + service({ lat: 12.121212, lon: 21.212121}, (err, results) => { + t.equals(err, `http://localhost:${port}/21.212121/12.121212 returned status 400: a bad request was made`); + t.notOk(results); + t.ok(logger.isErrorMessage(`http://localhost:${port}/21.212121/12.121212 returned status 400: a bad request was made`)); + t.end(); + + server.close(); + + }); + + }); + +}; + +module.exports.all = (tape, common) => { + function test(name, testFunction) { + return tape(`SERVICE /pointinpolygon ${name}`, testFunction); + } + + for( var testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +};