api: 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.

This also allows us to drop the `assert`'s and "TODO(log)"'s, because
the stacktrace we need can be retrieved after throwing these
`FormatException`'s.

This is similar to zulip-mobile code for parsing move data. The main
difference is that we check the value of `propagate_mode`, which is
documented to be present on message moves.  With this single indicator,
the logic is crisp for ruling out non-move message update events.
See Greg's comment on this:
  zulip#1311 (comment)

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

Author: PIG208

lib/api/model/events.dart

@@ -701,6 +701,70 @@ class MessageEvent extends Event {
   }
 }
 
+/// 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 newStreamId = (json['new_stream_id'] as num?)?.toInt() ?? 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 newTopic = json['subject'] == null ? origTopic
+        : TopicName.fromJson(json['subject'] as String);
+
+    if (propagateMode == null) {
+      if (origTopic != newTopic || origStreamId != newStreamId) {
+        throw FormatException('move but no propagateMode');
+      }
+      // There was no move.
+      return null;
+    }
+
+    if (origStreamId == newStreamId && origTopic == newTopic) {
+      // If neither the channel nor topic name changed, nothing moved.
+      // In that case `propagate_mode` (aka propagateMode) should have been null.
+      throw FormatException('move but unchanged newStreamId and newTopic');
+    }
+
+    return UpdateMessageMoveData(
+      origStreamId: origStreamId!,
+      newStreamId: newStreamId!,
+      propagateMode: propagateMode,
+      origTopic: origTopic!,
+      newTopic: newTopic!,
+    );
+  }
+
+  Object? toJson() => null;
+}
+
 /// A Zulip event of type `update_message`: https://zulip.com/api/get-events#update_message
 @JsonSerializable(fieldRename: FieldRename.snake)
 class UpdateMessageEvent extends Event {
@@ -718,16 +782,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)
+  final UpdateMessageMoveData? moveData;
 
   // final List<TopicLink> topicLinks; // TODO handle
 
@@ -747,18 +803,20 @@ 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, Object?> _readMoveData(Map<Object?, Object?> json, String key) {
+    // Parsing [UpdateMessageMoveData] requires `json`, not the default `json[key]`.
+    assert(json is Map<String, Object?>); // value came through `fromJson` with this type
+    return json as Map<String, Object?>;
+  }
+
   factory UpdateMessageEvent.fromJson(Map<String, dynamic> json) =>
     _$UpdateMessageEventFromJson(json);
 

lib/api/model/events.g.dart

@@ -172,46 +172,12 @@ class MessageStoreImpl with MessageStore {
     // 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; // null if topic-only move
-    final origTopic = event.origTopic;
-    final newTopic = event.newTopic;
-    final propagateMode = event.propagateMode;
-
-    if (origTopic == null) {
-      // There was no move.
-      assert(() {
-        if (newStreamId != null && origStreamId != null
-            && newStreamId != origStreamId) {
-          // This should be impossible; `orig_subject` (aka origTopic) is
-          // documented to be present when either the stream or topic changed.
-          debugLog('Malformed UpdateMessageEvent: stream move but no origTopic'); // TODO(log)
-        }
-        return true;
-      }());
+    final messageMove = event.moveData;
+    if (messageMove == null) {
+      // There is no message move.
       return;
     }
 
-    if (newStreamId == null && newTopic == null) {
-      // If neither the channel nor topic name changed, nothing moved.
-      // In that case `orig_subject` (aka origTopic) should have been null.
-      assert(debugLog('Malformed UpdateMessageEvent: move but no newStreamId or newTopic')); // TODO(log)
-      return;
-    }
-    if (origStreamId == null) {
-      // The `stream_id` field (aka origStreamId) is documented to be present on moves.
-      assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // 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 wasResolveOrUnresolve = (newStreamId == null
-      && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!));
-
     for (final messageId in event.messageIds) {
       final message = messages[messageId];
       if (message == null) continue;
@@ -221,17 +187,21 @@ class MessageStoreImpl with MessageStore {
         continue;
       }
 
-      if (newStreamId != null) {
-        message.streamId = newStreamId;
+      if (messageMove.origStreamId != messageMove.newStreamId) {
+        message.streamId = messageMove.newStreamId;
         // See [StreamMessage.displayRecipient] on why the invalidation is
         // needed.
         message.displayRecipient = null;
       }
 
-      if (newTopic != null) {
-        message.topic = newTopic;
+      if (messageMove.origTopic != messageMove.newTopic) {
+        message.topic = messageMove.newTopic;
       }
 
+      final wasResolveOrUnresolve =
+        messageMove.origStreamId == messageMove.newStreamId
+        && MessageEditState.topicMoveWasResolveOrUnresolve(
+             messageMove.origTopic, messageMove.newTopic);
       if (!wasResolveOrUnresolve
           && message.editState == MessageEditState.none) {
         message.editState = MessageEditState.moved;
@@ -240,12 +210,12 @@ class MessageStoreImpl with MessageStore {
 
     for (final view in _messageListViews) {
       view.messagesMoved(
-        origStreamId: origStreamId,
-        newStreamId: newStreamId ?? origStreamId,
-        origTopic: origTopic,
-        newTopic: newTopic ?? origTopic,
+        origStreamId: messageMove.origStreamId,
+        newStreamId: messageMove.newStreamId,
+        origTopic: messageMove.origTopic,
+        newTopic: messageMove.newTopic,
         messageIds: event.messageIds,
-        propagateMode: propagateMode,
+        propagateMode: messageMove.propagateMode,
       );
     }
   }

lib/model/message.dart

@@ -48,18 +48,23 @@ 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');
 }