-
Notifications
You must be signed in to change notification settings - Fork 221
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.
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
api: Add stream/update event #880
api: Add stream/update event #880
Conversation
Thanks for adding this! I'll leave this for the other rounds of review before reading in detail, but one nit in the main commit's commit message:
Let's leave the umbrella issue (#135) out of the commit message. Mentioning issues in commit messages is useful but has a trade-off: GitHub can turn it into noise on the issue thread when the commit is revised or rebased. That trade-off is worth it for the one commit or handful of commits that fix an issue, but for a broad umbrella issue like #135, the noise could get overwhelming. So just point to the specific issue #182, and let that one point to the umbrella issue. See also: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mention-fixed-issue |
e0e2040
to
4ae44a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sm-sayedi! other than one small nit, LGTM.
Marking for mentor review with @hackerkid.
@@ -308,6 +308,14 @@ enum UserRole{ | |||
/// in <https://zulip.com/api/register-queue>. | |||
@JsonSerializable(fieldRename: FieldRename.snake) | |||
class ZulipStream { | |||
// When adding a field to this class: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit message nit: Let's try to use more brief wording, say:
model [nfc]: Clarify how to add updatable fields to `ZulipStream`
or
model [nfc]: Clarify when to add non-final fields to `ZulipStream`
Also, I believe this is closely related to the previous commit
api: Add stream/update event
So, most probably this will get squashed into it during maintainer or integration review.
4ae44a6
to
1765ba8
Compare
d52da57
to
6ddfcb1
Compare
Is this ready for another review? Please post a comment on the PR after pushing changes that respond to a previous review, if it's ready for the next person's review. |
@chrisbobbe Yeah sure, it's ready for the maintainer review! I have requested the review through the UI and added the label, so I thought there was no need to mention it explicitly through a comment! 🙂 |
6ddfcb1
to
a1cc32d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Comments below.
lib/api/model/model.dart
Outdated
/// Get a [ChannelPostPolicy] from a nullable int. | ||
/// | ||
/// Example: | ||
/// 2 -> ChannelPostPolicy.administrators | ||
/// null -> ChannelPostPolicy.unknown | ||
static ChannelPostPolicy fromInt(int? value) => _byIntValue[value]!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Get a [ChannelPostPolicy] from a nullable int. | |
/// | |
/// Example: | |
/// 2 -> ChannelPostPolicy.administrators | |
/// null -> ChannelPostPolicy.unknown | |
static ChannelPostPolicy fromInt(int? value) => _byIntValue[value]!; | |
/// A [ChannelPostPolicy] from an API value or null. | |
static ChannelPostPolicy fromApiValueOrNull(int? value) => _byIntValue[value]!; |
I think this contains all the information it needs. (And it's more specific than "int": an API value for this enum can be one of a few ints, not any int.)
(Possibly the existing fromRawString
methods on other enums, like Emojiset
, could have been better named fromApiValue
as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…Ah, but see a comment elsewhere about how the null case doesn't seem necessary; so I think this could just be fromApiValue
, not fromApiValueOrNull
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good change and I'd go a step further: the new name covers all the information in the doc, so that would can leave the doc out.
lib/api/model/events.dart
Outdated
case ChannelPropertyName.channelPostPolicy: | ||
return ChannelPostPolicy.fromInt(value as int?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case ChannelPropertyName.channelPostPolicy: | |
return ChannelPostPolicy.fromInt(value as int?); | |
case ChannelPropertyName.channelPostPolicy: | |
return ChannelPostPolicy.fromInt(value as int); |
Do we expect to get an event with property: 'stream_post_policy'
and a missing or null value
? I don't think we do. In the API doc for the event and for the register response, it doesn't say we should expect missing or null, except from servers pre-3.0, and we don't support those servers.
With this change, ChannelPostPolicy.fromInt
can also tightened to expect an int
instead of int?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect to get an event with property: 'stream_post_policy' and a missing or null value?
Yeah, I don't think so. But the reason I added int?
instead of int
is that there is ChannelPostPolicy.unknown
for apiValue: null
. I think it was added for backward-compatibility with server-3 at that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. @gnprice, does that seem right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we should expect "property": "stream_post_policy"
to mean that value
will be an int.
The value ChannelPostPolicy.unknown
is there mainly for forward compatibility, not backward: it's "unknown" because it represents a hypothetical future server sending a value like 5 or 6 instead of 1 or 2 or 3 or 4, in which case that 5 or 6 would represent some new meaning that this client version doesn't know about.
In general zulip-flutter only supports down to Zulip Server 4:
https://github.com/zulip/zulip-flutter#editing-api-types
so we don't have any compatibility logic that's meant for server versions 3 or earlier (and stream_post_policy
was introduced in Zulip Server 3). If the field were missing in some versions we still support, then we'd represent that like
ChannelPostPolicy? channelPostPolicy;
i.e. with null
to represent missing.
(Rereading the code, I think we may not have actually correctly accomplished that forward-compatibility — I think we might just throw on an unexpected value like 5. For an example I believe does work correctly, see MessageFlag
.)
lib/api/model/model.dart
Outdated
static ChannelPostPolicy fromInt(int? value) => _byIntValue[value]!; | ||
|
||
// _$…EnumMap is thanks to `alwaysCreate: true` and `fieldRename: FieldRename.kebab` | ||
static final _byIntValue = _$ChannelPostPolicyEnumMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alwaysCreate: true
and fieldRename: FieldRename.kebab
aren't passed to the JsonEnum
annotation, so this comment is wrong. 🙂 Looks like it was copy-pasted from somewhere else.
_$ChannelPostPolicyEnumMap
is created because ChannelPostPolicy
is used as a field in a class (ZulipStream
) that's annotated with JsonSerializable
; see the dartdoc on JsonEnum.alwaysCreate
:
/// If `true`, `_$[enum name]EnumMap` is generated for in library containing
/// the `enum`, even if the `enum` is not used as a field in a class annotated
/// with [JsonSerializable].
///
/// The default, `false`, means no extra helpers are generated for this `enum`
/// unless it is used by a class annotated with [JsonSerializable].
final bool alwaysCreate;
Probably I'd just delete the comment.
lib/api/model/model.dart
Outdated
.map((key, value) => MapEntry(value, key)); | ||
} | ||
|
||
/// The name of the [ZulipStream] properties that gets updated through [ChannelUpdateEvent.property]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The name of the [ZulipStream] properties that gets updated through [ChannelUpdateEvent.property]. | |
/// The name of a property of [ZulipStream] that gets updated | |
/// through [ChannelUpdateEvent.property]. |
lib/api/model/model.dart
Outdated
/// In Zulip event-handling code (for [ChannelUpdateEvent]), | ||
/// we switch exhaustively on a value of this type | ||
/// to ensure that every property change in [ZulipStream] responds to the event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// In Zulip event-handling code (for [ChannelUpdateEvent]), | |
/// we switch exhaustively on a value of this type | |
/// to ensure that every property change in [ZulipStream] responds to the event. | |
/// In Zulip event-handling code (for [ChannelUpdateEvent]), | |
/// we switch exhaustively on a value of this type | |
/// to ensure that every property in [ZulipStream] responds to the event. |
lib/model/channel.dart
Outdated
|
||
case ChannelUpdateEvent(): | ||
final ChannelUpdateEvent(:streamId, name: String streamName) = event; | ||
assert(identical(streams[streamId], streamsByName[streamName])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use assert
s when there's an invariant we're managing purely in our own code. But here, the assert could fail because of unexpected server behavior. When we want to guard against unexpected server behavior, we don't use asserts. In production, asserts
are ignored, and also, arguments to them aren't evaluated: https://dart.dev/language/error-handling#assert . We might instead throw an error, or just give up on the thing we were trying to do and move on, usually with a // TODO(log)
.
Here, though, we don't need the channel name on the event, do we? We can just look it up on the ZulipStream
instance in our data structures. It seems potentially nice to avoid depending on the channel name in the event, to give the API more freedom to deprecate/remove that field as a cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the assert
is actually for making sure our two local copies in streams
and streamsByName
are the same. The same assert is used in ChannelDeleteEvent
.
streamName
is actually useful when updating streamsByName
when the event is about updating the channel name:
case ChannelPropertyName.name:
channel.name = event.value as String;
streamsByName.remove(streamName);
streamsByName[channel.name] = channel;
But if we really want to prevent getting streamName
from the event, we can get it by storing channel.name
in a temporary variable. Should I go with the latter option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
assert
is actually for making sure our two local copies instreams
andstreamsByName
are the same.
I see. Then I guess here, and also where we handle ChannelDeleteEvent
, it should be possible to do a targeted assert
for that internal data-structure check, using nothing but the data we have locally. We can look up the channel in our streams
map, get its name, look up the name in our streamsByName
map, and check that the ZulipStream
there matches the ZulipStream
in streams
.
And for the ways that unexpected server behavior can mislead our app, we can add checks that do run in production. Those ways, I think, are:
- The channel ID in the event isn't found in our data structures (our
streams
map). Let's early-return in this case, as in your PR, but add a// TODO(log)
. - The event's idea of the current channel name doesn't match our data structures. We can
// TODO(log)
in this case, but we can probably cheerfully continue handling the event, just ignoring the name in the event to keep ourstreams
andstreamsByName
consistent with each other. Since the server is giving unexpected behavior, I think there's no strong reason to trust the name in the event more than the name in our data structures. But theTODO(log)
could be helpful for catching bugs where our expectations are misaligned with server behavior.
Could you send a revision with a separate commit that does this for ChannelDeleteEvent
, and also does it here for ChannelUpdateEvent
?
But if we really want to prevent getting
streamName
from the event, we can get it by storingchannel.name
in a temporary variable. Should I go with the latter option?
Yeah, this seems helpful. I've just checked, and the legacy app doesn't use the channel name on this event. Since it's possible to avoid using it now (just by using the name from the entry our streams
map), let's avoid it, and that'll make it easy if the API wants to deprecate/remove the field in the future.
lib/model/channel.dart
Outdated
assert(subscriptions[streamId] == null | ||
|| identical(subscriptions[streamId], streams[streamId])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like the kind of assert
we do use: checking that our own data structures are consistent with each other.
lib/model/channel.dart
Outdated
case ChannelPropertyName.canRemoveSubscribersGroupId: | ||
channel.canRemoveSubscribersGroup = event.value as int?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably OK to accept null for this one, but from reading the API docs (including the register-queue doc), I'm not sure it's ever expected, and what it would mean. The field on ZulipStream
is nullable, but I think that's only for backwards compatibility with older servers that don't have the field:
// TODO(server-6): `canRemoveSubscribersGroupId` added in FL 142
// TODO(server-8): in FL 197 renamed to `canRemoveSubscribersGroup`
@JsonKey(readValue: _readCanRemoveSubscribersGroup)
int? canRemoveSubscribersGroup;
And if we're in this case
, then surely we're not on one of those older servers.
So, probably nothing to do here, and no harm in being a little more permissive with int?
instead of int
, I think, as long as the field on ZulipStream
remains nullable.
@@ -56,6 +56,17 @@ void main() { | |||
|
|||
await store.addSubscription(eg.subscription(stream1)); | |||
checkUnified(store); | |||
|
|||
await store.handleEvent(eg.channelUpdateEvent(store.streams[stream1.streamId]!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If adding this to the 'added by events'
test, maybe good to rename the test to 'added/updated by events'
.
a1cc32d
to
440f58d
Compare
Thanks @chrisbobbe for the review! New changes pushed. Please have a look. |
Thanks! LGTM except for a few thoughts above (one is for Greg): and I'll move this along to integration review so Greg can take a look too. |
…havior See the motivation behind this: zulip#880 (comment)
440f58d
to
00237ae
Compare
Thanks @chrisbobbe for the comments. Pushed the changes. Now ready for the integration review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sm-sayedi, and thanks @chrisbobbe for the previous review!
I didn't get through a full review today, but here's the comments I have.
lib/api/model/model.dart
Outdated
|
||
static ChannelPostPolicy fromApiValue(int value) => _byIntValue[value]!; | ||
|
||
static final _byIntValue = _$ChannelPostPolicyEnumMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
static final _byIntValue = _$ChannelPostPolicyEnumMap | |
static final _byApiValue = _$ChannelPostPolicyEnumMap |
- that aligns it with the closely-related
fromApiValue
- "by int value" isn't quite accurate — one of these keys is null, not an int
lib/model/channel.dart
Outdated
assert(identical(streams[stream.streamId], streamsByName[stream.name])); | ||
assert(subscriptions[stream.streamId] == null | ||
|| identical(subscriptions[stream.streamId], streams[stream.streamId])); | ||
streams.remove(stream.streamId); | ||
streamsByName.remove(stream.name); | ||
subscriptions.remove(stream.streamId); | ||
final localStream = streams[stream.streamId]; | ||
if (localStream == null) continue; // TODO(log) | ||
|
||
final ZulipStream(:streamId, name: String streamName) = localStream; | ||
if (streamName != stream.name) { | ||
// TODO(log) | ||
} | ||
assert(identical(streams[streamId], streamsByName[streamName])); | ||
assert(subscriptions[streamId] == null | ||
|| identical(subscriptions[streamId], streams[streamId])); | ||
streams.remove(streamId); | ||
streamsByName.remove(streamName); | ||
subscriptions.remove(streamId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it gets rather more complicated to follow (as well as longer — 6 lines to 13 lines). There's now multiple stream IDs going around, as well as multiple stream names.
So I'm not convinced this is a helpful change. (/cc @chrisbobbe from the previous comment #880 (comment) above)
lib/model/channel.dart
Outdated
assert(subscriptions[streamId] == null | ||
|| identical(subscriptions[streamId], streams[streamId])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case doesn't otherwise do anything with subscriptions
, so this assert can be left out.
lib/model/channel.dart
Outdated
if (streamName != event.name) { | ||
// TODO(log) | ||
} | ||
assert(identical(streams[streamId], streamsByName[streamName])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also doesn't do anything with streamsByName
except in the ChannelPropertyName.name
case. So this assert can be moved into that case.
Then so can the streamName
local.
And then I think we don't use the streamId
local at all, except in doing this second streams
lookup. (Which would be clearer as just saying stream
, optionally with a separate assert that stream.streamId
matches event.streamId
— as is, if these data structures get corrupted by a bug somewhere else in our code, there's nothing local in this code to assure us that when we looked up event.streamId
in streams
we got a ZulipStream whose stream ID is actually event.streamId
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also doesn't do anything with
streamsByName
except in theChannelPropertyName.name
case. So this assert can be moved into that case.
Moved it to its specific case. But this assert in here actually helped me spot a bug in one of my previous revisions of this PR (actually when working on #886 which is stacked on top of this PR). In that revision, I forgot to include:
case ChannelPropertyName.name:
stream.name = event.value as String;
+ streamsByName.remove(streamName);
+ streamsByName[stream.name] = stream;
So when first the channel name
is changed and then some other property, i.e., channelPostPolicy
, the assert would fail, simply because streamsByName
was not updated accordingly. And now if we somehow have a bug where we don't update streamsByName
, then the assert in the new location will not tell us about it. But now that it is covered in the tests, I think we're good to go. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense. I do think checking for that sort of inconsistency is useful. Two places we could do that that I think would work well (and which could coexist):
- We could use the same pattern as the
checkInvariants
function intest/model/message_list_test.dart
: we'd write a function that scrubs the entire data structure and checks every aspect for consistency, and then have all the test cases intest/model/channel_test.dart
arrange to call that function after every step they take. This is very thoroughly effective, for every situation that's exercised in those tests (even if the individual test doesn't think to check that aspect). - We could arrange so that in the live app (in debug mode), every access to a channel or subscription in ChannelStore asserts that
_streams
and_streamsByName
and_subscriptions
are consistent with each other about that particular channel. In that case the asserts should get factored into a single method (with "debug" at the start of its name), which we can call from various places inlib/model/channel.dart
. For accesses made from the rest of the app, we can extend MapView (perhaps better yet, UnmodifiableMapView?) and add the asserts tooperator[]
.
But I'm also content to leave those aside for now.
lib/api/model/model.dart
Outdated
@JsonValue('stream_post_policy') | ||
channelPostPolicy, | ||
canRemoveSubscribersGroup, | ||
canRemoveSubscribersGroupId, // TODO(Zulip-6): remove, replaced by canRemoveSubscribersGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
canRemoveSubscribersGroupId, // TODO(Zulip-6): remove, replaced by canRemoveSubscribersGroup | |
canRemoveSubscribersGroupId, // TODO(server-8): remove, replaced by canRemoveSubscribersGroup |
see other examples for the format; and in server versions 6 and 7 this does exist (in fact it's new in 6), it's version 8 where it goes away
00237ae
to
175239b
Compare
Thanks @gnprice for the previous review. New revisioned pushed addressing the comments. PTAL. |
These are the fields that gets updated via `ChannelUpdateEvent` in the next commit.
This helps make it easy to compare the class's fields to the enum's values and look for any discrepancies.
…sGroup This property on streams/channels is documented as type "integer": https://zulip.com/api/get-streams#response (And the same goes for its former name of canRemoveSubscribersGroupId.) So when the server supports this property at all, and therefore might send us an event for it, the value it supplies will be non-null. The corresponding field on ZulipStream is nullable only because of servers that don't yet support the property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! Here's a review of the rest of the PR.
The comments were small, so in the interest of efficiency I went ahead and made the changes; merging, with a few added commits on top:
1ee4bb6 api [nfc]: Put ChannelPropertyName right after ZulipStream
03f385a api [nfc]: Explain each missing ChannelPropertyName inline
fc3db42 api: Tighten value type on ChannelUpdateEvent for canRemoveSubscribersGroup
I also replied to a subthread above at #880 (comment) (didn't spot it earlier).
lib/api/model/model.dart
Outdated
@JsonValue('stream_post_policy') | ||
channelPostPolicy, | ||
canRemoveSubscribersGroup, | ||
canRemoveSubscribersGroupId, // TODO(sever-8): remove, replaced by canRemoveSubscribersGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
canRemoveSubscribersGroupId, // TODO(sever-8): remove, replaced by canRemoveSubscribersGroup | |
canRemoveSubscribersGroupId, // TODO(server-8): remove, replaced by canRemoveSubscribersGroup |
This is one place a comment's spelling is actually important, because it's here for use in grep 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, missed it! Thanks for catching it.
lib/api/model/model.dart
Outdated
@@ -382,6 +390,51 @@ enum ChannelPostPolicy { | |||
final int? apiValue; | |||
|
|||
int? toJson() => apiValue; | |||
|
|||
static ChannelPostPolicy fromApiValue(int value) => _byApiValue[value]!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's have the same method be used in both of the places we deserialize a ChannelPostPolicy — so not only in the event but in ZulipStream too
For examples, git grep -B2 -A20 ^enum lib/api/
, and pick one of the other enums that has a fromRawString
or similar method and look at where that enum is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… Hmm, there's actually a shortage of enums that work quite like this. (Where it's a numeric value, rather than a string that's the enum value's name, and where at the same time we have a named method we call to deserialize it rather than always doing so as a field on a JsonSerializable
class.)
Our enum handling in general feels a bit messy — and I think that's largely because json_serializable
is a bit messy in its API for enums — but this particular one is fine for now. Later sometime we can try to find a nicer pattern and sweep through cleaning them up.
lib/api/model/events.dart
Outdated
case ChannelPropertyName.canRemoveSubscribersGroup: | ||
case ChannelPropertyName.canRemoveSubscribersGroupId: | ||
case ChannelPropertyName.streamWeeklyTraffic: | ||
return value as int?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to #880 (comment) : canRemoveSubscribersGroup
and its old name should have int
values, not int?
, because those properties are never null on servers new enough to have them.
(OTOH streamWeeklyTraffic
can be null on a stream/channel: https://zulip.com/api/get-streams#response so its value in this event can potentially be null too.)
175239b
to
fc3db42
Compare
This was necessary for #674 to work properly (i.e., update UI based on changes in
ZulipStream.channelPostPolicy
).Fixes: #182