Skip to content

local echo (1/n): Add API support for local_id/local_message_id and queue_id #1454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 3, 2025
Merged

local echo (1/n): Add API support for local_id/local_message_id and queue_id #1454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
450d476
api [nfc]: Use JsonSerializable with MessageEvent
PIG208 Mar 25, 2025
1c077cc
test [nfc]: Switch from store.handleEvent to store.addMessage(s)
PIG208 Apr 2, 2025
e9eabdc
test [nfc]: Add messageEvent
PIG208 Mar 25, 2025
c982795
api: Use RawParameter when passing queue_id and local_id
PIG208 Mar 26, 2025
d224a5b
api: Add localMessageId to MessageEvent
PIG208 Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions lib/api/model/events.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,8 +665,7 @@ class UserTopicEvent extends Event {
}

/// A Zulip event of type `message`: https://zulip.com/api/get-events#message
// TODO use [JsonSerializable] here too, using its customization features,
// in order to skip the boilerplate in [fromJson] and [toJson].
Comment on lines -668 to -669
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, nice to have taken care of this.

@JsonSerializable(fieldRename: FieldRename.snake)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit-message nits:

It will make it easier to add addditional fields on the event later

needs period (commit-message bodies use complete sentences); and s/ddd/dd/

class MessageEvent extends Event {
@override
@JsonKey(includeToJson: true)
Expand All @@ -680,24 +679,30 @@ class MessageEvent extends Event {
// events and in the get-messages results is that `matchContent` and
// `matchTopic` are absent here. Already [Message.matchContent] and
// [Message.matchTopic] are optional, so no action is needed on that.
@JsonKey(readValue: _readMessageValue, includeToJson: false)
final Message message;

MessageEvent({required super.id, required this.message});
// When present, this equals the "local_id" parameter
// from a previous [sendMessage] call by us.
//
// This is not yet fully documented. See CZO discussion for reference:
// https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/local_id.2C.20queue_id.2Fsender_queue_id/near/2135340
final String? localMessageId;

MessageEvent({required super.id, required this.message, required this.localMessageId});

static Map<String, dynamic> _readMessageValue(Map<dynamic, dynamic> json, String key) =>
{...json['message'] as Map<String, dynamic>, 'flags': json['flags']};

factory MessageEvent.fromJson(Map<String, dynamic> json) => MessageEvent(
id: json['id'] as int,
message: Message.fromJson({
...json['message'] as Map<String, dynamic>,
'flags': (json['flags'] as List<dynamic>).map((e) => e as String).toList(),
}),
);
factory MessageEvent.fromJson(Map<String, dynamic> json) =>
_$MessageEventFromJson(json);

@override
Map<String, dynamic> toJson() {
final messageJson = message.toJson();
final flags = messageJson['flags'];
messageJson.remove('flags');
return {'id': id, 'type': type, 'message': messageJson, 'flags': flags};
return {..._$MessageEventToJson(this), 'message': messageJson, 'flags': flags};
}
}

Expand Down
15 changes: 15 additions & 0 deletions lib/api/model/events.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions lib/api/route/messages.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,8 @@ Future<SendMessageResult> sendMessage(
'to': destination.userIds,
}}),
'content': RawParameter(content),
if (queueId != null) 'queue_id': queueId, // TODO should this use RawParameter?
if (localId != null) 'local_id': localId, // TODO should this use RawParameter?
if (queueId != null) 'queue_id': RawParameter(queueId),
if (localId != null) 'local_id': RawParameter(localId),
Comment on lines -196 to +197
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to localId here on this route is now totally parallel to the change to queueId, so probably best done in the same commit.

Then the change adding localMessageId on the event can remain in its own subsequent commit.

if (readBySender != null) 'read_by_sender': readBySender,
},
overrideUserAgent: switch ((supportsReadBySender, readBySender)) {
Expand Down
1 change: 1 addition & 0 deletions test/api/model/events_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ extension SubscriptionUpdateEventChecks on Subject<SubscriptionUpdateEvent> {

extension MessageEventChecks on Subject<MessageEvent> {
Subject<Message> get message => has((e) => e.message, 'message');
Subject<String?> get localMessageId => has((e) => e.localMessageId, 'localMessageId');
}

extension UpdateMessageEventChecks on Subject<UpdateMessageEvent> {
Expand Down
4 changes: 2 additions & 2 deletions test/api/route/messages_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,8 @@ void main() {
'to': streamId.toString(),
'topic': topic,
'content': content,
'queue_id': '"abc:123"',
'local_id': '"456"',
'queue_id': 'abc:123',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is this really NFC?

api [nfc]: Use RawParameter when passing queue_id

Seems like it changes what we'll actually send to the server.

I guess it's NFC for the actual app, in that we don't currently pass this parameter in calls to sendMessage. Still I'd call this just api:, not api [nfc]:, because it's definitely a functional change to the API bindings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

Strings in general should not be JSON-encoded in Zulip APIs.

It's true that a string as a top-level parameter to a Zulip API endpoint is typically sent as itself, not JSON-encoded. That isn't always true, though. Right in this same endpoint, when passing a channel name to to, that's expected to be as JSON and not a raw string.

(The server does currently accept a raw stream name there, apparently, but that isn't and fundamentally can't be reliable.)

Moreover our docs don't really cover where this is the case or isn't, except via the curl examples; and the curl example for this endpoint doesn't have queue_id.

So for this change, I think the key point is that you've tested and the raw string is what works.

'local_id': '456',
'read_by_sender': 'true',
});
});
Expand Down
3 changes: 3 additions & 0 deletions test/example_data.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,9 @@ UserTopicEvent userTopicEvent(
);
}

MessageEvent messageEvent(Message message) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit-message nit:

example_data [nfc]: Add messageEvent

Better:

test [nfc]: Add eg.messageEvent

For examples of previous summary lines, try this command:

$ git log --format='%>(20)%an %cs %h %s' --grep 'eg\.|example' origin \
    | grep -P '\bexample|\beg\b'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I added a -E flag to the git log command to get this working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, thanks. I have this in my ~/.gitconfig, which I recommend:

[grep]
        # "Basic" regexps are a relic.  Extended would be fine; Perl is better.
        patternType = perl

That's equivalent to a -P flag, which goes farther in the same direction as -E.

MessageEvent(id: 0, message: message, localMessageId: null);

DeleteMessageEvent deleteMessageEvent(List<StreamMessage> messages) {
assert(messages.isNotEmpty);
final streamId = messages.first.streamId;
Expand Down
65 changes: 30 additions & 35 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,8 +367,7 @@ void main() {
List.generate(30, (i) => eg.streamMessage(stream: stream)));

check(model).messages.length.equals(30);
await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(stream: stream)));
await store.addMessage(eg.streamMessage(stream: stream));
checkNotifiedOnce();
check(model).messages.length.equals(31);
});
Expand All @@ -381,17 +380,15 @@ void main() {

check(model).messages.length.equals(30);
final otherStream = eg.stream();
await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(stream: otherStream)));
await store.addMessage(eg.streamMessage(stream: otherStream));
checkNotNotified();
check(model).messages.length.equals(30);
});

test('MessageEvent, before fetch', () async {
final stream = eg.stream();
await prepare(narrow: ChannelNarrow(stream.streamId));
await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(stream: stream)));
await store.addMessage(eg.streamMessage(stream: stream));
checkNotNotified();
check(model).fetched.isFalse();
});
Expand Down Expand Up @@ -1314,7 +1311,7 @@ void main() {
}

final message = eg.streamMessage(stream: stream, topic: 'hello');
await store.handleEvent(MessageEvent(id: 0, message: message));
await store.addMessage(message);

await store.handleEvent(
eg.reactionEvent(eg.unicodeEmojiReaction, ReactionOp.add, message.id));
Expand Down Expand Up @@ -1396,8 +1393,7 @@ void main() {
await prepare(narrow: ChannelNarrow(stream.streamId));
await prepareMessages(foundOldest: true, messages:
List.generate(30, (i) => eg.streamMessage(stream: stream)));
await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(stream: stream)));
await store.addMessage(eg.streamMessage(stream: stream));
checkNotifiedOnce();
check(model).messages.length.equals(31);

Expand Down Expand Up @@ -1454,28 +1450,28 @@ void main() {
.deepEquals(expected..insertAll(0, [101, 103, 105]));

// … and on MessageEvent.
await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(id: 301, stream: stream1, topic: 'A')));
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 301, stream: stream1, topic: 'A')));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301));

await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(id: 302, stream: stream1, topic: 'B')));
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 302, stream: stream1, topic: 'B')));
checkNotNotified();
check(model.messages.map((m) => m.id)).deepEquals(expected);

await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(id: 303, stream: stream2, topic: 'C')));
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 303, stream: stream2, topic: 'C')));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(303));

await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(id: 304, stream: stream2, topic: 'D')));
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 304, stream: stream2, topic: 'D')));
checkNotNotified();
check(model.messages.map((m) => m.id)).deepEquals(expected);

await store.handleEvent(MessageEvent(id: 0,
message: eg.dmMessage(id: 305, from: eg.otherUser, to: [eg.selfUser])));
await store.handleEvent(eg.messageEvent(
eg.dmMessage(id: 305, from: eg.otherUser, to: [eg.selfUser])));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(305));
});
Expand Down Expand Up @@ -1511,18 +1507,18 @@ void main() {
.deepEquals(expected..insertAll(0, [101, 102]));

// … and on MessageEvent.
await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(id: 301, stream: stream, topic: 'A')));
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 301, stream: stream, topic: 'A')));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301));

await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(id: 302, stream: stream, topic: 'B')));
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 302, stream: stream, topic: 'B')));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(302));

await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(id: 303, stream: stream, topic: 'C')));
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 303, stream: stream, topic: 'C')));
checkNotNotified();
check(model.messages.map((m) => m.id)).deepEquals(expected);
});
Expand Down Expand Up @@ -1553,8 +1549,8 @@ void main() {
.deepEquals(expected..insertAll(0, [101]));

// … and on MessageEvent.
await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(id: 301, stream: stream, topic: 'A')));
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 301, stream: stream, topic: 'A')));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301));
});
Expand Down Expand Up @@ -1593,7 +1589,7 @@ void main() {
// … and on MessageEvent.
final messages = getMessages(301);
for (var i = 0; i < 3; i += 1) {
await store.handleEvent(MessageEvent(id: 0, message: messages[i]));
await store.handleEvent(eg.messageEvent(messages[i]));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301 + i));
}
Expand Down Expand Up @@ -1631,7 +1627,7 @@ void main() {
// … and on MessageEvent.
final messages = getMessages(301);
for (var i = 0; i < 2; i += 1) {
await store.handleEvent(MessageEvent(id: 0, message: messages[i]));
await store.handleEvent(eg.messageEvent(messages[i]));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301 + i));
}
Expand All @@ -1644,8 +1640,7 @@ void main() {
await prepare(narrow: ChannelNarrow(stream.streamId));
await prepareMessages(foundOldest: true, messages: []);

await store.handleEvent(MessageEvent(id: 0,
message: eg.streamMessage(stream: stream)));
await store.addMessage(eg.streamMessage(stream: stream));
// Each [checkNotifiedOnce] call ensures there's been a [checkInvariants]
// call, where the [ContentNode] gets checked. The additional checks to
// make this test explicit.
Expand All @@ -1659,13 +1654,13 @@ void main() {
await prepare(narrow: ChannelNarrow(stream.streamId));
await prepareMessages(foundOldest: true, messages: []);

await store.handleEvent(MessageEvent(id: 0, message: eg.streamMessage(
await store.addMessage(eg.streamMessage(
stream: stream,
sender: eg.selfUser,
submessages: [
eg.submessage(senderId: eg.selfUser.userId,
content: eg.pollWidgetData(question: 'question', options: ['A'])),
])));
]));
// Each [checkNotifiedOnce] call ensures there's been a [checkInvariants]
// call, where the value of the [Poll] gets checked. The additional
// checks make this test explicit.
Expand Down Expand Up @@ -1723,11 +1718,11 @@ void main() {
checkNotified(count: 2);

// Then test MessageEvent, where a new header is needed…
await store.handleEvent(MessageEvent(id: 0, message: streamMessage(13)));
await store.handleEvent(eg.messageEvent(streamMessage(13)));
checkNotifiedOnce();

// … and where it's not.
await store.handleEvent(MessageEvent(id: 0, message: streamMessage(14)));
await store.handleEvent(eg.messageEvent(streamMessage(14)));
checkNotifiedOnce();

// Then test UpdateMessageEvent edits, where a header is and remains needed…
Expand Down
14 changes: 6 additions & 8 deletions test/model/message_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@ void main() {
}

Future<void> addMessages(Iterable<Message> messages) async {
for (final m in messages) {
await store.handleEvent(MessageEvent(id: 0, message: m));
}
await store.addMessages(messages);
checkNotified(count: messageList.fetched ? messages.length : 0);
}

Expand Down Expand Up @@ -131,7 +129,7 @@ void main() {
check(store.messages).isEmpty();

final newMessage = eg.streamMessage();
await store.handleEvent(MessageEvent(id: 1, message: newMessage));
await store.handleEvent(eg.messageEvent(newMessage));
check(store.messages).deepEquals({
newMessage.id: newMessage,
});
Expand All @@ -150,7 +148,7 @@ void main() {
});

final newMessage = eg.streamMessage();
await store.handleEvent(MessageEvent(id: 1, message: newMessage));
await store.handleEvent(eg.messageEvent(newMessage));
check(store.messages).deepEquals({
for (final m in messages) m.id: m,
newMessage.id: newMessage,
Expand All @@ -164,7 +162,7 @@ void main() {
check(store.messages).deepEquals({1: message});

final newMessage = eg.streamMessage(id: 1, content: '<p>bar</p>');
await store.handleEvent(MessageEvent(id: 1, message: newMessage));
await store.handleEvent(eg.messageEvent(newMessage));
check(store.messages).deepEquals({1: newMessage});
});
});
Expand Down Expand Up @@ -861,7 +859,7 @@ void main() {
]);

await prepare();
await store.handleEvent(MessageEvent(id: 0, message: message));
await store.handleEvent(eg.messageEvent(message));
}

test('smoke', () async {
Expand Down Expand Up @@ -932,7 +930,7 @@ void main() {
),
]);
await prepare();
await store.handleEvent(MessageEvent(id: 0, message: message));
await store.handleEvent(eg.messageEvent(message));
check(store.messages[message.id]).isNotNull().poll.isNull();
});
});
Expand Down
Loading