From 48aab29832b6bcb7f61081ccb3daa2c76cd8f790 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Nov 2023 18:29:10 -0700 Subject: [PATCH 1/8] api [nfc]: Document our practice of making constructor arguments explicit This came up in a code review here: https://github.com/zulip/zulip-flutter/pull/361#discussion_r1379374838 and it'd be good to have written down in a more accessible place. There are some existing parts of the API bindings -- particularly the parts written in the very first week or two of the prototype, before I thought through this pattern -- that don't follow this. I'll fix those up next. --- README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/README.md b/README.md index 4c8cf7824f..98490cd6a7 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,15 @@ to keep the generated files up to date: $ dart run build_runner watch --delete-conflicting-outputs ``` +In our API types, constructors should generally avoid default values for +their parameters, even `null`. This means writing e.g. `required this.foo` +rather than just `this.foo`, even when `foo` is nullable. +This is because it's common in the Zulip API for a null or missing value +to be quite salient in meaning, and not a boring value appropriate for a +default, so that it's best to ensure callers make an explicit choice. +If passing explicit values in tests is cumbersome, a factory function +in `test/example_data.dart` is an appropriate way to share defaults. + ### Upgrading Flutter From d09f0c7f7437893d0f82beec04cf54c76d44a34c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Nov 2023 17:37:24 -0700 Subject: [PATCH 2/8] api: Cut SendMessageResult.deliverAt, quietly deleted from API This appears to have been done in commit zulip/zulip@bd2545b0d. It's fine for it to have gone away -- the new API is better, and we weren't using this field -- but there's no notice of the change in the API changelog or the endpoint's own API docs. That shouldn't happen. I'll follow up. Anyway, now that I've sleuthed out the mystery of what happened, remove this from our bindings to match. --- lib/api/route/messages.dart | 2 -- lib/api/route/messages.g.dart | 2 -- test/api/route/route_checks.dart | 1 - 3 files changed, 5 deletions(-) diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index a82b38159f..21638ee44c 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -226,11 +226,9 @@ class DmDestination extends MessageDestination { @JsonSerializable(fieldRename: FieldRename.snake) class SendMessageResult { final int id; - final String? deliverAt; SendMessageResult({ required this.id, - this.deliverAt, }); factory SendMessageResult.fromJson(Map json) => diff --git a/lib/api/route/messages.g.dart b/lib/api/route/messages.g.dart index 6c34be86d7..3eea6dc1e1 100644 --- a/lib/api/route/messages.g.dart +++ b/lib/api/route/messages.g.dart @@ -43,13 +43,11 @@ Map _$GetMessagesResultToJson(GetMessagesResult instance) => SendMessageResult _$SendMessageResultFromJson(Map json) => SendMessageResult( id: json['id'] as int, - deliverAt: json['deliver_at'] as String?, ); Map _$SendMessageResultToJson(SendMessageResult instance) => { 'id': instance.id, - 'deliver_at': instance.deliverAt, }; UploadFileResult _$UploadFileResultFromJson(Map json) => diff --git a/test/api/route/route_checks.dart b/test/api/route/route_checks.dart index 8fe2b7fa6f..6d310ab200 100644 --- a/test/api/route/route_checks.dart +++ b/test/api/route/route_checks.dart @@ -3,7 +3,6 @@ import 'package:zulip/api/route/messages.dart'; extension SendMessageResultChecks on Subject { Subject get id => has((e) => e.id, 'id'); - Subject get deliverAt => has((e) => e.deliverAt, 'deliverAt'); } // TODO add similar extensions for other classes in api/route/*.dart From 93f1bf4315181db454b48bcc7c0883c43a690b78 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Nov 2023 17:44:01 -0700 Subject: [PATCH 3/8] api [nfc]: Make User.isSystemBot a bool, rather than `bool?` This reflects the real semantics, and will simplify things if we ever actually use this field. Also clarify what the todo-server comment refers to. --- lib/api/model/model.dart | 12 ++++++++---- lib/api/model/model.g.dart | 2 +- test/api/model/model_test.dart | 6 +++--- test/example_data.dart | 1 + 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 6cf4a06f01..d7ae1c4a7e 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -142,7 +142,7 @@ class User { Map? profileData; @JsonKey(readValue: _readIsSystemBot) - bool? isSystemBot; // TODO(server-5) + bool isSystemBot; static Map? _readProfileData(Map json, String key) { final value = (json[key] as Map?); @@ -153,8 +153,12 @@ class User { return (value != null && value.isNotEmpty) ? value : null; } - static bool? _readIsSystemBot(Map json, String key) { - return json[key] ?? json['is_cross_realm_bot']; + static bool _readIsSystemBot(Map json, String key) { + // This field is absent in `realm_users` and `realm_non_active_users`, + // which contain no system bots; it's present in `cross_realm_bots`. + return json[key] + ?? json['is_cross_realm_bot'] // TODO(server-5): renamed to `is_system_bot` + ?? false; } User({ @@ -176,7 +180,7 @@ class User { required this.avatarUrl, required this.avatarVersion, this.profileData, - this.isSystemBot, + required this.isSystemBot, }); factory User.fromJson(Map json) => _$UserFromJson(json); diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index 4a9fcb7640..74e7432f2e 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -95,7 +95,7 @@ User _$UserFromJson(Map json) => User( (k, e) => MapEntry(int.parse(k), ProfileFieldUserData.fromJson(e as Map)), ), - isSystemBot: User._readIsSystemBot(json, 'is_system_bot') as bool?, + isSystemBot: User._readIsSystemBot(json, 'is_system_bot') as bool, ); Map _$UserToJson(User instance) => { diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index 743eb50c21..90d6177e5a 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -59,9 +59,9 @@ void main() { }); test('is_system_bot', () { - check(mkUser({}).isSystemBot).isNull(); - check(mkUser({'is_cross_realm_bot': true}).isSystemBot).equals(true); - check(mkUser({'is_system_bot': true}).isSystemBot).equals(true); + check(mkUser({}).isSystemBot).isFalse(); + check(mkUser({'is_cross_realm_bot': true}).isSystemBot).isTrue(); + check(mkUser({'is_system_bot': true}).isSystemBot).isTrue(); }); }); diff --git a/test/example_data.dart b/test/example_data.dart index b18479879b..8567243784 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -47,6 +47,7 @@ User user({ avatarUrl: avatarUrl, avatarVersion: 0, profileData: profileData, + isSystemBot: false, ); } From c295bdecf937eedf5f9c12c3eda0f6a780aa4f10 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Nov 2023 18:14:18 -0700 Subject: [PATCH 4/8] api [nfc]: Make UpdateMessageEvent constructor fully explicit; add example data --- lib/api/model/events.dart | 28 ++++++++++++------------- test/example_data.dart | 34 +++++++++++++++++++++++++++++++ test/model/message_list_test.dart | 27 +++++------------------- test/model/unreads_test.dart | 4 ++-- 4 files changed, 55 insertions(+), 38 deletions(-) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index 241aa70a41..6ed37213de 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -384,23 +384,23 @@ class UpdateMessageEvent extends Event { UpdateMessageEvent({ required super.id, - this.userId, - this.renderingOnly, + required this.userId, + required this.renderingOnly, required this.messageId, required this.messageIds, required this.flags, - this.editTimestamp, - this.streamName, - this.streamId, - this.newStreamId, - this.propagateMode, - this.origSubject, - this.subject, - this.origContent, - this.origRenderedContent, - this.content, - this.renderedContent, - this.isMeMessage, + required this.editTimestamp, + required this.streamName, + required this.streamId, + required this.newStreamId, + required this.propagateMode, + required this.origSubject, + required this.subject, + required this.origContent, + required this.origRenderedContent, + required this.content, + required this.renderedContent, + required this.isMeMessage, }); factory UpdateMessageEvent.fromJson(Map json) => diff --git a/test/example_data.dart b/test/example_data.dart index 8567243784..2b6c76a532 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -280,6 +280,40 @@ const _unreadMsgs = unreadMsgs; // Events. // +UpdateMessageEvent updateMessageEditEvent( + Message origMessage, { + int? userId = -1, // null means null; default is [selfUser.userId] + bool? renderingOnly = false, + int? messageId, + List? flags, + int? editTimestamp, + String? streamName, + String? renderedContent, + bool isMeMessage = false, +}) { + messageId ??= origMessage.id; + return UpdateMessageEvent( + id: 0, + userId: userId == -1 ? selfUser.userId : userId, + renderingOnly: renderingOnly, + messageId: messageId, + messageIds: [messageId], + flags: flags ?? origMessage.flags, + editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp + streamName: streamName, + streamId: origMessage is StreamMessage ? origMessage.streamId : null, + newStreamId: null, + propagateMode: null, + origSubject: null, + subject: null, + origContent: 'some probably-mismatched old Markdown', + origRenderedContent: origMessage.content, + content: 'some probably-mismatched new Markdown', + renderedContent: renderedContent ?? origMessage.content, + isMeMessage: isMeMessage, + ); +} + UpdateMessageFlagsRemoveEvent updateMessageFlagsRemoveEvent( MessageFlag flag, Iterable messages, { diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index c49978ef58..68706f0be1 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -265,16 +265,11 @@ void main() async { test('update a message', () async { final originalMessage = eg.streamMessage( content: "

Hello, world

"); - final updateEvent = UpdateMessageEvent( - id: 1, - messageId: originalMessage.id, - messageIds: [originalMessage.id], + final updateEvent = eg.updateMessageEditEvent(originalMessage, flags: [MessageFlag.starred], renderedContent: "

Hello, edited

", editTimestamp: 99999, isMeMessage: true, - userId: 1, - renderingOnly: false, ); prepare(); await prepareMessages(foundOldest: true, messages: [originalMessage]); @@ -299,15 +294,9 @@ void main() async { test('ignore when message not present', () async { final originalMessage = eg.streamMessage( content: "

Hello, world

"); - final updateEvent = UpdateMessageEvent( - id: 1, + final updateEvent = eg.updateMessageEditEvent(originalMessage, messageId: originalMessage.id + 1, - messageIds: [originalMessage.id + 1], - flags: originalMessage.flags, renderedContent: "

Hello, edited

", - editTimestamp: 99999, - userId: 1, - renderingOnly: false, ); prepare(); await prepareMessages(foundOldest: true, messages: [originalMessage]); @@ -324,11 +313,7 @@ void main() async { final originalMessage = eg.streamMessage( lastEditTimestamp: 78492, content: "

Hello, world

"); - final updateEvent = UpdateMessageEvent( - id: 1, - messageId: originalMessage.id, - messageIds: [originalMessage.id], - flags: originalMessage.flags, + final updateEvent = eg.updateMessageEditEvent(originalMessage, renderedContent: "

Hello, world

Some link preview
", editTimestamp: 99999, renderingOnly: legacy ? null : true, @@ -618,10 +603,8 @@ void main() async { checkNotifiedOnce(); // Then test maybeUpdateMessage, where a header is and remains needed… - UpdateMessageEvent updateEvent(Message message) => UpdateMessageEvent( - id: 1, messageId: message.id, messageIds: [message.id], - flags: message.flags, - renderedContent: '${message.content}

edited

', + UpdateMessageEvent updateEvent(Message message) => eg.updateMessageEditEvent( + message, renderedContent: '${message.content}

edited

', ); model.maybeUpdateMessage(updateEvent(model.messages.first)); checkNotifiedOnce(); diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 0d2206f10a..11262d33ed 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -286,7 +286,7 @@ void main() { if (!isKnownToModel) { check(because: "no crash if message is in model's blindspots", () => model.handleUpdateMessageEvent( - UpdateMessageEvent(id: 0, messageId: message.id, messageIds: [], flags: newFlags), + eg.updateMessageEditEvent(message, flags: newFlags), )).returnsNormally(); // Rarely, this event will be about an unread that's unknown // to the model, or at least one of the model's components; @@ -307,7 +307,7 @@ void main() { continue; } model.handleUpdateMessageEvent( - UpdateMessageEvent(id: 0, messageId: message.id, messageIds: [], flags: newFlags), + eg.updateMessageEditEvent(message, flags: newFlags), ); if ( From a7dd0dc91af004bf5beb6bc1ec8839a203789665 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Nov 2023 18:17:11 -0700 Subject: [PATCH 5/8] api [nfc]: Make DeleteMessageEvent constructor fully explicit --- lib/api/model/events.dart | 4 ++-- test/model/unreads_test.dart | 12 +++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index 6ed37213de..4842f53eb6 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -434,8 +434,8 @@ class DeleteMessageEvent extends Event { required super.id, required this.messageIds, required this.messageType, - this.streamId, - this.topic, + required this.streamId, + required this.topic, }); factory DeleteMessageEvent.fromJson(Map json) { diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 11262d33ed..dc1b7cce4e 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -376,7 +376,13 @@ void main() { streamId: message.streamId, topic: message.subject, ), - DmMessage() => DeleteMessageEvent(id: 0, messageType: MessageType.private, messageIds: [message.id]), + DmMessage() => DeleteMessageEvent( + id: 0, + messageType: MessageType.private, + messageIds: [message.id], + streamId: null, + topic: null, + ), }; model.handleDeleteMessageEvent(event); checkNotifiedOnce(); @@ -412,6 +418,8 @@ void main() { id: 0, messageIds: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10], messageType: MessageType.private, + streamId: null, + topic: null, )); checkNotifiedOnce(); checkMatchesMessages([]); @@ -445,6 +453,8 @@ void main() { id: 0, messageIds: [message.id], messageType: MessageType.private, + streamId: null, + topic: null, )); // TODO improve implementation; then: // checkNotNotified(); From 18a0b76b00c45551bf8a6cc034a5de0b1df258a2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Nov 2023 17:49:52 -0700 Subject: [PATCH 6/8] api [nfc]: Make User constructor fully explicit --- lib/api/model/model.dart | 4 ++-- test/example_data.dart | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index d7ae1c4a7e..0c8e9f671c 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -173,8 +173,8 @@ class User { required this.isGuest, required this.isBillingAdmin, required this.isBot, - this.botType, - this.botOwnerId, + required this.botType, + required this.botOwnerId, required this.role, required this.timezone, required this.avatarUrl, diff --git a/test/example_data.dart b/test/example_data.dart index 2b6c76a532..137ab923bf 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -42,6 +42,8 @@ User user({ isGuest: false, isBillingAdmin: false, isBot: false, + botType: null, + botOwnerId: null, role: UserRole.member, timezone: 'UTC', avatarUrl: avatarUrl, From c10a715d1b6052976e13b9a6ba16b15fe7ce5085 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Nov 2023 18:19:55 -0700 Subject: [PATCH 7/8] api [nfc]: Explain the two constructors we leave with optional arguments --- lib/api/model/events.dart | 3 +++ lib/api/model/model.dart | 9 ++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index 4842f53eb6..4e2723eb59 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -249,6 +249,9 @@ class RealmUserUpdateEvent extends RealmUserEvent { RealmUserUpdateEvent({ required super.id, required this.userId, + // Unlike in most of the API bindings, we leave these constructor arguments + // optional. That's because in these events only one or a handful of these + // will appear in a given event value. this.fullName, this.avatarUrl, this.avatarVersion, diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 0c8e9f671c..93095e8250 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -194,7 +194,14 @@ class ProfileFieldUserData { final String value; final String? renderedValue; - ProfileFieldUserData({required this.value, this.renderedValue}); + ProfileFieldUserData({ + required this.value, + // Unlike in most of the API bindings, we leave this constructor argument + // optional. That's because for most types of custom profile fields, + // this property is always indeed absent, and because this constructor is + // otherwise convenient to write many calls to in our test code. + this.renderedValue, + }); factory ProfileFieldUserData.fromJson(Map json) => _$ProfileFieldUserDataFromJson(json); From 48369395303baee51ea9e352b977f9e2b8a27f32 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Nov 2023 17:35:08 -0700 Subject: [PATCH 8/8] api [nfc]: Make all remaining constructor arguments required This completes the sweep for the point of style we added to the README a few commits ago. --- lib/api/model/initial_snapshot.dart | 4 +-- lib/api/model/model.dart | 40 ++++++++++++++--------------- lib/api/route/account.dart | 2 +- lib/api/route/events.dart | 2 +- lib/api/route/realm.dart | 4 +-- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 8d7f983453..6f38f4fed6 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -73,11 +73,11 @@ class InitialSnapshot { } InitialSnapshot({ - this.queueId, + required this.queueId, required this.lastEventId, required this.zulipFeatureLevel, required this.zulipVersion, - this.zulipMergeBase, + required this.zulipMergeBase, required this.alertWords, required this.customProfileFields, required this.recentPrivateConversations, diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 93095e8250..86e70ef965 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -179,7 +179,7 @@ class User { required this.timezone, required this.avatarUrl, required this.avatarVersion, - this.profileData, + required this.profileData, required this.isSystemBot, }); @@ -328,22 +328,22 @@ class Subscription { required this.renderedDescription, required this.dateCreated, required this.inviteOnly, - this.desktopNotifications, - this.emailNotifications, - this.wildcardMentionsNotify, - this.pushNotifications, - this.audibleNotifications, + required this.desktopNotifications, + required this.emailNotifications, + required this.wildcardMentionsNotify, + required this.pushNotifications, + required this.audibleNotifications, required this.pinToTop, required this.emailAddress, required this.isMuted, - this.isWebPublic, + required this.isWebPublic, required this.color, required this.streamPostPolicy, - this.messageRetentionDays, + required this.messageRetentionDays, required this.historyPublicToSubscribers, - this.firstMessageId, - this.streamWeeklyTraffic, - this.canRemoveSubscribersGroupId, + required this.firstMessageId, + required this.streamWeeklyTraffic, + required this.canRemoveSubscribersGroupId, }); factory Subscription.fromJson(Map json) => @@ -406,7 +406,7 @@ sealed class Message { required this.contentType, required this.id, required this.isMeMessage, - this.lastEditTimestamp, + required this.lastEditTimestamp, required this.reactions, required this.recipientId, required this.senderEmail, @@ -416,8 +416,8 @@ sealed class Message { required this.subject, required this.timestamp, required this.flags, - this.matchContent, - this.matchSubject, + required this.matchContent, + required this.matchSubject, }); factory Message.fromJson(Map json) { @@ -471,7 +471,7 @@ class StreamMessage extends Message { required super.contentType, required super.id, required super.isMeMessage, - super.lastEditTimestamp, + required super.lastEditTimestamp, required super.reactions, required super.recipientId, required super.senderEmail, @@ -481,8 +481,8 @@ class StreamMessage extends Message { required super.subject, required super.timestamp, required super.flags, - super.matchContent, - super.matchSubject, + required super.matchContent, + required super.matchSubject, required this.displayRecipient, required this.streamId, }); @@ -573,7 +573,7 @@ class DmMessage extends Message { required super.contentType, required super.id, required super.isMeMessage, - super.lastEditTimestamp, + required super.lastEditTimestamp, required super.reactions, required super.recipientId, required super.senderEmail, @@ -583,8 +583,8 @@ class DmMessage extends Message { required super.subject, required super.timestamp, required super.flags, - super.matchContent, - super.matchSubject, + required super.matchContent, + required super.matchSubject, required this.displayRecipient, }); diff --git a/lib/api/route/account.dart b/lib/api/route/account.dart index e678932485..fdf57d1ce6 100644 --- a/lib/api/route/account.dart +++ b/lib/api/route/account.dart @@ -32,7 +32,7 @@ class FetchApiKeyResult { FetchApiKeyResult({ required this.apiKey, required this.email, - this.userId, + required this.userId, }); factory FetchApiKeyResult.fromJson(Map json) => diff --git a/lib/api/route/events.dart b/lib/api/route/events.dart index 68ea815086..1565fe422c 100644 --- a/lib/api/route/events.dart +++ b/lib/api/route/events.dart @@ -41,7 +41,7 @@ class GetEventsResult { GetEventsResult({ required this.events, - this.queueId, + required this.queueId, }); factory GetEventsResult.fromJson(Map json) => diff --git a/lib/api/route/realm.dart b/lib/api/route/realm.dart index 890709316e..7a8678de5f 100644 --- a/lib/api/route/realm.dart +++ b/lib/api/route/realm.dart @@ -52,7 +52,7 @@ class GetServerSettingsResult { required this.authenticationMethods, required this.zulipFeatureLevel, required this.zulipVersion, - this.zulipMergeBase, + required this.zulipMergeBase, required this.pushNotificationsEnabled, required this.isIncompatible, required this.emailAuthEnabled, @@ -61,7 +61,7 @@ class GetServerSettingsResult { required this.realmName, required this.realmIcon, required this.realmDescription, - this.realmWebPublicAccessEnabled, + required this.realmWebPublicAccessEnabled, }); factory GetServerSettingsResult.fromJson(Map json) =>