Merge pull request #246 from pelias/dont-trust-neighbourhood

feat(layers): Avoid trusting the WOF hierarchy of some layers (notably neighbourhood)

src/pip/index.js

@@ -108,6 +108,7 @@ module.exports.create = function createPIPService(datapath, layers, localizedAdm
           // bookkeeping object that tracks the progress of the request
           responseQueue[id] = {
             latLon: {latitude: latitude, longitude: longitude},
+            hierarchy: [],
             // copy of layers to search
             search_layers: search_layers.slice(),
             responseCallback: responseCallback
@@ -192,19 +193,47 @@ function handleResults(msg) {
       // if there are no results, then call the next layer but only if there are more layer to call
       searchWorker(msg.id);
     } else {
-      // no layers left to search, so return an empty array
-      responseQueue[msg.id].responseCallback(null, []);
+      // no layers left to search, check if there is a partial hierarchy from an untrusted layer
+      if (responseQueue[msg.id].hierarchy.length > 0) {
+        responseQueue[msg.id].responseCallback(null, responseQueue[msg.id].hierarchy);
+      } else {
+        //respond with empty aresult
+        responseQueue[msg.id].responseCallback(null, []);
+      }
 
       delete responseQueue[msg.id];
     }
 
   } else {
     // there was a hit, so find the hierachy and assemble all the pieces
-    const results = _.compact(msg.results.Hierarchy[0].map(id => wofData[id]));
+    const untrustedLayers = ['neighbourhood'];
 
-    responseQueue[msg.id].responseCallback(null, results);
+    if (untrustedLayers.includes(msg.layer)) {
+      // push _only_ the first layers results onto the hierarchy
+      const this_layer_id = msg.results.Id;
+      if (this_layer_id) {
+        responseQueue[msg.id].hierarchy.push(wofData[this_layer_id]);
+      }
 
-    delete responseQueue[msg.id];
-  }
+      // continue search if there are more layers. Otherwise return full hierarchy
+      if (responseQueue[msg.id].search_layers !== 0) {
+        searchWorker(msg.id);
+      } else {
+        // assemble full hierarchy and return
+        const results = _.compact(msg.results.Hierarchy[0].map(id => wofData[id]));
+
+        responseQueue[msg.id].responseCallback(null, results);
+
+        delete responseQueue[msg.id];
+      }
+
+    } else {
+      // assemble full hierarchy and return
+      const results = responseQueue[msg.id].hierarchy.concat(_.compact(msg.results.Hierarchy[0].map(id => wofData[id])));
+
+      responseQueue[msg.id].responseCallback(null, results);
 
+      delete responseQueue[msg.id];
+    }
+  }
 }

src/pip/worker.js

@@ -33,10 +33,11 @@ process.on('SIGTERM', () => {
 
 readStream(datapath, layer, localizedAdminNames, (features) => {
   // find all the properties of all features and write them to a file
-  // at the same time, limit the feature.properties to just Id since it's all that's needed in the worker
+  // at the same time, limit the feature.properties to just Id and Hierarchy since it's all that's needed in the worker
   const data = features.reduce((acc, feature) => {
     acc[feature.properties.Id] = feature.properties;
     feature.properties = {
+      Id: feature.properties.Id,
       Hierarchy: feature.properties.Hierarchy
     };
     return acc;

test/pip/index.js

@@ -66,7 +66,7 @@ tape('PiP tests', test => {
 
   });
 
-  test.test('requested lat/lon inside a polygon should return that record\'s hierarchy', t => {
+  test.test('requested lat/lon inside a neighbourhood polygon should return only neighbourhood', t => {
     temp.mkdir('tmp_wof_data', (err, temp_dir) => {
       fs.mkdirSync(path.join(temp_dir, 'data'));
       fs.mkdirSync(path.join(temp_dir, 'meta'));
@@ -138,6 +138,7 @@ tape('PiP tests', test => {
       fs.writeFileSync(path.join(temp_dir, 'data', 'borough_record.geojson'), JSON.stringify(borough_record));
 
       const service = pip.create(temp_dir, ['neighbourhood', 'borough'], false, (err, o) => {
+        // lookup of point that only hits neighbourhood will NOT return borough
         o.lookup(1.5, 1.5, undefined, (err, results) => {
           t.deepEquals(results, [
             {
@@ -150,6 +151,104 @@ tape('PiP tests', test => {
               },
               BoundingBox: '1,1,2,2',
               Hierarchy: [ [ 123, 456 ] ]
+            }
+          ]);
+          // must be explicitly ended or the test hangs
+          o.end();
+          t.end();
+        });
+
+      });
+
+    });
+
+  });
+
+  test.test('requested lat/lon inside a neighbourhood and parent borough should return full hierarchy', t => {
+    temp.mkdir('tmp_wof_data', (err, temp_dir) => {
+      fs.mkdirSync(path.join(temp_dir, 'data'));
+      fs.mkdirSync(path.join(temp_dir, 'meta'));
+
+      // write out the WOF meta files with the minimum required fields
+      fs.writeFileSync(
+        path.join(temp_dir, 'meta', 'whosonfirst-data-neighbourhood-latest.csv'),
+        `id,name,path${EOL}123,place name,neighbourhood_record.geojson${EOL}`);
+      fs.writeFileSync(
+        path.join(temp_dir, 'meta', 'whosonfirst-data-borough-latest.csv'),
+        `id,name,path${EOL}456,borough name,borough_record.geojson${EOL}`);
+
+      // setup a neighbourhood WOF record
+      const neighbourhood_record = {
+        id: 123,
+        type: 'Feature',
+        properties: {
+          'geom:bbox': '1,1,4,4',
+          'geom:latitude': 1.5,
+          'geom:longitude': 1.5,
+          'mz:hierarchy_label': 1,
+          'wof:hierarchy': [
+            {
+              locality_id: 123,
+              region_id: 456
+            }
+          ],
+          'wof:id': 123,
+          'wof:name': 'neighbourhood name',
+          'wof:placetype': 'neighbourhood'
+        },
+        geometry: {
+          coordinates: [
+            [
+              [1,1],[4,1],[4,4],[1,4],[1,1]
+            ]
+          ],
+          type: 'Polygon'
+        }
+      };
+
+      // setup a borough WOF record that doesn't contain the lookup point to
+      // show that hierarchy is used to establish the response
+      const borough_record = {
+        id: 456,
+        type: 'Feature',
+        properties: {
+          'geom:bbox': '3,3,4,4',
+          'geom:latitude': 3.5,
+          'geom:longitude': 3.5,
+          'mz:hierarchy_label': 1,
+          'wof:hierarchy': [],
+          'wof:id': 456,
+          'wof:name': 'borough name',
+          'wof:placetype': 'borough'
+        },
+        geometry: {
+          coordinates: [
+            [
+              [3,3],[3,4],[4,4],[4,3],[3,3]
+            ]
+          ],
+          type: 'Polygon'
+        }
+      };
+
+      // and write the records to file
+      fs.writeFileSync(path.join(temp_dir, 'data', 'neighbourhood_record.geojson'), JSON.stringify(neighbourhood_record));
+      fs.writeFileSync(path.join(temp_dir, 'data', 'borough_record.geojson'), JSON.stringify(borough_record));
+
+      const service = pip.create(temp_dir, ['neighbourhood', 'borough'], false, (err, o) => {
+        // lookup of point that only hits neighbourhood will NOT return borough
+        o.lookup(3.5, 3.5, undefined, (err, results) => {
+          t.deepEquals(results, [
+            {
+              Id: 123,
+              Name: 'neighbourhood name',
+              Placetype: 'neighbourhood',
+              Centroid: {
+                lat: 1.5,
+                lon: 1.5
+              },
+              BoundingBox: '1,1,4,4',
+              Hierarchy: [ [ 123, 456 ] ]
             },
             {
               Id: 456,

test/pip/worker.js

@@ -78,6 +78,7 @@ tape('worker tests', (test) => {
             type: 'results',
             layer: 'test_layer',
             results: {
+              Id: 17,
               Hierarchy: [ [11, 13] ]
             }
           }, 'the hierarchy of the WOF record should be returned');