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.

Sign up for GitHub

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

Jump to bottom

Add API for Headers on Parts of Multipart Form Data #331

Merged
merged 1 commit into from Sep 4, 2018
Merged

Add API for Headers on Parts of Multipart Form Data #331

merged 1 commit into from Sep 4, 2018

Conversation

jcaesar
Copy link
Contributor

@jcaesar jcaesar commented Aug 23, 2018

When working on rust-ipfs-api, I saw that it uses a somewhat uncommon format for some requests:

POST /api/v0/add?chunker=size-262144&encoding=json&hash=sha2-256&nocopy=true&pin=false&progress=true&recursive=false&stream-channels=true HTTP/1.1
Host: 127.0.0.1:5001
User-Agent: go-ipfs-cmds/http
Connection: close
Transfer-Encoding: chunked
Content-Type: multipart/form-data; boundary=cfc4c1c4ad2e8ad270952524bc619a97033c8e6429a52d1358992325ed8b
Accept-Encoding: gzip

--cfc4c1c4ad2e8ad270952524bc619a97033c8e6429a52d1358992325ed8b
Abspath: /home/ipfs/gentoo/releases/verify-digests.sh
Content-Disposition: file; filename="verify-digests.sh"
Content-Type: application/octet-stream

#!/bin/bash
# Name: verify-digests.sh
…

The Abspath header on the form data part is critical: it tells the server where to find the file on hard disk if it needs to re-read it later. (Whether that is good design…)

This PR does add a patch that allows composing requests like the above, but considering my unfamiliarity with Rust and the discussion in #312 I doubt that you want to accept it like this. If possible, I'd like you to consider this a feature request, and a request to tell me how to implement the functionality properly.

@seanmonstar
Copy link
Owner

Being able to set headers for a part seems reasonable. I'd suggest that instead of a HashMap, instead just have it be an http::HeaderMap.

@jcaesar jcaesar force-pushed the master branch 2 times, most recently from ccdc831 to e5a3189 Compare August 26, 2018 01:30
@jcaesar
Copy link
Contributor Author

jcaesar commented Aug 26, 2018

I've switched to http::HeaderMap and added a little test. I noticed that this may cause a re-appearance of hyperium/hyper#1492 Not sure what to do about that.

seanmonstar
seanmonstar reviewed Aug 29, 2018
View reviewed changes
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Looking good! I'm not worried yet about the lower-casing, it can dealt with in the future if it causes problems.

src/multipart_.rs Outdated
use http;

/// An alias to http::HeaderMap so http is unneded as an explicit dependency
pub type HeaderMap = http::HeaderMap<String>;
Copy link
Owner

Choose a reason for hiding this comment

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

I'd remove this alias, and use the default http::HeaderMap which uses HeaderValue instead of String.

src/multipart_.rs Outdated
@@ -197,6 +203,21 @@ impl Part {
self.file_name = Some(filename.into());
self
}

/// Returns a reference to the map of additional header fields
pub fn header_fields(&self) -> &HeaderMap {
Copy link
Owner

Choose a reason for hiding this comment

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

To match with the other APIs, I'd call this method just headers.

src/multipart_.rs Outdated
}

/// Returns a mutable reference to the map of additional header fields
pub fn header_fields_mut(&mut self) -> &mut HeaderMap {
Copy link
Owner

Choose a reason for hiding this comment

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

And this method could be named headers_mut.

src/multipart_.rs Outdated
}

/// Replaces the additional header fields
pub fn replace_header_fields(&mut self, hdr: HeaderMap) -> () {
Copy link
Owner

Choose a reason for hiding this comment

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

This method shouldn't be needed, as anyone can do the same with std::mem::replace combined with headers_mut.

src/multipart_.rs Outdated
@@ -205,6 +226,7 @@ impl fmt::Debug for Part {
.field("value", &self.value)
.field("mime", &self.mime)
.field("file_name", &self.file_name)
.field("hdr", &self.hdr)
Copy link
Owner

Choose a reason for hiding this comment

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

For debugging, we may as well call it the full word, headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the struct field, too…

src/multipart_.rs Outdated
field.hdr.iter().fold(
"".to_string(),
|hdrs, (k,v)|
format!("{}\r\n{}: {}", hdrs, k.as_str(), v)
Copy link
Owner

Choose a reason for hiding this comment

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

This will actually allocate a new string for each header, whereas they could just be pushed onto the existing one... Though, there shouldn't be that many for a single part, so it's probably not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope + on String does what I think it does.

@jcaesar jcaesar force-pushed the master branch 3 times, most recently from 0bb0824 to e520e9f Compare September 4, 2018 14:30
@jcaesar
Copy link
Contributor Author

jcaesar commented Sep 4, 2018

Review implemented and rebased on master (with pub(crate)). I hope I got the rebase right…

seanmonstar
seanmonstar approved these changes Sep 4, 2018
View reviewed changes
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for sticking with it!

@seanmonstar seanmonstar merged commit ca42e77 into seanmonstar:master Sep 4, 2018
@jcaesar
Copy link
Contributor Author

jcaesar commented Sep 5, 2018

Thank you for merging and enhancing it. (And sorry for the delay. I was moving over the weekend.)

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

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jcaesar @seanmonstar