search

babashka / babashka.curl

A This library is mostly replaced by https://github.com/babashka/http-client
Eclipse Public License 1.0
121 stars 9 forks source link

Support :multipart #26

Open borkdude opened 4 years ago

borkdude commented 4 years ago

@johnchristopherjones starting to look complete I think? :)

johnchristopherjones commented 4 years ago

@borkdude It's kinda close but I have some additional tests locally that are failing. I also realized that explicitly following clj-http semantics for :multipart leads to some problems. Is it more important to have fidelity to curl (I think so) or drop-in compatibility with clj-http?

  1. So, curl can do more with :multipart than clj-http apparently supports. This mismatch means that drop-in compatibility with clj-http comes at the cost of fidelity with curl. In particular, curl allows you to attach any number of named metadata fields (called "headers") to a keypair, while clj-http only supports mime-type metadata. This is particularly vexing because you can pass a seq of maps via :multipart and clj-http assigns special meaning to :name, :part-name, :content, and :mime-type keys, swallows any :encoding key for its own use, and doesn't support any metadata except :mime-type which it remaps to type metadata.

    • Because I'm effectively ignoring namespaces and literally matching keys, I could support drop-in compatibility with clj-http with un-namespaced keys and colliding names that are namespaced would pass through as headers. That is {:name "foo" :content "bar" ::name "baz"} produces ["--form", "foo=bar;name=baz"]. However, I think this is really obtuse and it's way more obvious to just break with drop-in compatibility.

  2. The second half of the curl manpage about multipart is about extensions to the syntax to support multipart SMTP. I haven't explicitly dealt with this yet. I think the smart thing is to extend the :multipart to take a seq and vary depending on the seq's structure:

    1. If it's a string, pass it literally to curl's -F, allowing you to do =( and =) like curl requires. (extension over clj-http)
    2. If it's a 2-tuple like a MapEntry those are the key and value. (clj-http compatible) You can also, for example provide [nil, "("] to begin an SMTP multipart (=().
    3. If it's a 3-tuple, the third item is a map of headers (extension over clj-http) aside: flirting with the idea of making this hiccup-compatible, but that's probably obscene.
    4. If it's a map, it works exactly like clj-http and you can have your key collisions. (clj-http compatible) You can also namespace your keys to make them definitely-headers (extension over clj-http).

  3. Finally, clj-http takes a bunch of care with encoding that I'm not sure I need to follow, because clj-http is really just trying to speak to Apache and babashka.curl is really just constructing a command line argument and encoding concerns have already been settled by the time we get here. This is just my hypothesis though; I haven't grappled with it.

borkdude commented 4 years ago

@johnchristopherjones babashka.curl only says it's inspired by clj-http but there's no need to be 100% compatible. The main goal is to write Clojure to work with curl, in a way that's idiomatic and hopefully familiar to Clojure users.

If curl supports more in this area than clj-http (admittedly, I haven't looked into this as deep as you have) I think your proposal makes sense. Feel free to go ahead with your proposed solution!

johnchristopherjones commented 4 years ago

@borkdude I don't think I'm going to have enough time to finish this in time for the release today. Sorry about that. I don't mind if someone else wants to push this along.

borkdude commented 4 years ago

@johnchristopherjones This doesn't have to make it into this week's release per se. The library can also be run from source with babashka, if people want to use the new feature and else they just have to wait a month.

borkdude commented 4 years ago

@johnchristopherjones I didn't think it would be this involved but I'd rather wait and have it the proper way like you suggested. Thanks for the work so far.

borkdude commented 4 years ago

Just released v0.2.1. Next release will be in around 4 weeks.