Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename Content-*Patch-* within Patches #120

Closed
wants to merge 2 commits into from

Conversation

toomim
Copy link
Member

@toomim toomim commented Aug 21, 2023

Draft Renaming Scheme for Issue #97

Synopsis of the issue:

  • We originally re-used HTTP's top-level Content-Length, Content-Type, and Content-Range headers to describe patches
  • But we noticed they read clearer as Patch-Length, Patch-Type, and Patch-Range in subscriptions
    • Semantically, these are more about "patches" than "content"
    • Patch-* more clearly separates the second level of patch headers from top-level Content-* headers
  • I'm hesitant to clobber or make redundant HTTP's existing Content-* headers at top-level
  • Question: How do we reconcile Patch-* with the top-level Content-* headers?
    • Consider: The common case of Patches: 1 is equivalent to a top-level HTTP patch

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):

  • Content-Length
  • Content-Range
  • Patch-Type

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

  • Patch-Length
  • Patch-Range
  • Patch-Type

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:

  • Uses Content-* uniformly.
  • Requires prior knowledge or feature detection on legacy servers.

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:

  • Unchanged.
  • Safe on legacy servers.

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:

  • Safe on legacy servers

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:

  • Uses Patch-* uniformly for all the patch information.

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:

  • Uses Content-* uniformly.

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:

  • Uses Patch-* uniformly for all the patch information.

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:

  • Uses the same Content-* as top-level patches in previous example.
  • Safe on legacy servers.

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:

  • Unchanged.
  • Safe on legacy servers.

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:

  • Safe on legacy servers.

Thoughts?

@toomim
Copy link
Member Author

toomim commented Aug 22, 2023

Discussion in Meeting-67:

  • Greg: Consider a single range header with multiple ranges, which describes multiple patches.
    • Could define range unit that allows multiple ranges
  • Paul: With greg on this— parsing the current format was difficult
  • Austin:
    • Multipart media lets you concat multiple parts
    • Can't use content-length header in streaming responses, with chunked encoding
    • Range is currently limited to only specifying integers in range, so you couldn't have the .messages[1:1], and HTTPWG seems very opinionated about requiring integers.
      • Today you can do Content-Range: bytes 1-5/6, and I proposed Content-Range: bytes 1-/* to express an offset of 1 byte with potentially indefinite length so we could append a stream to the end of byte 1, and they were kinda skeptical of that change. The discussion here is from email by mnot in call for adoption of my byte-range-patch proposal on the HTTPAPI mailing list.
  • Rahul: eventually we'll want to do framing with HTTP/2. We should think about that because it can impact the headers we're using at the top-level.
    • If we're using headers that combine patches together in a single header, that probably won't work as well if we're using http2 frames to separate them.
  • Paul: I support this renaming. It expresses the intent better than the original naming. But to me, the question of framing/packaging is still related to the names, because if we use multipart then we will be able to use content-type without any restrictions, so we won't need a separate header for that. Multipart also allows hierarchical structure, so you can express what greg is saying—one set of headers, but multiple changes within the same structure. You'd get that probably for free because lots of toolkits would already support that.
  • Mike: Do we want to open messasge framing for deliberation on draft 03?
    • Austin: I'm most concerned about what the HTTPWG will want to accept. "There's already a solution to do exactly this." If we can identity shortcomings, then we can raise it as a separate issue. Otherwise, use the syntax that's already standardized.
    • Rahul: 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.
    • Paul: I don't mind waiting. But I think it will have some impact on the decisions made in this version—so some of those decisions may need to be revisited.

@toomim
Copy link
Member Author

toomim commented Sep 3, 2023

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
Copy link

CxRes commented Sep 3, 2023

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
Copy link
Contributor

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.

@@ -358,7 +358,7 @@ Table of Contents
Patches: OK


Every patch MUST include either a Content-Type or a Content-Range.
Every patch MUST include either a Patch-Type or a Content-Range.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I missed a Content-Range->Patch-Range here. This one should be changed, too.

@toomim toomim changed the title Rename Content-{Length,Type,Range} → Patch-{Length,Type,Range} Rename Content-*Patch-* Sep 4, 2023
@toomim toomim changed the title Rename Content-*Patch-* Rename Content-*Patch-* within Patches Sep 5, 2023
@toomim
Copy link
Member Author

toomim commented Sep 18, 2023

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:

  • A PUT request
  • A POST request
  • A POST response
  • A PATCH request
  • A GET response
  • A GET Subscription

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
Copy link
Member Author

toomim commented Oct 18, 2023

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

@toomim toomim closed this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants