api: Add update_message and delete_message events

[chrisbobbe]

No description provided.

[gnprice]

Thanks! Comments below.

[gnprice]

Suggested change

	case 'delete_message': return UpdateMessageEvent.fromJson(json);
	case 'delete_message': return DeleteMessageEvent.fromJson(json);

🙂

[chrisbobbe]

Eep! Thanks for the catch.

[gnprice]

Suggested change

	final List<String> flags;
	final List<String> flags; // TODO enum

like on Message

[gnprice]

Suggested change

	final String? propagateMode;
	final String? propagateMode; // TODO enum

Or maybe just make the enum.

[gnprice]

More precisely, the elements of this list will be Map<String, dynamic>. (More precisely than Object, that is.)

But if you don't feel like writing down the details, probably best to leave the whole field for later, like in Message:

Suggested change

	final List<Object>? topicLinks; // TODO shape of objects
	// final List<TopicLink> topicLinks; // TODO handle

In fact, given that we leave the field out in Message, probably best to leave it out here too and leave that work for a single follow-up task across Message and these events.

[gnprice]

Hmm, the docs say "Clients should ignore this field."

I'd take that as a deprecation, even though it doesn't use the word "deprecated":

Suggested change

	final int? prevRenderedContentVersion;
	// final int? prevRenderedContentVersion; // deprecated

[gnprice]

This is really an enum in the API, not an arbitrary string, so it should be an enum here.

… Or given the behavior of streamId and topic, the most type-safe version would be to make DeleteMessageEvent abstract and have two subclasses DeleteStreamMessageEvent and DeleteDmMessageEvent, akin to Message/StreamMessage/DmMessage. But the amount of code that's going to care about these objects is pretty limited — should be a small amount of code for handling the event in various data structures, and then we discard the event — so sticking with one class is fine so long as that code comes out reasonably clean.

[chrisbobbe]

Yeah. Also, before using messageType, streamId, or topic for anything, I'd want to better nail down what they actually mean, since (with bulk_message_deletion ) we're given a list of message IDs, not a single message ID. Is the list guaranteed to be all stream messages or all DMs? If all stream messages, will they definitely all be in the same stream/topic?

For any given message in message_ids, it seems natural to just look up the message in our data structures and find out what its type is, and its stream/topic if relevant, if we need that information.

[gnprice]

Is the list guaranteed to be all stream messages or all DMs? If all stream messages, will they definitely all be in the same stream/topic?

Pretty sure if those properties are present, they'll apply to all the messages in the event. (And I guess one of them is always present.) But it'd be good to nail that down.

For any given message in message_ids, it seems natural to just look up the message in our data structures and find out what its type is, and its stream/topic if relevant, if we need that information.

Yeah. I'm not sure what the backstory was for why that other information is included here.

[chrisbobbe]

Thanks for the review! Revision pushed.

[chrisbobbe]

Thanks for the in-office discussion! Revision pushed.

[gnprice]

Thanks! Looks good; merging.

FTR the main conclusion in our in-office discussion was that we're happy for this fromJson to throw an exception if it sees a propagate_mode that isn't one of the documented values.

Recording the reasoning there:

• In general, an exception in any fromJson method in our API will result in a MalformedServerResponseException thrown from the endpoint binding.
• There are lots and lots of conditions like that; in particular all the as operators in the generated fromJson implementations. If some value is a string or object where we expected int, or vice versa, or is missing when we expected it to be present, that will cause an exception.
• The cost of this is that any server API change which would break one of those expectations is a breaking change, and we have to manage it with tools like client capabilities.
• But fundamentally such a change is going to be a breaking change anyway, unless what the client would actually do with the unexpected new form of data would be valid.
  • In the case of propagate_mode: if there's some novel value for that, should the client stay on the same narrow, or navigate to a new one? It's impossible to know in advance; in practice we'd probably effectively default to one of the known enum values, which might or might not turn out to give the right behavior.
• And conversely, if we propagate the uncertainty (by e.g. having an event object but having one of its fields represent that there's something unknown), then that adds complexity on any code that's consuming the data.
• So there's a balance: if we can sensibly handle the unknown, then it leaves a wider swath of future API changes to be able to skip the overhead of managing compatibility (e.g. with client capabilities), but it adds client complexity. And often we can't sensibly handle the unknown with much confidence anyway.

---

Then some further thoughts that come to mind as I write that down.

Because of that balance, there are a few things we routinely do to leave things unspecified:

• We never check for unexpected properties in a JSON object; adding a new property is always compatible. Effectively we assume that the old properties keep meaning what they meant; when that's not true, that's a breaking change after all.

  This is valuable because a large fraction of API changes are to add a new property.

• We have UnexpectedEvent for unexpected type values in an event. The assumption we're relying on here is that if the event has a type we don't know about, then it's for some obscure new feature, so that we can sensibly handle it by ignoring that feature. Conversely this is is valuable because we regularly do add event types for new features.

  (Plus in the prototype phase it helps manage the existence of a bunch of event types we just haven't implemented yet. A lot of those are on basically the same logic: they're for some feature that, while not new or obscure, we haven't yet implemented and so there's nothing for us to do with that event until we have.)

• When a property is deprecated, or when it seems like it should be deprecated even though the docs don't yet say so, then we treat it as if it already may not exist, even if the docs say it's always present.

  Where possible, this means we leave it out of our types (with a comment for the sake of anyone comparing our types to the docs — like UpdateMessageEvent.prevRenderedContentVersion in this PR); if we do need to consult it (perhaps to handle older servers), we make it nullable.

  By definition, if the property should be deprecated, then we know what to do without it. And conversely this is valuable because we do regularly remove old bits of API; the main bottleneck to doing so is when it'd be a breaking change and require managing compatibility.

But in the great majority of unexpected situations other than adding a new property, it's not clear in advance what a client should do. So I think there are few situations outside the ones described above where we do take any steps to handle unexpected data from the API.

[gnprice]

I find it pretty puzzling that the implementation of this goes and does a linear scan through all the possible values of the enum to find the one that matches:

  for (var entry in enumValues.entries) {
    if (entry.value == source) {
      return entry.key;
    }
  }

But this enum has only three values, and the other one in this PR has only two, so it's fine.

If we had an enum with like 100 values that we were going to decode frequently, then I'd want to intervene to make this a map lookup instead.