From 10241047a67bfff1e487d0fa4737c4b20d130b2d Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Fri, 1 Jun 2018 16:43:12 -0400 Subject: [PATCH] fix(interpolation): Ensure proper sorting of streets with interpolated addresses In cases where several conditions are met, it is possible for results to be returned from the API that are not sorted as they were intended. These conditions are: * over 10 results total were returned from Elasticsearch * the interpolation middleware was called * not all street results end up with possible interpolated address matches, and some of those streets come before other interpolated address records, necessitating a re-sorting of the results in the interpolation middleware In these cases, the ordering of streets as defined by Elasticsearch, such as by linguistic match or distance from a focus point, will no longer be respected in the results. This is because Node.js's `Array.prototype.sort` uses an [*un*stable QuickSort for arrays of size 11 or greater](https://github.com/nodejs/node/blob/master/deps/v8/src/js/array.js#L670). The solution is to switch to a sorting algorithm that is always stable. This ensures that whatever ordering was specified in the Elasticsearch queries is observed, without any of that logic having to be duplicated, and then possibly conflict. Stable sorting is provided by the [stable](http://npmjs.com/stable) NPM package. --- middleware/interpolate.js | 3 +- package.json | 1 + test/unit/middleware/interpolate.js | 75 +++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/middleware/interpolate.js b/middleware/interpolate.js index 10c3f7e8..8507709a 100644 --- a/middleware/interpolate.js +++ b/middleware/interpolate.js @@ -2,6 +2,7 @@ const async = require('async'); const logger = require( 'pelias-logger' ).get( 'api' ); const source_mapping = require('../helper/type_mapping').source_mapping; const _ = require('lodash'); +const stable = require('stable'); /** example response from interpolation web service: @@ -116,7 +117,7 @@ function setup(service, should_execute) { // sort the results to ensure that addresses show up higher than street centroids if (_.has(res, 'data')) { - res.data.sort((a, b) => { + res.data = stable(res.data, (a, b) => { if (a.layer === 'address' && b.layer !== 'address') { return -1; } if (a.layer !== 'address' && b.layer === 'address') { return 1; } return 0; diff --git a/package.json b/package.json index 3c5759da..196c8a78 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,7 @@ "pelias-sorting": "1.1.0", "predicates": "^2.0.0", "retry": "^0.10.1", + "stable": "^0.1.8", "stats-lite": "^2.0.4", "through2": "^2.0.3" }, diff --git a/test/unit/middleware/interpolate.js b/test/unit/middleware/interpolate.js index 3602592c..537c7444 100644 --- a/test/unit/middleware/interpolate.js +++ b/test/unit/middleware/interpolate.js @@ -277,6 +277,81 @@ module.exports.tests.success_conditions = (test, common) => { }); + test('results should be sorted first by address/non-address. previous ordering should otherwise be maintained via a stable sort', t => { + const service = (req, res, callback) => { + // results 5 and 7 will have interpolated results returned + // this is to ensure results are re-sorted to move the addresses first + if (res.id === 5 || res.id === 7) { + callback(null, { + properties: { + number: 17, + source: 'Source Abbr 1', + source_id: 'source 1 source id', + lat: 12.121212, + lon: 21.212121 + } + }); + } else { + // return empty results in most cases + callback(null, {}); + } + }; + + const logger = require('pelias-mock-logger')(); + + const controller = proxyquire('../../../middleware/interpolate', { + 'pelias-logger': logger + })(service, () => true); + + const req = { + clean: { + parsed_text: 'this is req.clean.parsed_text' + } + }; + const res = {}; + + // helper method to generate test results which default to streets + function generateTestStreets(id) { + return { + id: id+1, + layer: 'street', + name: { default: `name ${id+1}` }, + address_parts: {}, + source_id: 'original source_id' + }; + } + + // generate a set of street results of desired size + // NOTE: this set must be of 11 elements or greater + // Node.js uses stable insertion sort for arrays of 10 or fewer elements, + // but _unstable_ QuickSort for larger arrays + const resultCount = 11; + const sequence_array = Array.from(new Array(resultCount),(val,index)=>index); + res.data = sequence_array.map(generateTestStreets); + + controller(req, res, () => { + t.notOk(logger.hasErrorMessages(), 'there shouldn\'t be any error messages'); + + const results = res.data; + + t.equals(results.length, results.length, 'correct number of results should be returned'); + t.equals(results[0].layer, 'address', 'first result should be interpolated address'); + t.equals(results[1].layer, 'address', 'second result should be interpolated address'); + + // iterate through all remaining records, ensuring their ids are increasing, + // as was the case when the set of streets was originally generated + let previous_id; + for (let i = 2; i < results.length; i++) { + if (previous_id) { + t.ok(results[i].id > previous_id, `id ${results[i].id} should be higher than ${previous_id}, to ensure sort is stable`); + } + previous_id = results[i].id; + } + + t.end(); + }); + }); + test('service call returning error should not map in interpolated results for non-errors', t => { const service = (req, res, callback) => { if (res.id === 1) {