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

Added basic support for multi-part form uploads in HTTPClient #1178

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added basic support for multi-part form uploads in HTTPClient #1178

wants to merge 3 commits into from

Conversation

etcimon
Copy link
Contributor

@etcimon etcimon commented Jul 10, 2015

This still needs a test, but the logic should be right. These two objects (file, memory) will cover 99% of what's usually needed in multi-part form uploads. The alternative to using these is a lot of boilerplate and some specialized low-level protocol knowledge.

I'll write the test to complete the pull request if the structure is satisfying.

@etcimon
Copy link
Contributor Author

etcimon commented Jul 10, 2015

Any comments on the interface for this in general? Should I make the headers editable after the object is created?

@@ -599,6 +599,18 @@ final class HTTPClientRequest : HTTPRequest {
finalize();
}

/// Writes a multipart upload as a body to the request. This must be written in its entirety because the Content-Length
/// must be known beforehand.
/// Usage: req.writeBody(new FileMultiPart(req.headers, "Photo", "images/picture.jpg"));
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be reformatted to be rendered properly in the documentation:

/** Writes a multipart form as a body to the request.

    This must be written in its entirety because the Content-Length
    must be known beforehand.

    Usage: `req.writeBody(new FileMultiPart(req.headers, "Photo", "images/picture.jpg"));`
*/

Ideal would be to put the example into a documented unit test to verify that it doesn't get out of date.

@s-ludwig
Copy link
Member

Instead of putting the headers (and string data) into MemoryStreams, it would be good to instead directly write to the connection. I guess a similar approach as used for writeJsonBody can be used here (i.e. once generate the output on a special counting range to determine the length and then generate again and write to a StreamOutputRange). formattedWrite could also be used to make the header generation code a bit shorter.

@etcimon
Copy link
Contributor Author

etcimon commented Aug 16, 2015

Instead of putting the headers (and string data) into MemoryStreams, it would be good to instead directly write to the connection.

I think the headers need to be kept aside until being written because the multipart forms were designed like linked lists and can be nested, and the content-length needs to be known beforehand in many browsers/parsers.

@s-ludwig
Copy link
Member

I think the headers need to be kept aside until being written because the multipart forms were designed like linked lists and can be nested, and the content-length needs to be known beforehand in many browsers/parsers.

What about my suggestion to use a counting range to determine the length? I guess that should be pretty fast and should be an overall win over using dynamic memory allocations.

@etcimon
Copy link
Contributor Author

etcimon commented Aug 16, 2015

What about my suggestion to use a counting range to determine the length? I guess that should be pretty fast and should be an overall win over using dynamic memory allocations.

I have a hard time visualizing it, but as long as you have the length before you start writing the body it should be fine. After all, writePart doesn't support chunked transfer and content-length header is required so the name "Part" is a little misleading, it should be writeMultiformBody or something like that.

@WebFreak001
Copy link
Contributor

WebFreak001 commented Oct 31, 2017

@s-ludwig can you merge either this or #1876? I think this is a very useful feature and shouldn't have both PRs left unmerged

@dariusc93
Copy link
Contributor

Any update in regards to adding multipart for HTTPClient? Other than the conflicts github is reporting, is there anything else that would be required for this to get pulled into vibe-d or is there already a multipart implementation in works that isnt pushed into the master branch?

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.

4 participants