Add the CHATHISTORY extension specification #349
Conversation
|
|
||
| :irc.host BATCH -ID | ||
|
|
||
| @draft/label=ID :nick!ident@host ERR CHATHISTORY ERR_CODE |
There was a problem hiding this comment.
Clients should be able to associate the WARN_CODE / ERR_CODE with the specific chathistory return that's failing/warning. Feels like they should instead be inside the batch, rather than separately outside it. There's no compatibility issues with including 'em inside the batch because the only clients that'll receive these messages are ones that already support CHATHISTORY.
There was a problem hiding this comment.
We also need to define the top level err/warn more fully. Do they allow just free form text as a generic alert or do they always need a command and error code?
There was a problem hiding this comment.
#357 submitted, specifies what I think is a sane, easily extensible format for these response messages. Noted in there I've only defined FAIL to cover all fatal/serious/notice error messages, with whether to make WARN a separate message with the same arguments left for some discussion.
There was a problem hiding this comment.
I agree the error needs to be inside of the batch, that way you can at least "continue".
| #### Format | ||
| `chathistory` uses the following generic format: | ||
|
|
||
| @draft/label=ID CHATHISTORY <target> <subcommand> <start> <end> [<direction>] [<limit>] |
There was a problem hiding this comment.
This doesn’t really make sense as a generic description of the commands. Not all of them will take start and end or direction. And when direction is available on a command it needs to be explicit not optional.
I also just want to stop using confusing terms like start and end. Each subcommand probably needs its own syntax description.
There was a problem hiding this comment.
Yeah let's describe the format next to each subcommand. Especially up above in the big subcmd list, the variables are lacking context and it's difficult to follow what each message actually should look like / how it should be sent.
There was a problem hiding this comment.
To add to that, what does direction even do here?
There are BEFORE/AFTER commands, the BETWEEN command describes that limit can be positive or negative.
|
|
||
| :irc.host BATCH -ID | ||
|
|
||
| @draft/label=ID :nick!ident@host ERR CHATHISTORY ERR_CODE |
There was a problem hiding this comment.
We also need to define the top level err/warn more fully. Do they allow just free form text as a generic alert or do they always need a command and error code?
| email: "evan@muffinmedic.net" | ||
| --- | ||
| ## Description | ||
| This document describes the format of the `chathistory` extension. The `chathistory` extension uses the [chathistory][batch/chathistory] batch type, adding a client-side command for requesting `chathistory` content from the server. |
There was a problem hiding this comment.
"for requesting chat history content from"
the word "chathistory" by itself doesn't mean anything in that phrase, so splitting the words up makes sense
|
|
||
| The `chathistory` extension adds an optional `draft/msgid` to the `chathistory` batch reply. | ||
|
|
||
| Clients MUST support the [`batch`][batch] and [`server-time`][server-time] capabilities. Clients SHOULD support the [`draft/labeled-response`][draft/labeled-response] and [`draft/message-tags`][draft/message-tags] capabilities. |
There was a problem hiding this comment.
"To use this extension, clients MUST...", and "Clients SHOULD also support..."? As currently written no reason's given as to why clients MUST and SHOULD support those capabilities.
|
|
||
| Clients MUST support the [`batch`][batch] and [`server-time`][server-time] capabilities. Clients SHOULD support the [`draft/labeled-response`][draft/labeled-response] and [`draft/message-tags`][draft/message-tags] capabilities. | ||
|
|
||
| When a client with the above mentioned capabilities requests `chathistory` content from the server (using the `CHATHISTORY` command outlined below), the server SHOULD return to the client a single batch containing raw IRC lines starting with and exluduing the `start` parameter and up to and including the message at the `end` parameter. The raw IRC lines SHOULD be formatted and returned to the client as they were originally, with the addition of the above capability tags. |
There was a problem hiding this comment.
"the above mentioned capabilities" can just be "these capabilities", since they were just mentioned. In addition, this paragraph explicitly says what to do when the client does have these other caps and requests chathistory... what should happen if the client doesn't have these caps but requests chathistory?
exluduing -> excluding
|
|
||
| When a client with the above mentioned capabilities requests `chathistory` content from the server (using the `CHATHISTORY` command outlined below), the server SHOULD return to the client a single batch containing raw IRC lines starting with and exluduing the `start` parameter and up to and including the message at the `end` parameter. The raw IRC lines SHOULD be formatted and returned to the client as they were originally, with the addition of the above capability tags. | ||
|
|
||
| The `server-time` SHOULD be the time at which the message was received from the IRC server and the `batch id` a unique ID for the entire batch. `draft/label` SHOULD be included and MUST be a unique ID used to identify the `chathistory` request and any replies. A `draft/msgid` to identify each individual message MUST be the `draft/msgid` included when each message was first received from the IRC server. |
There was a problem hiding this comment.
This feels weirdly lacking context. I can half understand what's being referred to here with each key/tag, but... this needs to sit alongside an example which clearly displays each of these in action, so implementers can get a sense of how these are used (and where they are used) in the real world.
|
|
||
| When a client with the above mentioned capabilities requests `chathistory` content from the server (using the `CHATHISTORY` command outlined below), the server SHOULD return to the client a single batch containing raw IRC lines starting with and exluduing the `start` parameter and up to and including the message at the `end` parameter. The raw IRC lines SHOULD be formatted and returned to the client as they were originally, with the addition of the above capability tags. | ||
|
|
||
| The `server-time` SHOULD be the time at which the message was received from the IRC server and the `batch id` a unique ID for the entire batch. `draft/label` SHOULD be included and MUST be a unique ID used to identify the `chathistory` request and any replies. A `draft/msgid` to identify each individual message MUST be the `draft/msgid` included when each message was first received from the IRC server. |
There was a problem hiding this comment.
received from the IRC server should be received at the IRC server? from could mean different things here such as when the IRC server originally sent the line to the client which may not be the case.
There was a problem hiding this comment.
From is correct. Msgids are supplied by servers.
| The `server-time` SHOULD be the time at which the message was received from the IRC server and the `batch id` a unique ID for the entire batch. `draft/label` SHOULD be included and MUST be a unique ID used to identify the `chathistory` request and any replies. A `draft/msgid` to identify each individual message MUST be the `draft/msgid` included when each message was first received from the IRC server. | ||
|
|
||
| ### `CHATHISTORY` Command | ||
| `CHATHISTORY` content can be requested by the client to the server by sending the `CHATHISTORY` command to the server. A `batch` MUST be returned by the server. If no content exists to return, an empty batch SHOULD be returned to avoid the client waiting for a reply. Command support is sent to the client as the RPL_ISUPPORT 005 numeric `:irc.host 005 nick CHATHISTORY=max_messages :are supported by this server` |
There was a problem hiding this comment.
"Chat history content can be..." - as noted above, it's not a proper noun so let's go the more generic descriptor.
It feels like the ISUPPORT list doesn't fit too well here, in the opening paragraph description of the command. Feels like we should split this out into its own section (describing just the RPL_ISUPPORT token and max_messages, with no description of limit).
|
|
||
| Both the `start` and `end` parameters support `draft/msgid` and `timestamp`. If the number of lines between the `start` and `end` parameters exceeds the `max_messages`, the server SHOULD return a number of lines equal to the `max_messages` and the appropriate warning as described below. A`limit` or `max_messages` of 0 indicates that no limit or maximum exists. | ||
|
|
||
| The `target` parameter specifies a single channel or query from which history SHOULD be retrieved. Wildcards or multiple targets are not supported. |
There was a problem hiding this comment.
SHOULD? This language makes it seem like the history could come from any channel or query the server pleases. It feels like what's meant here is just that target is where the history will be retrieved from – and that we should leave the possible warning/error conditions to elsewhere in the document.
|
Closing some tabs, so just leaving a note that the latest changes to this are based on https://gist.github.com/DanielOaks/c104ad6e8759c01eb5c826d627caf80d |
|
I'd like to suggest this become a |
|
Mentioned by Alex on IRC: would also be useful for bouncers to know, since presumably if a client supported chathistory they could do opportunistic loading of channel history after connecting rather than needing all the history of every channel being sent their way automagically. |
|
Reposting from here #306 (comment)
|
|
Should there be a reply for an unsupported SUBCOMMAND to make this future-proof in case we add something in the future but the server doesn't support that subcommand? |
|
|
||
| `LATEST` get the most recent (up to `limit`) messages that have been sent since the given `timestamp` or `draft/msgid`. The `limit` MUST BE positive. | ||
|
|
||
| `AROUND` get a number of messages before and after the `timestamp` or `draft/msgid`. The `limit` MUST BE positive. |
There was a problem hiding this comment.
What issue does AROUND solve? It's not obvious to me when I would want to use this command.
There was a problem hiding this comment.
It seems like an odd case where a client requests "around 50" but the server only has 20 above and 30 after.
There was a problem hiding this comment.
From IRC:
Sunday, February 24th, 2019
13:45:35 <@jwheare> xPaw: AROUND is to enable stuff like search in clients. Search index returns a matching id/timestamp, useful to show context around it.
13:45:47 <@jwheare> Or like a highlights browser
14:11:17 <@xPaw> are you saying it should request AROUND for each search match?
16:25:11 <@jwheare> xPaw: it could do, possibly on demand
16:25:24 <@jwheare> (lazily)
16:26:35 <@xPaw> That would be ridiculously inefficient. Are you talking about server side searching in this case? (Which isn't defined in that spec)
16:28:27 <@jwheare> not necessarily that inefficient. search was just an example. anything that lets you jump back to a point in time in a client. it would be useful to load in context around that point
|
|
||
| `AROUND` get a number of messages before and after the `timestamp` or `draft/msgid`. The `limit` MUST BE positive. | ||
|
|
||
| `BETWEEN` get up to `limit` number of messages between the given `timestamps` or `draft/msgids`. The `limit` MAY positive or negative. If the `limit` is positive, messages will be returned in ascending order starting at the oldest message. If the `limit` is negative, messages will be returned in decending order starting at the newest message. |
There was a problem hiding this comment.
The positive/negative limit seems like an unnecessary complication to the spec. It's hard to tell what's even supposed to happen if there are more messages in response than the specified limit.
| #### Format | ||
| `chathistory` uses the following generic format: | ||
|
|
||
| @draft/label=ID CHATHISTORY <target> <subcommand> <start> <end> [<direction>] [<limit>] |
There was a problem hiding this comment.
To add to that, what does direction even do here?
There are BEFORE/AFTER commands, the BETWEEN command describes that limit can be positive or negative.
|
|
||
| :irc.host BATCH -ID | ||
|
|
||
| @draft/label=ID :nick!ident@host ERR CHATHISTORY ERR_CODE |
There was a problem hiding this comment.
I agree the error needs to be inside of the batch, that way you can at least "continue".
| ## Security Considerations | ||
| Secure identification of users and clients MUST exist in order to ensure that users cannot obtain history they are not authorized to view. Use of account names, internal account identifiers, or certificate fingerprints SHOULD be strongly considered when matching content to users. The server MUST verify the current user's identity matches that of the desired content. This information is not sent as part of the `CHATHISTORY` command and MUST be validated via other means, such as those stated above. Access MUST be checked first and return a `NO_SUCH_NICK` or `NO_SUCH_CHANNEL` error as appropriate. If no authorization error exists, the server can check for returnable content. If no returntable content is found, the server MUST send a `NO_TEXT_TO_SEND` error. The server MUST NOT expose the existence of valid targets to unauthorized users. | ||
|
|
||
| While a `max_messages` of 0 MAY be used to indicate no limit exists, servers SHOULD set and enforce a reasonable `max_messages` and properly throttle `CHATHISTORY` commands to prevent abuse. |
There was a problem hiding this comment.
Does max_messages specify how many messages are stored on the server, how many messages can be requested with a command (the limit param), or both?
If it's how many messages are stored, the note about enforcing a reasonable limit seems moot.
|
@MuffinMedic are you planning on making any changes based on the above comments? If not then someone can fork it and continue improving the spec |
|
@prawnsalad It's probably better if someone forks it. I no longer have the time to work on this. |
|
Ok cool, I'll take a look at this PR later this week as I'm in the middle of an implementation anyhow |
|
@prawnsalad If you have a more updated version of this would it be possible for you to make it public so that I can implement it? Thanks. |
|
What's the current state of the discussion around private messages? From a client-side perspective, private messages with different nicks are different "buffers" or "targets". But when I implemented this on the server side, I assumed that all private messages sent to the user were a single target, addressable under the user's own nickname. This solution has one advantage: it solves the problem of receiving a private message while offline from a new sender. With separate targets for each sender, the client doesn't know to request messages from that sender. With a single target for the recipient, the client can request the entire buffer and then display all the messages appropriately. I propose making this official: all private messages sent to, or from, the user should be under the same target, either the user's nickname or something like |
|
Something else that's come up: the use of msgids in any ordering-based query (before/after/latest/between, anything other than "around" really) is problematic because under server-to-server linking, there is no global ordering of msgids, i.e., no guarantee that the client's observed msgid order matches the server-time order, or matches the order the messages were stored in the history buffer. This is a problem for walltimes too, but at least with walltimes you can do something like "add a 30-second buffer" and then deduplicate on the client side by msgid. |
|
Let's pick a new owner for this? Now that message IDs are ratified, it would be really nice to move forward here. |
|
@slingamn I'm going over this today. Things have been busy so it had dropped down my todo |
|
Current overhaul: https://gist.github.com/prawnsalad/4a6cc3cf8c427cadc6bb1d492d7ee29c I'm yet to proofread this a little closer but the general outline is there. It's a rather large cleanup and change from this existing draft as there was so many confusing parts, so I'll create a new PR for us to work from once I've read over it. |
|
Closing in favour of #393 |
Reopened from #292 with a new base branch.
Rendered view available here.