search

braid-org / braid-spec

Working area for Braid extensions to HTTP
https://braid.org
233 stars 16 forks source link

Separate versions with blank lines in braid-http? #73

Closed toomim closed 3 years ago

toomim commented 3 years ago

There's a blank line separating each version in a subscription response in the spec's examples:

         HTTP/1.1 209 Subscription
         Subscribe: keep-alive

         Version: "ej4lhb9z78"                                 | Version
         Parents: "oakwn5b8qh", "uc9zwhw7mf"                   |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Content-Length: 64                                    |
                                                               |
         [{"text": "Hi, everyone!",                            | | Body
           "author": {"link": "/user/tommy"}}]                 | |
                                                                         << Blank line!
         Version: "g09ur8z74r"                                 | Version
         Parents: "ej4lhb9z78"                                 |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Patches: 1                                            |
                                                               |
         Content-Length: 53                                    | | Patch
         Content-Range: json=.messages[1:1]                    | |
                                                               | |
         [{"text": "Yo!",                                      | |
           "author": {"link": "/user/yobot"}]                  | |
                                                                         << Blank line!
         Version: "2bcbi84nsp"                                 | Version
         Parents: "g09ur8z74r"                                 |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Patches: 1                                            |
                                                               |
         Content-Length: 58                                    | | Patch
         Content-Range: json=.messages[2:2]                    | |
                                                               | |
         [{"text": "Hi, Tommy!",                               | |
           "author": {"link": "/user/sal"}}]                   | |
                                                                         << Blank line!

(See the << Blank line! marks above.)

These blank lines make it easier for a human to separate each version when reading the wire traffic, but they aren't discussed anywhere in the spec, nor are they strictly necessary -- the client can already detect the end of a version by counting the Content-Length or number of Patches.

Do we want to have these blank lines? In general, should they be:

katuma commented 3 years ago

Since patch/body content are terminated by newline too, it makes sense for version to do that as well on its upper level, which incidentally creates the visual aid.

Servers: Restrictive. MUST include the NL after body, patch AND the container version. Also clarify that none of the newlines - are to be included in patch/body Content-Lengths.

Clients: Permissive. SHOULD tolerate missing NL, as well as skip over arbitrary number of NL - their algorithm in practice is just skip over empty lines until next patch/version/body header is found.

Edit:

         HTTP/1.1 209 Subscription
         Subscribe: keep-alive

         Version: "ej4lhb9z78"                                 | Version
         Parents: "oakwn5b8qh", "uc9zwhw7mf"                   |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Content-Length: 64                                    |
                                                               |
         [{"text": "Hi, everyone!",                            | | Body
           "author": {"link": "/user/tommy"}}]                 | |
                                                                         << 2x NL - one for version, one for body/patch
         Version: "g09ur8z74r"                                 | Version
         Parents: "ej4lhb9z78"                                 |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Patches: 2                                            |
                                                               |
         Content-Length: 53                                    | | Patch
         Content-Range: json=.messages[1:1]                    | |
                                                               | |
         [{"text": "Yo!",                                      | |
           "author": {"link": "/user/yobot"}]                  | |      <-- 1X NL, because this is inter-patch in same version
         Content-Length: 58                                    | | Patch
         Content-Range: json=.messages[2:2]                    | |
                                                               | |
         [{"text": "Hi, Tommy!",                               | |
           "author": {"link": "/user/sal"}}]                   | |
                                                                        << 2x NL again
         Version: "ej4lhb9z78"                                 | Version
         Parents: "oakwn5b8qh", "uc9zwhw7mf"                   |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Content-Length: 64                                    |
                                                               |
         [{"text": "Hi, everyone!",                            | | Body
           "author": {"link": "/user/tommy"}}]                 | |
                                                                         << 2x NL again
toomim commented 3 years ago

I think this is a good idea! We should create a PR for it in the spec, and solicit comment.

josephg commented 3 years ago

We talked about this in the meeting. This works well with the heartbeat issue - where the direction is to say the client must accept any number of newlines.

The rough consensus is something like:

Servers: Restrictive. MAY/SHOULD include the NL after body, patch AND the container version. Also clarify that none of the newlines - are to be included in patch/body Content-Lengths.

Clients: Permissive. MUST tolerate missing NL, as well as skip over arbitrary number of NL - their algorithm in practice is just skip over empty lines until next patch/version/body header is found.