Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Better file support for kv:put #633

Merged
merged 2 commits into from
Sep 20, 2019
Merged

Better file support for kv:put #633

merged 2 commits into from
Sep 20, 2019

Conversation

phayes
Copy link
Contributor

@phayes phayes commented Sep 19, 2019

This PR adds better file support for kv:put

  1. Allows non-utf8 files to be used as values
  2. Streams the file directly from disk to the endpoint, instead of buffering them in memory first.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…om disk to the endpoint
@gabbifish
Copy link
Contributor

@phayes Thank you for this PR! This is fantastic.

@gabbifish gabbifish self-requested a review September 19, 2019 22:40
Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

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

one tiny change request :)

// If is_file is true, overwrite value to be the contents of the given
// filename in the 'value' arg.
let body_text = if is_file {
let mut res = if is_file {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be mutable?

Copy link
Contributor Author

@phayes phayes Sep 20, 2019

Choose a reason for hiding this comment

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

Weirdly, it does need to be mutable. (rustc complains if I remove the mut).

On line 73 we have let parsed = res.json();, which needs the response to be mutable. The signature of this method is:

pub fn json<T: DeserializeOwned>(&mut self) -> ::Result<T>

I think this is because when we return the response, the body hasn't been read yet. And if we have an error then we need to read the body, which mutates the response as we're reading it in.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense (and TIL)!

Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

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

LGTM!

@gabbifish gabbifish merged commit f5a3b86 into cloudflare:master Sep 20, 2019
@kristianfreeman kristianfreeman added feature Feature requests and suggestions changelog - feature labels Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Feature requests and suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants