From 91aa85a1c08d59d735cc6654ea6d15e6af1d3e08 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 11 Mar 2025 15:34:26 +0100 Subject: [PATCH] better handling of rate limited API map calls and other API errors * retry all unsuccessful map calls after waiting 8 seconds (spinner continues to indicate loading state) * also logged-in users can be rate limited: add dedicated error message * don't log users out when requests return 401/403 (except on the _get own user data_ request, which would indicate that the oauth token was revoked): it's better to show the error message if a legitimate api call was actually unauthorized closes #10299 --- CHANGELOG.md | 3 ++ data/core.yaml | 1 + modules/core/context.js | 6 --- modules/services/osm.js | 78 ++++++++++++++++++++------------------- modules/ui/account.js | 10 ++++- modules/ui/status.js | 30 ++++++++------- test/spec/services/osm.js | 57 ---------------------------- 7 files changed, 71 insertions(+), 114 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 304ba10b3..936edd877 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,13 +43,16 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :white_check_mark: Validation #### :bug: Bugfixes * fix some direction cones not appearing on railway tracks ([#10843], thanks [@k-yle]) +* better handling of rate limited API calls and other API errors ([#10299]) #### :earth_asia: Localization #### :hourglass: Performance #### :mortar_board: Walkthrough / Help #### :hammer: Development +[#10299]: https://github.com/openstreetmap/iD/issues/10299 [#10843]: https://github.com/openstreetmap/iD/pull/10843 + # 2.32.0 ##### 2025-03-05 diff --git a/data/core.yaml b/data/core.yaml index d2925ddaf..01e4e1d04 100644 --- a/data/core.yaml +++ b/data/core.yaml @@ -596,6 +596,7 @@ en: offline: The OpenStreetMap API is offline. Your edits are safe locally. Please come back later. readonly: The OpenStreetMap API is currently read-only. You can continue editing, but must wait to save your changes. rateLimit: The OpenStreetMap API is limiting anonymous connections. You can fix this by logging in. + rateLimited: The OpenStreetMap API is limiting your connection, please wait. local_storage_full: You have made too many edits to back up. Consider saving your changes now. retry: Retry commit: diff --git a/modules/core/context.js b/modules/core/context.js index 621b462fc..70f1eb3f9 100644 --- a/modules/core/context.js +++ b/modules/core/context.js @@ -118,12 +118,6 @@ export function coreContext() { function afterLoad(cid, callback) { return (err, result) => { if (err) { - // 400 Bad Request, 401 Unauthorized, 403 Forbidden.. - if (err.status === 400 || err.status === 401 || err.status === 403) { - if (_connection) { - _connection.logout(); - } - } if (typeof callback === 'function') { callback(err); } diff --git a/modules/services/osm.js b/modules/services/osm.js index bb329d2ef..172f9cea4 100644 --- a/modules/services/osm.js +++ b/modules/services/osm.js @@ -524,10 +524,6 @@ function updateRtree(item, replace) { function wrapcb(thisArg, callback, cid) { return function(err, result) { if (err) { - // 401 Unauthorized, 403 Forbidden - if (err.status === 401 || err.status === 403) { - thisArg.logout(); - } return callback.call(thisArg, err); } else if (thisArg.getConnectionId() !== cid) { @@ -640,39 +636,21 @@ export default { return; } - var isAuthenticated = that.authenticated(); + if ((err && _cachedApiStatus === 'online') || + (!err && _cachedApiStatus !== 'online')) { + // If the response's error state doesn't match the status, + // it's likely we lost or gained the connection so reload the status + that.reloadApiStatus(); + } - // 401 Unauthorized, 403 Forbidden - // Logout and retry the request. - if (isAuthenticated && err && err.status && - (err.status === 401 || err.status === 403)) { - that.logout(); - that.loadFromAPI(path, callback, options); - // else, no retry. - } else { - // 509 Bandwidth Limit Exceeded, 429 Too Many Requests - // Set the rateLimitError flag and trigger a warning. - if (!isAuthenticated && !_rateLimitError && err && err.status && - (err.status === 509 || err.status === 429)) { - _rateLimitError = err; - dispatch.call('change'); - that.reloadApiStatus(); - } else if ((err && _cachedApiStatus === 'online') || - (!err && _cachedApiStatus !== 'online')) { - // If the response's error state doesn't match the status, - // it's likely we lost or gained the connection so reload the status - that.reloadApiStatus(); - } - - if (callback) { - if (err) { - return callback(err); + if (callback) { + if (err) { + return callback(err); + } else { + if (path.indexOf('.json') !== -1) { + return parseJSON(payload, callback, options); } else { - if (path.indexOf('.json') !== -1) { - return parseJSON(payload, callback, options); - } else { - return parseXML(payload, callback, options); - } + return parseXML(payload, callback, options); } } } @@ -1098,6 +1076,12 @@ export default { var hadRequests = hasInflightRequests(_tileCache); abortUnwantedRequests(_tileCache, tiles); if (hadRequests && !hasInflightRequests(_tileCache)) { + if (_rateLimitError) { + // was rate limited, but has settled + _rateLimitError = undefined; + dispatch.call('change'); + this.reloadApiStatus(); + } dispatch.call('loaded'); // stop the spinner } @@ -1123,23 +1107,43 @@ export default { _tileCache.inflight[tile.id] = this.loadFromAPI( path + tile.extent.toParam(), - tileCallback, + tileCallback.bind(this), options ); function tileCallback(err, parsed) { - delete _tileCache.inflight[tile.id]; if (!err) { + delete _tileCache.inflight[tile.id]; delete _tileCache.toLoad[tile.id]; _tileCache.loaded[tile.id] = true; var bbox = tile.extent.bbox(); bbox.id = tile.id; _tileCache.rtree.insert(bbox); + } else { + // map tile loading error: e.g. network connection error, + // 509 Bandwidth Limit Exceeded, 429 Too Many Requests + if (!_rateLimitError && err.status === 509 || err.status === 429) { + // show "API rate limiting" warning + _rateLimitError = err; + dispatch.call('change'); + this.reloadApiStatus(); + } + setTimeout(() => { + // retry loading the tiles + delete _tileCache.inflight[tile.id]; + this.loadTile(tile, callback); + }, 8000); } if (callback) { callback(err, Object.assign({ data: parsed }, tile)); } if (!hasInflightRequests(_tileCache)) { + if (_rateLimitError) { + // was rate limited, but has settled + _rateLimitError = undefined; + dispatch.call('change'); + this.reloadApiStatus(); + } dispatch.call('loaded'); // stop the spinner } } diff --git a/modules/ui/account.js b/modules/ui/account.js index ee4fee1b0..245b729dc 100644 --- a/modules/ui/account.js +++ b/modules/ui/account.js @@ -12,7 +12,15 @@ export function uiAccount(context) { if (!osm.authenticated()) { // logged out render(selection, null); } else { - osm.userDetails((err, user) => render(selection, user)); + osm.userDetails((err, user) => { + if (err && err.status === 401) { + // 401 Unauthorized + // cannot load own user data: there must be something wrong (e.g. API token was revoked) + // -> log out to allow user to reauthenticate + osm.logout(); + } + render(selection, user); + }); } } diff --git a/modules/ui/status.js b/modules/ui/status.js index 081756845..fa01d812a 100644 --- a/modules/ui/status.js +++ b/modules/ui/status.js @@ -21,19 +21,23 @@ export function uiStatus(context) { return; } else if (apiStatus === 'rateLimited') { - selection - .call(t.append('osm_api_status.message.rateLimit')) - .append('a') - .attr('href', '#') - .attr('class', 'api-status-login') - .attr('target', '_blank') - .call(svgIcon('#iD-icon-out-link', 'inline')) - .append('span') - .call(t.append('login')) - .on('click.login', function(d3_event) { - d3_event.preventDefault(); - osm.authenticate(); - }); + if (!osm.authenticated()) { + selection + .call(t.append('osm_api_status.message.rateLimit')) + .append('a') + .attr('href', '#') + .attr('class', 'api-status-login') + .attr('target', '_blank') + .call(svgIcon('#iD-icon-out-link', 'inline')) + .append('span') + .call(t.append('login')) + .on('click.login', function(d3_event) { + d3_event.preventDefault(); + osm.authenticate(); + }); + } else { + selection.call(t.append('osm_api_status.message.rateLimited')); + } } else { // don't allow retrying too rapidly diff --git a/test/spec/services/osm.js b/test/spec/services/osm.js index 260d7e7fb..d4e1dedc8 100644 --- a/test/spec/services/osm.js +++ b/test/spec/services/osm.js @@ -163,63 +163,6 @@ describe('iD.serviceOsm', function () { expect(typeof payload).to.eql('object'); }); - it('retries an authenticated call unauthenticated if 401 Unauthorized', async () => { - fetchMock.mock('https://www.openstreetmap.org' + path, { - body: response, - status: 200, - headers: { 'Content-Type': 'application/json' } - }); - serverXHR.respondWith('GET', 'https://www.openstreetmap.org' + path, - [401, { 'Content-Type': 'text/plain' }, 'Unauthorized']); - - login(); - - const xml = promisify(connection.loadFromAPI).call(connection, path); - serverXHR.respond(); - - expect(typeof await xml).to.eql('object'); - expect(connection.authenticated()).to.be.not.ok; - expect(fetchMock.called()).to.be.true; - }); - - it('retries an authenticated call unauthenticated if 401 Unauthorized', async () => { - fetchMock.mock('https://www.openstreetmap.org' + path, { - body: response, - status: 200, - headers: { 'Content-Type': 'application/json' } - }); - serverXHR.respondWith('GET', 'https://www.openstreetmap.org' + path, - [401, { 'Content-Type': 'text/plain' }, 'Unauthorized']); - - login(); - - const xml = promisify(connection.loadFromAPI).call(connection, path); - serverXHR.respond(); - - expect(typeof await xml).to.eql('object'); - expect(connection.authenticated()).to.be.not.ok; - expect(fetchMock.called()).to.be.true; - }); - - it('retries an authenticated call unauthenticated if 403 Forbidden', async () => { - fetchMock.mock('https://www.openstreetmap.org' + path, { - body: response, - status: 200, - headers: { 'Content-Type': 'application/json' } - }); - serverXHR.respondWith('GET', 'https://www.openstreetmap.org' + path, - [403, { 'Content-Type': 'text/plain' }, 'Forbidden']); - - login(); - const xml = promisify(connection.loadFromAPI).call(connection, path); - serverXHR.respond(); - - expect(typeof await xml).to.eql('object'); - expect(connection.authenticated()).to.be.not.ok; - expect(fetchMock.called()).to.be.true; - }); - - it('dispatches change event if 509 Bandwidth Limit Exceeded', async () => { fetchMock.mock('https://www.openstreetmap.org' + path, { body: 'Bandwidth Limit Exceeded',