Browse Source

guard clauses, more tests, code cleanup

pull/850/head
Stephen Hess 8 years ago
parent
commit
8a14dcc48a
  1. 43
      controller/placeholder.js
  2. 531
      test/unit/controller/placeholder.js

43
controller/placeholder.js

@ -1,27 +1,28 @@
'use strict';
const _ = require('lodash');
const logger = require('pelias-logger').get('api');
const logging = require( '../helper/logging' );
const Document = require('pelias-model').Document;
// composition of toNumber and isFinite, useful for single call to convert a value
// to a number, then checking to see if it's finite
function isFiniteNumber() {
return _.flow(_.toNumber, _.isFinite);
}
const isFiniteNumber = _.flow(_.toNumber, _.isFinite);
// returns true if all 4 ,-delimited (max) substrings are parseable as numbers
// returns true if all 4 ,-delimited (max) substrings are parseable as finite numbers
// '12.12,21.21,13.13,31.31' returns true
// '12.12,21.21,13.13,31.31,14.14' returns false
// '12.12,21.21,13.13,blah' returns false
// '12.12,21.21,13.13,31.31,blah' returns false
function validBoundingBox(bbox) {
// '12.12,NaN,13.13,31.31' returns false
// '12.12,Infinity,13.13,31.31' returns false
function is4CommaDelimitedNumbers(bbox) {
return bbox.
split(',').
filter(isFiniteNumber).length === 4;
}
function hasName(result) {
return !_.isEmpty(_.trim(result.name));
}
function synthesizeDocs(result) {
const doc = new Document('whosonfirst', result.placetype, result.id.toString());
doc.setName('default', result.name);
@ -30,11 +31,11 @@ function synthesizeDocs(result) {
if (_.conformsTo(result.geom, { 'lat': isFiniteNumber, 'lon': isFiniteNumber } )) {
doc.setCentroid( { lat: result.geom.lat, lon: result.geom.lon } );
} else {
console.error('what\'s up with ' + result.id);
logger.error(`could not parse centroid for id ${result.id}`);
}
// lodash conformsTo verifies that an object has a property with a certain format
if (_.conformsTo(result.geom, { 'bbox': validBoundingBox } )) {
if (_.conformsTo(result.geom, { 'bbox': is4CommaDelimitedNumbers } )) {
const parsedBoundingBox = result.geom.bbox.split(',').map(_.toNumber);
doc.setBoundingBox({
upperLeft: {
@ -46,12 +47,13 @@ function synthesizeDocs(result) {
lon: parsedBoundingBox[2]
}
});
} else {
logger.error(`could not parse bbox for id ${result.id}: ${result.geom.bbox}`);
}
if (_.isEmpty(result.lineage)) {
// there are no hierarchies so just return what's been assembled so far
return buildESDoc(doc);
}
result.lineage.map((hierarchy) => {
@ -59,7 +61,9 @@ function synthesizeDocs(result) {
.filter(doc.isSupportedParent)
.filter((placetype) => { return !_.isEmpty(_.trim(hierarchy[placetype].name)); } )
.forEach((placetype) => {
if (hierarchy[placetype].hasOwnProperty('abbr') && placetype === 'country') {
if (placetype === 'country' &&
hierarchy[placetype].hasOwnProperty('abbr') &&
hierarchy[placetype].abbr.match(/^[a-zA-Z]{3}$/)) {
doc.setAlpha3(hierarchy[placetype].abbr);
}
@ -78,9 +82,7 @@ function synthesizeDocs(result) {
function buildESDoc(doc) {
const esDoc = doc.toESDocument();
esDoc.data._id = esDoc._id;
esDoc.data._type = esDoc._type;
return esDoc.data;
return _.extend(esDoc.data, { _id: esDoc._id, _type: esDoc._type });
}
function setup(placeholderService, should_execute) {
@ -103,7 +105,7 @@ function setup(placeholderService, should_execute) {
// filter that passes only results that match on requested layers
// passes everything if req.clean.layers is not found
const matchesLayers = (result) => {
return req.clean.layers.indexOf(result.placetype) >= 0;
return _.includes(req.clean.layers, result.placetype);
};
const layersFilter = _.get(req, 'clean.layers', []).length ?
matchesLayers : _.constant(true);
@ -111,8 +113,7 @@ function setup(placeholderService, should_execute) {
// filter that passes only documents that match on boundary.country
// passed everything if req.clean['boundary.country'] is not found
const matchesBoundaryCountry = (doc) => {
return doc.parent.country_a &&
doc.parent.country_a.indexOf(req.clean['boundary.country']) > -1;
return _.includes(doc.parent.country_a, req.clean['boundary.country']);
};
const countryFilter = _.has(req, ['clean', 'boundary.country']) ?
matchesBoundaryCountry : _.constant(true);
@ -122,12 +123,12 @@ function setup(placeholderService, should_execute) {
// lineages may produce different country docs
res.meta = {};
res.data = _.flatten(
results.filter(layersFilter).map(synthesizeDocs))
results.filter(hasName).filter(layersFilter).map(synthesizeDocs))
.filter(countryFilter);
const messageParts = [
'[controller:placeholder]',
`[result_count:${_.get(res, 'data', []).length}]`
`[result_count:${_.defaultTo(res.data, []).length}]`
];
logger.info(messageParts.join(' '));

531
test/unit/controller/placeholder.js

@ -2,6 +2,8 @@
const placeholder = require('../../../controller/placeholder');
const proxyquire = require('proxyquire').noCallThru();
const mock_logger = require('pelias-mock-logger');
const _ = require('lodash');
module.exports.tests = {};
@ -13,12 +15,12 @@ module.exports.tests.interface = (test, common) => {
});
};
module.exports.tests.should_execute_failure = (test, common) => {
test('should_execute returning false should return without calling service', (t) => {
let placeholderService_was_called = false;
module.exports.tests.should_execute = (test, common) => {
test('should_execute returning false should not call service', (t) => {
let placeholder_service_was_called = false;
const placeholderService = () => {
placeholderService_was_called = true;
const placeholder_service = () => {
placeholder_service_was_called = true;
};
const should_execute = (req, res) => {
@ -28,50 +30,51 @@ module.exports.tests.should_execute_failure = (test, common) => {
return false;
};
const controller = placeholder(placeholderService, should_execute);
const controller = placeholder(placeholder_service, should_execute);
const req = { a: 1 };
const res = { b: 2 };
controller(req, res, () => {
t.notOk(placeholderService_was_called);
t.notOk(placeholder_service_was_called);
t.deepEquals(res, { b: 2 });
t.end();
});
});
};
module.exports.tests.success = (test, common) => {
test('should_execute returning true should call service', (t) => {
let placeholderService_was_called = false;
let placeholder_service_was_called = false;
const placeholderService = (req, callback) => {
const placeholder_service = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
placeholderService_was_called = true;
placeholder_service_was_called = true;
callback(null, []);
};
const should_execute = (req, res) => {
return true;
};
const controller = placeholder(placeholderService, should_execute);
const controller = placeholder(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { b: 2 };
controller(req, res, () => {
t.ok(placeholderService_was_called);
t.ok(placeholder_service_was_called);
t.deepEquals(res.data, []);
t.end();
});
});
};
module.exports.tests.success = (test, common) => {
test('response from service should be converted', (t) => {
let placeholderService_was_called = false;
const logger = mock_logger();
const placeholder_response = [
const placeholder_service = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
const response = [
{
id: 123,
name: 'name 1',
@ -194,21 +197,17 @@ module.exports.tests.success = (test, common) => {
}
];
const placeholderService = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
placeholderService_was_called = true;
callback(null, placeholder_response);
};
const should_execute = (req, res) => {
return true;
callback(null, response);
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
@ -285,16 +284,20 @@ module.exports.tests.success = (test, common) => {
]
};
controller(req, res, () => {
t.ok(placeholderService_was_called);
t.deepEquals(res, expected_res);
t.ok(logger.isInfoMessage('[controller:placeholder] [result_count:2]'));
t.end();
});
});
test('results with no lineage should no set any parent fields', (t) => {
const placeholder_response = [
test('results with no lineage should not set any parent fields', (t) => {
const logger = require('pelias-mock-logger')();
const placeholder_service = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
const response = [
{
id: 123,
name: 'name 1',
@ -308,20 +311,17 @@ module.exports.tests.success = (test, common) => {
}
];
const placeholderService = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
callback(null, placeholder_response);
};
const should_execute = (req, res) => {
return true;
callback(null, response);
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
@ -347,100 +347,118 @@ module.exports.tests.success = (test, common) => {
]
};
controller(req, res, () => {
t.deepEquals(res, expected_res);
t.ok(logger.isInfoMessage('[controller:placeholder] [result_count:1]'));
t.end();
});
});
test('results with string geom.lat/geom.lon should convert to numbers', (t) => {
const placeholder_response = [
test('results with undefined or empty name should be skipped', (t) => {
[undefined, '', ' \t '].forEach((invalid_name) => {
const logger = require('pelias-mock-logger')();
const placeholder_service = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
const response = [
{
id: 1,
name: 'name 1',
id: 123,
name: invalid_name,
placetype: 'neighbourhood',
geom: {
area: 12.34,
lat: '14.141414',
lon: '41.414141'
area: 12.34
}
},
{
id: 456,
name: 'name 2',
placetype: 'neighbourhood',
geom: {
area: 12.34
}
}
];
const placeholderService = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
callback(null, placeholder_response);
};
const should_execute = (req, res) => {
return true;
callback(null, response);
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
{
_id: '1',
_id: '456',
_type: 'neighbourhood',
layer: 'neighbourhood',
source: 'whosonfirst',
source_id: '1',
center_point: {
lat: 14.141414,
lon: 41.414141
},
source_id: '456',
name: {
'default': 'name 1'
'default': 'name 2'
},
phrase: {
'default': 'name 1'
'default': 'name 2'
},
parent: { }
}
]
};
controller(req, res, () => {
t.deepEquals(res, expected_res);
t.end();
t.ok(logger.isInfoMessage('[controller:placeholder] [result_count:1]'));
});
});
t.end();
});
test('result without geom.bbox should leave bounding_box undefined', (t) => {
const placeholder_response = [
test('results with non-3-character country abbreviation should not set alpha3', (t) => {
['AB', 'ABCD'].forEach((country_abbr) => {
const logger = require('pelias-mock-logger')();
const placeholder_service = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
const response = [
{
id: 123,
name: 'name 1',
placetype: 'neighbourhood',
geom: {
area: 12.34,
lat: 14.141414,
lon: 41.414141
area: 12.34
},
lineage: [
{
country: {
id: 1,
name: 'country name 1',
abbr: country_abbr
}
}
]
}
];
const placeholderService = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
callback(null, placeholder_response);
};
const should_execute = (req, res) => {
return true;
callback(null, response);
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
@ -450,65 +468,78 @@ module.exports.tests.success = (test, common) => {
layer: 'neighbourhood',
source: 'whosonfirst',
source_id: '123',
center_point: {
lat: 14.141414,
lon: 41.414141
},
name: {
'default': 'name 1'
},
phrase: {
'default': 'name 1'
},
parent: { }
parent: {
country: ['country name 1'],
country_id: ['1'],
country_a: [country_abbr]
}
}
]
};
controller(req, res, () => {
t.deepEquals(res, expected_res);
t.end();
t.ok(logger.isInfoMessage('[controller:placeholder] [result_count:1]'));
});
});
test('result without geom.lat/geom.lon should leave centroid undefined', (t) => {
const placeholder_response = [
t.end();
});
test('results with string geom.lat/geom.lon should convert to numbers', (t) => {
const logger = require('pelias-mock-logger')();
const placeholder_service = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
const response = [
{
id: 123,
id: 1,
name: 'name 1',
placetype: 'neighbourhood',
geom: {
area: 12.34,
bbox: '21.212121,12.121212,31.313131,13.131313'
lat: '14.141414',
lon: '41.414141'
}
}
];
const placeholderService = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
callback(null, placeholder_response);
callback(null, response);
};
const should_execute = (req, res) => {
return true;
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
{
_id: '123',
_id: '1',
_type: 'neighbourhood',
layer: 'neighbourhood',
source: 'whosonfirst',
source_id: '123',
bounding_box: '{"min_lat":12.121212,"max_lat":13.131313,"min_lon":21.212121,"max_lon":31.313131}',
source_id: '1',
center_point: {
lat: 14.141414,
lon: 41.414141
},
name: {
'default': 'name 1'
},
@ -520,17 +551,28 @@ module.exports.tests.success = (test, common) => {
]
};
controller(req, res, () => {
t.deepEquals(res, expected_res);
t.ok(logger.isInfoMessage('[controller:placeholder] [result_count:1]'));
t.end();
});
});
};
module.exports.tests.result_filtering = (test, common) => {
test('only results matching explicit layers should be returned', (t) => {
let placeholderService_was_called = false;
const logger = mock_logger();
const placeholder_service = (req, callback) => {
t.deepEqual(req, {
param1: 'param1 value',
clean: {
layers: ['neighbourhood', 'locality', 'county']
}
});
const placeholder_response = [
const response = [
{
id: 1,
name: 'name 1',
@ -588,22 +630,12 @@ module.exports.tests.success = (test, common) => {
}
];
const placeholderService = (req, callback) => {
t.deepEqual(req, {
param1: 'param1 value',
clean: {
layers: ['neighbourhood', 'locality', 'county']
}
});
placeholderService_was_called = true;
callback(null, placeholder_response);
};
const should_execute = (req, res) => {
return true;
callback(null, response);
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = {
param1: 'param1 value',
@ -617,6 +649,7 @@ module.exports.tests.success = (test, common) => {
};
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
@ -677,18 +710,25 @@ module.exports.tests.success = (test, common) => {
]
};
controller(req, res, () => {
t.ok(placeholderService_was_called);
t.deepEquals(res, expected_res);
t.ok(logger.isInfoMessage('[controller:placeholder] [result_count:3]'));
t.end();
});
});
test('only synthesized docs matching explicit boundary.country should be returned', (t) => {
let placeholderService_was_called = false;
const logger = require('pelias-mock-logger')();
const placeholder_service = (req, callback) => {
t.deepEqual(req, {
param1: 'param1 value',
clean: {
'boundary.country': 'ABC'
}
});
const placeholder_response = [
const response = [
{
id: 1,
name: 'name 1',
@ -744,22 +784,12 @@ module.exports.tests.success = (test, common) => {
}
];
const placeholderService = (req, callback) => {
t.deepEqual(req, {
param1: 'param1 value',
clean: {
'boundary.country': 'ABC'
}
});
placeholderService_was_called = true;
callback(null, placeholder_response);
};
const should_execute = (req, res) => {
return true;
callback(null, response);
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = {
param1: 'param1 value',
@ -769,6 +799,7 @@ module.exports.tests.success = (test, common) => {
};
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
@ -821,9 +852,127 @@ module.exports.tests.success = (test, common) => {
]
};
t.deepEquals(res, expected_res);
t.ok(logger.isInfoMessage('[controller:placeholder] [result_count:2]'));
t.end();
});
});
};
module.exports.centroid_errors = (test, common) => {
test('result without geom.lat/geom.lon should leave centroid undefined', (t) => {
const logger = require('pelias-mock-logger')();
const placeholder_service = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
const response = [
{
id: 123,
name: 'name 1',
placetype: 'neighbourhood',
geom: {
area: 12.34,
bbox: '21.212121,12.121212,31.313131,13.131313'
}
}
];
callback(null, response);
};
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { };
controller(req, res, () => {
t.ok(placeholderService_was_called);
const expected_res = {
meta: {},
data: [
{
_id: '123',
_type: 'neighbourhood',
layer: 'neighbourhood',
source: 'whosonfirst',
source_id: '123',
bounding_box: '{"min_lat":12.121212,"max_lat":13.131313,"min_lon":21.212121,"max_lon":31.313131}',
name: {
'default': 'name 1'
},
phrase: {
'default': 'name 1'
},
parent: { }
}
]
};
t.deepEquals(res, expected_res);
t.ok(logger.isErrorMessage(`could not parse centroid for id 123`));
t.end();
});
});
test('result with non-number-parseable geom.lat/geom.lon should leave centroid undefined and log error', (t) => {
const logger = mock_logger();
const placeholder_service = (req, callback) => {
t.deepEqual(req, { param1: 'param1 value' });
const response = [
{
id: 123,
name: 'name 1',
placetype: 'neighbourhood',
geom: {
area: 12.34,
bbox: '21.212121,12.121212,31.313131,13.131313',
lat: 'this is not a number',
lon: 'this is not a number'
}
}
];
callback(null, response);
};
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
{
_id: '123',
_type: 'neighbourhood',
layer: 'neighbourhood',
source: 'whosonfirst',
source_id: '123',
bounding_box: '{"min_lat":12.121212,"max_lat":13.131313,"min_lon":21.212121,"max_lon":31.313131}',
name: {
'default': 'name 1'
},
phrase: {
'default': 'name 1'
},
parent: { }
}
]
};
t.deepEquals(res, expected_res);
t.ok(logger.isErrorMessage(`could not parse centroid for id 123`));
t.end();
});
@ -831,17 +980,91 @@ module.exports.tests.success = (test, common) => {
};
module.exports.boundingbox_errors = (test, common) => {
test('result with invalid geom.bbox should leave bounding_box undefined and log error', (t) => {
[
undefined,
'21.212121,12.121212,31.313131,13.131313,41.414141',
'21.212121,12.121212,31.313131',
'21.212121,this is not parseable as a number,31.313131,13.131313',
'21.212121,NaN,31.313131,13.131313',
'21.212121,Infinity,31.313131,13.131313'
].forEach((bbox) => {
const logger = mock_logger();
const placeholder_service = (req, callback) => {
const response = [
{
id: 123,
name: 'name 1',
placetype: 'neighbourhood',
geom: {
area: 12.34,
bbox: bbox,
lat: 14.141414,
lon: 41.414141
}
}
];
t.deepEqual(req, { param1: 'param1 value' });
callback(null, response);
};
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = { param1: 'param1 value' };
const res = { };
controller(req, res, () => {
const expected_res = {
meta: {},
data: [
{
_id: '123',
_type: 'neighbourhood',
layer: 'neighbourhood',
source: 'whosonfirst',
source_id: '123',
center_point: {
lat: 14.141414,
lon: 41.414141
},
name: {
'default': 'name 1'
},
phrase: {
'default': 'name 1'
},
parent: { }
}
]
};
t.deepEquals(res, expected_res);
t.ok(logger.isErrorMessage(`could not parse bbox for id 123: ${bbox}`));
});
});
t.end();
});
};
module.exports.tests.error_conditions = (test, common) => {
test('service return error string should add to req.errors', (t) => {
const placeholderService = (req, callback) => {
callback('placeholder service error', []);
};
const logger = mock_logger();
const should_execute = (req, res) => {
return true;
const placeholder_service = (req, callback) => {
callback('placeholder service error', []);
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = {
errors: []
@ -849,14 +1072,18 @@ module.exports.tests.error_conditions = (test, common) => {
const res = {};
controller(req, res, () => {
t.deepEquals(res, {}, 'res should not have been modified');
t.deepEquals(req.errors, ['placeholder service error']);
t.notOk(logger.isInfoMessage(/\\[controller:placeholder\\] \\[result_count:\\d+\\]/));
t.end();
});
});
test('service return error object should add message to req.errors', (t) => {
const placeholderService = (req, callback) => {
const logger = mock_logger();
const placeholder_service = (req, callback) => {
callback(Error('placeholder service error'), []);
};
@ -864,7 +1091,9 @@ module.exports.tests.error_conditions = (test, common) => {
return true;
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = {
errors: []
@ -872,22 +1101,24 @@ module.exports.tests.error_conditions = (test, common) => {
const res = {};
controller(req, res, () => {
t.deepEquals(res, {}, 'res should not have been modified');
t.deepEquals(req.errors, ['placeholder service error']);
t.notOk(logger.isInfoMessage(/\\[controller:placeholder\\] \\[result_count:\\d+\\]/));
t.end();
});
});
test('service return error object should add stringified error to req.errors', (t) => {
const placeholderService = (req, callback) => {
callback({ error_key: 'error_value' }, []);
};
const logger = mock_logger();
const should_execute = (req, res) => {
return true;
const placeholder_service = (req, callback) => {
callback({ error_key: 'error_value' }, []);
};
const controller = placeholder(placeholderService, should_execute);
const controller = proxyquire('../../../controller/placeholder', {
'pelias-logger': logger
})(placeholder_service, _.constant(true));
const req = {
errors: []
@ -895,7 +1126,9 @@ module.exports.tests.error_conditions = (test, common) => {
const res = {};
controller(req, res, () => {
t.deepEquals(res, {}, 'res should not have been modified');
t.deepEquals(req.errors, [{ error_key: 'error_value' }]);
t.notOk(logger.isInfoMessage(/\\[controller:placeholder\\] \\[result_count:\\d+\\]/));
t.end();
});

Loading…
Cancel
Save