Skip to content

Commit 91aa85a

Browse files
tyrasd
committed
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
1 parent 68eb90f commit 91aa85a

File tree

Image for: File tree

7 files changed

Image for: 7 files changed
+71
-114
lines changed

7 files changed

Image for: 7 files changed
+71
-114
lines changed

‎CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,16 @@ _Breaking developer changes, which may affect downstream projects or sites that
4343
#### :white_check_mark: Validation
4444
#### :bug: Bugfixes
4545
* fix some direction cones not appearing on railway tracks ([#10843], thanks [@k-yle])
46+
* better handling of rate limited API calls and other API errors ([#10299])
4647
#### :earth_asia: Localization
4748
#### :hourglass: Performance
4849
#### :mortar_board: Walkthrough / Help
4950
#### :hammer: Development
5051

52+
[#10299]: https://github.com/openstreetmap/iD/issues/10299
5153
[#10843]: https://github.com/openstreetmap/iD/pull/10843
5254

55+
5356
# 2.32.0
5457
##### 2025-03-05
5558

‎data/core.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,7 @@ en:
596596
offline: The OpenStreetMap API is offline. Your edits are safe locally. Please come back later.
597597
readonly: The OpenStreetMap API is currently read-only. You can continue editing, but must wait to save your changes.
598598
rateLimit: The OpenStreetMap API is limiting anonymous connections. You can fix this by logging in.
599+
rateLimited: The OpenStreetMap API is limiting your connection, please wait.
599600
local_storage_full: You have made too many edits to back up. Consider saving your changes now.
600601
retry: Retry
601602
commit:

‎modules/core/context.js

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,12 +118,6 @@ export function coreContext() {
118118
function afterLoad(cid, callback) {
119119
return (err, result) => {
120120
if (err) {
121-
// 400 Bad Request, 401 Unauthorized, 403 Forbidden..
122-
if (err.status === 400 || err.status === 401 || err.status === 403) {
123-
if (_connection) {
124-
_connection.logout();
125-
}
126-
}
127121
if (typeof callback === 'function') {
128122
callback(err);
129123
}

‎modules/services/osm.js

Lines changed: 41 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -524,10 +524,6 @@ function updateRtree(item, replace) {
524524
function wrapcb(thisArg, callback, cid) {
525525
return function(err, result) {
526526
if (err) {
527-
// 401 Unauthorized, 403 Forbidden
528-
if (err.status === 401 || err.status === 403) {
529-
thisArg.logout();
530-
}
531527
return callback.call(thisArg, err);
532528

533529
} else if (thisArg.getConnectionId() !== cid) {
@@ -640,39 +636,21 @@ export default {
640636
return;
641637
}
642638

643-
var isAuthenticated = that.authenticated();
644-
645-
// 401 Unauthorized, 403 Forbidden
646-
// Logout and retry the request.
647-
if (isAuthenticated && err && err.status &&
648-
(err.status === 401 || err.status === 403)) {
649-
that.logout();
650-
that.loadFromAPI(path, callback, options);
651-
// else, no retry.
652-
} else {
653-
// 509 Bandwidth Limit Exceeded, 429 Too Many Requests
654-
// Set the rateLimitError flag and trigger a warning.
655-
if (!isAuthenticated && !_rateLimitError && err && err.status &&
656-
(err.status === 509 || err.status === 429)) {
657-
_rateLimitError = err;
658-
dispatch.call('change');
659-
that.reloadApiStatus();
660-
} else if ((err && _cachedApiStatus === 'online') ||
661-
(!err && _cachedApiStatus !== 'online')) {
662-
// If the response's error state doesn't match the status,
663-
// it's likely we lost or gained the connection so reload the status
664-
that.reloadApiStatus();
665-
}
639+
if ((err && _cachedApiStatus === 'online') ||
640+
(!err && _cachedApiStatus !== 'online')) {
641+
// If the response's error state doesn't match the status,
642+
// it's likely we lost or gained the connection so reload the status
643+
that.reloadApiStatus();
644+
}
666645

667-
if (callback) {
668-
if (err) {
669-
return callback(err);
646+
if (callback) {
647+
if (err) {
648+
return callback(err);
649+
} else {
650+
if (path.indexOf('.json') !== -1) {
651+
return parseJSON(payload, callback, options);
670652
} else {
671-
if (path.indexOf('.json') !== -1) {
672-
return parseJSON(payload, callback, options);
673-
} else {
674-
return parseXML(payload, callback, options);
675-
}
653+
return parseXML(payload, callback, options);
676654
}
677655
}
678656
}
@@ -1098,6 +1076,12 @@ export default {
10981076
var hadRequests = hasInflightRequests(_tileCache);
10991077
abortUnwantedRequests(_tileCache, tiles);
11001078
if (hadRequests && !hasInflightRequests(_tileCache)) {
1079+
if (_rateLimitError) {
1080+
// was rate limited, but has settled
1081+
_rateLimitError = undefined;
1082+
dispatch.call('change');
1083+
this.reloadApiStatus();
1084+
}
11011085
dispatch.call('loaded'); // stop the spinner
11021086
}
11031087

@@ -1123,23 +1107,43 @@ export default {
11231107

11241108
_tileCache.inflight[tile.id] = this.loadFromAPI(
11251109
path + tile.extent.toParam(),
1126-
tileCallback,
1110+
tileCallback.bind(this),
11271111
options
11281112
);
11291113

11301114
function tileCallback(err, parsed) {
1131-
delete _tileCache.inflight[tile.id];
11321115
if (!err) {
1116+
delete _tileCache.inflight[tile.id];
11331117
delete _tileCache.toLoad[tile.id];
11341118
_tileCache.loaded[tile.id] = true;
11351119
var bbox = tile.extent.bbox();
11361120
bbox.id = tile.id;
11371121
_tileCache.rtree.insert(bbox);
1122+
} else {
1123+
// map tile loading error: e.g. network connection error,
1124+
// 509 Bandwidth Limit Exceeded, 429 Too Many Requests
1125+
if (!_rateLimitError && err.status === 509 || err.status === 429) {
1126+
// show "API rate limiting" warning
1127+
_rateLimitError = err;
1128+
dispatch.call('change');
1129+
this.reloadApiStatus();
1130+
}
1131+
setTimeout(() => {
1132+
// retry loading the tiles
1133+
delete _tileCache.inflight[tile.id];
1134+
this.loadTile(tile, callback);
1135+
}, 8000);
11381136
}
11391137
if (callback) {
11401138
callback(err, Object.assign({ data: parsed }, tile));
11411139
}
11421140
if (!hasInflightRequests(_tileCache)) {
1141+
if (_rateLimitError) {
1142+
// was rate limited, but has settled
1143+
_rateLimitError = undefined;
1144+
dispatch.call('change');
1145+
this.reloadApiStatus();
1146+
}
11431147
dispatch.call('loaded'); // stop the spinner
11441148
}
11451149
}

‎modules/ui/account.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,15 @@ export function uiAccount(context) {
1212
if (!osm.authenticated()) { // logged out
1313
render(selection, null);
1414
} else {
15-
osm.userDetails((err, user) => render(selection, user));
15+
osm.userDetails((err, user) => {
16+
if (err && err.status === 401) {
17+
// 401 Unauthorized
18+
// cannot load own user data: there must be something wrong (e.g. API token was revoked)
19+
// -> log out to allow user to reauthenticate
20+
osm.logout();
21+
}
22+
render(selection, user);
23+
});
1624
}
1725
}
1826

‎modules/ui/status.js

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,23 @@ export function uiStatus(context) {
2121
return;
2222

2323
} else if (apiStatus === 'rateLimited') {
24-
selection
25-
.call(t.append('osm_api_status.message.rateLimit'))
26-
.append('a')
27-
.attr('href', '#')
28-
.attr('class', 'api-status-login')
29-
.attr('target', '_blank')
30-
.call(svgIcon('#iD-icon-out-link', 'inline'))
31-
.append('span')
32-
.call(t.append('login'))
33-
.on('click.login', function(d3_event) {
34-
d3_event.preventDefault();
35-
osm.authenticate();
36-
});
24+
if (!osm.authenticated()) {
25+
selection
26+
.call(t.append('osm_api_status.message.rateLimit'))
27+
.append('a')
28+
.attr('href', '#')
29+
.attr('class', 'api-status-login')
30+
.attr('target', '_blank')
31+
.call(svgIcon('#iD-icon-out-link', 'inline'))
32+
.append('span')
33+
.call(t.append('login'))
34+
.on('click.login', function(d3_event) {
35+
d3_event.preventDefault();
36+
osm.authenticate();
37+
});
38+
} else {
39+
selection.call(t.append('osm_api_status.message.rateLimited'));
40+
}
3741
} else {
3842

3943
// don't allow retrying too rapidly

‎test/spec/services/osm.js

Lines changed: 0 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -163,63 +163,6 @@ describe('iD.serviceOsm', function () {
163163
expect(typeof payload).to.eql('object');
164164
});
165165

166-
it('retries an authenticated call unauthenticated if 401 Unauthorized', async () => {
167-
fetchMock.mock('https://www.openstreetmap.org' + path, {
168-
body: response,
169-
status: 200,
170-
headers: { 'Content-Type': 'application/json' }
171-
});
172-
serverXHR.respondWith('GET', 'https://www.openstreetmap.org' + path,
173-
[401, { 'Content-Type': 'text/plain' }, 'Unauthorized']);
174-
175-
login();
176-
177-
const xml = promisify(connection.loadFromAPI).call(connection, path);
178-
serverXHR.respond();
179-
180-
expect(typeof await xml).to.eql('object');
181-
expect(connection.authenticated()).to.be.not.ok;
182-
expect(fetchMock.called()).to.be.true;
183-
});
184-
185-
it('retries an authenticated call unauthenticated if 401 Unauthorized', async () => {
186-
fetchMock.mock('https://www.openstreetmap.org' + path, {
187-
body: response,
188-
status: 200,
189-
headers: { 'Content-Type': 'application/json' }
190-
});
191-
serverXHR.respondWith('GET', 'https://www.openstreetmap.org' + path,
192-
[401, { 'Content-Type': 'text/plain' }, 'Unauthorized']);
193-
194-
login();
195-
196-
const xml = promisify(connection.loadFromAPI).call(connection, path);
197-
serverXHR.respond();
198-
199-
expect(typeof await xml).to.eql('object');
200-
expect(connection.authenticated()).to.be.not.ok;
201-
expect(fetchMock.called()).to.be.true;
202-
});
203-
204-
it('retries an authenticated call unauthenticated if 403 Forbidden', async () => {
205-
fetchMock.mock('https://www.openstreetmap.org' + path, {
206-
body: response,
207-
status: 200,
208-
headers: { 'Content-Type': 'application/json' }
209-
});
210-
serverXHR.respondWith('GET', 'https://www.openstreetmap.org' + path,
211-
[403, { 'Content-Type': 'text/plain' }, 'Forbidden']);
212-
213-
login();
214-
const xml = promisify(connection.loadFromAPI).call(connection, path);
215-
serverXHR.respond();
216-
217-
expect(typeof await xml).to.eql('object');
218-
expect(connection.authenticated()).to.be.not.ok;
219-
expect(fetchMock.called()).to.be.true;
220-
});
221-
222-
223166
it('dispatches change event if 509 Bandwidth Limit Exceeded', async () => {
224167
fetchMock.mock('https://www.openstreetmap.org' + path, {
225168
body: 'Bandwidth Limit Exceeded',

0 commit comments

Image for: 0 commit comments
Comments
 (0)