store: Handle invalid API key on register-queue #1183
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import '../api/backoff.dart'; | |
| import '../api/route/realm.dart'; | ||
| import '../log.dart'; | ||
| import '../notifications/receive.dart'; | ||
| import 'actions.dart'; | ||
| import 'autocomplete.dart'; | ||
| import 'database.dart'; | ||
| import 'emoji.dart'; | ||
|
|
@@ -149,8 +150,36 @@ abstract class GlobalStore extends ChangeNotifier { | |
| /// and/or [perAccountSync]. | ||
| Future<PerAccountStore> loadPerAccount(int accountId) async { | ||
|
There was a problem hiding this comment. nit in commit message: I wouldn't call either of these "the common case" (which implies all other cases are rare or edge cases) — I think both of these are common. When I open the app it quite often was already in the background, but had been for more than 10 minutes, and that hits case 1. |
||
| assert(_accounts.containsKey(accountId)); | ||
| final PerAccountStore store; | ||
| try { | ||
| store = await doLoadPerAccount(accountId); | ||
| } catch (e) { | ||
| switch (e) { | ||
| case HttpException(httpStatus: 401): | ||
| // The API key is invalid and the store can never be loaded | ||
| // unless the user retries manually. | ||
|
Comment on lines
+158
to
+160
There was a problem hiding this comment. Let's raise a thread on (For most things in the API we just work from the API docs with no further consultation — but this is an area of the API that's not clearly documented. The closest is https://chat.zulip.org/api/rest-error-handling , but for most errors that doesn't specify HTTP status codes, and indeed it specifies a Zulip API error code of INVALID_API_KEY which we learned at #1183 (comment) isn't what the server actually produces in the most obvious case of an API key being invalid.) There was a problem hiding this comment. From that thread, we confirmed that checking for 401 is the desired approach. |
||
| final account = getAccount(accountId); | ||
| if (account == null) { | ||
| // The account was logged out during `await doLoadPerAccount`. | ||
| // Here, that seems possible only by the user's own action; | ||
| // the logout can't have been done programmatically. | ||
| // Even if it were, it would have come with its own UI feedback. | ||
| // Anyway, skip showing feedback, to not be confusing or repetitive. | ||
| throw AccountNotFoundException(); | ||
| } | ||
| final zulipLocalizations = GlobalLocalizations.zulipLocalizations; | ||
| reportErrorToUserModally( | ||
| zulipLocalizations.errorCouldNotConnectTitle, | ||
| message: zulipLocalizations.errorInvalidApiKeyMessage( | ||
| account.realmUrl.toString())); | ||
| await logOutAccount(this, accountId); | ||
| throw AccountNotFoundException(); | ||
| default: | ||
| rethrow; | ||
| } | ||
| } | ||
| if (!_accounts.containsKey(accountId)) { | ||
| // TODO(#1354): handle this earlier | ||
| // [removeAccount] was called during [doLoadPerAccount]. | ||
| store.dispose(); | ||
| throw AccountNotFoundException(); | ||
|
|
@@ -913,12 +942,19 @@ class UpdateMachine { | |
| try { | ||
| return await registerQueue(connection); | ||
| } catch (e, s) { | ||
| // TODO(#890): tell user if initial-fetch errors persist, or look non-transient | ||
| switch (e) { | ||
| case HttpException(httpStatus: 401): | ||
| // We cannot recover from this error through retrying. | ||
| // Leave it to [GlobalStore.loadPerAccount]. | ||
| rethrow; | ||
| default: | ||
| assert(debugLog('Error fetching initial snapshot: $e')); | ||
| // Print stack trace in its own log entry; log entries are truncated | ||
| // at 1 kiB (at least on Android), and stack can be longer than that. | ||
| assert(debugLog('Stack:\n$s')); | ||
| } | ||
| assert(debugLog('Backing off, then will retry…')); | ||
| await (backoffMachine ??= BackoffMachine()).wait(); | ||
| assert(debugLog('… Backoff wait complete, retrying initial fetch.')); | ||
| } | ||
|
|
@@ -1177,6 +1213,7 @@ class UpdateMachine { | |
| store.isLoading = true; | ||
|
|
||
| bool isUnexpected; | ||
| // TODO(#1054): handle auth failure | ||
| switch (error) { | ||
| case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'): | ||
| assert(debugLog('Lost event queue for $store. Replacing…')); | ||
|
|
@@ -1218,8 +1255,14 @@ class UpdateMachine { | |
| if (_disposed) return; | ||
| } | ||
|
|
||
| try { | ||
| await store._globalStore._reloadPerAccount(store.accountId); | ||
| } on AccountNotFoundException { | ||
|
Comment on lines
+1258
to
+1260
There was a problem hiding this comment. Is this try/catch something we should have even before the new handling of auth errors? I think it is: suppose the app loses its event queue and goes to reload initial data, and while it's doing that the user navigates to the choose-account page and logs out the account. Then I think So this can be moved to a separate prep commit. I think doing that will also help simplify the explanation in the main commit's commit message. There was a problem hiding this comment. I found it pretty hard to hit this existing final account = globalStore.getAccount(accountId)!;For both implementations, static Future<UpdateMachine> load(GlobalStore globalStore, int accountId) async {
Account account = globalStore.getAccount(accountId)!;
final connection = globalStore.apiConnectionFromAccount(account);
final stopwatch = Stopwatch()..start();
final initialSnapshot = await _registerQueueWithRetry(connection);
final t = (stopwatch..stop()).elapsed;
assert(debugLog("initial fetch time: ${t.inMilliseconds}ms"));
if (initialSnapshot.zulipVersion != account.zulipVersion
|| initialSnapshot.zulipMergeBase != account.zulipMergeBase
|| initialSnapshot.zulipFeatureLevel != account.zulipFeatureLevel) {
account = await globalStore.updateAccount(accountId, AccountsCompanion(
zulipVersion: Value(initialSnapshot.zulipVersion),
zulipMergeBase: Value(initialSnapshot.zulipMergeBase),
zulipFeatureLevel: Value(initialSnapshot.zulipFeatureLevel),
));
connection.zulipFeatureLevel = initialSnapshot.zulipFeatureLevel;
}
final store = PerAccountStore.fromInitialSnapshot(
globalStore: globalStore,
accountId: accountId,
connection: connection,
initialSnapshot: initialSnapshot,
);
final updateMachine = UpdateMachine.fromInitialSnapshot(
store: store, initialSnapshot: initialSnapshot);
updateMachine.poll();
if (initialSnapshot.serverEmojiDataUrl != null) {
// TODO(server-6): If the server is ancient, just skip trying to have
// a list of its emoji. (The old servers that don't provide
// serverEmojiDataUrl are already unsupported at time of writing.)
unawaited(updateMachine.fetchEmojiData(initialSnapshot.serverEmojiDataUrl!));
}
// TODO do registerNotificationToken before registerQueue:
// https://github.com/zulip/zulip-flutter/pull/325#discussion_r1365982807
unawaited(updateMachine.registerNotificationToken());
return updateMachine;
}I think this is a bug that the Modulo the bug, it would be helpful to have the try/catch here, except that it is unreachable until later. Opened #1183 for this. There was a problem hiding this comment.
Did you mean a different issue/PR number here? That number refers to the current PR. There was a problem hiding this comment. Cool, thanks — as discussed on #1354, I agree that there should be a check earlier that throws AccountNotFoundException. For exercising this case in a test (before the main changes at the tip of this PR), see new comment below: #1183 (comment) |
||
| assert(debugLog('… Event queue not replaced; account was logged out.')); | ||
| return; | ||
| } finally { | ||
| assert(_disposed); | ||
| } | ||
| assert(debugLog('… Event queue replaced.')); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit-message nit: "generalized"