Merge pull request #99 from permaweb/tillathehun0/nested-body-bug

fix(hb_codec_http): ensure nested message structure is preserved, even with only sub bodies #13
permaweb · Jan 24, 2025 · 2d02910 · 2d02910
2 parents 1a02c51 + ddd8909
commit 2d02910
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 7 deletions.
diff --git a/src/hb_codec_http.erl b/src/hb_codec_http.erl
@@ -247,7 +247,8 @@ from_body_parts(TABM, [Part | Rest]) ->
 %%% @doc Convert a TABM into an HTTP Message. The HTTP Message is a simple Erlang Map
 %%% that can translated to a given web server Response API
 to(Bin) when is_binary(Bin) -> Bin;
-to(TABM) when is_map(TABM) ->
+to(TABM) -> to(TABM, []).
+to(TABM, Opts) when is_map(TABM) ->
     % PublicMsg = hb_private:reset(TABM),
     % MinimizedMsg = hb_message:minimize(PublicMsg),
     Http = maps:fold(
@@ -265,13 +266,20 @@ to(TABM) when is_map(TABM) ->
     ),
     BodyMap = maps:get(<<"body">>, Http),
     NewHttp =
-        case BodyMap of
+        case {BodyMap, lists:member(sub_part, Opts)} of
             % If the body map is empty, then simply set the body to be a corresponding empty binary.
-            X when map_size(X) =:= 0 ->
+            {X, _} when map_size(X) =:= 0 ->
                 maps:put(<<"body">>, <<>>, Http);
             % Simply set the sole body binary as the body of the
             % HTTP message, no further encoding required
-            #{ <<"body">> := UserBody } when map_size(BodyMap) =:= 1 andalso is_binary(UserBody) ->
+            % 
+            % NOTE: this may only be done for the top most message as sub-messages MUST be
+            % encoded as sub-parts, in order to preserve the nested hierarchy of messages,
+            % even in the case of a sole body binary.
+            % 
+            % In all other cases, the mapping fallsthrough to the case below that properly
+            % encodes a nested body within a sub-part
+            {#{ <<"body">> := UserBody }, false} when map_size(BodyMap) =:= 1 andalso is_binary(UserBody) ->
                 ?event({encoding_single_body, {body, UserBody}, {http, Http}}),
                 maps:put(<<"body">>, UserBody, Http);
             % Otherwise, we need to encode the body map as the
@@ -295,18 +303,44 @@ to(TABM) when is_map(TABM) ->
                                 <<"body">> -> <<"inline">>;
                                 _ -> <<"form-data;name=", "\"", PartName/binary, "\"">>
                             end,
+                            % Sub-parts MUST have at least one header, according to the multipart spec.
+                            % Adding the Content-Disposition not only satisfies that requirement,
+                            % but also encodes the HB message field that resolves to the sub-message
                             EncodedBodyPart = case BodyPart of
                                 BPMap when is_map(BPMap) ->
-                                    SubHttp = to(BPMap),
+                                    WithDisposition = maps:put(
+                                        <<"Content-Disposition">>,
+                                        Disposition,
+                                        BPMap
+                                    ),
+                                    SubHttp = to(WithDisposition, [sub_part]),
                                     EncodedHttp = encode_http_msg(SubHttp),
                                     EncodedHttp;
                                 BPBin when is_binary(BPBin) ->
-                                    BPBin
+                                    case PartName of
+                                        % A properly encoded inlined body part MUST have a CRLF between
+                                        % it and the header block, so we MUST use two CRLF:
+                                        % - first to signal end of the Content-Disposition header
+                                        % - second to signal the end of the header block
+                                        <<"body">> -> 
+                                            <<
+                                                "Content-Disposition: ", Disposition/binary, ?CRLF/binary,
+                                                ?CRLF/binary,
+                                                BPBin/binary
+                                            >>;
+                                        % All other binary values are encoded as a header in their
+                                        % respective sub-part
+                                        _ ->
+                                            <<
+                                                "Content-Disposition: ", Disposition/binary, ?CRLF/binary,
+                                                BPBin/binary
+                                            >>
+                                    end
+
                             end,
                             [
                                 <<
                                     "--", Boundary/binary, ?CRLF/binary,
-                                    "Content-Disposition: ", Disposition/binary, ?CRLF/binary,
                                     EncodedBodyPart/binary
                                 >>
                             |

diff --git a/src/hb_message.erl b/src/hb_message.erl
@@ -641,6 +641,24 @@ deeply_nested_message_with_content_test(Codec) ->
     Decoded = convert(Encoded, converge, Codec, #{}),
     ?assert(match(Msg, Decoded)).
 
+deeply_nested_message_with_only_content(Codec) ->
+    MainBodyKey =
+        case Codec of
+            tx -> <<"data">>;
+            _ -> <<"body">>
+        end,
+    Msg = #{
+         <<"depth">> => <<"outer">>,
+        MainBodyKey => #{
+            MainBodyKey => #{
+                MainBodyKey => <<"DATA">>    
+            }
+        }
+    },
+    Encoded = convert(Msg, Codec, converge, #{}),
+    Decoded = convert(Encoded, converge, Codec, #{}),
+    ?assert(match(Msg, Decoded)).
+
 nested_structured_fields_test(Codec) ->
     NestedMsg = #{ <<"a">> => #{ <<"b">> => 1 } },
     Encoded = convert(NestedMsg, Codec, converge, #{}),
@@ -786,6 +804,8 @@ message_suite_test_() ->
             fun nested_message_with_large_content_test/1},
         {"deeply nested message with content test",
             fun deeply_nested_message_with_content_test/1},
+        {"deeply nested message with only content test",
+            fun deeply_nested_message_with_only_content/1},
         {"structured field atom parsing test",
             fun structured_field_atom_parsing_test/1},
         {"structured field decimal parsing test",