Skip to content

Commit 6b75bae

Browse files
PIG208gnprice
PIG208
authored and
gnprice
committed
wip 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. This is NFC for debug builds because we were using assertions before, and now start to throw `FormatException`s with the parsing code. Signed-off-by: Zixuan James Li <zixuan@zulip.com>
1 parent 4392beb commit 6b75bae

File tree

9 files changed

+232
-96
lines changed

9 files changed

+232
-96
lines changed

‎lib/api/model/events.dart‎

Lines changed: 85 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -718,16 +718,8 @@ class UpdateMessageEvent extends Event {
718718

719719
// final String? streamName; // ignore
720720

721-
@JsonKey(name: 'stream_id')
722-
final int? origStreamId;
723-
final int? newStreamId;
724-
725-
final PropagateMode? propagateMode;
726-
727-
@JsonKey(name: 'orig_subject')
728-
final TopicName? origTopic;
729-
@JsonKey(name: 'subject')
730-
final TopicName? newTopic;
721+
@JsonKey(readValue: _readMoveData, fromJson: UpdateMessageMoveData.tryParseFromJson, includeToJson: false)
722+
final UpdateMessageMoveData? moveData;
731723

732724
// final List<TopicLink> topicLinks; // TODO handle
733725

@@ -747,25 +739,103 @@ class UpdateMessageEvent extends Event {
747739
required this.messageIds,
748740
required this.flags,
749741
required this.editTimestamp,
750-
required this.origStreamId,
751-
required this.newStreamId,
752-
required this.propagateMode,
753-
required this.origTopic,
754-
required this.newTopic,
742+
required this.moveData,
755743
required this.origContent,
756744
required this.origRenderedContent,
757745
required this.content,
758746
required this.renderedContent,
759747
required this.isMeMessage,
760748
});
761749

750+
static Map<String, dynamic> _readMoveData(Map<dynamic, dynamic> json, String key) {
751+
// Parsing [UpdateMessageMoveData] requires `json`, not the default `json[key]`.
752+
assert(json is Map<String, dynamic>); // value came through `fromJson` with this type
753+
return json as Map<String, dynamic>;
754+
}
755+
762756
factory UpdateMessageEvent.fromJson(Map<String, dynamic> json) =>
763757
_$UpdateMessageEventFromJson(json);
764758

765759
@override
766760
Map<String, dynamic> toJson() => _$UpdateMessageEventToJson(this);
767761
}
768762

763+
/// Data structure representing a message move.
764+
class UpdateMessageMoveData {
765+
final int origStreamId;
766+
final int newStreamId;
767+
768+
final PropagateMode propagateMode;
769+
770+
final TopicName origTopic;
771+
final TopicName newTopic;
772+
773+
UpdateMessageMoveData({
774+
required this.origStreamId,
775+
required this.newStreamId,
776+
required this.propagateMode,
777+
required this.origTopic,
778+
required this.newTopic,
779+
}) : assert(origStreamId != newStreamId || origTopic != newTopic);
780+
781+
/// Try to extract [UpdateMessageMoveData] from the JSON object for an
782+
/// [UpdateMessageEvent].
783+
///
784+
/// Returns `null` if there was no message move.
785+
///
786+
/// Throws an error if the data is malformed.
787+
// When parsing this, 'stream_id', which is also present when there was only
788+
// a content edit, cannot be recovered if this ends up returning `null`.
789+
// This may matter if we ever need 'stream_id' when no message move occurred.
790+
static UpdateMessageMoveData? tryParseFromJson(Map<String, Object?> json) {
791+
final origStreamId = (json['stream_id'] as num?)?.toInt();
792+
final newStreamIdRaw = (json['new_stream_id'] as num?)?.toInt();
793+
final newStreamId = newStreamIdRaw ?? origStreamId;
794+
795+
final propagateModeString = json['propagate_mode'] as String?;
796+
final propagateMode = propagateModeString == null ? null
797+
: PropagateMode.fromRawString(propagateModeString);
798+
799+
final origTopic = json['orig_subject'] == null ? null
800+
: TopicName.fromJson(json['orig_subject'] as String);
801+
final newTopicRaw = json['subject'] == null ? null
802+
: TopicName.fromJson(json['subject'] as String);
803+
final newTopic = newTopicRaw ?? origTopic;
804+
805+
if (origTopic == newTopic && origStreamId == newStreamId) {
806+
if (propagateMode != null) {
807+
throw FormatException(
808+
'Malformed UpdateMessageEvent: incoherent message-move fields; '
809+
'propagate_mode present but no new channel or topic');
810+
}
811+
return null;
812+
}
813+
814+
if (origStreamId == null || newStreamId == null) {
815+
// The `stream_id` field (aka origStreamId) is documented to be present on moves;
816+
// newStreamId should not be null either because it falls back to origStreamId.
817+
throw FormatException('Malformed UpdateMessageEvent: move but no origStreamId');
818+
}
819+
if (origTopic == null || newTopic == null) {
820+
// The `orig_subject` field (aka origTopic) is documented to be present on moves;
821+
// newTopic should not be null either because it falls back to origTopic.
822+
throw FormatException('Malformed UpdateMessageEvent: move but no origTopic');
823+
}
824+
if (propagateMode == null) {
825+
// The `propagate_mode` field (aka propagateMode) is documented to be present on moves.
826+
throw FormatException('Malformed UpdateMessageEvent: move but no propagateMode');
827+
}
828+
829+
return UpdateMessageMoveData(
830+
origStreamId: origStreamId,
831+
newStreamId: newStreamId,
832+
propagateMode: propagateMode,
833+
origTopic: origTopic,
834+
newTopic: newTopic,
835+
);
836+
}
837+
}
838+
769839
/// A Zulip event of type `delete_message`: https://zulip.com/api/get-events#delete_message
770840
@JsonSerializable(fieldRename: FieldRename.snake)
771841
class DeleteMessageEvent extends Event {

‎lib/api/model/events.g.dart‎

Lines changed: 3 additions & 24 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎lib/model/message.dart‎

Lines changed: 5 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -169,37 +169,14 @@ class MessageStoreImpl with MessageStore {
169169
}
170170

171171
void _handleUpdateMessageEventMove(UpdateMessageEvent event) {
172-
// The interaction between the fields of these events are a bit tricky.
173-
// For reference, see: https://zulip.com/api/get-events#update_message
174-
175-
final origStreamId = event.origStreamId;
176-
final newStreamId = event.newStreamId ?? origStreamId;
177-
final origTopic = event.origTopic;
178-
final newTopic = event.newTopic ?? origTopic;
179-
final propagateMode = event.propagateMode;
180-
181-
if (newStreamId == origStreamId && newTopic == origTopic) {
172+
final messageMove = event.moveData;
173+
if (messageMove == null) {
182174
// There was no move.
183175
return;
184176
}
185177

186-
if (origStreamId == null || newStreamId == null) {
187-
// The `stream_id` field (aka origStreamId) is documented to be present on moves.
188-
// newStreamId should not be null either because it falls back to origStreamId.
189-
assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log)
190-
return;
191-
}
192-
if (origTopic == null || newTopic == null) {
193-
// The `orig_subject` field (aka origTopic) is documented to be present on moves.
194-
// newTopic should not be null either because it falls back to origTopic.
195-
assert(debugLog('Malformed UpdateMessageEvent: move but no origTopic')); // TODO(log)
196-
return;
197-
}
198-
if (propagateMode == null) {
199-
// The `propagate_mode` field (aka propagateMode) is documented to be present on moves.
200-
assert(debugLog('Malformed UpdateMessageEvent: move but no propagateMode')); // TODO(log)
201-
return;
202-
}
178+
final UpdateMessageMoveData(
179+
:origStreamId, :newStreamId, :origTopic, :newTopic) = messageMove;
203180

204181
final wasResolveOrUnresolve = newStreamId == origStreamId
205182
&& MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic);
@@ -231,14 +208,7 @@ class MessageStoreImpl with MessageStore {
231208
}
232209

233210
for (final view in _messageListViews) {
234-
view.messagesMoved(
235-
origStreamId: origStreamId,
236-
newStreamId: newStreamId,
237-
origTopic: origTopic,
238-
newTopic: newTopic,
239-
messageIds: event.messageIds,
240-
propagateMode: propagateMode,
241-
);
211+
view.messagesMoved(messageMove: messageMove, messageIds: event.messageIds);
242212
}
243213
}
244214

‎lib/model/message_list.dart‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -720,13 +720,12 @@ class MessageListView with ChangeNotifier, _MessageSequence {
720720
}
721721

722722
void messagesMoved({
723-
required int origStreamId,
724-
required int newStreamId,
725-
required TopicName origTopic,
726-
required TopicName newTopic,
723+
required UpdateMessageMoveData messageMove,
727724
required List<int> messageIds,
728-
required PropagateMode propagateMode,
729725
}) {
726+
final UpdateMessageMoveData(
727+
:origStreamId, :newStreamId, :origTopic, :newTopic, :propagateMode,
728+
) = messageMove;
730729
switch (narrow) {
731730
case DmNarrow():
732731
// DMs can't be moved (nor created by moves),

‎test/api/model/events_checks.dart‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,18 +48,22 @@ extension UpdateMessageEventChecks on Subject<UpdateMessageEvent> {
4848
Subject<List<int>> get messageIds => has((e) => e.messageIds, 'messageIds');
4949
Subject<List<MessageFlag>> get flags => has((e) => e.flags, 'flags');
5050
Subject<int?> get editTimestamp => has((e) => e.editTimestamp, 'editTimestamp');
51-
Subject<int?> get origStreamId => has((e) => e.origStreamId, 'origStreamId');
52-
Subject<int?> get newStreamId => has((e) => e.newStreamId, 'newStreamId');
53-
Subject<PropagateMode?> get propagateMode => has((e) => e.propagateMode, 'propagateMode');
54-
Subject<TopicName?> get origTopic => has((e) => e.origTopic, 'origTopic');
55-
Subject<TopicName?> get newTopic => has((e) => e.newTopic, 'newTopic');
51+
Subject<UpdateMessageMoveData?> get moveData => has((e) => e.moveData, 'moveData');
5652
Subject<String?> get origContent => has((e) => e.origContent, 'origContent');
5753
Subject<String?> get origRenderedContent => has((e) => e.origRenderedContent, 'origRenderedContent');
5854
Subject<String?> get content => has((e) => e.content, 'content');
5955
Subject<String?> get renderedContent => has((e) => e.renderedContent, 'renderedContent');
6056
Subject<bool?> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
6157
}
6258

59+
extension UpdateMessageMoveDataChecks on Subject<UpdateMessageMoveData> {
60+
Subject<int> get origStreamId => has((e) => e.origStreamId, 'origStreamId');
61+
Subject<int> get newStreamId => has((e) => e.newStreamId, 'newStreamId');
62+
Subject<PropagateMode> get propagateMode => has((e) => e.propagateMode, 'propagateMode');
63+
Subject<TopicName> get origTopic => has((e) => e.origTopic, 'origTopic');
64+
Subject<TopicName> get newTopic => has((e) => e.newTopic, 'newTopic');
65+
}
66+
6367
extension DeleteMessageEventChecks on Subject<DeleteMessageEvent> {
6468
Subject<MessageType?> get messageType => has((e) => e.messageType, 'messageType');
6569
}

‎test/api/model/events_test.dart‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,11 +106,26 @@ void main() {
106106
'propagate_mode': 'change_all',
107107
};
108108

109+
test('smoke moveData', () {
110+
check(Event.fromJson({ ...baseMoveJson,
111+
'stream_id': 1,
112+
'new_stream_id': 2,
113+
'orig_subject': 'foo',
114+
'subject': 'bar',
115+
'propagate_mode': 'change_all',
116+
})).isA<UpdateMessageEvent>().moveData.isNotNull()
117+
..origStreamId.equals(1)
118+
..newStreamId.equals(2)
119+
..origTopic.equals(const TopicName('foo'))
120+
..newTopic.equals(const TopicName('bar'))
121+
..propagateMode.equals(PropagateMode.changeAll);
122+
});
123+
109124
test('stream_id -> origStreamId', () {
110125
check(Event.fromJson({ ...baseMoveJson,
111126
'stream_id': 1,
112127
'new_stream_id': 2,
113-
})).isA<UpdateMessageEvent>()
128+
})).isA<UpdateMessageEvent>().moveData.isNotNull()
114129
..origStreamId.equals(1)
115130
..newStreamId.equals(2);
116131
});
@@ -119,10 +134,34 @@ void main() {
119134
check(Event.fromJson({ ...baseMoveJson,
120135
'orig_subject': 'foo',
121136
'subject': 'bar',
122-
})).isA<UpdateMessageEvent>()
137+
})).isA<UpdateMessageEvent>().moveData.isNotNull()
123138
..origTopic.equals(const TopicName('foo'))
124139
..newTopic.equals(const TopicName('bar'));
125140
});
141+
142+
test('new channel, same topic: fill in newTopic', () {
143+
// The server omits 'subject' in this situation.
144+
check(Event.fromJson({ ...baseMoveJson,
145+
'orig_subject': 'foo',
146+
'subject': null,
147+
'stream_id': 1,
148+
'new_stream_id': 2,
149+
})).isA<UpdateMessageEvent>().moveData.isNotNull()
150+
..origTopic.equals(const TopicName('foo'))
151+
..newTopic.equals(const TopicName('foo'));
152+
});
153+
154+
test('same channel, new topic; fill in newStreamId', () {
155+
// The server omits 'new_stream_id' in this situation.
156+
check(Event.fromJson({ ...baseMoveJson,
157+
'orig_subject': 'foo',
158+
'subject': 'bar',
159+
'stream_id': 1,
160+
'new_stream_id': null,
161+
})).isA<UpdateMessageEvent>().moveData.isNotNull()
162+
..origStreamId.equals(1)
163+
..newStreamId.equals(1);
164+
});
126165
});
127166

128167
test('delete_message: require streamId and topic for stream messages', () {

‎test/api/model/model_checks.dart‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ extension TopicNameChecks on Subject<TopicName> {
5353

5454
extension StreamMessageChecks on Subject<StreamMessage> {
5555
Subject<String?> get displayRecipient => has((e) => e.displayRecipient, 'displayRecipient');
56+
Subject<int> get streamId => has((e) => e.streamId, 'streamId');
5657
Subject<TopicName> get topic => has((e) => e.topic, 'topic');
5758
}
5859

‎test/example_data.dart‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -655,11 +655,7 @@ UpdateMessageEvent updateMessageEditEvent(
655655
messageIds: [messageId],
656656
flags: flags ?? origMessage.flags,
657657
editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp
658-
origStreamId: origMessage is StreamMessage ? origMessage.streamId : null,
659-
newStreamId: null,
660-
propagateMode: null,
661-
origTopic: null,
662-
newTopic: null,
658+
moveData: null,
663659
origContent: 'some probably-mismatched old Markdown',
664660
origRenderedContent: origMessage.content,
665661
content: 'some probably-mismatched new Markdown',
@@ -692,11 +688,13 @@ UpdateMessageEvent _updateMessageMoveEvent(
692688
messageIds: messageIds,
693689
flags: flags,
694690
editTimestamp: 1234567890, // TODO generate timestamp
695-
origStreamId: origStreamId,
696-
newStreamId: newStreamId,
697-
propagateMode: propagateMode,
698-
origTopic: origTopic,
699-
newTopic: newTopic,
691+
moveData: UpdateMessageMoveData(
692+
origStreamId: origStreamId,
693+
newStreamId: newStreamId ?? origStreamId,
694+
propagateMode: propagateMode,
695+
origTopic: origTopic,
696+
newTopic: newTopic ?? origTopic,
697+
),
700698
origContent: origContent,
701699
origRenderedContent: origContent,
702700
content: newContent,

0 commit comments

Comments
 (0)