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

[Body] Support the different sorts of bodies #65

Closed
haf opened this issue Apr 5, 2015 · 7 comments · Fixed by #71
Closed

[Body] Support the different sorts of bodies #65

haf opened this issue Apr 5, 2015 · 7 comments · Fixed by #71

Comments

@haf
Copy link
Owner

haf commented Apr 5, 2015

Description

As a part of making Http.fs a nice general-purpose Http client for .Net, we should support multipart/form-data bodies. They are needed to avoid the exploding size of application/x-www-form-urlencoded. They are needed to name file uploads and to upload files in general. By using them, we can set a MIME type for each body entity.

Implementation

Suggesting that we implement a discriminated union for each of:

  • multipart/form-data bodies
    • multipart/form-data -- DONE
    • multipart/mixed inside form-data -- DONE
    • plain encoding of files -- DONE
    • base64 encoding of files -- DONE
    • binary encoding of files -- this could be done when we have the 'socket' type, by composing it nicely DONE
  • application/x-www-form-urlencoded -- DONE
  • raw bytes
    • base64 encoded bytes
    • binary write-straight-to-stream bytes *DONE
  • socket -- awaiting discussion

The raw-bytes body is well put to use for intermediate uploads, from 30MB to half a gig, depending on your machines' memory configuration.

The socket body can be used to stream data to the target web server, and if we choose to do #64, will fit well together with it. It would allow plugins like a websocket client, SSE client, webrtc client etc.

The form-data body type will contain randomized boundaries, so there should be a way to generate bodies deterministically.

Handling data structure

We have these sorts of data; binary blob, streamed (both of which are easy and low on abstraction), form-data (key-value like), file-data (key-key-mime-base64 like). Both form-data and file-data can be formatted for multipart/form-data and application/x-www-form-urlencoded so I suggest a typed model for key-value and key-key-mime-base64 typed values, with can then be written using formatters. RawBytes bodies could be a specialisation of Socket (/cc @ademar - couldn't we do this for the new 'writePreamble' in Suave, too? Composing writers, one of which would be responsible for writing the preamble, into the SocketTask?).

Let's see what I can cook; right now I have a need for form-data and urlencoded, so I will focus on that.

@relentless
Copy link
Collaborator

Sounds good. Would be an APi change, but as per previous comments that looks to be happening anyway (although if the API is going to undergo a change, it would be good to do it together so as not to have to update the major version several times).

The only thing that concerns me is that this complicates the API, when the original purpose of Http.fs was to simplify things. But as long as we can do it without making the more standard use cases too complicated, it's all good.

@rodrigovidal
Copy link

I think we should create a milestone with all of these issues targeting the next version.

@haf
Copy link
Owner Author

haf commented Apr 6, 2015

I would like to think it's very possible to do this and still letting withBodyString be used just as easily. The upside of hiding all of this in the Impl module is that it's all hidden from the user, but if the user wants, he can opt-in to the extra complexity -- and extra features.

@haf
Copy link
Owner Author

haf commented Apr 6, 2015

Ok, I think this is well enough done now. If we ever need to format multipart in binary, we might want to consider using the SocketTask abstraction first, then refactor, then implement it, so that we don't have to transform the binary data into a String type first, like the code is geared towards now.

@haf
Copy link
Owner Author

haf commented Apr 6, 2015

The way I've done it; it does urlencoding if there are no files, otherwise it uses the multipart/form-data encoding. Both work, but it would probably be better to follow the Content-Type header; alternatively set the Content-Type header automatically if we give FormData. What do you say?

@haf
Copy link
Owner Author

haf commented May 12, 2015

I've had some time to test my branch with some production code and it seems there are a few issues in many web servers accepting base64-encoded data over just the inline-binary-blob that is the alternative.

I've updated the TODO list based on this.

I would like to get your input on the getResponse changes that I've done - basically allowing you to get the raw bytes if you're not getting a string. So while the ability to get bytes is very important, the current API I rushed through to support it (getResponse(Async|Body|...)) is shit and requires you to check for ResponseString | ResponseBytes every time. So I want to get help to design that API:

  • We need a getResponse function that returns 'everything' in a way that doesn't pre-consume the stream - what if I'm downloading a large image for example (my use-case) and I just want to proxy/forward the stream -- (Suave.Proxy here we come)? In that case it doesn't make sense that the client does all the Content-Type matching -> returning the discriminated union corresponding to the header. Instead there should be a Response and the option to get a body, just like MSFT's HttpClient has.
  • I've been looking at @h2non's amazing work with https://www.npmjs.com/package/resilient -- I'd love for our language to have a similar http client. Perhaps you could browse his code and steal some of his ideas for this project? =)

@haf
Copy link
Owner Author

haf commented May 20, 2015

I think this issue is fixed in the current PR. We don't need base64-encoding of binary files, as most web servers support the purely binary encoding.

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 a pull request may close this issue.

3 participants