From b44bd7a427e50757d66f2d7ec5bda3f28c3450f1 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 18 Apr 2017 21:15:47 -0400 Subject: [PATCH 1/7] Increase debounce threshold for taginfo --- modules/services/taginfo.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/services/taginfo.js b/modules/services/taginfo.js index 8c019072e..bf2ac6c77 100644 --- a/modules/services/taginfo.js +++ b/modules/services/taginfo.js @@ -129,7 +129,7 @@ function sortKeys(a, b) { } -var debounced = _.debounce(d3.json, 100, true); +var debounced = _.debounce(d3.json, 750, true); function request(url, debounce, callback) { From c2d6c84aee1f10638d55daba42292a60647e2149 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 18 Apr 2017 23:29:01 -0400 Subject: [PATCH 2/7] Exclude popular keys from values lookups.. see #3955 --- modules/services/taginfo.js | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/modules/services/taginfo.js b/modules/services/taginfo.js index bf2ac6c77..83eac26f8 100644 --- a/modules/services/taginfo.js +++ b/modules/services/taginfo.js @@ -5,6 +5,7 @@ import { utilQsString } from '../util/index'; var endpoint = 'https://taginfo.openstreetmap.org/api/4/', taginfoCache = {}, + popularKeys = {}, tag_sorts = { point: 'count_nodes', vertex: 'count_nodes', @@ -152,7 +153,29 @@ function request(url, debounce, callback) { export default { - init: function() { taginfoCache = {}; }, + init: function() { + taginfoCache = {}; + popularKeys = {}; + + // Fetch popular keys. We'll exclude these from `values` + // lookups because they stress taginfo, and they aren't likely + // to yield meaningful autocomplete results.. see #3955 + this.keys({ + rp: 100, + sortname: 'values_all', + sortorder: 'desc', + page: 1, + debounce: false + }, function(err, data) { + if (err) return; + data.forEach(function(d) { + if (d === 'opening_hours') return; // exception + popularKeys[d.value] = true; + }); + }); + }, + + reset: function() { taginfoCache = {}; }, @@ -200,6 +223,13 @@ export default { values: function(parameters, callback) { + // Exclude popular keys from values lookups.. see #3955 + var key = parameters.key; + if (key && popularKeys[key]) { + callback(null, []); + return; + } + var debounce = parameters.debounce; parameters = clean(setSort(setFilter(parameters))); request(endpoint + 'key/values?' + From 494dd979a98065bfbd5603031217bcf2211d8ad3 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Wed, 19 Apr 2017 09:25:09 -0400 Subject: [PATCH 3/7] Never reset the taginfo cache The data on taginfo is refreshed once per day, so there's no reason to reissue those API calls during a normal editing session. reset() gets called after the user saves their changes to OSM --- modules/services/taginfo.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/services/taginfo.js b/modules/services/taginfo.js index 83eac26f8..f83540241 100644 --- a/modules/services/taginfo.js +++ b/modules/services/taginfo.js @@ -176,7 +176,7 @@ export default { }, - reset: function() { taginfoCache = {}; }, + reset: function() { }, keys: function(parameters, callback) { From 62d0bc0bf516795a2e56d7f0e854396a43ca0a9b Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Wed, 19 Apr 2017 13:13:12 -0400 Subject: [PATCH 4/7] Debounce should debounce all requests even cached, fix tests (see https://github.com/openstreetmap/iD/pull/3975#issue-222747308) --- modules/services/taginfo.js | 215 ++++++++++++++++------------------ test/spec/services/taginfo.js | 197 +++++++++++++++++++++---------- 2 files changed, 235 insertions(+), 177 deletions(-) diff --git a/modules/services/taginfo.js b/modules/services/taginfo.js index f83540241..62fe3b6b4 100644 --- a/modules/services/taginfo.js +++ b/modules/services/taginfo.js @@ -1,6 +1,6 @@ import * as d3 from 'd3'; import _ from 'lodash'; -import { utilQsString } from '../util/index'; +import { utilQsString } from '../util'; var endpoint = 'https://taginfo.openstreetmap.org/api/4/', @@ -34,31 +34,31 @@ var endpoint = 'https://taginfo.openstreetmap.org/api/4/', }; -function sets(parameters, n, o) { - if (parameters.geometry && o[parameters.geometry]) { - parameters[n] = o[parameters.geometry]; +function sets(params, n, o) { + if (params.geometry && o[params.geometry]) { + params[n] = o[params.geometry]; } - return parameters; + return params; } -function setFilter(parameters) { - return sets(parameters, 'filter', tag_filters); +function setFilter(params) { + return sets(params, 'filter', tag_filters); } -function setSort(parameters) { - return sets(parameters, 'sortname', tag_sorts); +function setSort(params) { + return sets(params, 'sortname', tag_sorts); } -function setSortMembers(parameters) { - return sets(parameters, 'sortname', tag_sort_members); +function setSortMembers(params) { + return sets(params, 'sortname', tag_sort_members); } -function clean(parameters) { - return _.omit(parameters, 'geometry', 'debounce'); +function clean(params) { + return _.omit(params, 'geometry', 'debounce'); } @@ -130,23 +130,18 @@ function sortKeys(a, b) { } -var debounced = _.debounce(d3.json, 750, true); +var debouncedRequest = _.debounce(request, 750, { leading: false }); - -function request(url, debounce, callback) { +function request(url, params, callback) { if (taginfoCache[url]) { callback(null, taginfoCache[url]); - } else if (debounce) { - debounced(url, done); } else { - d3.json(url, done); - } - - function done(err, data) { - if (!err) { - taginfoCache[url] = data; - } - callback(err, data); + d3.json(url, function (err, data) { + if (!err) { + taginfoCache[url] = data; + } + callback(err, data); + }); } } @@ -160,13 +155,8 @@ export default { // Fetch popular keys. We'll exclude these from `values` // lookups because they stress taginfo, and they aren't likely // to yield meaningful autocomplete results.. see #3955 - this.keys({ - rp: 100, - sortname: 'values_all', - sortorder: 'desc', - page: 1, - debounce: false - }, function(err, data) { + var params = { rp: 100, sortname: 'values_all', sortorder: 'desc', page: 1, debounce: false }; + this.keys(params, function(err, data) { if (err) return; data.forEach(function(d) { if (d === 'opening_hours') return; // exception @@ -179,114 +169,107 @@ export default { reset: function() { }, - keys: function(parameters, callback) { - var debounce = parameters.debounce; - parameters = clean(setSort(parameters)); - request(endpoint + 'keys/all?' + - utilQsString(_.extend({ - rp: 10, - sortname: 'count_all', - sortorder: 'desc', - page: 1 - }, parameters)), debounce, function(err, d) { - if (err) { - callback(err); - } else { - var f = filterKeys(parameters.filter); - callback(null, d.data.filter(f).sort(sortKeys).map(valKey)); - } - } + keys: function(params, callback) { + var req = params.debounce ? debouncedRequest : request; + params = clean(setSort(params)); + + var qs = utilQsString( + _.extend({ rp: 10, sortname: 'count_all', sortorder: 'desc', page: 1 }, params) ); + + + req(endpoint + 'keys/all?' + qs, params, function(err, d) { + if (err) { + callback(err); + } else { + var f = filterKeys(params.filter); + callback(null, d.data.filter(f).sort(sortKeys).map(valKey)); + } + }); }, - multikeys: function(parameters, callback) { - var debounce = parameters.debounce; - parameters = clean(setSort(parameters)); - var prefix = parameters.query; - request(endpoint + 'keys/all?' + - utilQsString(_.extend({ - rp: 25, - sortname: 'count_all', - sortorder: 'desc', - page: 1 - }, parameters)), debounce, function(err, d) { - if (err) { - callback(err); - } else { - var f = filterMultikeys(prefix); - callback(null, d.data.filter(f).map(valKey)); - } - } + multikeys: function(params, callback) { + var req = params.debounce ? debouncedRequest : request; + params = clean(setSort(params)); + var prefix = params.query; + + var qs = utilQsString( + _.extend({ rp: 25, sortname: 'count_all', sortorder: 'desc', page: 1 }, params) ); + + req(endpoint + 'keys/all?' + qs, params, function(err, d) { + if (err) { + callback(err); + } else { + var f = filterMultikeys(prefix); + callback(null, d.data.filter(f).map(valKey)); + } + }); }, - values: function(parameters, callback) { + values: function(params, callback) { // Exclude popular keys from values lookups.. see #3955 - var key = parameters.key; + var key = params.key; if (key && popularKeys[key]) { callback(null, []); return; } - var debounce = parameters.debounce; - parameters = clean(setSort(setFilter(parameters))); - request(endpoint + 'key/values?' + - utilQsString(_.extend({ - rp: 25, - sortname: 'count_all', - sortorder: 'desc', - page: 1 - }, parameters)), debounce, function(err, d) { - if (err) { - callback(err); - } else { - // In most cases we prefer taginfo value results with lowercase letters. - // A few OSM keys expect values to contain uppercase values (see #3377). - // This is not an exhaustive list (e.g. `name` also has uppercase values) - // but these are the fields where taginfo value lookup is most useful. - var re = /network|taxon|genus|species|brand|grape_variety|_hours|_times/; - var allowUpperCase = (parameters.key.match(re) !== null); - var f = filterValues(allowUpperCase); - callback(null, d.data.filter(f).map(valKeyDescription)); - } - } + var req = params.debounce ? debouncedRequest : request; + params = clean(setSort(setFilter(params))); + + var qs = utilQsString( + _.extend({ rp: 25, sortname: 'count_all', sortorder: 'desc', page: 1 }, params) ); + + req(endpoint + 'key/values?' + qs, params, function(err, d) { + if (err) { + callback(err); + } else { + // In most cases we prefer taginfo value results with lowercase letters. + // A few OSM keys expect values to contain uppercase values (see #3377). + // This is not an exhaustive list (e.g. `name` also has uppercase values) + // but these are the fields where taginfo value lookup is most useful. + var re = /network|taxon|genus|species|brand|grape_variety|_hours|_times/; + var allowUpperCase = (params.key.match(re) !== null); + var f = filterValues(allowUpperCase); + callback(null, d.data.filter(f).map(valKeyDescription)); + } + }); }, - roles: function(parameters, callback) { - var debounce = parameters.debounce; - var geometry = parameters.geometry; - parameters = clean(setSortMembers(parameters)); - request(endpoint + 'relation/roles?' + - utilQsString(_.extend({ - rp: 25, - sortname: 'count_all_members', - sortorder: 'desc', - page: 1 - }, parameters)), debounce, function(err, d) { - if (err) { - callback(err); - } else { - var f = filterRoles(geometry); - callback(null, d.data.filter(f).map(roleKey)); - } - } + roles: function(params, callback) { + var req = params.debounce ? debouncedRequest : request; + var geometry = params.geometry; + params = clean(setSortMembers(params)); + + var qs = utilQsString( + _.extend({ rp: 25, sortname: 'count_all_members', sortorder: 'desc', page: 1 }, params) ); + + req(endpoint + 'relation/roles?' + qs, params, function(err, d) { + if (err) { + callback(err); + } else { + var f = filterRoles(geometry); + callback(null, d.data.filter(f).map(roleKey)); + } + }); }, - docs: function(parameters, callback) { - var debounce = parameters.debounce; - parameters = clean(setSort(parameters)); + docs: function(params, callback) { + var req = params.debounce ? debouncedRequest : request; + params = clean(setSort(params)); var path = 'key/wiki_pages?'; - if (parameters.value) path = 'tag/wiki_pages?'; - else if (parameters.rtype) path = 'relation/wiki_pages?'; + if (params.value) path = 'tag/wiki_pages?'; + else if (params.rtype) path = 'relation/wiki_pages?'; - request(endpoint + path + utilQsString(parameters), debounce, function(err, d) { + req(endpoint + path + utilQsString(params), params, function(err, d) { if (err) { callback(err); } else { diff --git a/test/spec/services/taginfo.js b/test/spec/services/taginfo.js index 7ffff2840..fab95b44e 100644 --- a/test/spec/services/taginfo.js +++ b/test/spec/services/taginfo.js @@ -4,7 +4,16 @@ describe('iD.serviceTaginfo', function() { beforeEach(function() { server = sinon.fakeServer.create(); taginfo = iD.services.taginfo; - taginfo.reset(); + + // prepopulate popular keys list with "name" + taginfo.init(); + server.respondWith('GET', + new RegExp('\/keys\/all.*sortname=values_all'), + [200, { 'Content-Type': 'application/json' }, + '{"data":[{"count_all":56136034,"key":"name","count_all_fraction":0.0132}]}'] + ); + server.respond(); + server = sinon.fakeServer.create(); }); afterEach(function() { @@ -15,55 +24,67 @@ describe('iD.serviceTaginfo', function() { return iD.utilStringQs(url.substring(url.indexOf('?') + 1)); } + describe('#keys', function() { it('calls the given callback with the results of the keys query', function() { var callback = sinon.spy(); taginfo.keys({query: 'amen'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/keys/all'), + server.respondWith('GET', /\/keys\/all/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"count_all":5190337,"key":"amenity","count_all_fraction":1.0}]}']); + '{"data":[{"count_all":5190337,"key":"amenity","count_all_fraction":1.0}]}'] + ); server.respond(); expect(query(server.requests[0].url)).to.eql( - {query: 'amen', page: '1', rp: '10', sortname: 'count_all', sortorder: 'desc'}); - expect(callback).to.have.been.calledWith(null, [{'title':'amenity', 'value':'amenity'}]); + {query: 'amen', page: '1', rp: '10', sortname: 'count_all', sortorder: 'desc'} + ); + expect(callback).to.have.been.calledWith( + null, [{'title':'amenity', 'value':'amenity'}] + ); }); it('includes popular keys', function() { var callback = sinon.spy(); taginfo.keys({query: 'amen'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/keys/all'), + server.respondWith('GET', /\/keys\/all/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"count_all":5190337,"key":"amenity","count_all_fraction":1.0, "count_nodes_fraction":1.0},' - + '{"count_all":1,"key":"amenityother","count_all_fraction":0.0, "count_nodes_fraction":0.0}]}']); + '{"data":[{"count_all":5190337,"key":"amenity","count_all_fraction":1.0,"count_nodes_fraction":1.0},' + + '{"count_all":1,"key":"amenityother","count_all_fraction":0.0,"count_nodes_fraction":0.0}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'title':'amenity', 'value':'amenity'}]); + expect(callback).to.have.been.calledWith( + null, [{'title':'amenity', 'value':'amenity'}] + ); }); it('includes popular keys with an entity type filter', function() { var callback = sinon.spy(); taginfo.keys({query: 'amen', filter: 'nodes'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/keys/all'), + server.respondWith('GET', /\/keys\/all/, [200, { 'Content-Type': 'application/json' }, '{"data":[{"count_all":5190337,"count_nodes":500000,"key":"amenity","count_all_fraction":1.0, "count_nodes_fraction":1.0},' - + '{"count_all":1,"key":"amenityother","count_all_fraction":0.0, "count_nodes":100}]}']); + + '{"count_all":1,"key":"amenityother","count_all_fraction":0.0, "count_nodes":100}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'title':'amenity', 'value':'amenity'}]); + expect(callback).to.have.been.calledWith( + null, [{'title':'amenity', 'value':'amenity'}] + ); }); it('includes unpopular keys with a wiki page', function() { var callback = sinon.spy(); taginfo.keys({query: 'amen'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/keys/all'), + server.respondWith('GET', /\/keys\/all/, [200, { 'Content-Type': 'application/json' }, '{"data":[{"count_all":5190337,"key":"amenity","count_all_fraction":1.0, "count_nodes_fraction":1.0},' - + '{"count_all":1,"key":"amenityother","count_all_fraction":0.0, "count_nodes_fraction":0.0, "in_wiki": true}]}']); + + '{"count_all":1,"key":"amenityother","count_all_fraction":0.0, "count_nodes_fraction":0.0, "in_wiki": true}]}'] + ); server.respond(); expect(callback).to.have.been.calledWith(null, [ @@ -76,12 +97,16 @@ describe('iD.serviceTaginfo', function() { var callback = sinon.spy(); taginfo.keys({query: 'ref'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/keys/all'), + server.respondWith('GET', /\/keys\/all/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"key":"ref:bag","count_all":9790586,"count_all_fraction":0.0028},{"key":"ref","count_all":7933528,"count_all_fraction":0.0023}]}']); + '{"data":[{"key":"ref:bag","count_all":9790586,"count_all_fraction":0.0028},' + + '{"key":"ref","count_all":7933528,"count_all_fraction":0.0023}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'title':'ref', 'value':'ref'},{'title':'ref:bag', 'value':'ref:bag'}]); + expect(callback).to.have.been.calledWith( + null, [{'title':'ref', 'value':'ref'},{'title':'ref:bag', 'value':'ref:bag'}] + ); }); }); @@ -90,40 +115,50 @@ describe('iD.serviceTaginfo', function() { var callback = sinon.spy(); taginfo.multikeys({query: 'recycling:'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/keys/all'), + server.respondWith('GET', /\/keys\/all/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"count_all":69593,"key":"recycling:glass","count_all_fraction":0.0}]}']); + '{"data":[{"count_all":69593,"key":"recycling:glass","count_all_fraction":0.0}]}'] + ); server.respond(); expect(query(server.requests[0].url)).to.eql( - {query: 'recycling:', page: '1', rp: '25', sortname: 'count_all', sortorder: 'desc'}); - expect(callback).to.have.been.calledWith(null, [{'title':'recycling:glass', 'value':'recycling:glass'}]); + {query: 'recycling:', page: '1', rp: '25', sortname: 'count_all', sortorder: 'desc'} + ); + expect(callback).to.have.been.calledWith( + null, [{'title':'recycling:glass', 'value':'recycling:glass'}] + ); }); it('excludes multikeys with extra colons', function() { var callback = sinon.spy(); taginfo.multikeys({query: 'service:bicycle:'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/keys/all'), + server.respondWith('GET', /\/keys\/all/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"count_all":4426,"key":"service:bicycle:retail","count_all_fraction":0.0},' - + '{"count_all":22,"key":"service:bicycle:retail:ebikes","count_all_fraction":0.0}]}']); + '{"data":[{"count_all":4426,"key":"service:bicycle:retail","count_all_fraction":0.0},' + + '{"count_all":22,"key":"service:bicycle:retail:ebikes","count_all_fraction":0.0}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'title':'service:bicycle:retail', 'value':'service:bicycle:retail'}]); + expect(callback).to.have.been.calledWith( + null, [{'title':'service:bicycle:retail', 'value':'service:bicycle:retail'}] + ); }); it('excludes multikeys with wrong prefix', function() { var callback = sinon.spy(); taginfo.multikeys({query: 'service:bicycle:'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/keys/all'), + server.respondWith('GET', /\/keys\/all/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"count_all":4426,"key":"service:bicycle:retail","count_all_fraction":0.0},' - + '{"count_all":22,"key":"disused:service:bicycle","count_all_fraction":0.0}]}']); + '{"data":[{"count_all":4426,"key":"service:bicycle:retail","count_all_fraction":0.0},' + + '{"count_all":22,"key":"disused:service:bicycle","count_all_fraction":0.0}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'title':'service:bicycle:retail', 'value':'service:bicycle:retail'}]); + expect(callback).to.have.been.calledWith( + null, [{'title':'service:bicycle:retail', 'value':'service:bicycle:retail'}] + ); }); }); @@ -132,37 +167,59 @@ describe('iD.serviceTaginfo', function() { var callback = sinon.spy(); taginfo.values({key: 'amenity', query: 'par'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/key/values'), + server.respondWith('GET', /\/key\/values/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"value":"parking","description":"A place for parking cars", "fraction":0.1}]}']); + '{"data":[{"value":"parking","description":"A place for parking cars", "fraction":0.1}]}'] + ); server.respond(); expect(query(server.requests[0].url)).to.eql( - {key: 'amenity', query: 'par', page: '1', rp: '25', sortname: 'count_all', sortorder: 'desc'}); - expect(callback).to.have.been.calledWith(null, [{'value':'parking','title':'A place for parking cars'}]); + {key: 'amenity', query: 'par', page: '1', rp: '25', sortname: 'count_all', sortorder: 'desc'} + ); + expect(callback).to.have.been.calledWith( + null, [{'value':'parking','title':'A place for parking cars'}] + ); }); it('includes popular values', function() { var callback = sinon.spy(); taginfo.values({key: 'amenity', query: 'par'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/key/values'), + server.respondWith('GET', /\/key\/values/, [200, { 'Content-Type': 'application/json' }, '{"data":[{"value":"parking","description":"A place for parking cars", "fraction":1.0},' + - '{"value":"party","description":"A place for partying", "fraction":0.0}]}']); + '{"value":"party","description":"A place for partying", "fraction":0.0}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'value':'parking','title':'A place for parking cars'}]); + expect(callback).to.have.been.calledWith( + null, [{'value':'parking','title':'A place for parking cars'}] + ); + }); + + it('does not get values for extremely popular keys', function() { + var callback = sinon.spy(); + taginfo.values({key: 'name', query: 'ste'}, callback); + + server.respondWith('GET', /\/key\/values/, + [200, { 'Content-Type': 'application/json' }, + '{"data":[{"value":"Rue Pasteur","description":"", "fraction":0.0001},' + + '{"value":"Via Trieste","description":"", "fraction":0.0001}]}'] + ); + server.respond(); + + expect(callback).to.have.been.calledWith(null, []); }); it('includes unpopular values with a wiki page', function() { var callback = sinon.spy(); taginfo.values({key: 'amenity', query: 'par'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/key/values'), + server.respondWith('GET', /\/key\/values/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"value":"parking","description":"A place for parking cars", "fraction":1.0},' - + '{"value":"party","description":"A place for partying", "fraction":0.0, "in_wiki": true}]}']); + '{"data":[{"value":"parking","description":"A place for parking cars", "fraction":1.0},' + + '{"value":"party","description":"A place for partying", "fraction":0.0, "in_wiki": true}]}'] + ); server.respond(); expect(callback).to.have.been.calledWith(null, [ @@ -175,29 +232,33 @@ describe('iD.serviceTaginfo', function() { var callback = sinon.spy(); taginfo.values({key: 'amenity', query: 'par'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/key/values'), + server.respondWith('GET', /\/key\/values/, [200, { 'Content-Type': 'application/json' }, '{"data":[{"value":"parking","description":"A place for parking cars", "fraction":0.2},' + '{"value":"PArking","description":"A common mispelling", "fraction":0.2},' + '{"value":"parking;partying","description":"A place for parking cars *and* partying", "fraction":0.2},' + '{"value":"parking, partying","description":"A place for parking cars *and* partying", "fraction":0.2},' - + '{"value":"*","description":"", "fraction":0.2}]}']); + + '{"value":"*","description":"", "fraction":0.2}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'value':'parking','title':'A place for parking cars'}]); + expect(callback).to.have.been.calledWith( + null, [{'value':'parking','title':'A place for parking cars'}] + ); }); it('includes network values with capital letters and some punctuation', function() { var callback = sinon.spy(); taginfo.values({key: 'network', query: 'us'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/key/values'), + server.respondWith('GET', /\/key\/values/, [200, { 'Content-Type': 'application/json' }, '{"data":[{"value":"US:TX:FM","description":"Farm to Market Roads in the U.S. state of Texas.", "fraction":0.34},' + '{"value":"US:KY","description":"Primary and secondary state highways in the U.S. state of Kentucky.", "fraction":0.31},' + '{"value":"US:US","description":"U.S. routes in the United States.", "fraction":0.19},' + '{"value":"US:I","description":"Interstate highways in the United States.", "fraction":0.11},' - + '{"value":"US:MD","description":"State highways in the U.S. state of Maryland.", "fraction":0.06}]}']); + + '{"value":"US:MD","description":"State highways in the U.S. state of Maryland.", "fraction":0.06}]}'] + ); server.respond(); expect(callback).to.have.been.calledWith(null, [ @@ -213,36 +274,45 @@ describe('iD.serviceTaginfo', function() { var callback = sinon.spy(); taginfo.values({key: 'genus', query: 'qu'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/key/values'), + server.respondWith('GET', /\/key\/values/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"value":"Quercus","description":"Oak", "fraction":0.5}]}']); + '{"data":[{"value":"Quercus","description":"Oak", "fraction":0.5}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'value':'Quercus','title':'Oak'}]); + expect(callback).to.have.been.calledWith( + null, [{'value':'Quercus','title':'Oak'}] + ); }); it('includes biological taxon values with capital letters', function() { var callback = sinon.spy(); taginfo.values({key: 'taxon', query: 'qu'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/key/values'), + server.respondWith('GET', /\/key\/values/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"value":"Quercus robur","description":"Oak", "fraction":0.5}]}']); + '{"data":[{"value":"Quercus robur","description":"Oak", "fraction":0.5}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'value':'Quercus robur','title':'Oak'}]); + expect(callback).to.have.been.calledWith( + null, [{'value':'Quercus robur','title':'Oak'}] + ); }); it('includes biological species values with capital letters', function() { var callback = sinon.spy(); taginfo.values({key: 'species', query: 'qu'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/key/values'), + server.respondWith('GET', /\/key\/values/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"value":"Quercus robur","description":"Oak", "fraction":0.5}]}']); + '{"data":[{"value":"Quercus robur","description":"Oak", "fraction":0.5}]}'] + ); server.respond(); - expect(callback).to.have.been.calledWith(null, [{'value':'Quercus robur','title':'Oak'}]); + expect(callback).to.have.been.calledWith( + null, [{'value':'Quercus robur','title':'Oak'}] + ); }); }); @@ -251,14 +321,16 @@ describe('iD.serviceTaginfo', function() { var callback = sinon.spy(); taginfo.roles({rtype: 'route', query: 's', geometry: 'relation'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/relation/roles'), + server.respondWith('GET', /\/relation\/roles/, [200, { 'Content-Type': 'application/json' }, '{"data":[{"role":"stop","count_relation_members_fraction":0.1757},' + - '{"role":"south","count_relation_members_fraction":0.0035}]}']); + '{"role":"south","count_relation_members_fraction":0.0035}]}'] + ); server.respond(); expect(query(server.requests[0].url)).to.eql( - {rtype: 'route', query: 's', page: '1', rp: '25', sortname: 'count_relation_members', sortorder: 'desc'}); + {rtype: 'route', query: 's', page: '1', rp: '25', sortname: 'count_relation_members', sortorder: 'desc'} + ); expect(callback).to.have.been.calledWith(null, [ {'value': 'stop', 'title': 'stop'}, {'value': 'south', 'title': 'south'} @@ -271,15 +343,18 @@ describe('iD.serviceTaginfo', function() { var callback = sinon.spy(); taginfo.docs({key: 'amenity', value: 'parking'}, callback); - server.respondWith('GET', new RegExp('https://taginfo.openstreetmap.org/api/4/tag/wiki_page'), + server.respondWith('GET', /\/tag\/wiki_page/, [200, { 'Content-Type': 'application/json' }, - '{"data":[{"on_way":false,"lang":"en","on_area":true,"image":"File:Car park2.jpg"}]}']); + '{"data":[{"on_way":false,"lang":"en","on_area":true,"image":"File:Car park2.jpg"}]}'] + ); server.respond(); expect(query(server.requests[0].url)).to.eql( - {key: 'amenity', value: 'parking'}); - expect(callback).to.have.been.calledWith(null, - [{'on_way':false,'lang':'en','on_area':true,'image':'File:Car park2.jpg'}]); + {key: 'amenity', value: 'parking'} + ); + expect(callback).to.have.been.calledWith( + null, [{'on_way':false,'lang':'en','on_area':true,'image':'File:Car park2.jpg'}] + ); }); }); From 02688a1fcf4a6fec29e4a30eda5d4480d8481522 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Thu, 20 Apr 2017 14:08:38 -0400 Subject: [PATCH 5/7] Check cache for shortened query that returns fewer than max results --- modules/services/taginfo.js | 128 ++++++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 48 deletions(-) diff --git a/modules/services/taginfo.js b/modules/services/taginfo.js index 62fe3b6b4..a4d82498c 100644 --- a/modules/services/taginfo.js +++ b/modules/services/taginfo.js @@ -3,9 +3,10 @@ import _ from 'lodash'; import { utilQsString } from '../util'; -var endpoint = 'https://taginfo.openstreetmap.org/api/4/', - taginfoCache = {}, +var apibase = 'https://taginfo.openstreetmap.org/api/4/', + inflight = {}, popularKeys = {}, + taginfoCache = {}, tag_sorts = { point: 'count_nodes', vertex: 'count_nodes', @@ -132,23 +133,49 @@ function sortKeys(a, b) { var debouncedRequest = _.debounce(request, 750, { leading: false }); -function request(url, params, callback) { - if (taginfoCache[url]) { - callback(null, taginfoCache[url]); - } else { - d3.json(url, function (err, data) { - if (!err) { - taginfoCache[url] = data; - } - callback(err, data); - }); - } +function request(url, params, exactMatch, callback, loaded) { + if (inflight[url]) return; + + if (checkCache(url, params, exactMatch, callback)) return; + + inflight[url] = d3.json(url, function (err, data) { + delete inflight[url]; + loaded(err, data); + }); +} + + +function checkCache(url, params, exactMatch, callback) { + var rp = params.rp || 25, + testQuery = params.query || '', + testUrl = url; + + do { + var hit = taginfoCache[testUrl]; + + // exact match, or shorter match yielding fewer than max results (rp) + if (hit && (url === testUrl || hit.length < rp)) { + callback(null, hit); + return true; + } + + // don't try to shorten the query + if (exactMatch || !testQuery.length) return false; + + // do shorten the query to see if we already have a cached result + // that has returned fewer than max results (rp) + testQuery = testQuery.slice(0, -1); + testUrl = url.replace(/&query=(.*?)&/, '&query=' + testQuery + '&'); + } while (testQuery.length >= 0); + + return false; } export default { init: function() { + inflight = {}; taginfoCache = {}; popularKeys = {}; @@ -166,44 +193,46 @@ export default { }, - reset: function() { }, + reset: function() { + _.forEach(inflight, function(req) { req.abort(); }); + inflight = {}; + }, keys: function(params, callback) { - var req = params.debounce ? debouncedRequest : request; + var doRequest = params.debounce ? debouncedRequest : request; params = clean(setSort(params)); + params = _.extend({ rp: 10, sortname: 'count_all', sortorder: 'desc', page: 1 }, params); - var qs = utilQsString( - _.extend({ rp: 10, sortname: 'count_all', sortorder: 'desc', page: 1 }, params) - ); - - - req(endpoint + 'keys/all?' + qs, params, function(err, d) { + var url = apibase + 'keys/all?' + utilQsString(params); + doRequest(url, params, false, callback, function(err, d) { if (err) { callback(err); } else { var f = filterKeys(params.filter); - callback(null, d.data.filter(f).sort(sortKeys).map(valKey)); + var result = d.data.filter(f).sort(sortKeys).map(valKey); + taginfoCache[url] = result; + callback(null, result); } }); }, multikeys: function(params, callback) { - var req = params.debounce ? debouncedRequest : request; + var doRequest = params.debounce ? debouncedRequest : request; params = clean(setSort(params)); + params = _.extend({ rp: 25, sortname: 'count_all', sortorder: 'desc', page: 1 }, params); var prefix = params.query; - var qs = utilQsString( - _.extend({ rp: 25, sortname: 'count_all', sortorder: 'desc', page: 1 }, params) - ); - - req(endpoint + 'keys/all?' + qs, params, function(err, d) { + var url = apibase + 'keys/all?' + utilQsString(params); + doRequest(url, params, true, callback, function(err, d) { if (err) { callback(err); } else { var f = filterMultikeys(prefix); - callback(null, d.data.filter(f).map(valKey)); + var result = d.data.filter(f).map(valKey); + taginfoCache[url] = result; + callback(null, result); } }); }, @@ -217,14 +246,12 @@ export default { return; } - var req = params.debounce ? debouncedRequest : request; + var doRequest = params.debounce ? debouncedRequest : request; params = clean(setSort(setFilter(params))); + params = _.extend({ rp: 25, sortname: 'count_all', sortorder: 'desc', page: 1 }, params); - var qs = utilQsString( - _.extend({ rp: 25, sortname: 'count_all', sortorder: 'desc', page: 1 }, params) - ); - - req(endpoint + 'key/values?' + qs, params, function(err, d) { + var url = apibase + 'key/values?' + utilQsString(params); + doRequest(url, params, false, callback, function(err, d) { if (err) { callback(err); } else { @@ -235,53 +262,58 @@ export default { var re = /network|taxon|genus|species|brand|grape_variety|_hours|_times/; var allowUpperCase = (params.key.match(re) !== null); var f = filterValues(allowUpperCase); - callback(null, d.data.filter(f).map(valKeyDescription)); + + var result = d.data.filter(f).map(valKeyDescription); + taginfoCache[url] = result; + callback(null, result); } }); }, roles: function(params, callback) { - var req = params.debounce ? debouncedRequest : request; + var doRequest = params.debounce ? debouncedRequest : request; var geometry = params.geometry; params = clean(setSortMembers(params)); + params = _.extend({ rp: 25, sortname: 'count_all_members', sortorder: 'desc', page: 1 }, params); - var qs = utilQsString( - _.extend({ rp: 25, sortname: 'count_all_members', sortorder: 'desc', page: 1 }, params) - ); - - req(endpoint + 'relation/roles?' + qs, params, function(err, d) { + var url = apibase + 'relation/roles?' + utilQsString(params); + doRequest(url, params, true, callback, function(err, d) { if (err) { callback(err); } else { var f = filterRoles(geometry); - callback(null, d.data.filter(f).map(roleKey)); + var result = d.data.filter(f).map(roleKey); + taginfoCache[url] = result; + callback(null, result); } }); }, docs: function(params, callback) { - var req = params.debounce ? debouncedRequest : request; + var doRequest = params.debounce ? debouncedRequest : request; params = clean(setSort(params)); var path = 'key/wiki_pages?'; if (params.value) path = 'tag/wiki_pages?'; else if (params.rtype) path = 'relation/wiki_pages?'; - req(endpoint + path + utilQsString(params), params, function(err, d) { + var url = apibase + path + utilQsString(params); + doRequest(url, params, true, callback, function(err, d) { if (err) { callback(err); } else { + taginfoCache[url] = d.data; callback(null, d.data); } }); }, - endpoint: function(_) { - if (!arguments.length) return endpoint; - endpoint = _; + apibase: function(_) { + if (!arguments.length) return apibase; + apibase = _; return this; } From 43b8ab17545f9a2edec67bf68d5bcd4db19412a1 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Thu, 20 Apr 2017 14:13:10 -0400 Subject: [PATCH 6/7] Set debounce wait to 500ms, still feels responsive --- modules/services/taginfo.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/services/taginfo.js b/modules/services/taginfo.js index a4d82498c..78dcde620 100644 --- a/modules/services/taginfo.js +++ b/modules/services/taginfo.js @@ -131,7 +131,7 @@ function sortKeys(a, b) { } -var debouncedRequest = _.debounce(request, 750, { leading: false }); +var debouncedRequest = _.debounce(request, 500, { leading: false }); function request(url, params, exactMatch, callback, loaded) { if (inflight[url]) return; From 108150faf95337cfb77276f85772b0c7d2d7cb0e Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Thu, 20 Apr 2017 14:45:22 -0400 Subject: [PATCH 7/7] Don't re-request inflight nominatim requests --- modules/services/nominatim.js | 56 ++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/modules/services/nominatim.js b/modules/services/nominatim.js index 8031f7326..b5434306a 100644 --- a/modules/services/nominatim.js +++ b/modules/services/nominatim.js @@ -6,13 +6,22 @@ import { utilQsString } from '../util/index'; var apibase = 'https://nominatim.openstreetmap.org/', + inflight = {}, nominatimCache; export default { - init: function() { nominatimCache = rbush(); }, - reset: function() { nominatimCache = rbush(); }, + init: function() { + inflight = {}; + nominatimCache = rbush(); + }, + + reset: function() { + _.forEach(inflight, function(req) { req.abort(); }); + inflight = {}; + nominatimCache = rbush(); + }, countryCode: function (location, callback) { @@ -24,32 +33,37 @@ export default { return callback(null, countryCodes[0].data); } - d3.json(apibase + 'reverse?' + - utilQsString({ - format: 'json', - addressdetails: 1, - lat: location[1], - lon: location[0] - }), function(err, result) { - if (err) - return callback(err); - else if (result && result.error) - return callback(result.error); + var params = { format: 'json', addressdetails: 1, lat: location[1], lon: location[0] }; + var url = apibase + 'reverse?' + utilQsString(params); + if (inflight[url]) return; - var extent = geoExtent(location).padByMeters(1000); - nominatimCache.insert(_.assign(extent.bbox(), - { data: result.address.country_code } - )); + inflight[url] = d3.json(url, function(err, result) { + delete inflight[url]; - callback(null, result.address.country_code); - } - ); + if (err) + return callback(err); + else if (result && result.error) + return callback(result.error); + + var extent = geoExtent(location).padByMeters(1000); + nominatimCache.insert(_.assign(extent.bbox(), + { data: result.address.country_code } + )); + + callback(null, result.address.country_code); + }); }, search: function (val, callback) { var searchVal = encodeURIComponent(val); - d3.json(apibase + 'search/' + searchVal + '?limit=10&format=json', callback); + var url = apibase + 'search/' + searchVal + '?limit=10&format=json'; + if (inflight[url]) return; + + inflight[url] = d3.json(url, function(err, result) { + delete inflight[url]; + callback(err, result); + }); } };