Permit use of commas in stream names #3734
Conversation
|
Thanks! Ouch, yeah. I think a good solution to that last question is: in a commit before the one that fixes this bug, we add some logic for |
rk-for-zulip
force-pushed
the
commas-in-streams
branch
from
ed760bd
to
61e76fe
Compare
|
On reflection, the last two commits ("send messages sequentially" and "continue sending messages on client errors") are a little out of scope. If there are issues with them, but not with the first three commits, it's probably better to close this PR with just those three merged – the other two can wait for their own issue+PR. |
There was a problem hiding this comment.
Thanks @ray-kraesig for this fix!
Let's move the last two commits to a separate PR, as you suggested -- they have some complexity, so let's narrow the scope for this bugfix.
On the first three commits, a few comments below. Also one branch-level comment:
- Let's put the "discard ancient unsent messages" commit before the fix to how we send these messages. That's the logical order, because it avoids there being a version that would have the flush-backlog problem you mentioned in the PR description above.
| } else { | ||
| // HACK: the server attempts to interpret this argument as JSON, then | ||
| // CSV, then a literal. To avoid misparsing, always use JSON. | ||
| return JSON.stringify([item.display_recipient]); |
There was a problem hiding this comment.
Huh, interesting that this is a list -- so I think it'll be like ["general"]. Is that really the right form?
The API docs, fwiw, say (at https://zulipchat.com/api/send-message):
The destination stream, or a CSV/JSON-encoded list containing the usernames (emails) of the recipients.
which suggests a list is only applicable for PMs.
When changing the format of what we send in a request, it'd also be good to confirm that older server versions already accept the new format; then if they don't, we'll figure out how to handle that.
There was a problem hiding this comment.
JSON has been accepted since at least zulip/zulip@97d7d31 – sometime in the Humbug 0.1.4 period, in mid-2013 – and given the presence of json_to_list in the "before" side of that diff, it's probably been accepted for longer than that.
(I'll make a note in the commit message.)
[...] which suggests a list is only applicable for PMs.
Arguably it's a server bug that it tries to interpret to as a list even for stream messages, but that is indeed what it does. (Reported as zulip/zulip#13836.)
There was a problem hiding this comment.
Cool. That sure is reassuringly old. :-)
I think then my remaining question is: the commit message says "Always use JSON" for this field; but it looks like the code actually uses JSON just for the stream case, and continues to use a simple CSV for the PMs case.
I think I'd be OK with that difference between the two cases; but I'd want to have the commit message, and the comments, be in agreement with what's happening.
Alternatively, I think it would work well to do as the commit message says: always use JSON. More specifically, always use a JSON-encoded list. In the PMs case, I think this would be just something like
- return narrow[0].operand;
+ return JSON.stringify(narrow[0].operand.split(','));
(And then perhaps celebrate the shared JSON.stringify part by pulling it out to deduplicate.)
rk-for-zulip
force-pushed
the
commas-in-streams
branch
2 times, most recently
from
d047907
to
3340d0c
Compare
After poking at the relevant code a bit, I'm going to have to abandon those commits entirely. The bug they're trying to fix presently masks the potentially severe effects of other bugs. 😞 I don't think there's a simpler way to fix the control flow here than a total rewrite. I'll file a new issue with some notes. (EDIT: #3881.) In the meantime, 🏓. |
|
Thanks for the revisions! Replied above. |
rk-for-zulip
force-pushed
the
commas-in-streams
branch
from
3340d0c
to
98dbf9e
Compare
Clarified. (The other option would have required testing.) |
|
Withdrawn pending research and discussion.. The short version is that we may want to use a stream ID here rather than a stream name. |
|
The ability to use stream IDs was only added in 2.0 (zulip/zulip@b822155). While it might be a good idea once we can do version detection, we'll have to make do with JSONified names for now. |
... rather than having a seven-parameter-long signature.
... at which point, they're more likely to generate confusion than to be useful. (This isn't a great solution, since it *does* discard data -- but it's probably the lesser evil.)
... to avoid its potential server-side misinterpretation as CSV when the stream name has commas. The Zulip server appears to have accepted JSON in this field at least as far back as 2013, and possibly farther -- the relevant code dates to zulip/zulip@97d7d31 or thereabouts in its current form, but JSON appears to have been accepted even then. Fixes zulip#3729.
gnprice
force-pushed
the
commas-in-streams
branch
from
98dbf9e
to
e4a83be
Compare
|
Looks good, thanks -- merged! |
Should fix #3729.
This is just a draft PR because it doesn't take into account the problem of what to do about existing messages in the outbox. (We probably don't want to flush a potential backlog of hundreds of messages all at once. Perhaps, during the following migration, we should drop messages stalled due to this bug that are more than a day old?)