Browse Source

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.
pull/1150/head
Julian Simioni 7 years ago
parent
commit
10241047a6
No known key found for this signature in database
GPG Key ID: B9EEB0C6EE0910A1
  1. 3
      middleware/interpolate.js
  2. 1
      package.json
  3. 75
      test/unit/middleware/interpolate.js

3
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;

1
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"
},

75
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) {

Loading…
Cancel
Save