search

braid-org / braid-spec

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

Add top level Patch-Type to subscriptions #106

Closed josephg closed 1 year ago

josephg commented 3 years ago

The normal case for a subscription is that the server sends a stream of patches. When using a custom patch type, (and assuming #97 is merged) the protocol looks like this:

HTTP/1.1 209 Subscription
content-type: text/plain
transfer-encoding: chunked
current-versions: "ABC"

patches: 1
version: "B"

patch-type: ot-text-unicode
patch-length: 7

[1,"b"]

patches: 1
version: "C"

patch-type: ot-text-unicode
patch-length: 7

[2,"c"]

patches: 1
version: "D"

patch-type: ot-text-unicode
patch-length: 7

[3,"d"]

.... etc

This is very redundant.

Most implementations will never mix patch types, so adding patch-type: ot-text-unicode to every patch seems excessive. And this implies the patch type might change update to update, when it actually it won't.

The byte bloat is somewhat alleviated by h2 header compression (which can be used inside a stream apparently) but I don't want to rely on that. In http1.1 I'm not sure how much state gzip will actually reuse between flushed frames (since each update needs to explicitly flush).

Anyway, I propose we allow the patch-type header to be promoted to the top (http header) level in a subscription. Eg:

HTTP/1.1 209 Subscription
content-type: text/plain
transfer-encoding: chunked
current-versions: "ABC"
patch-type: ot-text-unicode     # <--- Added!

patches: 1
version: "B"

patch-length: 7       # <--- patches no longer need a patch-type

[1,"b"]

...

There are two possible semantics here:

  1. This guarantees the patch-type will remain constant for the subscription. Specifying patch-type in a patch is invalid, or
  2. The patch-type specified at the top level is the default for this subscription. It can be overridden in any given patch

I'm not sure which semantics I prefer, but in either case this change will make the protocol cleaner.

canadaduane commented 3 years ago

I like this change.

Does anyone have a use case where it makes sense to have multiple patch types in the same subscription, and ending the subscription and resubscribing doesn't suffice?

On Mon, Mar 15, 2021, 7:44 PM Seph Gentle @.***> wrote:

The normal case for a subscription is that the server sends a stream of patches. When using a custom patch type, (and assuming #97 https://github.com/braid-org/braid-spec/issues/97 is merged) the protocol looks like this:

HTTP/1.1 209 Subscription content-type: text/plain transfer-encoding: chunked current-versions: "ABC"

patches: 1 version: "B"

patch-type: ot-text-unicode patch-length: 7

[1,"b"]

patches: 1 version: "C"

patch-type: ot-text-unicode patch-length: 7

[2,"c"]

patches: 1 version: "D"

patch-type: ot-text-unicode patch-length: 7

[3,"d"]

.... etc

This is very redundant.

Most implementations will never mix patch types, so adding patch-type: ot-text-unicode to every patch seems excessive. And this implies the patch type might change update to update, when it actually it won't.

The byte bloat is somewhat alleviated by h2 header compression (which can be used inside a stream apparently) but I don't want to rely on that. In http1.1 I'm not sure how much state gzip will actually reuse between flushed frames (since each update needs to explicitly flush).

Anyway, I propose we allow the patch-type header to be promoted to the top (http header) level in a subscription. Eg:

HTTP/1.1 209 Subscription content-type: text/plain transfer-encoding: chunked current-versions: "ABC" patch-type: ot-text-unicode # <--- Added!

patches: 1 version: "B"

patch-length: 7 # <--- patches no longer need a patch-type

[1,"b"]

...

There are two possible semantics here:

  1. This guarantees the patch-type will remain constant for the subscription. Specifying patch-type in a patch is invalid, or
  2. The patch-type specified at the top level is the default for this subscription. It can be overridden in any given patch

I'm not sure which semantics I prefer, but in either case this change will make the protocol cleaner.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/braid-org/braid-spec/issues/106, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAABAI6MCGNOVHSUGPRVTDTD2ZYNANCNFSM4ZHTGLNA .

toomim commented 1 year ago

I like this idea— perhaps we want to generalize it to other headers as well. For instance, the subscriptions example in Section 3 redundantly specifies Content-Type and Merge-Type with every update:

         HTTP/1.1 209 Subscription
         Subscribe: keep-alive

         Version: "ej4lhb9z78"                              | Update
         Parents: "oakwn5b8qh", "uc9zwhw7mf"                |
         Content-Type: application/json                     |
         Merge-Type: sync9                                  |
         Content-Length: 64                                 |
                                                            |
         [{"text": "Hi, everyone!",                         | | Snapshot
           "author": {"link": "/user/tommy"}}]              | |

         Version: "g09ur8z74r"                              | Update
         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"}]               | |

         Version: "2bcbi84nsp"                              | Update
         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"}}]                | |

         Version: "up12vyc5ib"                              | Update
         Parents: "2bcbi84nsp"                              |
         Content-Type: application/json                     |
         Merge-Type: sync9                                  |
         Patches: 1                                         |
                                                            |
         Content-Length: 288                                | | Patch
         Content-Type: application/json-patch+json          | |
                                                            | |
         [                                                  | |
          {"op": "test", "path": "/a/b/c", "value": "foo"}, | |
          {"op": "remove", "path": "/a/b/c"},               | |
          {"op": "add", "path": "/a/b/c", "value": []},     | |
          {"op": "replace", "path": "/a/b/c", "value": 42}, | |
          {"op": "move", "from": "/a/b", "path": "/a/d"},   | |
          {"op": "copy", "from": "/a/d", "path": "/a/d/e"}  | |
         ]                                                  | |

Perhaps we want to allow any header to be promoted to a higher level, or specified at a lower level, to change the current state of the resource, update/version, or patch.

This would allow us to reduce the example above to:

         HTTP/1.1 209 Subscription
         Subscribe: keep-alive
         Merge-Type: sync9
         Content-Type: application/json

         Version: "ej4lhb9z78"                              | Update
         Parents: "oakwn5b8qh", "uc9zwhw7mf"                |
         Content-Length: 64                                 |
                                                            |
         [{"text": "Hi, everyone!",                         | | Snapshot
           "author": {"link": "/user/tommy"}}]              | |

         Version: "g09ur8z74r"                              | Update
         Parents: "ej4lhb9z78"                              |
         Patches: 1                                         |
                                                            |
         Content-Length: 53                                 | | Patch
         Content-Range: json .messages[1:1]                 | |
                                                            | |
         [{"text": "Yo!",                                   | |
           "author": {"link": "/user/yobot"}]               | |

         Version: "2bcbi84nsp"                              | Update
         Parents: "g09ur8z74r"                              |
         Patches: 1                                         |
                                                            |
         Content-Length: 58                                 | | Patch
         Content-Range: json .messages[2:2]                 | |
                                                            | |
         [{"text": "Hi, Tommy!",                            | |
           "author": {"link": "/user/sal"}}]                | |

         Version: "up12vyc5ib"                              | Update
         Parents: "2bcbi84nsp"                              |
         Patches: 1                                         |
                                                            |
         Content-Length: 288                                | | Patch
         Content-Type: application/json-patch+json          | |
                                                            | |
         [                                                  | |
          {"op": "test", "path": "/a/b/c", "value": "foo"}, | |
          {"op": "remove", "path": "/a/b/c"},               | |
          {"op": "add", "path": "/a/b/c", "value": []},     | |
          {"op": "replace", "path": "/a/b/c", "value": 42}, | |
          {"op": "move", "from": "/a/b", "path": "/a/d"},   | |
          {"op": "copy", "from": "/a/d", "path": "/a/d/e"}  | |
         ]                                                  | |

My only concern is that parsers could become trickier to write. They would need to look for the same headers at different levels of the AST. I would like to try implementing it to find out.

toomim commented 1 year ago

Chair: Consolidating this discussion into https://github.com/braid-org/braid-spec/issues/116, and closing this issue.