From 724c6b9c54d34a30df2b3039e4d343711d6252c4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Feb 2025 20:07:17 -0800 Subject: [PATCH 1/2] api [nfc]: Assert ZulipApiException.data is free of redundant keys These three keys appear in the server's JSON for error responses, but get pulled out into their own dedicated fields in ZulipApiException. (See the field's doc, and the constructor's one non-test call site.) The assertion is useful for tests, for keeping test data realistic. Fix the two test cases that had missed this nuance. --- lib/api/exception.dart | 5 ++++- test/model/actions_test.dart | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/api/exception.dart b/lib/api/exception.dart index d4bebeeb62..d495ec9ff8 100644 --- a/lib/api/exception.dart +++ b/lib/api/exception.dart @@ -81,7 +81,10 @@ class ZulipApiException extends HttpException { required this.code, required this.data, required super.message, - }) : assert(400 <= httpStatus && httpStatus <= 499); + }) : assert(400 <= httpStatus && httpStatus <= 499), + assert(!data.containsKey('result') + && !data.containsKey('code') + && !data.containsKey('msg')); @override String toString() { diff --git a/test/model/actions_test.dart b/test/model/actions_test.dart index e24bb3223c..2b38d640a6 100644 --- a/test/model/actions_test.dart +++ b/test/model/actions_test.dart @@ -105,7 +105,7 @@ void main() { final exception = ZulipApiException( httpStatus: 401, code: 'UNAUTHORIZED', - data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"}, + data: {}, routeName: 'removeEtcEtcToken', message: 'Invalid API key', ); @@ -174,7 +174,7 @@ void main() { ..prepare(exception: ZulipApiException( httpStatus: 401, code: 'UNAUTHORIZED', - data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"}, + data: {}, routeName: 'removeEtcEtcToken', message: 'Invalid API key', )); From fcc892603adc2c3d79deb27d3f535e5173aea6c6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Feb 2025 20:20:45 -0800 Subject: [PATCH 2/2] test [nfc]: Pull out an example UNAUTHORIZED API exception, and add doc Prompted by seeing we'll need more copies of this soon: https://github.com/zulip/zulip-flutter/pull/1183#discussion_r1955516825 --- test/example_data.dart | 19 +++++++++++++++++++ test/model/actions_test.dart | 18 +++--------------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 6b84bf185c..6cab496157 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -1,6 +1,7 @@ import 'dart:convert'; import 'dart:math'; +import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; @@ -18,10 +19,28 @@ void _checkPositive(int? value, String description) { assert(value == null || value > 0, '$description should be positive'); } +//////////////////////////////////////////////////////////////// +// Error objects. +// + Object nullCheckError() { try { null!; } catch (e) { return e; } // ignore: null_check_always_fails } +/// The error the server gives when the client's credentials +/// (API key together with email and realm URL) are no longer valid. +/// +/// This isn't really documented, but comes from experiment and from +/// reading the server implementation. See: +/// https://github.com/zulip/zulip-flutter/pull/1183#discussion_r1945865983 +/// https://chat.zulip.org/#narrow/channel/378-api-design/topic/general.20handling.20HTTP.20status.20code.20401/near/2090024 +ZulipApiException apiExceptionUnauthorized({String routeName = 'someRoute'}) { + return ZulipApiException( + routeName: routeName, + httpStatus: 401, code: 'UNAUTHORIZED', + data: {}, message: 'Invalid API key'); +} + //////////////////////////////////////////////////////////////// // Realm-wide (or server-wide) metadata. // diff --git a/test/model/actions_test.dart b/test/model/actions_test.dart index 2b38d640a6..46cc2c6bdd 100644 --- a/test/model/actions_test.dart +++ b/test/model/actions_test.dart @@ -2,7 +2,6 @@ import 'package:checks/checks.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; -import 'package:zulip/api/exception.dart'; import 'package:zulip/model/actions.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/notifications/receive.dart'; @@ -102,13 +101,7 @@ void main() { check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id); const unregisterDelay = Duration(seconds: 5); assert(unregisterDelay > TestGlobalStore.removeAccountDuration); - final exception = ZulipApiException( - httpStatus: 401, - code: 'UNAUTHORIZED', - data: {}, - routeName: 'removeEtcEtcToken', - message: 'Invalid API key', - ); + final exception = eg.apiExceptionUnauthorized(routeName: 'removeEtcEtcToken'); final newConnection = separateConnection() ..prepare(delay: unregisterDelay, exception: exception); @@ -170,14 +163,9 @@ void main() { test('connection closed if request errors', () => awaitFakeAsync((async) async { await prepare(ackedPushToken: '123'); + final exception = eg.apiExceptionUnauthorized(routeName: 'removeEtcEtcToken'); final newConnection = separateConnection() - ..prepare(exception: ZulipApiException( - httpStatus: 401, - code: 'UNAUTHORIZED', - data: {}, - routeName: 'removeEtcEtcToken', - message: 'Invalid API key', - )); + ..prepare(exception: exception); final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); async.elapse(Duration.zero); await future;