Rename subject/topic in API types, and fix orig/new names in UpdateMessageEvent #751
Conversation
| final String subject; // TODO call it "topic" internally; also similar others | ||
| @JsonKey(name: 'subject') | ||
| final String topic; |
There was a problem hiding this comment.
api [nfc]: Rename Message.topic, from "subject"
I wondered what would happen if the payload already contained a 'topic' key, and I wasn't immediately sure. I could imagine servers adding one as part of an API migration.
We just ignore it, it turns out, which is definitely fine for before that API migration happens. But if we rejected it, we'd be putting up an obstacle to the migration. Do we want to test that we don't reject it?
test('does not reject if servers add a new `topic` field', () {
check(() => Message.fromJson(baseStreamJson()..['topic'] = 'hello'))
.returnsNormally();
});(Similarly for the other renames in this PR, I guess.)
There was a problem hiding this comment.
In general our API types and their fromJson constructors always have the property that they don't notice or care if the JSON they receive has additional properties in it that weren't expected. That's a fact about json_serializable / json_annotation (there's a flag to get the opposite behavior, JsonSerializable.disallowUnrecognizedKeys, but it's off by default), and it's one we rely on regularly for routine forward-compatibility with server changes.
So I'm content to leave that as part of the routine boring behavior of the generated deserialization code, which we don't test.
This field is obsoleted by `stream_id`.
The name "subject" is long obsolete for Zulip topics. It persists in a few places in the API, but we should keep it out of our code. We had it here on the Message type really just because this is a part of the code that I wrote in the very first hours of the prototype of this app, before I'd read up enough on the json_serialization package to find `JsonKey.name`. There's a TODO referring to a few other similar names in the API; we'll handle those shortly.
Much like the `topic` field in the previous commit.
This completes the renaming from "subject" to "topic" in our code.
In the server API here, "stream_id" is the old value and contrasts with "new_stream_id", but "subject" is the new value and contrasts with "orig_subject". This makes them confusing to read, and is the way it is only for historical reasons: I believe the story is that before Zulip Server 3.0 added support for moving topics between streams, three of those properties were already present and then "new_stream_id" was squeezed in at the best name still available. Fixing that in the API will be a pain because it'll need a migration path for compatibility. But we can fix it here within our code.
gnprice
force-pushed
the
pr-new-topic-renames
branch
from
d41bb8d to
6c949ea
Compare
|
Thanks for the review! Rebased, and replied above; merging. |
The name "subject" is long obsolete for Zulip topics. It persists
in a few places in the API, but we should keep it out of our code.
We had it here on the Message type really just because this is a
part of the code that I wrote in the very first hours of the
prototype of this app, before I'd read up enough on the
json_serialization package to find
JsonKey.name.In the server API on UpdateMessageEvent, "stream_id" is the old value and contrasts
with "new_stream_id", but "subject" is the new value and contrasts
with "orig_subject".
This makes them confusing to read, and is the way it is only for
historical reasons: I believe the story is that before Zulip
Server 3.0 added support for moving topics between streams, three
of those properties were already present and then "new_stream_id"
was squeezed in at the best name still available.
Fixing that in the API will be a pain because it'll need a migration
path for compatibility. But we can fix it here within our code.
The renames in UpdateMessageEvent are from a draft I'd started a few weeks ago. Then while I was polishing that up to send (in particular, writing tests for it), I decided to go back and take care of the
Message.subjectrename too, since that's been an occasional small annoyance./cc @PIG208 because when you go to handle UpdateMessageEvent for #750 / #171, you'll want to have rebased atop these renames.
I have some other draft commits that touch the
MessageStoreImpl.handleUpdateMessageEventimplementation itself. I'll take a look through those next and see if any of them are relevant to #171, or if they become relevant only when you get to #150. If there are changes relevant to #171, I'll get those in shape too and send a PR.