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

Resumable Uploads: application/partial-upload media type #2743

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Acconut
Copy link
Member

@Acconut Acconut commented Mar 1, 2024

As discussed in #2501 and #2610, the PATCH requests needs a suiting media type to describe the message content.

Since there does not seem to be an existing, suitable media type, we proposed the application/partial-upload media type on the media-types mailing list. This PR is a first, rough attempt at including such a media type in the draft.

@@ -324,7 +324,7 @@ The client SHOULD NOT automatically retry if a client error status code between

Upload appending is used for resuming an existing upload.

The request MUST use the `PATCH` method and be sent to the upload resource. The `Upload-Offset` field value ({{upload-offset}}) MUST be set to the resumption offset.
The request MUST use the `PATCH` method and `application/partial-upload` media type and be sent to the upload resource. The `Upload-Offset` field value ({{upload-offset}}) MUST be set to the resumption offset.
Copy link

Choose a reason for hiding this comment

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

I feel that this sentence is a bit off, probably due to language barriers on my end.

Suggested edit:

The request MUST use the PATCH method with application/partial-upload as content type and be sent to the upload resource. The Upload-Offset field value ({{upload-offset}}) MUST be set to the resumption offset.

@@ -336,6 +336,8 @@ The client MUST NOT perform multiple upload transfers for the same upload resour

If the offset indicated by the `Upload-Offset` field value does not match the offset provided by the immediate previous offset retrieval ({{offset-retrieving}}), or the end offset of the immediate previous incomplete successful transfer, the server MUST respond with a `409 (Conflict)` status code.

The server applies the patch document of the `application/partial-upload` media type by appending the unmodified request content to the targeted upload resource.
Copy link

Choose a reason for hiding this comment

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

To me "unmodified" reads as "take the entire thing and apply it without changes" which is probably not what is intended here? The server is not allowed to pick bytes that it appends, but I think we need to clarify that the entire request must not complete for the server to append data. I.e. if the connection breaks down after 100 bytes then as much as possible of those 100 bytes should be appended and what is appended must be appended without modification.

# Media Type `application/partial-upload`

The `application/partial-upload` media type describes a contiguous block of data that should be uploaded to a resource. There is no minimum size on the block of data and it might be empty. The start and end of block of data might align with the start and end of the file that should be uploaded, but are not required to do so.

Copy link

Choose a reason for hiding this comment

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

Minor nit:

The start and end of THE block of data might align with the start and end of the file that should be uploaded, but are not required to do so.

or

The start and end of THE data might align with the start and end of the file that should be uploaded, but are not required to do so.

Should we avoid using "file" as the thing to be uploaded and use something more generic like "resource that should be uploaded"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants