braid-org / braid-spec

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

Rename `Content-*` → `Patch-*` within Patches #120

Closed toomim closed 11 months ago

toomim commented 1 year ago

Draft Renaming Scheme for Issue #97

Synopsis of the issue:

This PR proposes a naming rule, illustrates it with examples, and provides edits to the spec to support it.

The Proposed Rule

Updates expressed at the top level of PUT and GET use existing HTTP headers (except Patch-Type, which is new):

Updates expressed within a Patches: N block use the Patch-* variant:

The PATCH method continues to use Content-Type instead of Patch-Type.

Advantages

Examples

Range Patch with PUT at top level

     PUT /chat
     Content-Type: application/json
     Content-Range: json .messages[1:1]
     Content-Length: 189

     [{"text": "Yo!",
       "author": {"link": "/user/yobot"}]

Notes:

Range Patch with PUT within a Patches: 1 block

     PUT /chat
     Content-Type: application/json
     Patches: 1

     Patch-Range: json .messages[1:1]
     Patch-Length: 189

     [{"text": "Yo!",
       "author": {"link": "/user/yobot"}]

Notes:

Custom Patch Type at top-level using PATCH

     PATCH /chat
     Content-Type: text/json-ot
     Content-Length: 8

     [0, 'n']

Notes:

Custom Patch Type with PUT at top-level

     PUT /chat
     Content-Type: application/json
     Patch-Type: text/json-ot
     Content-Length: 8

     [0, 'n']

Note:

Custom Patch Type using PUT within Patches: 1:

     PUT /chat
     Content-Type: application/json
     Patches: 1

     Patch-Type: text/json-ot
     Patch-Length: 8

     [0, 'n']

Note:

GET Subscription with Patches: N blocks

     HTTP/1.1 209 Subscription
     Subscribe:
     Version: "g09ur8z74r"                              | Update
     Parents: "ej4lhb9z78"                              |
     Content-Type: application/json                     |
     Merge-Type: sync9                                  |
     Patches: 1                                         |
                                                        |
     Patch-Range: json .messages[1:1]                   | | Patch
     Patch-Length: 53                                   | |
                                                        | |
     [{"text": "Yo!",                                   | |
       "author": {"link": "/user/yobot"}]               | |

     Version: "2bcbi84nsp"                              | Update
     Parents: "g09ur8z74r"                              |
     Content-Type: application/json                     |
     Merge-Type: sync9                                  |
     Patches: 1                                         |
                                                        |
     Patch-Range: json .messages[2:2]                   | | Patch
     Patch-Length: 58                                   | |
                                                        | |
     [{"text": "Hi, Tommy!",                            | |
       "author": {"link": "/user/sal"}}]                | |

Note:

GET Subscription with snapshots

     HTTP/1.1 209 Subscription
     Subscribe:
     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: "g2hd8asdf6"                              | Update
     Parents: "ej4lhb9z78"                              |
     Content-Type: application/json                     |
     Merge-Type: sync9                                  |
     Content-Length: 63                                 |
                                                        |
     [{"text": "Hi, someone!",                          | | Snapshot
       "author": {"link": "/user/tommy"}}]              | |

Note:

GET responding with Custom Patch Type in Patches: N block

     HTTP/1.1 200 OK
     Version: "up12vyc5ib"                              | Update
     Parents: "2bcbi84nsp"                              |
     Content-Type: application/json                     |
     Merge-Type: sync9                                  |
     Patches: 1                                         |
                                                        |
     Patch-Type: application/json-patch+json            | | Patch
     Patch-Length: 288                                  | |
                                                        | |
     [                                                  | |
      {"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"}  | |
     ]                                                  | |

Note:

GET responding with Custom Patch Type at top level

Note: Impossible, because Content-Type clobbers.

Comparisons with current spec

Range Patch with PUT at top-level

     PUT /chat
     Content-Type: application/json
     Content-Range: json .messages[1:1]
     Content-Length: 189

     [{"text": "Yo!",
       "author": {"link": "/user/yobot"}]

Notes:

Range Patch with PUT within Patches: 1 block

     PUT /chat
     Content-Type: application/json
     Patches: 1

     Content-Range: json .messages[1:1]
     Content-Length: 189

     [{"text": "Yo!",
       "author": {"link": "/user/yobot"}]

Notes:

Custom Patch Type with PUT is Impossible

Note: Impossible, because Content-Type clobbers:

     PUT /chat
     Content-Type: application/json        <-- Type of the /chat resource
     Content-Type: text/json-ot            <-- Type of the custom patch format
     Content-Length: 8

     [0, 'n']

Custom Patch Type with PATCH

     PATCH /chat
     Content-Type: text/json-ot
     Content-Length: 8

     [0, 'n']

Notes:

Custom Patch Type within a Patches: 1 block

Uses Content-*

     PUT /chat
     Content-Type: application/json
     Patches: 1

     Content-Type: text/json-ot
     Content-Length: 8

     [0, 'n']

Notes:

Thoughts?

toomim commented 1 year ago

Discussion in Meeting-67:

toomim commented 1 year ago

Chair summary: The discussion above (from Meeting-67) is mostly about framing rather than header names, which we should discuss in a separate issue and PR.

On the other hand, @CxRes points out that if we do switch to multipart framing, and want to obey the MIME spec strictly, we might be forced to use header names that are prefixed with Content-*:

There's one restriction in multipart that all headers must start with content-*, so if you're using multipart, you can't have an individual part that starts with patch-* or something like that.

We had further elaboration about the MIME spec on Discord:

@pkulchenko:

Just to follow-up on our discussion about headers in MIME multipart/* messages. The MIME spec has the following wording (https://www.rfc-editor.org/rfc/rfc2045#section-9):

Additional MIME Header Fields

   Future documents may elect to define additional MIME header fields
   for various purposes.  Any new header field that further describes
   the content of a message should begin with the string "Content-" to
   allow such fields which appear in a message header to be
   distinguished from ordinary RFC 822 message header fields.

which appears to indicate that only Content-* headers are allowed and yet RFC1341 example has From/Subject headers: https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html. Not sure yet how to interpret this.

@aawright:

It's just saying that headers that describe the content should begin with Content- and headers that don't (e.g. how to route the message) shouldn't... I haven't seen anything that should pose a problem, and if the requirement is there, you just have to ask, what interoperability purpose is this serving? And maybe we find out the original reason turns out to be obsolete.

So as we evaluate naming in this draft, we can keep in mind that if we later want to adopt MIME multipart framing, we might (or might not) be restricted to using Content-* names.

CxRes commented 1 year ago

RFC2046 -> 5.1. Multipart Media Type Page 17

The only header fields that have defined meaning for body parts are those the names of which begin with "Content-". All other header fields may be ignored in body parts. Although they should generally be retained if at all possible, they may be discarded by gateways if necessary. Such other fields are permitted to appear in body parts but must not be depended on. "X-" fields may be created for experimental or private purposes, with the recognition that the information they contain may be lost at some gateways.

This is not a real impediment, even if you have to follow the standard strictly, provided you accept the following trick. You can leave the header of the part (of the multipart) empty (or with any Content-* headers), leave the empty line that separates headers and body of the part, and then put Patch-* headers in the body. The compromise is that body is itself of the format message/rfc822 (which is the default for mutipart/mixed) with the body of the part body being the actual patch.

pkulchenko commented 1 year ago

This is not a real impediment, even if you have to follow the standard strictly, provided you accept the following trick. You can leave the header of the part (of the multipart) empty (or with any Content- headers), leave the empty line that separates headers and body of the part, and then put Patch- headers in the body.

I'd not be happy about this trick, as this approach is similar to the one that is currently used (although without multipart/* formatting), but it requires some special processing of message bodies (to assume that they may start with headers), which makes it difficult to work with existing web frameworks and tools.

toomim commented 1 year ago

After some reflection, I want to withdraw this proposal. I'll explain why here.

I'm seeing a deeper meaning to why everyone is talking about framing here— this PR is subtly (and confusingly) trying to solve a framing issue by making a naming change. Recall that the motivation for this PR is that Patch-* "reads clearer" by "separating the second level of patch headers from top-level Content-* headers."

The framing issue we're trying to solve is just a perceptual aesthetic— when an update uses a Patches: N header to embed multiple patches, you get lots of blocks separated by lots of blank lines, and it's hard for a human to visually distinguish the start and end of an Update vs. the start and end of each Patch, which is why we add those explanatory | Update and | Patch notes to distinguish regions in the spec:

     HTTP/1.1 209 Subscription
     Subscribe:
     Version: "g09ur8z74r"                              | Update
     Parents: "ej4lhb9z78"                              |
     Content-Type: application/json                     |
     Merge-Type: sync9                                  |
     Patches: 2                                         |
                                                        |
     Content-Range: json .messages[1:1]                 | | Patch
     Content-Length: 53                                 | |
                                                        | |
     [{"text": "Yo!",                                   | |
       "author": {"link": "/user/yobot"}]               | |
                                                        |
     Content-Range: json .messages[2:2]                 | | Patch
     Content-Length: 53                                 | |
                                                        | |
     [{"text": "Go!",                                   | |
       "author": {"link": "/user/gobot"}]               | |

     Version: "2bcbi84nsp"                              | Update
     Parents: "g09ur8z74r"                              |
     Content-Type: application/json                     |
     Merge-Type: sync9                                  |
     Patches: 1                                         |
                                                        |
     Content-Range: json .messages[2:2]                 | | Patch
     Content-Length: 58                                 | |
                                                        | |
     [{"text": "Hi, Tommy!",                            | |
       "author": {"link": "/user/sal"}}]                | |

However, having multiple Patches per Update is actually the rare case. The common case will be a single Patch per Update, like this:

     HTTP/1.1 209 Subscription
     Subscribe:
     Version: "g09ur8z74r"                              | Update
     Parents: "ej4lhb9z78"                              |
     Content-Type: application/json                     |
     Merge-Type: sync9                                  |
     Content-Range: json .messages[1:1]                 |
     Content-Length: 53                                 |
                                                        |
     [{"text": "Yo!",                                   |
       "author": {"link": "/user/yobot"}]               |

     Version: "2bcbi84nsp"                              | Update
     Parents: "g09ur8z74r"                              |
     Content-Type: application/json                     |
     Merge-Type: sync9                                  |
     Content-Range: json .messages[2:2]                 |
     Content-Length: 58                                 |
                                                        |
     [{"text": "Hi, Tommy!",                            |
       "author": {"link": "/user/sal"}}]                |

It's not as big a deal here.

And unfortunately, this PR has the cost of making header names inconsistent in the rare case, by renaming all Content-Range and Content-Length headers to Patch-Range and Patch-Length, just because they appear within a multi-patch update.

I think this change is not worth the cost.

Consider that Patches can appear in many places in HTTP messages:

It would be very nice to have a uniform syntax for patches across all situations!

But this PR introduces an inconsistency—any patch within a Patches: N block has to use a different set of names. That's going to feel arbitrary and confusing.

I don't think we should be solving one confusing thing by introducing another confusion. If we want to make framing more legible, I think we should change framing directly.

So I offer to withdraw this proposal to rename Content-Range and Content-Length within Patches: N.

On the other hand, for the specific issue of specifying the type of a patch, I think it does makes sense to use Patch-Type instead of Content-Type. I'll submit a separate PR for that.

toomim commented 11 months ago

(Chair:) Nobody has objected. Withdrawing this PR.