open-api-spex / open_api_spex

Open API Specifications for Elixir Plug applications
Mozilla Public License 2.0
698 stars 182 forks source link

multipart/form-data validation failing due to Plug.Conn.Query.decode #441

Open christmoore opened 2 years ago

christmoore commented 2 years ago

A little backstory - operations which contain a multipart/form-data _requestbody , which within themselves contain an array field don't get sent as arrays via SwaggerUI. curl is being generated without appending to the array field:

 operation(
.....
 request_body:
      {"Multipart form", "multipart/form-data",
       MyApp.Multipart, required: true},
....
def Multipart do
...
OpenApiSpex.schema(%{
      type: :object,
      properties: %{
        array_items: %Schema{
          description: """
          array of files
          """,
          type: :array,
          minItems: 1,
          items: %Schema{
            type: :string,
            format: :binary
          }
        }
      }
}

This generates the following Swagger UI: image

Note the curl here. array_items needs to be array_items[] in order for Plug to create an array of Plug.Upload constructs. Without this, the conn object contains:

"array_items" => %Plug.Upload{
      content_type: "application/pdf",
      filename: "empty_pdf_2.pdf",
      path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648671114-838101121406784-2"
    }

This is a known issue with swagger. The solution has been to add brackets to the name of the fields so that curl interpolates currently. See https://stackoverflow.com/questions/53213067/swagger-ui-open-api-3-multipart-form-data-array-problem

Taking that curl from the example above and modifying to array_items[] creates the array as expected:

"array_items" => [
      %Plug.Upload{
        content_type: "application/pdf",
        filename: "empty_pdf_1.pdf",
        path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648671649-87151574074633-4"
      },
      %Plug.Upload{
        content_type: "application/pdf",
        filename: "empty_pdf_2.pdf",
        path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648671649-724027952677069-4"
      }
    ]

To resolve this, I've got a hacky solution that involves changing the field name to include brackets:

OpenApiSpex.schema(%{
      type: :object,
      properties: %{
        :"array_items[]": %Schema{
          description: """
          array of files
          """,
          type: :array,
          minItems: 1,
          items: %Schema{
            type: :string,
            format: :binary
          }
        }
      }
}

But downstream, validations will fail with required fields for :"array_items[]" due to the way this ends up getting interpolated. Plug.Conn.Query.decode cuts out brackets. If I end up just putting :array_items in the required field, I won't be able to submit my request in Swagger due to missing a required field (Swagger sees array_items[] not array_items)

OpenApiSpex.schema(%{
      type: :object,
      properties: %{
        :"array_items[]": %Schema{
          description: """
          array of files
          """,
          type: :array,
          minItems: 1,
          items: %Schema{
            type: :string,
            format: :binary
          }
        }
      },
    required: [:"array_items[]"]
}
..... open api request sent, request passes through MULTIPART parser, which runs through Plug.Conn.Query for decoding array_items[] to array_items list

params: %{
    "array_items" => [
      %Plug.Upload{
        content_type: "application/pdf",
        filename: "empty_pdf_1.pdf",
        path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648673751-336866617448758-3"
      },
      %Plug.Upload{
        content_type: "application/pdf",
        filename: "empty_pdf_2.pdf",
        path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648673751-11087878838975-3"
      }
    ]
  }
....
.... validation error
{
  "errors": [
    {
      "detail": "Missing field: array_items[]",
      "source": {
        "pointer": "/array_items[]"
      },
      "title": "Invalid value"
    }
  ]
}

Ideally validations follow the conventions for removing brackets on field names for multipart data. Ergo, if required: [:"array_items[]"] is set, on validation it will be checked against array_items


EDIT: It's Plug.Conn.Query.decode that cuts out brackets from field names.

iex(1)> Plug.Conn.Query.decode "a[]=1&a[]=2"
%{"a" => ["1", "2"]}
iex(2)> Plug.Conn.Query.encode %{"a" => ["1", "2"]}
"a[]=1&a[]=2"
iex(3)> Plug.Conn.Query.decode "a[1]=1&a[2]=2"
%{"a" => %{"1" => "1", "2" => "2"}}
iex(4)> Plug.Conn.Query.encode %{"a" => %{"1" => "1", "2" => "2"}}
"a[1]=1&a[2]=2"

https://hexdocs.pm/plug/Plug.Conn.Query.html#content

christmoore commented 2 years ago

I've updated to remove requirement for changing properties to support string, as the core issue here is that multipart fields that are arrays are parsed to remove brackets in field names by Plug.Conn.Query. Since this is convention, should open_api_spex be modified to accommodate for this?

AndriiKlymchuk commented 2 years ago

@MrManicotti, as workaround I have created plug which splits specified parameters by pattern and updates both conn.body_params and conn.params:

defmodule MyAppWeb.StringBodyParamToListPlug do
  @behaviour Plug

  @impl Plug
  def init(opts), do: opts

  @impl Plug
  def call(conn, opts)do
    params =
      Enum.reduce(opts[:keys], %{}, fn key, acc ->
        case conn.body_params do
          %{^key => string_val} when is_binary(string_val) ->
            list_val = String.split(string_val, opts[:pattern])
            Map.put(acc, key, list_val)
          _ ->
            conn
        end
      end)

    conn
    |> Map.update!(:body_params, & Map.merge(&1, params))
    |> Map.update!(:params, & Map.merge(&1, params))
  end
end

Call it before OpenApiSpex.Plug.CastAndValidate

plug MyAppWeb.StringBodyParamToListPlug, [keys: ["field_name"], pattern: ","]

plug OpenApiSpex.Plug.CastAndValidate
christmoore commented 2 years ago

I'm not quite sure this resolves the issue with validation here, as the problem specifically is in regards to what field name gets validated by CastAndValidate, rather than the actual structure of the data.

Plug.Conn.Query will cast

params: %{
    "array_items[]" => [
      %Plug.Upload{
        content_type: "application/pdf",
        filename: "empty_pdf_1.pdf",
        path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648673751-336866617448758-3"
      },
      %Plug.Upload{
        content_type: "application/pdf",
        filename: "empty_pdf_2.pdf",
        path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648673751-11087878838975-3"
      }
    ]
  }

to

params: %{
    "array_items" => [
      %Plug.Upload{
        content_type: "application/pdf",
        filename: "empty_pdf_1.pdf",
        path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648673751-336866617448758-3"
      },
      %Plug.Upload{
        content_type: "application/pdf",
        filename: "empty_pdf_2.pdf",
        path: "/var/folders/lx/8gvgf9jd24x6mcc11g747j500000gn/T//plug-1648/multipart-1648673751-11087878838975-3"
      }
    ]
  }

The later is correct and what's expected conventionally, however there's a problem with open-api-spex's validation. The issue is two-fold really:

  1. If I required: [:"array_items[]"], I can send my request through swagger, but open-api-spex will not see the array_items[] field due to the aforementioned decoding.
    {
    "errors": [
    {
      "detail": "Missing field: array_items[]",
      "source": {
        "pointer": "/array_items[]"
      },
      "title": "Invalid value"
    }
    ]
    }
  2. If I, knowing this instead required: [:"array_items"], I cannot send my request through swagger because the array_items field is not what's provided to it. It instead sees array_items[]

I believe to resolve this we may need to pass field names through the decoder prior to validation, so that array_items[] has its requirements checked against array_items on validation

christmoore commented 2 years ago

I've got a hacky solution in the meantime to fix the spec prior to validation:

spec =
      spec
      |> update_in(
        [Access.key(:components), Access.key(:schemas), "RequestMultiPart", Access.key(:properties)],
        &with({value, map} <- Map.pop(&1, :"array_items[]"), do: Map.put(map, :array_items, value))
      )
      |> update_in(
        [Access.key(:components), Access.key(:schemas), "RequestMultiPart", Access.key(:required)],
        &[:array_items | Enum.reject(&1, fn x -> x == :"array_items[]" end)]
      )
....
case OpenApiSpex.cast_and_validate(spec, operation, conn, content_type) do
      {:ok, _} ->
        conn

      {:error, errors} ->
        errors = render_error.init(errors)
...

I had to create my own cast_and_validate plug and inject this modified spec