From e16e2bbb4e368b24d5b18b138b6d433c3d244b7f Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 09:36:57 -0400 Subject: [PATCH 1/9] fixed comment --- sanitiser/_geo_reverse.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index 4b5ee959..9de74fb0 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -14,7 +14,7 @@ module.exports = function sanitize( raw, clean ){ geo_common.sanitize_point( 'point', clean, raw, LAT_LON_IS_REQUIRED ); // this hack is to allow point.lat/point.lon to be used interchanagbly - // with boundary.circle.lat/boundary.circle/lon + // with boundary.circle.lat/boundary.circle.lon if( !clean.hasOwnProperty('boundary.circle.lat') && clean.hasOwnProperty('point.lat') ){ raw['boundary.circle.lat'] = clean['point.lat']; } From 947797f41e84247e7ee4a1e7c1bc093937ca1a1c Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 13:28:48 -0400 Subject: [PATCH 2/9] added warning if any of boundary.circle.lat/lon/radius are supplied + tests --- sanitiser/_geo_common.js | 3 +- sanitiser/_geo_reverse.js | 11 +++++ test/unit/run.js | 1 + test/unit/sanitiser/_geo_reverse.js | 67 +++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 test/unit/sanitiser/_geo_reverse.js diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index b88e9453..4e0f8c9b 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -55,7 +55,6 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { - // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; @@ -86,7 +85,7 @@ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { // radius was specified without lat or lon else if( raw.hasOwnProperty( key_prefix + '.radius' ) ){ var format2 = 'missing circle param \'%s\' requires all of: \'%s\' to be present'; - throw new Error( util.format( format2, key_prefix, mandatoryProps.join('\',\'') ) ); + throw new Error( util.format( format2, key_prefix, mandatoryProps.join('\',\'') ) ); } // fields required, eg. ( totalFieldsSpecified === 0 && bbox_is_required === true ) else if( circle_is_required ){ diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index 9de74fb0..6dc134cc 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -10,11 +10,22 @@ module.exports = function sanitize( raw, clean ){ // error & warning messages var messages = { errors: [], warnings: [] }; + // helper function to determine if raw has a boundary.circle property + var hasBoundaryCircleField = function(field) { + return raw.hasOwnProperty('boundary.circle.' + field); + }; + + if (['lat', 'lon', 'radius'].some(hasBoundaryCircleField)) { + messages.warnings.push('boundary.circle is currently unsupported and being ignored'); + } + try { geo_common.sanitize_point( 'point', clean, raw, LAT_LON_IS_REQUIRED ); // this hack is to allow point.lat/point.lon to be used interchanagbly // with boundary.circle.lat/boundary.circle.lon + // + // if clean doesn't have boundary.circle.lat but clean has point.lat, set boundary.circle.lat to point.lat if( !clean.hasOwnProperty('boundary.circle.lat') && clean.hasOwnProperty('point.lat') ){ raw['boundary.circle.lat'] = clean['point.lat']; } diff --git a/test/unit/run.js b/test/unit/run.js index efa0c4de..5e9aca75 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -30,6 +30,7 @@ var tests = [ require('./middleware/distance'), require('./middleware/confidenceScoreReverse'), require('./sanitiser/_size'), + require('./sanitiser/_geo_reverse'), ]; tests.map(function(t) { diff --git a/test/unit/sanitiser/_geo_reverse.js b/test/unit/sanitiser/_geo_reverse.js new file mode 100644 index 00000000..74f349d0 --- /dev/null +++ b/test/unit/sanitiser/_geo_reverse.js @@ -0,0 +1,67 @@ +var sanitize = require('../../../sanitiser/_geo_reverse'); + +module.exports.tests = {}; + +module.exports.tests.sanitize_boundary_country = function(test, common) { + test('raw with boundary.circle.lat should add warning about ignored boundary.circle', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.lat': '13.131313' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(clean['boundary.circle.lat'], 12.121212, 'should be set to point.lat'); + t.deepEquals(errorsAndWarnings, { + errors: [], + warnings: ['boundary.circle is currently unsupported and being ignored'] + }, 'no warnings/errors'); + t.end(); + }); + + test('raw with boundary.circle.lon should add warning about ignored boundary.circle', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.lon': '31.313131' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(clean['boundary.circle.lon'], 21.212121, 'should be set to point.lon'); + t.deepEquals(errorsAndWarnings, { + errors: [], + warnings: ['boundary.circle is currently unsupported and being ignored'] + }, 'no warnings/errors'); + t.end(); + }); + + test('raw with boundary.circle.radius should add warning about ignored boundary.circle', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.radius': '17' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + // t.equals(clean['boundary.circle.radius'], 12.121212, 'should be set to point.lat') + t.deepEquals(errorsAndWarnings, { + errors: [], + warnings: ['boundary.circle is currently unsupported and being ignored'] + }, 'no warnings/errors'); + t.end(); + }); + +}; + +module.exports.all = function (tape, common) { + function test(name, testFunction) { + return tape('SANTIZE _geo_reverse ' + name, testFunction); + } + + for( var testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; From 9fdddd3834bbd1d14ced16d237ff7363d2880e84 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 13:56:54 -0400 Subject: [PATCH 3/9] unrolled not-not conditional to positive conditional --- sanitiser/_geo_common.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index 4e0f8c9b..00112d89 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -2,7 +2,8 @@ * helper sanitiser methods for geo parameters */ var util = require('util'), - check = require('check-types'); + check = require('check-types'), + _ = require('lodash'); /** * Parse and validate rect parameter @@ -55,6 +56,8 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { + // "boundary.circle", clean, raw, true + // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; @@ -144,7 +147,7 @@ function sanitize_point( key_prefix, clean, raw, point_is_required ) { */ function sanitize_coord( key, clean, param, latlon_is_required ) { var value = parseFloat( param ); - if ( !isNaN( value ) ) { + if ( _.isFinite( value ) ) { clean[key] = value; } else if (latlon_is_required) { From 0595a290524d9fbfd720bcc6f7d0703a4fbe9d46 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 14:11:54 -0400 Subject: [PATCH 4/9] renamed parameters for readability --- sanitiser/_geo_common.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index 00112d89..c74db609 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -56,8 +56,6 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { - // "boundary.circle", clean, raw, true - // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; @@ -142,13 +140,13 @@ function sanitize_point( key_prefix, clean, raw, point_is_required ) { * * @param {string} key * @param {object} clean - * @param {string} param + * @param {string} rawValue * @param {bool} latlon_is_required */ -function sanitize_coord( key, clean, param, latlon_is_required ) { - var value = parseFloat( param ); - if ( _.isFinite( value ) ) { - clean[key] = value; +function sanitize_coord( key, clean, rawValue, latlon_is_required ) { + var parsedValue = parseFloat( rawValue ); + if ( _.isFinite( parsedValue ) ) { + clean[key] = parsedValue; } else if (latlon_is_required) { throw new Error( util.format( 'missing param \'%s\'', key ) ); From 0f6d08c0ecff8bc963905c7afa103df44a2dcb33 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 14:51:13 -0400 Subject: [PATCH 5/9] set boundary.circle.radius to default if caller did not supply it --- sanitiser/_geo_common.js | 2 ++ sanitiser/_geo_reverse.js | 21 +++++++------ test/unit/sanitiser/_geo_reverse.js | 47 +++++++++++++++++++++++++++++ test/unit/sanitiser/reverse.js | 2 ++ 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index c74db609..153128bc 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -56,6 +56,8 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { + // "boundary.circle", clean, raw, false + // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index 6dc134cc..bac20b29 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -1,5 +1,7 @@ var geo_common = require ('./_geo_common'); +var check = require('check-types'); +var defaults = require('../query/defaults'); var LAT_LON_IS_REQUIRED = true, CIRCLE_IS_REQUIRED = false, CIRCLE_MUST_BE_COMPLETE = false; @@ -20,20 +22,21 @@ module.exports = function sanitize( raw, clean ){ } try { + // first verify that point.lat/point.lon are valid geo_common.sanitize_point( 'point', clean, raw, LAT_LON_IS_REQUIRED ); - // this hack is to allow point.lat/point.lon to be used interchanagbly - // with boundary.circle.lat/boundary.circle.lon - // - // if clean doesn't have boundary.circle.lat but clean has point.lat, set boundary.circle.lat to point.lat - if( !clean.hasOwnProperty('boundary.circle.lat') && clean.hasOwnProperty('point.lat') ){ - raw['boundary.circle.lat'] = clean['point.lat']; - } - if( !clean.hasOwnProperty('boundary.circle.lon') && clean.hasOwnProperty('point.lon') ){ - raw['boundary.circle.lon'] = clean['point.lon']; + // overwrite boundary.circle.lat/lon with point.lat/lon + raw['boundary.circle.lat'] = clean['point.lat']; + raw['boundary.circle.lon'] = clean['point.lon']; + + // if no radius was passed, set the default + if (!check.assigned(raw['boundary.circle.radius'])) { + raw['boundary.circle.radius'] = defaults['boundary:circle:radius']; } + // santize the boundary.circle geo_common.sanitize_circle( 'boundary.circle', clean, raw, CIRCLE_IS_REQUIRED ); + } catch (err) { messages.errors.push( err.message ); diff --git a/test/unit/sanitiser/_geo_reverse.js b/test/unit/sanitiser/_geo_reverse.js index 74f349d0..c7fb778a 100644 --- a/test/unit/sanitiser/_geo_reverse.js +++ b/test/unit/sanitiser/_geo_reverse.js @@ -1,4 +1,5 @@ var sanitize = require('../../../sanitiser/_geo_reverse'); +var defaults = require('../../../query/defaults'); module.exports.tests = {}; @@ -54,6 +55,52 @@ module.exports.tests.sanitize_boundary_country = function(test, common) { t.end(); }); + test('boundary.circle.lat/lon should be overridden with point.lat/lon', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.lat': '13.131313', + 'boundary.circle.lon': '31.313131' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(raw['boundary.circle.lat'], 12.121212, 'should be set to point.lat'); + t.equals(raw['boundary.circle.lon'], 21.212121, 'should be set to point.lon'); + t.equals(clean['boundary.circle.lat'], 12.121212, 'should be set to point.lat'); + t.equals(clean['boundary.circle.lon'], 21.212121, 'should be set to point.lon'); + t.end(); + }); + + test('no boundary.circle.radius supplied should be set to default', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(raw['boundary.circle.radius'], defaults['boundary:circle:radius'], 'should be from defaults'); + t.equals(clean['boundary.circle.radius'], parseFloat(defaults['boundary:circle:radius']), 'should be same as raw'); + t.end(); + + }); + + test('explicit boundary.circle.radius should be used instead of default', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.radius': '3248732857km' // this will never be the default + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(raw['boundary.circle.radius'], '3248732857km', 'should be parsed float'); + t.equals(clean['boundary.circle.radius'], 3248732857.0, 'should be copied from raw'); + t.end(); + + }); + }; module.exports.all = function (tape, common) { diff --git a/test/unit/sanitiser/reverse.js b/test/unit/sanitiser/reverse.js index 759aef28..85943c73 100644 --- a/test/unit/sanitiser/reverse.js +++ b/test/unit/sanitiser/reverse.js @@ -4,11 +4,13 @@ var reverse = require('../../../sanitiser/reverse'), sanitize = reverse.sanitize, middleware = reverse.middleware, + defaults = require('../../../query/defaults'), defaultError = 'missing param \'lat\'', defaultClean = { 'point.lat': 0, 'point.lon': 0, 'boundary.circle.lat': 0, 'boundary.circle.lon': 0, + 'boundary.circle.radius': parseFloat(defaults['boundary:circle:radius']), types: { }, size: 10, From 5da4e4b36bdc8dbbb8870b2de5327da38c5ec4fa Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 14:52:54 -0400 Subject: [PATCH 6/9] removed unused variable --- sanitiser/_geo_reverse.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index bac20b29..a930c66b 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -3,8 +3,7 @@ var geo_common = require ('./_geo_common'); var check = require('check-types'); var defaults = require('../query/defaults'); var LAT_LON_IS_REQUIRED = true, - CIRCLE_IS_REQUIRED = false, - CIRCLE_MUST_BE_COMPLETE = false; + CIRCLE_IS_REQUIRED = false; // validate inputs, convert types and apply defaults module.exports = function sanitize( raw, clean ){ From 56d8600218546b5ab13d399d1a7b49cfbfe63aaf Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 14:54:10 -0400 Subject: [PATCH 7/9] modified warning message for brevity --- sanitiser/_geo_reverse.js | 2 +- test/unit/sanitiser/_geo_reverse.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index a930c66b..fb77add3 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -17,7 +17,7 @@ module.exports = function sanitize( raw, clean ){ }; if (['lat', 'lon', 'radius'].some(hasBoundaryCircleField)) { - messages.warnings.push('boundary.circle is currently unsupported and being ignored'); + messages.warnings.push('boundary.circle is currently unsupported'); } try { diff --git a/test/unit/sanitiser/_geo_reverse.js b/test/unit/sanitiser/_geo_reverse.js index c7fb778a..e03e7ddd 100644 --- a/test/unit/sanitiser/_geo_reverse.js +++ b/test/unit/sanitiser/_geo_reverse.js @@ -16,7 +16,7 @@ module.exports.tests.sanitize_boundary_country = function(test, common) { t.equals(clean['boundary.circle.lat'], 12.121212, 'should be set to point.lat'); t.deepEquals(errorsAndWarnings, { errors: [], - warnings: ['boundary.circle is currently unsupported and being ignored'] + warnings: ['boundary.circle is currently unsupported'] }, 'no warnings/errors'); t.end(); }); @@ -33,7 +33,7 @@ module.exports.tests.sanitize_boundary_country = function(test, common) { t.equals(clean['boundary.circle.lon'], 21.212121, 'should be set to point.lon'); t.deepEquals(errorsAndWarnings, { errors: [], - warnings: ['boundary.circle is currently unsupported and being ignored'] + warnings: ['boundary.circle is currently unsupported'] }, 'no warnings/errors'); t.end(); }); @@ -50,7 +50,7 @@ module.exports.tests.sanitize_boundary_country = function(test, common) { // t.equals(clean['boundary.circle.radius'], 12.121212, 'should be set to point.lat') t.deepEquals(errorsAndWarnings, { errors: [], - warnings: ['boundary.circle is currently unsupported and being ignored'] + warnings: ['boundary.circle is currently unsupported'] }, 'no warnings/errors'); t.end(); }); From 2ba206f1220fa92b8eacb6c68b768202139000a8 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 15:18:08 -0400 Subject: [PATCH 8/9] removed debug --- sanitiser/_geo_common.js | 1 - 1 file changed, 1 deletion(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index 153128bc..bc5a4bcc 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -56,7 +56,6 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { - // "boundary.circle", clean, raw, false // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; From 1c066b65e73c6f4a11722ae889f174f0b3b27291 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 15:47:17 -0400 Subject: [PATCH 9/9] switched undefined check to positive lodash call --- sanitiser/_geo_reverse.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index fb77add3..c15af48b 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -1,6 +1,6 @@ var geo_common = require ('./_geo_common'); -var check = require('check-types'); +var _ = require('lodash'); var defaults = require('../query/defaults'); var LAT_LON_IS_REQUIRED = true, CIRCLE_IS_REQUIRED = false; @@ -29,7 +29,7 @@ module.exports = function sanitize( raw, clean ){ raw['boundary.circle.lon'] = clean['point.lon']; // if no radius was passed, set the default - if (!check.assigned(raw['boundary.circle.radius'])) { + if ( _.isUndefined( raw['boundary.circle.radius'] ) ) { raw['boundary.circle.radius'] = defaults['boundary:circle:radius']; }