api: Add getSingleMessage binding for GET messages/{message_id} #5465

chrisbobbe · 2022-08-11T22:52:22Z

We'll use this for #5306; see the plan in discussion:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930

In particular, we want the stream and topic for a stream message
that we fetch using this endpoint (which we'd do as a fallback for
when we don't already have the message in our data structures.)

gnprice · 2022-08-27T00:21:17Z

src/api/messages/getSingleMessage.js

+export type ServerReaction = {|
+  +emoji_name: string,
+  +emoji_code: string,
+  +reaction_type: ReactionType,
+
+  // Our binding assumes FL 120 (see jsdoc), so we don't have to handle FLs
+  // <2 that don't provide `user_id`.
+  +user_id: UserId,
+
+  // Deprecated and "to be removed in a future release":
+  //   https://zulip.com/api/get-message
+  -user: $ReadOnly<{|
+    id: UserId,
+    email: string,
+    full_name: string,
+
+    // Not observed, so marked optional (doc says required)
+    is_mirror_dummy?: boolean,
+  |}>,
+|};
+
+/**
+ * The elements of Message.edit_history found in the actual server response.
+ */
+export type ServerMessageEdit = {|
+  // Our binding assumes FL 120 (see jsdoc), so we don't have to support
+  // the pre-FL 118 shape. This type is written accordingly.
+
+  +prev_content?: string,
+  +prev_rendered_content?: string,
+  +prev_rendered_content_version?: number,
+  +prev_stream?: number,
+  +prev_topic?: string,
+  +stream?: number,
+  +timestamp: number,
+  +topic?: string,
+  +user_id: UserId | null,
+|};
+
+// How `ServerMessage` relates to `Message`, in a way that applies
+// uniformly to `Message`'s subtypes.
+type ServerMessageOf<M: Message> = {|
+  ...$Exact<M>,
+
+  // Can be null. The server hard-codes client_gravatar:
+  //   https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60client_gravatar.60.20in.20.60messages.2F.7Bmessage_id.7D.60/near/1418337
+  +avatar_url: string | null,
+
+  +reactions: $ReadOnlyArray<ServerReaction>,
+
+  // Unlike Message['edit_history'], this can't be `null`.
+  +edit_history?: $ReadOnlyArray<ServerMessageEdit>,
+|};
+
+export type ServerMessage = ServerMessageOf<PmMessage> | ServerMessageOf<StreamMessage>;
+
+// The actual response from the server.  We convert the message to a proper
+// Message before returning it to application code.
+type ServerApiResponseSingleMessage = {|
+  ...$Exact<ApiResponseSuccess>,
+  -raw_content: string, // deprecated
+  +message: ServerMessage,
+|};
+
+/** Exported for tests only. */
+export const migrateMessage = (
+  message: ServerMessage,
+  identity: Identity,
+  allowEditHistory: boolean,
+): Message => ({
+  ...message,
+  avatar_url: AvatarURL.fromUserOrBotData({
+    rawAvatarUrl: message.avatar_url,
+    email: message.sender_email,
+    userId: message.sender_id,
+    realm: identity.realm,
+  }),
+  reactions: message.reactions.map(reaction => {
+    const { user_id, emoji_name, reaction_type, emoji_code } = reaction;
+    return { user_id, emoji_name, reaction_type, emoji_code };
+  }),
+
+  // Why condition on allowEditHistory? See MessageBase['edit_history'].
+  edit_history: allowEditHistory
+    ? (message.edit_history: $ReadOnlyArray<MessageEdit> | void)
+    : null,
+});


A lot of this seems to be in common with getMessages. What would it look like if we reused more of that, rather than duplicating it?

The main difference I see is that this endpoint only relevantly exists at FL ≥120, and so these types have some simplifications that reflect that. But as far as those are concerned, I think I'd rather have a single implementation that handles both old and new formats, rather than one that does that and a second one that's simplified by only handling the new.

Could well be another difference I'm missing, though. One reason this sort of duplication is costly is that it makes it harder to spot other differences 🙂

For how we type the data we expect from the server—ServerMessage—I think we first want to make it match the server API doc, and second want to make it easy to verify that it matches the server API doc. When some reused type feeds into ServerMessage here and ServerMessage in getMessages, there's some risk to both of those goals:

Maybe the reused type is correct for both places now, but the server isn't bound to keep that true forever. Also, looking closer at each of the two ServerMessage types against its respective API doc: even where a reused component is accurate, it's sometimes silly how imprecise it is, because we've reused it in even more places. See ServerMessage['flags'] in either file.

For that ServerMessage['flags'] example, note how much effort it takes to actually decipher what precise type is intended, to compare it with the API doc. Command-clicking through in the IDE, and then reading a pretty old comment… In fact, it isn't even explicit: in that comment, we just assume that the MESSAGE_FETCH_COMPLETE case applies, based on our background knowledge.

On the other hand, I do know that message objects are complex to type, hmm…

It's true there are potential trade-offs in trying to deduplicate this code.

I am interested in understanding what the differences are, though. That's partly because for each difference, there's a good chance that one or the other side is a bug. (Or potentially a problem in the API itself.) Do you know whether there are other differences?

Makes sense; I'll list the differences below. 🙂

Thinking about why I've been cautious about abstracting too far away from literal details of what we expect from the API (as in an API doc), I'm reminded of a time I thought I'd found a model that applied equally to two things from the server, but then we studied the API more closely and found it was wrong. In particular, a user status in the initial data, and a user status in the update-user-status event. In fact, I think I had one wrong model after another:

First wrong model: they're both a complete user status object.

Second wrong model: they're both an update from the then-current user status.

Rather, the event holds an update from the then-current user status, and the initial data holds an update from the "zero" value. See discussion where you found a nice way to model that accurately.

And see an instance where I think we might again be tempted to reach for something reusable, UserTopic, but it turns out to be wrong (as in my first wrong model, above).

Anyway, in this case it's simpler because these are both "fetch data" endpoints that are outside the event system. I acknowledge that it really would be surprising for a server to send differently shaped messages between GET /messages and GET /messages/{message_id}, and I don't think that's likely to change after studying the API more closely.

Anyway, I think these are the differences between ServerMessage in getMessage and my new getSingleMessage file, excluding the FL >=120 simplifications:

The use of individual +s vs. $ReadOnly<…>, causing typesEquivalent to complain of one property being writable while its counterpart isn't. I think typesEquivalent might be wrong in these instances, but I don't know, and it's probably worth erasing these differences anyway.

I marked ServerReaction['user'] as write-only in getSingleMessage, so we don't try to read that deprecated property. This should apply to both ServerReactions, I'm sure.

I added ServerReaction['user']['is_mirror_dummy'] in getSingleMessage, because I saw it in the relevant doc. Seeing now that is_mirror_dummy is also documented in GET /messages, we should probably add it in getMessages.

I made this optional in getSingleMessage, despite the doc saying it's required, because I observed it being absent. We could also mark it optional in the other ServerReaction, if we expect it to be optional there too, or if we test and find that it's sometimes absent there too, or just because optional would still be accurate, if imprecise, if it turns out to be required there.

In both files, ServerMessage['avatar_url'] is string | null.

In getMessages, IIUC, the | null is because we send client_gravatar: true to servers FL <92, and because we don't send client_gravatar: false for servers FL >=92.

In getSingleMessage, the server, which we know is FL >=120 since we're receiving this data, acts like it has acted for getMessages since FL 92—except that it doesn't actually accept a client_gravatar param. So the presence of | null there isn't conditional on anything. See discussion.

Thanks for those details!

Yeah, it sounds like it'd be good to deduplicate these. We just discussed in the office, and a file like src/api/rawModelTypes.js would be a good home for ServerMessage and the associated types and conversions.

…e, etc. getMessages is a binding for the GET /messages endpoint. We're about to add a binding for the GET /message/{message_id} endpoint, which since zulip/zulip@82837304e (FL 120) has given us a full message object in the same shape. For both bindings, we want: (a) a type to describe the data given by the server to represent a message (ServerMessage before this commit), and (b) a function to transform that data into our Message type (migrateMessage before this commit). And it's good to deduplicate both (a) and (b), in part because any differences would be surprising and warrant investigation; see discussion: zulip#5465 (comment) The old code was already clear that it was about fetched data, not data from events, because it was in the file that defined api.getMessages. Keep that meaning intact by making the names more specific: ServerMessage → FetchedMessage (the "server" part is clear from the new file's name) migrateMessage → transformFetchedMessage (the orthogonal change from "migrate" to "transform" should help prevent confusion with src/storage/migrations.js, which has nothing to do with what this function does.) It's useful to remember that this code was developed specifically for these fetch-message(s) endpoints, e.g., for a future investigation aiming to deduplicate most of the corresponding 'message'-event code with this code. (That last bit reminds me that we don't even have a deliberate type for how we expect servers to represent a message in 'message' events. I'll write a TODO for that in the next commit.)

chrisbobbe · 2022-09-07T01:14:04Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision!

This looks good -- just small comments below. Then please merge at will.

gnprice · 2022-09-08T00:20:46Z

src/api/rawModelTypes.js

+  edit_history?: $ReadOnlyArray<FetchedMessageEdit>,
+|}>;
+
+export type FetchedMessage = FetchedMessageOf<PmMessage> | FetchedMessageOf<StreamMessage>;


The old code was already clear that it was about fetched data, not data from events, because it was in the file that defined api.getMessages. Keep that meaning intact by making the names more specific: ServerMessage → FetchedMessage (the "server" part is clear from the new file's name)

I like this naming idea!

gnprice · 2022-09-08T00:21:00Z

src/api/messages/getSingleMessage.js

+  -raw_content: string, // deprecated
+
+  // Until we narrow FetchedMessage into its FL 120+ form, FetchedMessage
+  // will be a bit less precise that we could be here. That's because we


nit: s/that we/than we/

gnprice · 2022-09-08T00:21:32Z

src/api/messages/getSingleMessage.js

+ * Gives undefined if the `message` field is missing, which it will be for
+ *   FL <120.


nit: no extra indentation; this is just a normal paragraph

gnprice · 2022-09-08T00:22:49Z

src/api/messages/getSingleMessage.js

+  auth: Auth,
+  args: {|
+    +message_id: number,
+    +apply_markdown: boolean,


We just hardcode this to true in the actual request, right? So we can drop it from the function's parameters.

Ah, yeah, right.

Also, I noticed we were accidentally passing client_gravatar: true as a param to apiGet, which this endpoint doesn't accept, as mentioned: #5465 (comment) so I stopped doing that.

This could be easy to miss when we address the TODO(server-5.0) on ServerMessageEdit.

…ssage

…e, etc. getMessages is a binding for the GET /messages endpoint. We're about to add a binding for the GET /message/{message_id} endpoint, which since zulip/zulip@82837304e (FL 120) has given us a full message object in the same shape. For both bindings, we want: (a) a type to describe the data given by the server to represent a message (ServerMessage before this commit), and (b) a function to transform that data into our Message type (migrateMessage before this commit). And it's good to deduplicate both (a) and (b), in part because any differences would be surprising and warrant investigation; see discussion: zulip#5465 (comment) The old code was already clear that it was about fetched data, not data from events, because it was in the file that defined api.getMessages. Keep that meaning intact by making the names more specific: ServerMessage → FetchedMessage (the "server" part is clear from the new file's name) migrateMessage → transformFetchedMessage (the orthogonal change from "migrate" to "transform" should help prevent confusion with src/storage/migrations.js, which has nothing to do with what this function does.) It's useful to remember that this code was developed specifically for these fetch-message(s) endpoints, e.g., for a future investigation aiming to deduplicate most of the corresponding 'message'-event code with this code. (That last bit reminds me that we don't even have a deliberate type for how we expect servers to represent a message in 'message' events. I'll write a TODO for that in the next commit.)

… data

We'll use this for zulip#5306; see the plan in discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5306.20Follow.20.2Fnear.2F.20links.20through.20topic.20moves.2Frenames/near/1407930 In particular, we want the stream and topic for a stream message that might not be in our data structures. We'll use this endpoint to fetch that information.

chrisbobbe · 2022-09-13T01:32:56Z

Thanks for the revision!

This looks good -- just small comments below. Then please merge at will.

Thanks for the review! Done.

chrisbobbe added P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release. labels Aug 11, 2022

chrisbobbe requested a review from gnprice August 11, 2022 22:52

chrisbobbe force-pushed the pr-get-single-message-binding branch from 4fee291 to 8c17242 Compare August 11, 2022 22:54

gnprice reviewed Aug 27, 2022

View reviewed changes

chrisbobbe force-pushed the pr-get-single-message-binding branch from 8c17242 to d4fb353 Compare September 6, 2022 20:46

chrisbobbe marked this pull request as draft September 6, 2022 20:56

chrisbobbe force-pushed the pr-get-single-message-binding branch from d4fb353 to 598ec8d Compare September 7, 2022 01:14

chrisbobbe marked this pull request as ready for review September 7, 2022 01:14

gnprice reviewed Sep 8, 2022

View reviewed changes

chrisbobbe added 6 commits September 12, 2022 17:48

fetchActions tests [nfc]: Remove a TODO about jest.mocking a module

505f78f

api types [nfc]: Comment on subtlety in ServerMessageOf['edit_history']

9f75da8

This could be easy to miss when we address the TODO(server-5.0) on ServerMessageEdit.

getMessages [nfc]: Change migrate-messages function to just do one me…

8689ab8

…ssage

eventTypes [nfc]: Comment on MessageEvent not representing raw server…

04d2c95

… data

chrisbobbe force-pushed the pr-get-single-message-binding branch from 598ec8d to c35b883 Compare September 13, 2022 00:57

chrisbobbe merged commit c35b883 into zulip:main Sep 13, 2022

chrisbobbe deleted the pr-get-single-message-binding branch September 13, 2022 01:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

api: Add getSingleMessage binding for GET messages/{message_id} #5465

api: Add getSingleMessage binding for GET messages/{message_id} #5465

Uh oh!

chrisbobbe commented Aug 11, 2022

Uh oh!

gnprice Aug 27, 2022

Uh oh!

chrisbobbe Aug 27, 2022 •

edited

Loading

Uh oh!

gnprice Aug 31, 2022

Uh oh!

chrisbobbe Sep 2, 2022 •

edited

Loading

Uh oh!

gnprice Sep 2, 2022

Uh oh!

chrisbobbe commented Sep 7, 2022

Uh oh!

gnprice left a comment

Uh oh!

gnprice Sep 8, 2022

Uh oh!

gnprice Sep 8, 2022

Uh oh!

gnprice Sep 8, 2022

Uh oh!

gnprice Sep 8, 2022

Uh oh!

chrisbobbe Sep 13, 2022 •

edited

Loading

Uh oh!

chrisbobbe commented Sep 13, 2022

Uh oh!

Uh oh!

		* Gives undefined if the `message` field is missing, which it will be for
		* FL <120.

Uh oh!

api: Add getSingleMessage binding for GET messages/{message_id} #5465

api: Add getSingleMessage binding for GET messages/{message_id} #5465

Uh oh!

Conversation

chrisbobbe commented Aug 11, 2022

Uh oh!

gnprice Aug 27, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Sep 7, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Sep 13, 2022

Uh oh!

Uh oh!

chrisbobbe Aug 27, 2022 •

edited

Loading

chrisbobbe Sep 2, 2022 •

edited

Loading

chrisbobbe Sep 13, 2022 •

edited

Loading