api: Extract and parse UpdateMessageMoveData from UpdateMessageEvent

This is mostly NFC except that we were logging and ignoring malformed
event data before, and now start to throw errors within the parsing
code.

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.

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,101 @@ 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 TopicName origTopic;
+  final TopicName newTopic;
+  final PropagateMode propagateMode;
+
+  UpdateMessageMoveData({
+    required this.origStreamId,
+    required this.newStreamId,
+    required this.origTopic,
+    required this.newTopic,
+    required this.propagateMode,
+  }) : assert(newStreamId != origStreamId || newTopic != origTopic);
+
+  /// 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 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;
+
+    final propagateModeString = json['propagate_mode'] as String?;
+    final propagateMode = propagateModeString == null ? null
+      : PropagateMode.fromRawString(propagateModeString);
+
+    if (newStreamId == origStreamId && newTopic == origTopic) {
+      if (propagateMode != null) {
+        throw FormatException(
+          'Malformed UpdateMessageEvent: incoherent message-move fields; '
+          'propagate_mode present but no new channel or topic');
+      }
+      return null;
+    }
+
+    if (origStreamId == null || newStreamId == null) {
+      // The `stream_id` field (aka origStreamId) is documented to be present on moves;
+      // newStreamId should not be null either because it falls back to origStreamId.
+      throw FormatException('Malformed UpdateMessageEvent: move but no origStreamId');
+    }
+    if (origTopic == null || newTopic == null) {
+      // The `orig_subject` field (aka origTopic) is documented to be present on moves;
+      // newTopic should not be null either because it falls back to origTopic.
+      throw FormatException('Malformed UpdateMessageEvent: move but no origTopic');
+    }
+    if (propagateMode == null) {
+      // The `propagate_mode` field (aka propagateMode) is documented to be present on moves.
+      throw FormatException('Malformed UpdateMessageEvent: move but no propagateMode');
+    }
+
+    return UpdateMessageMoveData(
+      origStreamId: origStreamId,
+      newStreamId: newStreamId,
+      origTopic: origTopic,
+      newTopic: newTopic,
+      propagateMode: propagateMode,
+    );
+  }
+}
+
 /// 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,42 +169,14 @@ 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 (newStreamId == origStreamId && newTopic == origTopic) {
+    final messageMove = event.moveData;
+    if (messageMove == null) {
       // There was no move.
-      if (propagateMode != null) {
-        assert(debugLog(
-          'Malformed UpdateMessageEvent: incoherent message-move fields; '
-          'propagate_mode present but no new channel or topic')); // TODO(log)
-      }
       return;
     }
 
-    if (origStreamId == null || newStreamId == null) {
-      // The `stream_id` field (aka origStreamId) is documented to be present on moves.
-      // newStreamId should not be null either because it falls back to origStreamId.
-      assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log)
-      return;
-    }
-    if (origTopic == null || newTopic == null) {
-      // The `orig_subject` field (aka origTopic) is documented to be present on moves.
-      // newTopic should not be null either because it falls back to origTopic.
-      assert(debugLog('Malformed UpdateMessageEvent: move but no origTopic')); // TODO(log)
-      return;
-    }
-    if (propagateMode == null) {
-      // The `propagate_mode` field (aka propagateMode) is documented to be present on moves.
-      assert(debugLog('Malformed UpdateMessageEvent: move but no propagateMode')); // TODO(log)
-      return;
-    }
+    final UpdateMessageMoveData(
+      :origStreamId, :newStreamId, :origTopic, :newTopic) = messageMove;
 
     final wasResolveOrUnresolve = newStreamId == origStreamId
       && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic);
@@ -236,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<TopicName> get origTopic => has((e) => e.origTopic, 'origTopic');
+  Subject<TopicName> get newTopic => has((e) => e.newTopic, 'newTopic');
+  Subject<PropagateMode> get propagateMode => has((e) => e.propagateMode, 'propagateMode');
+}
+
 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,73 @@ 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', () {
+      // The server omits 'subject' in this situation.
+      check(Event.fromJson({ ...baseMoveJson,
+        'stream_id': 1,
+        'new_stream_id': 2,
+        'orig_subject': 'foo',
+        'subject': null,
+      })).isA<UpdateMessageEvent>().moveData.isNotNull()
+        ..origTopic.equals(const TopicName('foo'))
+        ..newTopic.equals(const TopicName('foo'));
+    });
+
+    test('same channel, new topic; fill in newStreamId', () {
+      // The server omits 'new_stream_id' in this situation.
+      check(Event.fromJson({ ...baseMoveJson,
+        'stream_id': 1,
+        'new_stream_id': null,
+        'orig_subject': 'foo',
+        'subject': 'bar',
+      })).isA<UpdateMessageEvent>().moveData.isNotNull()
+        ..origStreamId.equals(1)
+        ..newStreamId.equals(1);
+    });
+
+    test('no message move', () {
+      check(Event.fromJson({ ...baseJson,
+        'orig_content': 'foo',
+        'orig_rendered_content': 'foo',
+        'content': 'bar',
+        'rendered_content': 'bar',
+      })).isA<UpdateMessageEvent>().moveData.isNull();
+    });
+
+    test('stream move but no orig_subject', () {
+      check(() => Event.fromJson({ ...baseMoveJson,
+        'stream_id': 1,
+        'new_stream_id': 2,
+        'orig_subject': null,
+      })).throws<void>();
+    });
+
+    test('move but no subject or new_stream_id', () {
+      check(() => Event.fromJson({ ...baseMoveJson,
+        'new_stream_id': null,
+        'subject': null,
+      })).throws<void>();
+    });
+
+    test('move but no orig_stream_id', () {
+      check(() => Event.fromJson({ ...baseMoveJson,
+        'stream_id': null,
+        'new_stream_id': 2,
+      })).throws<void>();
+    });
+
+    test('move but no propagate_mode', () {
+      check(() => Event.fromJson({ ...baseMoveJson,
+        'orig_subject': 'foo',
+        'subject': 'bar',
+        'propagate_mode': null,
+      })).throws<void>();
+    });
   });
 
   test('delete_message: require streamId and topic for stream messages', () {

test/api/model/events_test.dart

@@ -53,6 +53,7 @@ extension TopicNameChecks on Subject<TopicName> {
 
 extension StreamMessageChecks on Subject<StreamMessage> {
   Subject<String?> get displayRecipient => has((e) => e.displayRecipient, 'displayRecipient');
+  Subject<int> get streamId => has((e) => e.streamId, 'streamId');
   Subject<TopicName> get topic => has((e) => e.topic, 'topic');
 }