api: Parse UpdateMessageMoveData with inter-related constraints

This moves some assertions 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.

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

Author: PIG208

lib/api/model/events.dart

@@ -703,13 +703,13 @@ class MessageEvent extends Event {
 
 /// Data structure representing a message move.
 class UpdateMessageMoveData {
-  final int? origStreamId;
-  final int? newStreamId;
+  final int origStreamId;
+  final int newStreamId;
 
-  final PropagateMode? propagateMode;
+  final PropagateMode propagateMode;
 
-  final TopicName? origTopic;
-  final TopicName? newTopic;
+  final TopicName origTopic;
+  final TopicName newTopic;
 
   UpdateMessageMoveData({
     required this.origStreamId,
@@ -719,11 +719,16 @@ class UpdateMessageMoveData {
     required this.newTopic,
   });
 
-  /// Extract [UpdateMessageMoveData] from the JSON object for an
+  /// 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.
-  factory UpdateMessageMoveData.fromJson(Object? json) {
+  // 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(Object? json) {
     json as Map<String, Object?>;
     final origStreamId = (json['stream_id'] as num?)?.toInt();
     final newStreamId = (json['new_stream_id'] as num?)?.toInt();
@@ -735,12 +740,40 @@ class UpdateMessageMoveData {
     final newTopic = json['subject'] == null ? null
       : TopicName.fromJson(json['subject'] as String);
 
+    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.
+          throw FormatException('stream move but no origTopic');
+        }
+        return true;
+      }());
+      return null;
+    }
+
+    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.
+      throw FormatException('move but no newStreamId or newTopic');
+    }
+    if (origStreamId == null) {
+      // The `stream_id` field (aka origStreamId) is documented to be present on moves.
+      throw FormatException('move but no origStreamId');
+    }
+    if (propagateMode == null) {
+      // The `propagate_mode` field (aka propagateMode) is documented to be present on moves.
+      throw FormatException('move but no propagateMode');
+    }
+
     return UpdateMessageMoveData(
       origStreamId: origStreamId,
-      newStreamId: newStreamId,
+      newStreamId: newStreamId ?? origStreamId,
       propagateMode: propagateMode,
       origTopic: origTopic,
-      newTopic: newTopic,
+      newTopic: newTopic ?? origTopic,
     );
   }
 
@@ -764,8 +797,8 @@ class UpdateMessageEvent extends Event {
 
   // final String? streamName; // ignore
 
-  @JsonKey(readValue: _readMoveData)
-  final UpdateMessageMoveData moveData;
+  @JsonKey(readValue: _readMoveData, fromJson: UpdateMessageMoveData.tryParseFromJson)
+  final UpdateMessageMoveData? moveData;
 
   // final List<TopicLink> topicLinks; // TODO handle
 

lib/api/model/events.g.dart

@@ -172,47 +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 moveData = event.moveData;
-    final origStreamId = moveData.origStreamId;
-    final newStreamId = moveData.newStreamId; // null if topic-only move
-    final origTopic = moveData.origTopic;
-    final newTopic = moveData.newTopic;
-    final propagateMode = moveData.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;
@@ -222,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;
@@ -241,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

@@ -127,7 +127,7 @@ void main() {
         ..origStreamId.equals(1)
         ..newStreamId.equals(2)
         ..origTopic.equals(TopicName('foo'))
-        ..newTopic.isNull();
+        ..newTopic.equals(TopicName('foo'));
     });
 
     test('orig_subject -> origTopic, subject -> newTopic, new_stream_id = stream_id', () {
@@ -139,7 +139,7 @@ void main() {
         'propagate_mode': 'change_all',
       })).isA<UpdateMessageEvent>().moveData.isNotNull()
         ..origStreamId.equals(1)
-        ..newStreamId.isNull()
+        ..newStreamId.equals(1)
         ..origTopic.equals(const TopicName('foo'))
         ..newTopic.equals(const TopicName('bar'));
     });
@@ -151,11 +151,47 @@ void main() {
         'orig_rendered_content': 'foo',
         'content': 'bar',
         'rendered_content': 'bar',
-      })).isA<UpdateMessageEvent>().moveData.isNotNull()
-        ..origStreamId.equals(1)
-        ..newStreamId.isNull()
-        ..origTopic.isNull()
-        ..newTopic.isNull();
+      })).isA<UpdateMessageEvent>().moveData.isNull();
+    });
+
+    test('stream move but no orig_subject', () {
+      check(() => Event.fromJson({...baseJson,
+        'stream_id': 1,
+        'new_stream_id': 2,
+        'orig_subject': null,
+        'subject': null,
+        'propagate_mode': 'change_all',
+      })).throws<FormatException>();
+    });
+
+    test('move but no subject or new_stream_id', () {
+      check(() => Event.fromJson({...baseJson,
+        'stream_id': 1,
+        'new_stream_id': null,
+        'orig_subject': 'foo',
+        'subject': null,
+        'propagate_mode': 'change_all',
+      })).throws<FormatException>();
+    });
+
+    test('move but no orig_stream_id', () {
+      check(() => Event.fromJson({...baseJson,
+        'stream_id': null,
+        'new_stream_id': 2,
+        'orig_subject': 'foo',
+        'subject': 'bar',
+        'propagate_mode': 'change_all',
+      })).throws<FormatException>();
+    });
+
+    test('move but no propagate_mode', () {
+      check(() => Event.fromJson({...baseJson,
+        'stream_id': 1,
+        'new_stream_id': 2,
+        'orig_subject': 'foo',
+        'subject': 'bar',
+        'propagate_mode': null,
+      })).throws<FormatException>();
     });
   });
 

test/api/model/events_test.dart

@@ -653,13 +653,7 @@ UpdateMessageEvent updateMessageEditEvent(
     messageIds: [messageId],
     flags: flags ?? origMessage.flags,
     editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp
-    moveData: UpdateMessageMoveData(
-      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',
@@ -694,10 +688,10 @@ UpdateMessageEvent _updateMessageMoveEvent(
     editTimestamp: 1234567890, // TODO generate timestamp
     moveData: UpdateMessageMoveData(
       origStreamId: origStreamId,
-      newStreamId: newStreamId,
+      newStreamId: newStreamId ?? origStreamId,
       propagateMode: propagateMode,
       origTopic: origTopic,
-      newTopic: newTopic,
+      newTopic: newTopic ?? origTopic,
     ),
     origContent: origContent,
     origRenderedContent: origContent,