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

How can I send a multipart POST request? #181

Open
TikhonJelvis opened this Issue Oct 29, 2014 · 12 comments

Comments

Projects
None yet
6 participants
@TikhonJelvis

TikhonJelvis commented Oct 29, 2014

How can I send a multipart POST request (ie with the Client module)? I'm using the lwt-based API, if that's important.

Currently, I'm working on some bindings for the Inbox API and need to send a multipart request to upload an email attachment.

@dsheets

This comment has been minimized.

Member

dsheets commented Oct 29, 2014

Do you want to send a multipart/form-data message? As far as I know, cohttp does not provide any functionality at the HTTP message body layer such as multipart subtype codecs, boundary handling, or part headers.

It may make sense to add this functionality to cohttp or to create a new library, say, "multipart-form-data", and have that depend on cohttp for header processing. Or perhaps the functionality should start in cohttp and be split out if it seems to be getting large. This would make the extension of cohttp's header processing easier, for example.

As you know, you can probably directly compose a multipart/form-data message without any specific functional or type support but there are dangers in going that route. If you do end up writing some generic multipart/form-data code, I'm sure this project would be happy to integrate it.

@TikhonJelvis

This comment has been minimized.

TikhonJelvis commented Oct 29, 2014

Yep, multipart/form-data. I'll have to read a bit more about the specific details involved, but if I do end up writing some generic code for it I'll definitely try to contribute it back.

@avsm avsm referenced this issue Nov 8, 2014

Closed

1.0 release checklist #198

2 of 7 tasks complete

@avsm avsm modified the milestone: 1.0 Stable API Nov 9, 2014

@emillon

This comment has been minimized.

Contributor

emillon commented Jun 9, 2016

Hi,
I'd like to process multipart/form-data in a cohttp server. It seems to me that this issue applies to server code too, so if you're still interesting in integrating this I may be able to provide a parser (there are pieces of code doing that in ocamlnet and ocsigen but I'm not sure how extractable it is).

@emillon

This comment has been minimized.

Contributor

emillon commented Jun 14, 2016

I pushed my WIP parser to https://github.com/cryptosense/multipart-form-data while I'm refining the API. Once it's stable enough we can discuss if it's better to keep it as an external repo or if it's better maintained as part of cohttp. Le me know what you think.

@avsm

This comment has been minimized.

Member

avsm commented Jun 23, 2016

I'd definitely like to integrate this into Cohttp...

@nv-vn

This comment has been minimized.

nv-vn commented Jun 24, 2016

Sweet! Been waiting for something like this for a while, glad it's finally happen. For clarification, does this currently implement sending multipart/form-data or is it just for parsing? I'd be happy to add sending if that's something you still need, as I have written (somewhat fragile) code for this in the past and I'd be happy to contribute it/refine it if that's something that's needed.

@emillon

This comment has been minimized.

Contributor

emillon commented Jun 27, 2016

Great!

I think that we can centralize the development on cryptosense/multipart-form-data until we can have a feature-complete PR.

There are indeed a couple things that are not satisfying. In particular:

  • there's only the server side implementation done. There's some working code in #293 (cc @rgrinberg). @nv-vn I'll happily accept that!
  • there are a couple Lwt_stream.clone calls to store the state of iterators. This is so that if there are two files in the request, their contents can be exposed as two Lwt_stream.t at the correct positions. I am not very familiar with lwt semantics but it does not seem to make deep copies. A couple benchmarks would be necessary to make sure. If anyone's interested in reviewing it I'm very interested.
  • there's a quadratic function called within a loop (cryptosense/multipart-form-data#3). Once again, benchmarking would be necessary to determine whether it can blow up or not (it's only called once per chunk, so I assume that it's not a problem unless the Cohttp_lwt_body being processed has chunks of the order of the length of boundary).
  • more tests are needed. I'm sure that there are a couple unquote calls missing, that header parsing will fail if the filename contains a semicolon, that trailing newlines may be ignored, etc.

Thanks to everyone interested!

@talex5

This comment has been minimized.

Contributor

talex5 commented Oct 25, 2016

@emillon I just tried your library - it seems to be working fine! How close is it to being released?

@emillon

This comment has been minimized.

Contributor

emillon commented Oct 25, 2016

I fixed the performance issue and we're using it in production without problems. The file API is not super satisfying but can be improved later. I think I'll prepare a PR on cohttp in the next few days if you're OK with that plan.

@talex5

This comment has been minimized.

Contributor

talex5 commented Oct 25, 2016

That would be great - thanks!

@avsm

This comment has been minimized.

Member

avsm commented Nov 3, 2016

@emillon just wanted to check in on the status of this PR, as it would be good to get it into a release. Also could the license be ISC to match the current repo (as the multipart-form-data is currently MIT -- almost the same but different :-)

@emillon

This comment has been minimized.

Contributor

emillon commented Nov 3, 2016

Currently working on it - the current blockers are 1/ figuring out how Oasis works (that part is mostly OK) 2/ where to make this fit in cohttp, given that my implementation depends on lwt. ISC license won't be a problem.

The lwt dependency is because the parser consumes a string Lwt_stream.t from Cohttp_lwt_body, and it relies on Lwt_stream to stream the contents of large files directly to a callback instead of storing it in memory. What do you recommend so that it fits with cohttp's architecture? Do I need to parameterize my implementation around S for streaming I/O?

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