api [nfc]: Extract and parse UpdateMessageMoveData from UpdateMessageEvent

This data structure encapsulates some checks so that we can make all
fields non-nullable, with reasonable fallback values.  As of writing,
we do not use origStreamId (a.k.a.: 'stream_id') when there was no
message move, even though it is present if there were content edits
This makes dropping 'stream_id' when parsing `moveData` into `null`
acceptable for now.

Despite the NFC, a subtlety of this change is that `FormatException`'s
or null-check errors will be thrown earlier in the polling loop.
Because we now parse the move data upfront, instead of dealing with it
when handling the message event.  Both will end up resulting in the
event queue being replaced if such an error occurs.

Signed-off-by: Zixuan James Li <[email protected]>

Author: PIG208

lib/api/model/events.dart

@@ -718,16 +718,8 @@ class UpdateMessageEvent extends Event {
 
   // final String? streamName; // ignore
 
-  @JsonKey(name: 'stream_id')
-  final int? origStreamId;
-  final int? newStreamId;
-
-  final PropagateMode? propagateMode;
-
-  @JsonKey(name: 'orig_subject')
-  final TopicName? origTopic;
-  @JsonKey(name: 'subject')
-  final TopicName? newTopic;
+  @JsonKey(readValue: _readMoveData, fromJson: UpdateMessageMoveData.tryParseFromJson, includeToJson: false)
+  final UpdateMessageMoveData? moveData;
 
   // final List<TopicLink> topicLinks; // TODO handle
 
@@ -747,25 +739,88 @@ class UpdateMessageEvent extends Event {
     required this.messageIds,
     required this.flags,
     required this.editTimestamp,
-    required this.origStreamId,
-    required this.newStreamId,
-    required this.propagateMode,
-    required this.origTopic,
-    required this.newTopic,
+    required this.moveData,
     required this.origContent,
     required this.origRenderedContent,
     required this.content,
     required this.renderedContent,
     required this.isMeMessage,
   });
 
+  static Map<String, dynamic> _readMoveData(Map<dynamic, dynamic> json, String key) {
+    // Parsing [UpdateMessageMoveData] requires `json`, not the default `json[key]`.
+    assert(json is Map<String, dynamic>); // value came through `fromJson` with this type
+    return json as Map<String, dynamic>;
+  }
+
   factory UpdateMessageEvent.fromJson(Map<String, dynamic> json) =>
     _$UpdateMessageEventFromJson(json);
 
   @override
   Map<String, dynamic> toJson() => _$UpdateMessageEventToJson(this);
 }
 
+/// Data structure representing a message move.
+class UpdateMessageMoveData {
+  final int origStreamId;
+  final int newStreamId;
+
+  final PropagateMode propagateMode;
+
+  final TopicName origTopic;
+  final TopicName newTopic;
+
+  UpdateMessageMoveData({
+    required this.origStreamId,
+    required this.newStreamId,
+    required this.propagateMode,
+    required this.origTopic,
+    required this.newTopic,
+  }) : assert(origStreamId != newStreamId || origTopic != newTopic);
+
+  /// Try to extract [UpdateMessageMoveData] from the JSON object for an
+  /// [UpdateMessageEvent].
+  ///
+  /// Returns `null` if there was no message move.
+  ///
+  /// Throws an error if the data is malformed.
+  // When parsing this, 'stream_id', which is also present when there was only
+  // a content edit, cannot be recovered if this ends up returning `null`.
+  // This may matter if we ever need 'stream_id' when no message move occurred.
+  static UpdateMessageMoveData? tryParseFromJson(Map<String, Object?> json) {
+    final origStreamId = (json['stream_id'] as num?)?.toInt();
+    final newStreamIdRaw = (json['new_stream_id'] as num?)?.toInt();
+    final newStreamId = newStreamIdRaw ?? origStreamId;
+
+    final propagateModeString = json['propagate_mode'] as String?;
+    final propagateMode = propagateModeString == null ? null
+      : PropagateMode.fromRawString(propagateModeString);
+
+    final origTopic = json['orig_subject'] == null ? null
+      : TopicName.fromJson(json['orig_subject'] as String);
+    final newTopicRaw = json['subject'] == null ? null
+      : TopicName.fromJson(json['subject'] as String);
+    final newTopic = newTopicRaw ?? origTopic;
+
+    if (origTopic == newTopic && origStreamId == newStreamId) {
+      if (propagateMode != null) {
+        throw FormatException(
+          'UpdateMessageEvent: incoherent message-move fields; '
+          'propagate_mode present but no new channel or topic');
+      }
+      return null;
+    }
+
+    return UpdateMessageMoveData(
+      origStreamId: origStreamId!,
+      newStreamId: newStreamId!,
+      propagateMode: propagateMode!,
+      origTopic: origTopic!,
+      newTopic: newTopic!,
+    );
+  }
+}
+
 /// A Zulip event of type `delete_message`: https://zulip.com/api/get-events#delete_message
 @JsonSerializable(fieldRename: FieldRename.snake)
 class DeleteMessageEvent extends Event {

lib/api/model/events.g.dart

@@ -169,27 +169,17 @@ class MessageStoreImpl with MessageStore {
   }
 
   void _handleUpdateMessageEventMove(UpdateMessageEvent event) {
-    // The interaction between the fields of these events are a bit tricky.
-    // For reference, see: https://zulip.com/api/get-events#update_message
-
-    final origStreamId = event.origStreamId;
-    final newStreamId = event.newStreamId ?? origStreamId;
-    final origTopic = event.origTopic;
-    final newTopic = event.newTopic ?? origTopic;
-    final propagateMode = event.propagateMode;
-
-    if (origTopic == newTopic && origStreamId == newStreamId) {
-      if (propagateMode != null) {
-        throw FormatException(
-          'UpdateMessageEvent: incoherent message-move fields; '
-          'propagate_mode present but no new channel or topic');
-      }
+    final messageMove = event.moveData;
+    if (messageMove == null) {
       // There was no move.
       return;
     }
 
+    final UpdateMessageMoveData(
+      :origStreamId, :newStreamId, :origTopic, :newTopic) = messageMove;
+
     final wasResolveOrUnresolve = origStreamId == newStreamId
-      && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic!, newTopic!);
+      && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic);
 
     for (final messageId in event.messageIds) {
       final message = messages[messageId];
@@ -201,7 +191,7 @@ class MessageStoreImpl with MessageStore {
       }
 
       if (origStreamId != newStreamId) {
-        message.streamId = newStreamId!;
+        message.streamId = newStreamId;
         // See [StreamMessage.displayRecipient] on why the invalidation is
         // needed.
         message.displayRecipient = null;
@@ -218,14 +208,7 @@ class MessageStoreImpl with MessageStore {
     }
 
     for (final view in _messageListViews) {
-      view.messagesMoved(
-        origStreamId: origStreamId!,
-        newStreamId: newStreamId!,
-        origTopic: origTopic!,
-        newTopic: newTopic!,
-        messageIds: event.messageIds,
-        propagateMode: propagateMode!,
-      );
+      view.messagesMoved(messageMove: messageMove, messageIds: event.messageIds);
     }
   }
 

lib/model/message.dart

@@ -720,13 +720,12 @@ class MessageListView with ChangeNotifier, _MessageSequence {
   }
 
   void messagesMoved({
-    required int origStreamId,
-    required int newStreamId,
-    required TopicName origTopic,
-    required TopicName newTopic,
+    required UpdateMessageMoveData messageMove,
     required List<int> messageIds,
-    required PropagateMode propagateMode,
   }) {
+    final UpdateMessageMoveData(
+      :origStreamId, :newStreamId, :origTopic, :newTopic, :propagateMode,
+    ) = messageMove;
     switch (narrow) {
       case DmNarrow():
         // DMs can't be moved (nor created by moves),

lib/model/message_list.dart

@@ -48,18 +48,22 @@ extension UpdateMessageEventChecks on Subject<UpdateMessageEvent> {
   Subject<List<int>> get messageIds => has((e) => e.messageIds, 'messageIds');
   Subject<List<MessageFlag>> get flags => has((e) => e.flags, 'flags');
   Subject<int?> get editTimestamp => has((e) => e.editTimestamp, 'editTimestamp');
-  Subject<int?> get origStreamId => has((e) => e.origStreamId, 'origStreamId');
-  Subject<int?> get newStreamId => has((e) => e.newStreamId, 'newStreamId');
-  Subject<PropagateMode?> get propagateMode => has((e) => e.propagateMode, 'propagateMode');
-  Subject<TopicName?> get origTopic => has((e) => e.origTopic, 'origTopic');
-  Subject<TopicName?> get newTopic => has((e) => e.newTopic, 'newTopic');
+  Subject<UpdateMessageMoveData?> get moveData => has((e) => e.moveData, 'moveData');
   Subject<String?> get origContent => has((e) => e.origContent, 'origContent');
   Subject<String?> get origRenderedContent => has((e) => e.origRenderedContent, 'origRenderedContent');
   Subject<String?> get content => has((e) => e.content, 'content');
   Subject<String?> get renderedContent => has((e) => e.renderedContent, 'renderedContent');
   Subject<bool?> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
 }
 
+extension UpdateMessageMoveDataChecks on Subject<UpdateMessageMoveData> {
+  Subject<int> get origStreamId => has((e) => e.origStreamId, 'origStreamId');
+  Subject<int> get newStreamId => has((e) => e.newStreamId, 'newStreamId');
+  Subject<PropagateMode> get propagateMode => has((e) => e.propagateMode, 'propagateMode');
+  Subject<TopicName> get origTopic => has((e) => e.origTopic, 'origTopic');
+  Subject<TopicName> get newTopic => has((e) => e.newTopic, 'newTopic');
+}
+
 extension DeleteMessageEventChecks on Subject<DeleteMessageEvent> {
   Subject<MessageType?> get messageType => has((e) => e.messageType, 'messageType');
 }

test/api/model/events_checks.dart

@@ -106,11 +106,26 @@ void main() {
       'propagate_mode': 'change_all',
     };
 
+    test('smoke moveData', () {
+      check(Event.fromJson({ ...baseMoveJson,
+        'stream_id': 1,
+        'new_stream_id': 2,
+        'orig_subject': 'foo',
+        'subject': 'bar',
+        'propagate_mode': 'change_all',
+      })).isA<UpdateMessageEvent>().moveData.isNotNull()
+        ..origStreamId.equals(1)
+        ..newStreamId.equals(2)
+        ..origTopic.equals(const TopicName('foo'))
+        ..newTopic.equals(const TopicName('bar'))
+        ..propagateMode.equals(PropagateMode.changeAll);
+    });
+
     test('stream_id -> origStreamId', () {
       check(Event.fromJson({ ...baseMoveJson,
         'stream_id': 1,
         'new_stream_id': 2,
-      })).isA<UpdateMessageEvent>()
+      })).isA<UpdateMessageEvent>().moveData.isNotNull()
         ..origStreamId.equals(1)
         ..newStreamId.equals(2);
     });
@@ -119,10 +134,32 @@ void main() {
       check(Event.fromJson({ ...baseMoveJson,
         'orig_subject': 'foo',
         'subject': 'bar',
-      })).isA<UpdateMessageEvent>()
+      })).isA<UpdateMessageEvent>().moveData.isNotNull()
         ..origTopic.equals(const TopicName('foo'))
         ..newTopic.equals(const TopicName('bar'));
     });
+
+    test('new channel, same topic: fill in newTopic', () {
+      check(Event.fromJson({ ...baseMoveJson,
+        'orig_subject': 'foo',
+        'subject': null,
+        'stream_id': 1,
+        'new_stream_id': 2,
+      })).isA<UpdateMessageEvent>().moveData.isNotNull()
+        ..origTopic.equals(const TopicName('foo'))
+        ..newTopic.equals(const TopicName('foo'));
+    });
+
+    test('same channel, new topic; fill in newStreamId', () {
+      check(Event.fromJson({ ...baseMoveJson,
+        'orig_subject': 'foo',
+        'subject': 'bar',
+        'stream_id': 1,
+        'new_stream_id': null,
+      })).isA<UpdateMessageEvent>().moveData.isNotNull()
+        ..origStreamId.equals(1)
+        ..newStreamId.equals(1);
+    });
   });
 
   test('delete_message: require streamId and topic for stream messages', () {

test/api/model/events_test.dart

@@ -653,11 +653,7 @@ UpdateMessageEvent updateMessageEditEvent(
     messageIds: [messageId],
     flags: flags ?? origMessage.flags,
     editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp
-    origStreamId: origMessage is StreamMessage ? origMessage.streamId : null,
-    newStreamId: null,
-    propagateMode: null,
-    origTopic: null,
-    newTopic: null,
+    moveData: null,
     origContent: 'some probably-mismatched old Markdown',
     origRenderedContent: origMessage.content,
     content: 'some probably-mismatched new Markdown',
@@ -690,11 +686,13 @@ UpdateMessageEvent _updateMessageMoveEvent(
     messageIds: messageIds,
     flags: flags,
     editTimestamp: 1234567890, // TODO generate timestamp
-    origStreamId: origStreamId,
-    newStreamId: newStreamId,
-    propagateMode: propagateMode,
-    origTopic: origTopic,
-    newTopic: newTopic,
+    moveData: UpdateMessageMoveData(
+      origStreamId: origStreamId,
+      newStreamId: newStreamId ?? origStreamId,
+      propagateMode: propagateMode,
+      origTopic: origTopic,
+      newTopic: newTopic ?? origTopic,
+    ),
     origContent: origContent,
     origRenderedContent: origContent,
     content: newContent,