diff --git a/controller/coarse_reverse.js b/controller/coarse_reverse.js index dc48d6db..d080298c 100644 --- a/controller/coarse_reverse.js +++ b/controller/coarse_reverse.js @@ -1,6 +1,7 @@ const logger = require('pelias-logger').get('coarse_reverse'); const _ = require('lodash'); const Document = require('pelias-model').Document; +const isDNT = require('../helper/logging').isDNT; const granularities = [ 'neighbourhood', @@ -88,7 +89,7 @@ function setup(service, should_execute) { lon: req.clean['point.lon'] }; - service(centroid, (err, results) => { + service(centroid, isDNT(req), (err, results) => { // if there's an error, log it and bail if (err) { logger.error(err); diff --git a/middleware/normalizeParentIds.js b/middleware/normalizeParentIds.js index ae22a3c6..22fc43f8 100644 --- a/middleware/normalizeParentIds.js +++ b/middleware/normalizeParentIds.js @@ -32,7 +32,16 @@ function normalizeParentIds(place) { if (place) { placeTypes.forEach(function (placeType) { if (place[placeType] && place[placeType].length > 0 && place[placeType][0]) { - place[placeType + '_gid'] = [ makeNewId(placeType, place[placeType + '_gid']) ]; + var source = 'whosonfirst'; + + // looking forward to the day we can remove all geonames specific hacks, but until then... + // geonames sometimes has its own ids in the parent hierarchy, so it's dangerous to assume that + // it's always WOF ids and hardcode to that + if (place.source === 'geonames' && place.source_id === place[placeType + '_gid'][0]) { + source = place.source; + } + + place[placeType + '_gid'] = [ makeNewId(source, placeType, place[placeType + '_gid']) ]; } }); } @@ -48,12 +57,12 @@ function normalizeParentIds(place) { * @param {number} id * @return {string} */ -function makeNewId(placeType, id) { +function makeNewId(source, placeType, id) { if (!id) { return; } - var doc = new Document('whosonfirst', placeType, id); + var doc = new Document(source, placeType, id); return doc.getGid(); } diff --git a/package.json b/package.json index e9d354cc..730b3a54 100644 --- a/package.json +++ b/package.json @@ -75,7 +75,7 @@ "difflet": "^1.0.1", "istanbul": "^0.4.2", "jshint": "^2.5.6", - "npm-check": "^5.4.0", + "npm-check": "git://github.com/orangejulius/npm-check.git#disable-update-check", "nsp": "^2.2.0", "precommit-hook": "^3.0.0", "proxyquire": "^1.7.10", diff --git a/service/pointinpolygon.js b/service/pointinpolygon.js index 21b6fdcf..1e58dfa3 100644 --- a/service/pointinpolygon.js +++ b/service/pointinpolygon.js @@ -2,14 +2,51 @@ const logger = require( 'pelias-logger' ).get( 'pointinpolygon' ); const request = require('request'); const _ = require('lodash'); +function sanitizeUrl(requestUrl) { + return requestUrl.replace(/^(.+)\/.+\/.+$/, (match, base) => { + return `${base}/[removed]/[removed]`; + }); +} + +function parseErrorMessage(requestUrl, body, do_not_track) { + if (do_not_track) { + return `${sanitizeUrl(requestUrl)} returned status 200 but with non-JSON response: ${body}`; + } + + return `${requestUrl} returned status 200 but with non-JSON response: ${body}`; + +} + +function serviceErrorMessage(requestUrl, statusCode, body, do_not_track) { + if (do_not_track) { + return `${sanitizeUrl(requestUrl)} returned status ${statusCode}: ${body}`; + + } else { + return `${requestUrl} returned status ${statusCode}: ${body}`; + + } + +} + module.exports = (url) => { if (!_.isEmpty(url)) { logger.info(`using point-in-polygon service at ${url}`); - return function pointinpolygon( centroid, callback ) { + return function pointinpolygon( centroid, do_not_track, callback ) { const requestUrl = `${url}/${centroid.lon}/${centroid.lat}`; - request.get(requestUrl, (err, response, body) => { + const options = { + method: 'GET', + url: requestUrl + }; + + if (do_not_track) { + options.headers = { + dnt: '1' + }; + } + + request(options, (err, response, body) => { if (err) { logger.error(JSON.stringify(err)); callback(err); @@ -20,13 +57,17 @@ module.exports = (url) => { 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}`); + const parseMsg = parseErrorMessage(requestUrl, body, do_not_track); + + logger.error(parseMsg); + callback(parseMsg); } } else { - logger.error(`${requestUrl} returned status ${response.statusCode}: ${body}`); - callback(`${requestUrl} returned status ${response.statusCode}: ${body}`); + const errorMsg = serviceErrorMessage(requestUrl, response.statusCode, body, do_not_track); + + logger.error(errorMsg); + callback(errorMsg); } }); @@ -35,8 +76,9 @@ module.exports = (url) => { } else { logger.warn('point-in-polygon service disabled'); - return (centroid, callback) => { - callback(`point-in-polygon service disabled, unable to resolve ${JSON.stringify(centroid)}`); + return (centroid, do_not_track, callback) => { + callback(`point-in-polygon service disabled, unable to resolve ` + + (do_not_track ? 'centroid' : JSON.stringify(centroid))); }; } diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js index d8071e62..2389cc43 100644 --- a/test/unit/controller/coarse_reverse.js +++ b/test/unit/controller/coarse_reverse.js @@ -46,7 +46,8 @@ module.exports.tests.early_exit_conditions = (test, common) => { module.exports.tests.error_conditions = (test, common) => { test('service error should log and call next', (t) => { - const service = (point, callback) => { + const service = (point, do_not_track, callback) => { + t.equals(do_not_track, 'do_not_track value'); callback('this is an error'); }; @@ -54,7 +55,12 @@ module.exports.tests.error_conditions = (test, common) => { const should_execute = () => { return true; }; const controller = proxyquire('../../../controller/coarse_reverse', { - 'pelias-logger': logger + 'pelias-logger': logger, + '../helper/logging': { + isDNT: () => { + return 'do_not_track value'; + } + } })(service, should_execute); const req = { @@ -86,7 +92,8 @@ module.exports.tests.error_conditions = (test, common) => { module.exports.tests.success_conditions = (test, common) => { test('service returning results should use first entry for each layer', (t) => { - const service = (point, callback) => { + const service = (point, do_not_track, callback) => { + t.equals(do_not_track, 'do_not_track value'); const results = { neighbourhood: [ { @@ -146,7 +153,12 @@ module.exports.tests.success_conditions = (test, common) => { const should_execute = () => { return true; }; const controller = proxyquire('../../../controller/coarse_reverse', { - 'pelias-logger': logger + 'pelias-logger': logger, + '../helper/logging': { + isDNT: () => { + return 'do_not_track value'; + } + } })(service, should_execute); const req = { @@ -235,7 +247,8 @@ module.exports.tests.success_conditions = (test, common) => { }); test('layers missing from results should be ignored', (t) => { - const service = (point, callback) => { + const service = (point, do_not_track, callback) => { + t.equals(do_not_track, 'do_not_track value'); const results = { neighbourhood: [ { @@ -258,7 +271,12 @@ module.exports.tests.success_conditions = (test, common) => { const should_execute = () => { return true; }; const controller = proxyquire('../../../controller/coarse_reverse', { - 'pelias-logger': logger + 'pelias-logger': logger, + '../helper/logging': { + isDNT: () => { + return 'do_not_track value'; + } + } })(service, should_execute); const req = { @@ -319,7 +337,8 @@ module.exports.tests.success_conditions = (test, common) => { }); test('most granular layer missing centroid should not set', (t) => { - const service = (point, callback) => { + const service = (point, do_not_track, callback) => { + t.equals(do_not_track, 'do_not_track value'); const results = { neighbourhood: [ { @@ -338,7 +357,12 @@ module.exports.tests.success_conditions = (test, common) => { const should_execute = () => { return true; }; const controller = proxyquire('../../../controller/coarse_reverse', { - 'pelias-logger': logger + 'pelias-logger': logger, + '../helper/logging': { + isDNT: () => { + return 'do_not_track value'; + } + } })(service, should_execute); const req = { @@ -395,7 +419,8 @@ module.exports.tests.success_conditions = (test, common) => { }); test('most granular layer missing bounding_box should not set', (t) => { - const service = (point, callback) => { + const service = (point, do_not_track, callback) => { + t.equals(do_not_track, 'do_not_track value'); const results = { neighbourhood: [ { @@ -417,7 +442,12 @@ module.exports.tests.success_conditions = (test, common) => { const should_execute = () => { return true; }; const controller = proxyquire('../../../controller/coarse_reverse', { - 'pelias-logger': logger + 'pelias-logger': logger, + '../helper/logging': { + isDNT: () => { + return 'do_not_track value'; + } + } })(service, should_execute); const req = { @@ -480,7 +510,8 @@ module.exports.tests.success_conditions = (test, common) => { module.exports.tests.failure_conditions = (test, common) => { test('service returning 0 results at the requested layer should return nothing', (t) => { - const service = (point, callback) => { + const service = (point, do_not_track, callback) => { + t.equals(do_not_track, 'do_not_track value'); // response without neighbourhood results const results = { borough: [ @@ -528,7 +559,12 @@ module.exports.tests.failure_conditions = (test, common) => { const should_execute = () => { return true; }; const controller = proxyquire('../../../controller/coarse_reverse', { - 'pelias-logger': logger + 'pelias-logger': logger, + '../helper/logging': { + isDNT: () => { + return 'do_not_track value'; + } + } })(service, should_execute); const req = { diff --git a/test/unit/fixture/reverse_boundary_circle.js b/test/unit/fixture/reverse_boundary_circle.js deleted file mode 100644 index 4fc728a2..00000000 --- a/test/unit/fixture/reverse_boundary_circle.js +++ /dev/null @@ -1,43 +0,0 @@ -var vs = require('../../../query/reverse_defaults'); - -module.exports = { - 'query': { - 'filtered': { - 'query': { - 'bool': {} - }, - 'filter': { - 'bool': { - 'must': [ - { - 'geo_distance': { - 'distance': vs.distance, - 'distance_type': 'plane', - 'optimize_bbox': 'indexed', - 'center_point': { - 'lat': 29.49136, - 'lon': -82.50622 - } - } - } - ] - } - } - } - }, - 'sort': [ - '_score', - { - '_geo_distance': { - 'center_point': { - 'lat': 29.49136, - 'lon': -82.50622 - }, - 'order': 'asc', - 'distance_type': 'plane' - } - } - ], - 'size': vs.size, - 'track_scores': true -}; diff --git a/test/unit/middleware/normalizeParentIds.js b/test/unit/middleware/normalizeParentIds.js index 472df0e3..8388f67d 100644 --- a/test/unit/middleware/normalizeParentIds.js +++ b/test/unit/middleware/normalizeParentIds.js @@ -77,6 +77,112 @@ module.exports.tests.interface = function(test, common) { }); }); + + test('Geonames ids do not override parent hierarchy with WOF equivalents', function(t) { + + var input = { + data: [{ + 'parent': { + 'country_id': [ '85633793' ], + 'country': [ 'United States' ], + 'country_a': [ 'USA' ], + 'region_id': [ '85688579' ], + 'region': [ 'Mississippi' ], + 'region_a': [ 'MS' ] + }, + 'source': 'geonames', + 'source_id': '4436296', + 'layer': 'region', + 'country': [ 'United States' ], + 'country_a': [ 'USA' ], + 'country_gid': [ '85633793' ], + 'region': [ 'Mississippi' ], + 'region_a': [ 'MS' ], + 'region_gid': [ '85688579' ] + }] + }; + + var expected = { + data: [{ + 'parent': { + 'country_id': [ '85633793' ], + 'country': [ 'United States' ], + 'country_a': [ 'USA' ], + 'region_id': [ '85688579' ], + 'region': [ 'Mississippi' ], + 'region_a': [ 'MS' ] + }, + 'layer': 'region', + 'source': 'geonames', + 'source_id': '4436296', + 'country': ['United States'], + 'country_gid': ['whosonfirst:country:85633793'], + 'country_a': ['USA'], + 'region': ['Mississippi'], + 'region_gid': ['whosonfirst:region:85688579'], + 'region_a': ['MS'] + }] + }; + + normalizer({}, input, function () { + t.deepEqual(input, expected); + t.end(); + }); + + }); + + test('Geonames ids do not override parent hierarchy with WOF equivalents', function(t) { + + var input = { + data: [{ + 'parent': { + 'country_id': [ '85633793' ], + 'country': [ 'United States' ], + 'country_a': ['USA'], + 'region_id': [ '4436296' ], + 'region': [ 'Mississippi' ], + 'region_a': [ 'MS' ] + }, + 'source': 'geonames', + 'source_id': '4436296', + 'layer': 'region', + 'country': [ 'United States' ], + 'country_a': [ 'USA' ], + 'country_gid': ['85633793'], + 'region': [ 'Mississippi' ], + 'region_a': [ 'MS' ], + 'region_gid': [ '4436296' ] + }] + }; + + var expected = { + data: [{ + 'parent': { + 'country_id': [ '85633793' ], + 'country': [ 'United States' ], + 'country_a': [ 'USA' ], + 'region_id': [ '4436296' ], + 'region': [ 'Mississippi' ], + 'region_a': [ 'MS' ] + }, + 'layer': 'region', + 'source': 'geonames', + 'source_id': '4436296', + 'country': ['United States'], + 'country_gid': ['whosonfirst:country:85633793'], + 'country_a': ['USA'], + 'region': ['Mississippi'], + 'region_gid': ['geonames:region:4436296'], + 'region_a': ['MS'] + }] + }; + + normalizer({}, input, function () { + t.deepEqual(input, expected); + t.end(); + }); + + }); }; module.exports.all = function (tape, common) { diff --git a/test/unit/service/pointinpolygon.js b/test/unit/service/pointinpolygon.js index ac08b3f7..7280aa7f 100644 --- a/test/unit/service/pointinpolygon.js +++ b/test/unit/service/pointinpolygon.js @@ -25,7 +25,7 @@ module.exports.tests.do_nothing_service = (test, common) => { 'pelias-logger': logger })(); - service({ lat: 12.121212, lon: 21.212121 }, (err) => { + service({ lat: 12.121212, lon: 21.212121 }, false, (err) => { t.deepEquals(logger.getWarnMessages(), [ 'point-in-polygon service disabled' ]); @@ -35,6 +35,23 @@ module.exports.tests.do_nothing_service = (test, common) => { }); + test('service unavailable message should not output centroid when do_not_track is true', (t) => { + const logger = require('pelias-mock-logger')(); + + const service = proxyquire('../../../service/pointinpolygon', { + 'pelias-logger': logger + })(); + + service({ lat: 12.121212, lon: 21.212121 }, true, (err) => { + t.deepEquals(logger.getWarnMessages(), [ + 'point-in-polygon service disabled' + ]); + t.equals(err, 'point-in-polygon service disabled, unable to resolve centroid'); + t.end(); + }); + + }); + }; module.exports.tests.success = (test, common) => { @@ -56,13 +73,81 @@ module.exports.tests.success = (test, common) => { 'pelias-logger': logger })(`http://localhost:${port}`); - service({ lat: 12.121212, lon: 21.212121}, (err, results) => { + service({ lat: 12.121212, lon: 21.212121}, false, (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(); + + }); + + }); + + test('do_not_track=true should pass DNT header', (t) => { + const pipServer = require('express')(); + pipServer.get('/:lon/:lat', (req, res, next) => { + t.ok(req.headers.hasOwnProperty('dnt')); + 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}, true, (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(); + + }); + + }); + + test('do_not_track=false should not pass DNT header', (t) => { + const pipServer = require('express')(); + pipServer.get('/:lon/:lat', (req, res, next) => { + t.notOk(req.headers.hasOwnProperty('dnt')); + 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}, false, (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(); @@ -92,10 +177,46 @@ module.exports.tests.failure = (test, common) => { '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`); + const expectedErrorMsg = `http://localhost:${port}/21.212121/12.121212 returned ` + + `status 200 but with non-JSON response: this is not JSON`; + + service({ lat: 12.121212, lon: 21.212121}, false, (err, results) => { + t.equals(err, expectedErrorMsg); t.notOk(results); - t.ok(logger.isErrorMessage(`http://localhost:${port}/21.212121/12.121212: could not parse response body: this is not JSON`)); + t.ok(logger.isErrorMessage(expectedErrorMsg)); + t.end(); + + server.close(); + + }); + + }); + + test('server returning 200 & non-JSON body should log sanitized error and return no results when do_not_track', (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}`); + + const expectedErrorMsg = `http://localhost:${port}/[removed]/[removed] returned ` + + `status 200 but with non-JSON response: this is not JSON`; + + service({ lat: 12.121212, lon: 21.212121}, true, (err, results) => { + t.equals(err, expectedErrorMsg); + t.notOk(results); + t.ok(logger.isErrorMessage(expectedErrorMsg)); t.end(); server.close(); @@ -117,7 +238,7 @@ module.exports.tests.failure = (test, common) => { 'pelias-logger': logger })(`http://localhost:${port}`); - service({ lat: 12.121212, lon: 21.212121}, (err, results) => { + service({ lat: 12.121212, lon: 21.212121}, false, (err, results) => { t.equals(err.code, 'ECONNREFUSED'); t.notOk(results); t.ok(logger.isErrorMessage(/ECONNREFUSED/), 'there should be a connection refused error message'); @@ -144,10 +265,43 @@ module.exports.tests.failure = (test, common) => { '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`); + const expectedErrorMsg = `http://localhost:${port}/21.212121/12.121212 returned ` + + `status 400: a bad request was made`; + + service({ lat: 12.121212, lon: 21.212121}, false, (err, results) => { + t.equals(err, expectedErrorMsg); + t.notOk(results); + t.ok(logger.isErrorMessage(expectedErrorMsg)); + t.end(); + + server.close(); + + }); + + }); + + test('non-OK status should log sanitized error and return no results when do_not_track=true', (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}`); + + const expectedErrorMsg = `http://localhost:${port}/[removed]/[removed] returned ` + + `status 400: a bad request was made`; + + service({ lat: 12.121212, lon: 21.212121}, true, (err, results) => { + t.equals(err, expectedErrorMsg); t.notOk(results); - t.ok(logger.isErrorMessage(`http://localhost:${port}/21.212121/12.121212 returned status 400: a bad request was made`)); + t.ok(logger.isErrorMessage(expectedErrorMsg)); t.end(); server.close();