Merge pull request #99 from opentripplanner/pelias-sources-fix

Do not send Pelias query sources by default

__tests__/test-utils/fixtures/geocoding/pelias/autocomplete-response.json

@@ -11,12 +11,6 @@
         "Ends"
       ],
       "size": 10,
-      "sources": [
-        "geonames",
-        "openaddresses",
-        "openstreetmap",
-        "whosonfirst"
-      ],
       "private": false,
       "focus.point.lat": 45.52,
       "focus.point.lon": -122.67,

__tests__/util/__snapshots__/geocoder.js.snap

@@ -195,12 +195,6 @@ Object {
       "parser": "addressit",
       "private": false,
       "size": 10,
-      "sources": Array [
-        "geonames",
-        "openaddresses",
-        "openstreetmap",
-        "whosonfirst",
-      ],
       "text": "Mill Ends",
       "tokens": Array [
         "Mill",
@@ -215,7 +209,6 @@ Object {
   },
   "isomorphicMapzenSearchQuery": Object {
     "api_key": "dummy-mapzen-key",
-    "sources": "gn,oa,osm,wof",
     "text": "Mill Ends",
   },
   "type": "FeatureCollection",

__tests__/util/geocoder.js

@@ -1,6 +1,6 @@
 import nock from 'nock'
 
-import getGeocoder from '../../lib/util/geocoder'
+import getGeocoder, { PeliasGeocoder } from '../../lib/util/geocoder'
 
 function mockResponsePath (geocoder, file) {
   return `__tests__/test-utils/fixtures/geocoding/${geocoder}/${file}`
@@ -118,6 +118,71 @@ describe('geocoder', () => {
         const result = await getGeocoder(geocoder).getLocationFromGeocodedFeature(mockFeature)
         expect(result).toMatchSnapshot()
       })
+
+      // geocoder-specific tests
+      if (geocoderType === 'PELIAS') {
+        const mockSources = 'gn,oa,osm,wof'
+
+        // sources should not be sent unless they are explicitly defined in the
+        // query. See https://github.com/ibi-group/trimet-mod-otp/issues/239
+        it('should not send sources in autocomplete by default', () => {
+          // create mock API to check query
+          const mockPeliasAPI = {
+            autocomplete: query => {
+              expect(query.sources).not.toBe(expect.anything())
+              return Promise.resolve()
+            }
+          }
+          const pelias = new PeliasGeocoder(mockPeliasAPI, geocoder)
+          pelias.autocomplete({ text: 'Mill Ends' })
+        })
+
+        // should send sources if they're defined in the config
+        it('should send sources in autocomplete if defined in config', () => {
+          // create mock API to check query
+          const mockPeliasAPI = {
+            autocomplete: query => {
+              expect(query.sources).toBe(mockSources)
+              return Promise.resolve()
+            }
+          }
+          const pelias = new PeliasGeocoder(
+            mockPeliasAPI,
+            { ...geocoder, sources: mockSources }
+          )
+          pelias.autocomplete({ text: 'Mill Ends' })
+        })
+
+        // sources should not be sent unless they are explicitly defined in the
+        // query. See https://github.com/ibi-group/trimet-mod-otp/issues/239
+        it('should not send sources in search by default', () => {
+          // create mock API to check query
+          const mockPeliasAPI = {
+            search: query => {
+              expect(query.sources).not.toBe(expect.anything())
+              return Promise.resolve()
+            }
+          }
+          const pelias = new PeliasGeocoder(mockPeliasAPI, geocoder)
+          pelias.search({ text: 'Mill Ends' })
+        })
+
+        // should send sources if they're defined in the config
+        it('should send sources in search if defined in config', () => {
+          // create mock API to check query
+          const mockPeliasAPI = {
+            search: query => {
+              expect(query.sources).toBe(mockSources)
+              return Promise.resolve()
+            }
+          }
+          const pelias = new PeliasGeocoder(
+            mockPeliasAPI,
+            { ...geocoder, sources: mockSources }
+          )
+          pelias.search({ text: 'Mill Ends' })
+        })
+      }
     })
   })
 })

lib/util/geocoder.js

@@ -20,14 +20,8 @@ class Geocoder {
    * address or POI, attempt to find possible matches.
    */
   autocomplete (query) {
-    const {apiKey, baseUrl, boundary, focusPoint} = this.geocoderConfig
-    return this.api.autocomplete({
-      apiKey,
-      boundary,
-      focusPoint,
-      url: baseUrl ? `${baseUrl}/autocomplete` : undefined,
-      ...query
-    }).then(this._rewriteAutocompleteResponse)
+    return this.api.autocomplete(this.getAutocompleteQuery(query))
+      .then(this.rewriteAutocompleteResponse)
   }
 
   /**
@@ -50,46 +44,76 @@ class Geocoder {
    * GPS coordiante.
    */
   reverse (query) {
+    return this.api.reverse(this.getReverseQuery(query))
+      .then(this.rewriteReverseResponse)
+  }
+
+  /**
+   * Perform a search query. A search query is different from autocomplete in
+   * that it is assumed that the text provided is more or less a complete
+   * well-fromatted address.
+   */
+  search (query) {
+    return this.api.search(this.getSearchQuery(query))
+      .then(this.rewriteSearchResponse)
+  }
+
+  /**
+   * Default autocomplete query generator
+   */
+  getAutocompleteQuery (query) {
+    const {apiKey, baseUrl, boundary, focusPoint} = this.geocoderConfig
+    return {
+      apiKey,
+      boundary,
+      focusPoint,
+      url: baseUrl ? `${baseUrl}/autocomplete` : undefined,
+      ...query
+    }
+  }
+
+  /**
+   * Default reverse query generator
+   */
+  getReverseQuery (query) {
     const {apiKey, baseUrl} = this.geocoderConfig
-    return this.api.reverse({
+    return {
       apiKey,
       format: true,
       url: baseUrl ? `${baseUrl}/reverse` : undefined,
       ...query
-    }).then(this._rewriteReverseResponse)
+    }
   }
 
   /**
-   * Perform a search query. This query assumes that the text being searched
-   * is more-or-less an exact address or POI.
+   * Default search query generator.
    */
-  search (query) {
+  getSearchQuery (query) {
     const {apiKey, baseUrl, boundary, focusPoint} = this.geocoderConfig
-    return this.api.search({
+    return {
       apiKey,
       boundary,
       focusPoint,
-      sources: null,
       url: baseUrl ? `${baseUrl}/search` : undefined,
       format: false, // keep as returned GeoJSON,
       ...query
-    }).then(this._rewriteSearchResponse)
+    }
   }
 
   /**
    * Default rewriter for autocomplete responses
    */
-  _rewriteAutocompleteResponse (response) { return response }
+  rewriteAutocompleteResponse (response) { return response }
 
   /**
    * Default rewriter for reverse responses
    */
-  _rewriteReverseResponse (response) { return response }
+  rewriteReverseResponse (response) { return response }
 
   /**
    * Default rewriter for search responses
    */
-  _rewriteSearchResponse (response) { return response }
+  rewriteSearchResponse (response) { return response }
 }
 
 /**
@@ -118,7 +142,7 @@ class ArcGISGeocoder extends Geocoder {
    * Rewrite an autocomplete response into an application specific data format.
    * Also, filter out any results that are collections.
    */
-  _rewriteAutocompleteResponse (response) {
+  rewriteAutocompleteResponse (response) {
     return {
       // remove any autocomplete results that are collections
       // (eg multiple Starbucks)
@@ -137,7 +161,7 @@ class ArcGISGeocoder extends Geocoder {
    * Rewrite the response into an application-specific data format using the
    * first feature returned from the geocoder.
    */
-  _rewriteReverseResponse (response) {
+  rewriteReverseResponse (response) {
     const { features, query } = response
     const { lat, lon } = query
     return {
@@ -159,31 +183,31 @@ class NoApiGeocoder extends Geocoder {
    * Use coordinate string parser.
    */
   autocomplete (query) {
-    return this._parseCoordinateString(query.text)
+    return this.parseCoordinateString(query.text)
   }
 
   /**
    * Always return the lat/lon.
    */
   reverse (query) {
     let { lat, lon } = query.point
-    lat = this._roundGPSDecimal(lat)
-    lon = this._roundGPSDecimal(lon)
+    lat = this.roundGPSDecimal(lat)
+    lon = this.roundGPSDecimal(lon)
     return Promise.resolve({ lat, lon, name: `${lat}, ${lon}` })
   }
 
   /**
    * Use coordinate string parser.
    */
   search (query) {
-    return this._parseCoordinateString(query.text)
+    return this.parseCoordinateString(query.text)
   }
 
   /**
    * Attempt to parse the input as a GPS coordinate. If parseable, return a
    * feature.
    */
-  _parseCoordinateString (string) {
+  parseCoordinateString (string) {
     let feature
     try {
       feature = {
@@ -201,7 +225,7 @@ class NoApiGeocoder extends Geocoder {
     return Promise.resolve({ features: [feature] })
   }
 
-  _roundGPSDecimal (number) {
+  roundGPSDecimal (number) {
     const roundFactor = 100000
     return Math.round(number * roundFactor) / roundFactor
   }
@@ -211,14 +235,55 @@ class NoApiGeocoder extends Geocoder {
  * Geocoder implementation for the Pelias geocoder.
  * See https://pelias.io
  *
+ * This is exported for testing purposes only.
+ *
  * @extends Geocoder
  */
-class PeliasGeocoder extends Geocoder {
+export class PeliasGeocoder extends Geocoder {
+  /**
+   * Generate an autocomplete query specifically for the Pelias API. The
+   * `sources` parameter is a Pelias-specific option.
+   */
+  getAutocompleteQuery (query) {
+    const {apiKey, baseUrl, boundary, focusPoint, sources} = this.geocoderConfig
+    return {
+      apiKey,
+      boundary,
+      focusPoint,
+      // explicitly send over null for sources if provided sources is not truthy
+      // in order to avoid default isomorphic-mapzen-search sources form being
+      // applied
+      sources: sources || null,
+      url: baseUrl ? `${baseUrl}/autocomplete` : undefined,
+      ...query
+    }
+  }
+
+  /**
+   * Generate a search query specifically for the Pelias API. The
+   * `sources` parameter is a Pelias-specific option.
+   */
+  getSearchQuery (query) {
+    const {apiKey, baseUrl, boundary, focusPoint, sources} = this.geocoderConfig
+    return {
+      apiKey,
+      boundary,
+      focusPoint,
+      // explicitly send over null for sources if provided sources is not truthy
+      // in order to avoid default isomorphic-mapzen-search sources form being
+      // applied
+      sources: sources || null,
+      url: baseUrl ? `${baseUrl}/search` : undefined,
+      format: false, // keep as returned GeoJSON,
+      ...query
+    }
+  }
+
   /**
    * Rewrite the response into an application-specific data format using the
    * first feature returned from the geocoder.
    */
-  _rewriteReverseResponse (response) {
+  rewriteReverseResponse (response) {
     const { 'point.lat': lat, 'point.lon': lon } = response.isomorphicMapzenSearchQuery
     return {
       lat,