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

What is Patches: OK for? #67

Closed
toomim opened this issue Jun 8, 2020 · 8 comments
Closed

What is Patches: OK for? #67

toomim opened this issue Jun 8, 2020 · 8 comments
Assignees

Comments

@toomim
Copy link
Member

toomim commented Jun 8, 2020

I noticed that the example HTTP responses have a Patches: OK header, but these aren't actually discussed in the spec. What are these for? We should either explain them, or remove them from the examples.

Here's an example: https://github.com/braid-work/braid-spec/blob/master/draft-toomim-httpbis-braid-http-03.txt#L253

@balupton
Copy link

Seems it is a way for the server to confirm to the client that it understood the response and knew how to process the response. As just a 200 OK response doesn't indicate anything from the server that it is braid-supported endpoint.

Somewhat akin to how if a server doesn't know about braid, it shouldn't be using the Version header in its responses, which would indicate to the client, that it does know what braid is.

If the server does not support historical versions, it MAY ignore
the Version header and respond as usual, but MUST NOT include the
Version header in its response.

@balupton
Copy link

balupton commented Jan 19, 2021

That said, they could probably just respond with the version or parents header that was sent, as a confirmation.

Also, what occurs here if the patch was not okay? Like the conflict resolver algorithm failed to merge it, or it threw an exception. Shall there be a Patches: Not OK header? Shall an existing HTTP status code be used? Some details about this would be nice, as right now the only detailed error case is:

If a server has dropped the history that a client requests, the
server can return a 410 GONE response, to tell the client "sorry, I
don't have the history necessary to synchronize with you."

@toomim toomim added this to the Braid-HTTP 03 milestone Feb 13, 2021
@toomim
Copy link
Member Author

toomim commented Feb 20, 2021

Two more things:

  1. The goal of this Patches: OK header might be redundant with the Accept-Range-Patch: header proposed in Maybe add Accept-Range-Patch header #49.

  2. In addition to needing to define Patches: OK in responses, we also need to better define Patches: N in requests. Specifically, I think we should point out that Patches: 1 can be semantically equivalent to omitting a Patches header:

PUT /chat
Version: "a9x7"
Content-Type: "application/json"
Patches: 1

Content-Range: json [-0:-0]
Content-Length: 80

{text: "Hey! This is a new message"}
PUT /chat
Version: "a9x7"
Content-Type: "application/json"
Content-Range: json [-0:-0]
Content-Length: 80

{text: "Hey! This is a new message"}

These two requests are identical in semantics. I think it would help to make this clear in the spec. We could say explicitly that "HTTP already allows a single patch in a request body, but the Patches: N header allows multiple patches to be expressed." This might help us understand what the Patches: N and Patches: OK headers mean.

@josephg
Copy link
Contributor

josephg commented Mar 11, 2021

If the intent is to tell clients information about allowed HTTP methods, this information might be better off added to an OPTIONS response. (I might want to send patches to a document without ever subscribing to it.)

@josephg
Copy link
Contributor

josephg commented Mar 17, 2021

Also I think this issue is inverted. The issue should be "I propose adding Patches: OK header with XXX semantics to solve YYY use case".

Our default action should be to remove the header, unless that case can be sustained with good justification.

@josephg
Copy link
Contributor

josephg commented Mar 22, 2021

  • Does the server allow patches from the client
  • If so, what type?
  • Are patches going to be sent in subscriptions?
  • If so, what types?

Moving forward:

  • Remove Patches: OK header from spec for 03
  • Loop back for milestone 04

@toomim toomim removed this from the Braid-HTTP 03 milestone Mar 22, 2021
@toomim toomim self-assigned this Aug 13, 2023
@toomim
Copy link
Member Author

toomim commented Aug 13, 2023

I've been looking into this, and now I believe the Patches: OK header was an early attempt at advertising a server's ability to accept a patch. It's doing the same thing as #49.

This is necessary to prevent data loss, when a client tries to submit a range-patch as a PUT with a range:, but the server doesn't understand how to do so. The server is likely to ignore the range: header, and just obliterate the existing contents with the patch itself, instead of applying the patch to the contents.

@toomim
Copy link
Member Author

toomim commented Aug 20, 2023

PR is implemented in #119.

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

No branches or pull requests

3 participants