Repository: clj-commons/aleph (clojure)

Issue:
The client ignores content type when using byte arrays in multipart data
`clj-http`  uses `org.apache.http.entity.mime.content.ByteArrayBody` for byte arrays and it allows setting content type.

Netty uses attributes and file uploads. The former don't allow setting content type, the latter do.
The issue with Aleph is that it uses file uploads only for on-disk files, even though Netty provides `io.netty.handler.codec.http.multipart.MemoryFileUpload`.

Seems like the solution here is to change `aleph.http.multipart/encode-request` in such a way so that:
- For on-disk files it uses the current approach with `.addBodyFileUpload`
- For in-memory data with no content type it also uses the current approach with `MemoryAttribute`
- For in-memory data with content type it creates `MemoryFileUpload`

Pull request description:
Fixes #649

Patch:
diff --git a/src/aleph/http/multipart.clj b/src/aleph/http/multipart.clj
index b28ca881..1dc9af4e 100644
--- a/src/aleph/http/multipart.clj
+++ b/src/aleph/http/multipart.clj
@@ -10,7 +10,8 @@
    [manifold.stream :as s])
   (:import
    [java.util
-    Locale]
+    Locale
+    UUID]
    [java.io
     File]
    [java.nio
@@ -28,6 +29,7 @@
    [io.netty.handler.codec.http.multipart
     Attribute
     MemoryAttribute
+    MemoryFileUpload
     FileUpload
     DefaultHttpDataFactory
     HttpPostRequestDecoder
@@ -127,37 +129,68 @@
       (.flip buf)
       (bs/to-byte-array buf))))
 
-(defn encode-request [^DefaultHttpRequest req parts]
+(defn- ensure-charset [charset]
+  (cond
+    (nil? charset)
+    HttpConstants/DEFAULT_CHARSET
+
+    (string? charset)
+    (Charset/forName charset)
+
+    (instance? Charset charset)
+    charset))
+
+(defn- encode-file-upload
+  "Encode the `content` as a `FileUpload`."
+  [^HttpPostRequestEncoder encoder {:keys [part-name name content mime-type charset]}]
+  (let [filename  (.getName ^File content)
+        name      (or name filename)
+        mime-type (or mime-type
+                      (URLConnection/guessContentTypeFromName filename))
+        content-type (mime-type-descriptor mime-type charset)]
+    (.addBodyFileUpload encoder
+                        (or part-name name)
+                        ;; Netty's multipart encoder ignores empty strings here
+                        (or filename "")
+                        content
+                        content-type
+                        false)))
+
+(defn- encode-memory-file-upload
+  "Encode the `content` as a `MemoryFileUpload`."
+  [^HttpPostRequestEncoder encoder {:keys [part-name name content mime-type charset]}]
+  (let [content-type (mime-type-descriptor mime-type charset)
+        charset      (ensure-charset charset)
+        filename     (str (UUID/randomUUID))
+        content      (netty/to-byte-buf content)
+        size         (.readableBytes content)
+        file-upload  (doto (MemoryFileUpload. (or part-name name) filename content-type "binary" charset size)
+                       (.addContent (netty/to-byte-buf content) true))]
+    (.addBodyHttpData encoder file-upload)))
+
+(defn- encode-memory-attribute
+  "Encode the `content` as a `MemoryAttribute`."
+  [^HttpPostRequestEncoder encoder {:keys [part-name name content charset]}]
+  (let [^Charset charset (ensure-charset charset)
+        part-name (or part-name name)
+        attr      (if (string? content)
+                    (MemoryAttribute. ^String part-name ^String content charset)
+                    (doto (MemoryAttribute. ^String part-name charset)
+                      (.addContent (netty/to-byte-buf content) true)))]
+    (.addBodyHttpData encoder attr)))
+
+(defn ^:no-doc encode-request [^DefaultHttpRequest req parts]
   (let [^HttpPostRequestEncoder encoder (HttpPostRequestEncoder. req true)]
-    (doseq [{:keys [part-name content mime-type charset name]} parts]
-      (if (instance? File content)
-        (let [filename (.getName ^File content)
-              name (or name filename)
-              mime-type (or mime-type
-                            (URLConnection/guessContentTypeFromName filename))
-              content-type (mime-type-descriptor mime-type charset)]
-          (.addBodyFileUpload encoder
-                              (or part-name name)
-                              ;; Netty's multipart encoder ignores empty strings here
-                              (or filename "")
-                              content
-                              content-type
-                              false))
-        (let [^Charset charset (cond
-                                 (nil? charset)
-                                 HttpConstants/DEFAULT_CHARSET
-
-                                 (string? charset)
-                                 (Charset/forName charset)
-
-                                 (instance? Charset charset)
-                                 charset)
-              part-name (or part-name name)
-              attr (if (string? content)
-                     (MemoryAttribute. ^String part-name ^String content charset)
-                     (doto (MemoryAttribute. ^String part-name charset)
-                       (.addContent (netty/to-byte-buf content) true)))]
-          (.addBodyHttpData encoder attr))))
+    (doseq [{:keys [content mime-type] :as part} parts]
+      (cond
+        (instance? File content)
+        (encode-file-upload encoder part)
+
+        (some? mime-type)
+        (encode-memory-file-upload encoder part)
+
+        :else
+        (encode-memory-attribute encoder part)))
     (let [req' (.finalizeRequest encoder)]
       [req' (when (.isChunked encoder) encoder)])))
 


Test patch:
diff --git a/test/aleph/http/multipart_test.clj b/test/aleph/http/multipart_test.clj
index 3aa50f7e..382b69dd 100644
--- a/test/aleph/http/multipart_test.clj
+++ b/test/aleph/http/multipart_test.clj
@@ -3,8 +3,8 @@
    [aleph.http :as http]
    [aleph.http.core :as core]
    [aleph.http.multipart :as mp]
+   [aleph.netty :as netty]
    [clj-commons.byte-streams :as bs]
-   [clojure.edn :as edn]
    [clojure.string :as str]
    [clojure.test :refer [deftest is testing]]
    [manifold.deferred :as d]
@@ -91,9 +91,9 @@
 #_{:clj-kondo/ignore [:deprecated-var]}
 (deftest reject-unknown-transfer-encoding
   (is (thrown? IllegalArgumentException
-      (mp/encode-body [{:part-name "part1"
-                        :content "content1"
-                        :transfer-encoding :uknown-transfer-encoding}]))))
+       (mp/encode-body [{:part-name "part1"
+                         :content "content1"
+                         :transfer-encoding :uknown-transfer-encoding}]))))
 
 #_{:clj-kondo/ignore [:deprecated-var]}
 (deftest test-content-as-file
@@ -156,7 +156,10 @@
              :content file-to-send}
             {:part-name "#5-file-with-charset"
              :content file-to-send
-             :charset "ISO-8859-1"}])
+             :charset "ISO-8859-1"}
+            {:part-name "#6-bytes-with-mime-type"
+             :mime-type "text/plain"
+             :content (.getBytes "CONTENT3" "UTF-8")}])
 
 (defn echo-handler [{:keys [body]}]
   {:status 200
@@ -188,52 +191,55 @@
 
     (.close ^java.io.Closeable s)))
 
-(defn- pack-chunk [{:keys [content] :as chunk}]
-  (cond-> (dissoc chunk :file)
-    (not (string? content))
-    (dissoc :content)))
-
 (defn- decode-handler [req]
-  (let [chunks (-> req
+  (let [is     (:body req)
+        data   (bs/to-string is)
+        chunks (-> (assoc req :body (netty/to-byte-buf-stream (.getBytes data) 512))
                    mp/decode-request
                    s/stream->seq)]
     {:status 200
-     :body (pr-str (map pack-chunk chunks))}))
+     :body (pr-str {:chunks (mapv #(update % :content bs/to-string) chunks)
+                    :encoded-data data})}))
 
 (defn- test-decoder [port url raw-stream?]
   (let [s (http/start-server decode-handler {:port port
                                              :shutdown-timeout 0
-                                             :raw-stream? raw-stream?})
-        chunks (-> (http/post url {:multipart parts})
-                   (deref 1e3 {:body "timeout"})
-                   :body
-                   bs/to-string
-                   clojure.edn/read-string
-                   vec)]
-    (is (= 6 (count chunks)))
+                                             :raw-stream? raw-stream?})]
+    (try
+      (let [req          (http/post url {:multipart parts})
+            resp         (deref req 1e3 {:body "timeout"})
+            body         (-> (:body resp) bs/to-string read-string)
+            encoded-data (:encoded-data body)
+            chunks       (-> body :chunks vec)]
+        (is (= 7 (count chunks)))
 
-    ;; part-names
-    (is (= (map :part-name parts)
-           (map :part-name chunks)))
+        ;; part-names
+        (is (= (map :part-name parts)
+               (map :part-name chunks)))
 
-    ;; content
-    (is (= "CONTENT1" (get-in chunks [0 :content])))
+        ;; content
+        (is (= "CONTENT1" (get-in chunks [0 :content])))
 
-    ;; mime type
-    (is (= "text/plain" (get-in chunks [2 :mime-type])))
-    (is (= "application/png" (get-in chunks [3 :mime-type])))
+        ;; mime type
+        (is (= "text/plain" (get-in chunks [2 :mime-type])))
+        (is (= "application/png" (get-in chunks [3 :mime-type])))
 
-    ;; filename
-    (is (= "file.txt" (get-in chunks [3 :name])))
-    (is (= "file.txt" (get-in chunks [4 :name])))
+        ;; filename
+        (is (= "file.txt" (get-in chunks [3 :name])))
+        (is (= "file.txt" (get-in chunks [4 :name])))
 
-    ;; charset
-    (is (= "ISO-8859-1" (get-in chunks [5 :charset])))
+        ;; charset
+        (is (= "ISO-8859-1" (get-in chunks [5 :charset])))
 
-    (.close ^java.io.Closeable s)))
+        ;; mime-type + memory data
+        (is (re-find #"content-disposition: form-data; name=\"#6-bytes-with-mime-type\"; filename=\".*\"\r\ncontent-length: 8\r\ncontent-type: text/plain\r\ncontent-transfer-encoding: binary\r\n\r\nCONTENT3\r\n" encoded-data))
+        (is (= "CONTENT3" (get-in chunks [6 :content])))
+        (is (= "text/plain" (get-in chunks [6 :mime-type]))))
+
+      (finally (.close ^java.io.Closeable s)))))
 
-(deftest test-mutlipart-request-decode-with-ring-handler
+(deftest test-multipart-request-decode-with-ring-handler
   (test-decoder port2 url2 false))
 
-(deftest test-mutlipart-request-decode-with-raw-handler
+(deftest test-multipart-request-decode-with-raw-handler
   (test-decoder port3 url3 true))