Replace Patches:+Patch-Length: with Patch-Lengths:

[canadaduane]

NOTE: We're currently discussing changing "Content-Length" to "Patch-Length" in #97 so to be clear: This issue relates to "Patch-Length" if we decide to adopt that header name.

Thanks to @toomim and @josephg's comments in #98, as well as discussion in #99, I've been thinking about the current spec implementing patches with a Content-Length (or maybe Patch-Length) header.

I think we should move it to the Version-level header. This would simplify implementations like I wanted in #99 but provide all of the benefits of having Patches in the spec, as Mike would like.

So, concretely, I'm suggesting replacing "Patches" with "Patch-Lengths" and removing "Content-Length" from the patch-level headers, e.g. in a Request:

      Request:

         PUT /chat
         Version: "g09ur8z74r"                                 | Version
         Parents: "ej4lhb9z78"                                 |
         Content-Type: application/json                        |
         Merge-Type: sync9                                     |
         Patch-Lengths: 90, 78                                 |
                                                               |
         Content-Range: json=.messages[1:1]                    | | Patch
                                                               | |
         [{"text": "Yo!",                                      | |
           "author": {"link": "/user/yobot"}]                  | |
                                                               |
         Content-Range: json=.latest_change                    | | Patch
                                                               | |
         {"type": "date", "value": 1573952202370}              | |

Implications:

• the "length" of a patch would now include the length of the patch's headers (previously, it was just the length of the patch body)
• the number of patches is now the number of comma-separated values (lengths) in the Patch-Lengths field
• the implementation is simplified by making it necessary to grab "Content-Length" from just the version-level headers.

[josephg]

Hm!

Things I like about this:

• This makes the one-patch-per-update case really simple, while also supporting sending multiple patches per update

Things I dislike:

• This implicitly limits the number of patches that can be contained within an update based on the maximum length of a header field

Another approach here would be to lean on the multipart/form-data spec, which is already implemented in browsers, doesn't have the limit of Patch-lengths: ... and supports streaming out of the box (if we ever want that). In the current spec it looks like this:

POST / HTTP/1.1
[[ Less interesting headers ... ]]
Content-Type: multipart/form-data; boundary=---------------------------735323031399963166993862150
Content-Length: xxx (optional)

-----------------------------735323031399963166993862150
Content-Disposition: form-data; name="text1"

text default
-----------------------------735323031399963166993862150
Content-Disposition: form-data; name="file3"; filename="binary"
Content-Type: application/octet-stream

aωb
-----------------------------735323031399963166993862150--

For us, maybe something like this:

     PUT /chat
     Version: "g09ur8z74r"                                 | Version
     Parents: "ej4lhb9z78"                                 |
     Content-Type: application/braid-patches; boundary=____ABC123DEF____
     Merge-Type: sync9                                     |

     ____ABC123DEF____
     Content-Range: json=.messages[1:1]                    | | Patch
                                                           | |
     [{"text": "Yo!",                                      | |
       "author": {"link": "/user/yobot"}]                  | |
     ____ABC123DEF____
     Content-Range: json=.latest_change                    | | Patch
                                                           | |
     {"type": "date", "value": 1573952202370}              | |
     ____ABC123DEF____

[brynbellomy]

I've always been a strong proponent of explicitly-declared lengths as opposed to magical delimiter strings (whether \n\n or ___ABC123DEF___ or something else). I don't think I agree that much is gained here, and it seems that we lose something concrete (namely, the ability to have our chosen magical delimiter string ever appear within a patch body, however unlikely that may be).

Other than a small gain in spec simplicity, what's the downside to having each patch specify its length?

[josephg]

A sufficiently random & long delimiter string will act like a uuid, and should work without worrying about escaping or whatever.

The nice thing about an approach like this is that there will almost always be just one patch. Writing Patches: 1 over the wire for every update feels silly to me.

[canadaduane]

Things I dislike:

* This implicitly limits the number of patches that can be contained within an update based on the maximum length of a header field

Since this is the "Version" block header field (not the HTTP header field), I don't think there is such a limit?

[josephg]

Yes, true in subscriptions but not in PUT / PATCH messages - which should probably have the same syntax

[mitar]

What is nice with Patch-Lengths: 90, 78 is that it can also be defined that one can do:

Patch-Lengths: 90
Patch-Lengths: 78

For same effect. Which can then also mitigate any issues with value length limits. This practice that you can use comma-separated values or repeat header multiple times is pretty common. And with HTTP2 is compressed well.

(BTW, I would rename Patch-Lengths to Patch-Length.)