Casted conn.params and conn.body_params do not match type spec. Dialyzer raises warnings.

[moxley]

The type spec for params and body_params is params() :: %{required(binary()) => param()} (https://hexdocs.pm/plug/1.7.2/Plug.Conn.html#t:params/0), but OpenApiSpex turns them into something like %{__struct__: module(), required(atom()) => term()}, or something else that doesn't match the spec. This causes dialyzer complaints in the controller actions for some situations.

IMO, OpenApiSpex shouldn't change conn.params or conn.body_params, as it breaks the contract with Plug. Instead, Spex can put the casted values in the :private attribute of the %Conn{}.

[mbuhot]

👍 It would be good to provide a custom Phoenix action plug that users can optionally mix in to controllers to get the cast params passed to the handler functions.

[hauleth]

@mbuhot this is what I am using in my project:

      def action(conn, _) do
        name = action_name(conn)

        if function_exported?(__MODULE__, name, 3) do
          apply(__MODULE__, name, [conn, conn.params, conn.body_params])
        else
          apply(__MODULE__, name, [conn, conn.params])
        end
      end

It could be changed to use :private or anything else in the first case.

[moxley]

It could be changed to use :private or anything else in the first case.

It would need to be done in Operation2

[mbuhot]

@moxley Shall we put a v4.0 milestone on this one? I can't come up with a non-breaking way to fix this.

[moxley]

@mbuhot: Yeah, I think it'll have to be in a major version.

[bforchhammer]

In case anyone else stumbles on this issue; this is how we solved it for ourselves:

defmodule Api.Spex do
  alias OpenApiSpex.{Info, OpenApi, Paths}
  @behaviour OpenApi

  @impl OpenApi
  def spec do
    %OpenApi{
      info: %Info{
        title: "My App",
        version: "1.0"
      },
      paths: Paths.from_router(Api.Router)
    }
    |> OpenApiSpex.resolve_schema_modules()
  end

  defmacro __using__(opts \\ []) do
    operations_module = Keyword.fetch!(opts, :operations_mod)

    quote do
      plug OpenApiSpex.Plug.CastAndValidate
      defdelegate open_api_operation(action), to: unquote(operations_module)

      # OpenApiSpex.Plug.CastAndValidate replaces the body_params with a struct based on the input schema; this breaks
      # dialyzer compatibility therefore we undo the change here and move the parsed and structified parameters into
      # conn.private.spex_params. We also supply them to the controller action as the 2nd argument which is a lot more
      # ergonomic than digging into the conn struct.
      # See https://github.com/open-api-spex/open_api_spex/issues/92
      plug :restore_body_params

      def restore_body_params(%Plug.Conn{body_params: %{__struct__: _} = body_params} = conn, _) do
        conn
        |> Plug.Conn.put_private(:spex_params, body_params)
        |> Map.put(:body_params, Map.from_struct(body_params))
      end

      def restore_body_params(conn, _), do: conn

      def action(conn, _) do
        name = action_name(conn)
        spex_params = conn.private[:spex_params]
        apply(__MODULE__, name, [conn, spex_params])
      end
    end
  end
end

This allows us to write controllers like this:

defmodule Api.UserController do
  use Api, :controller
  use Api.Spex, operations_mod: Api.Spex.UserOperations

  def create(%Plug.Conn{} = conn, %Api.Spex.UserCreateRequestSchema{username: username}) do
    # ...
    conn |> render(username: username)
  end
end

✨

[moxley]

Here is a temporary workaround for this problem:

defmodule MyAppWeb.MyController do
  use MyAppWeb, :controller
  use OpenApiSpex.Controller

  plug OpenApiSpex.Plug.CastAndValidate, json_render_error_v2: true

  # Use body params
  def create(conn, _params) do
    # Dialyzer misses the fact that body_params is being accessed
    body_params = Map.get(conn, :body_params)
    # The code is now free to reference compile-time atom keys from body params, without complaint from Dialyzer
    %{user: user_params} = body_params
  end

  # Use other params
  def create(conn, params) do
    # Dialyzer misses the fact that a param value is being accessed using an atom key
    id = Map.get(params, :id)
  end
end

[rsimmonsjr]

I dont think that last workaround will give the CastAndValidate functionality. The one from @bforchhammer might.

[moxley]

I dont think that last workaround will give the CastAndValidate functionality. The one from @bforchhammer might.

I'm not saying that it provides the CastAndValidate functionality. I'm saying is works around the issue described on this page.

[moxley]

Putting this here for future reference.

Here is the full output from dialyxer when running it against the example phoenix_app in open_api_spex:

lib/phoenix_app_web/controllers/user_controller.ex:50:call
The function call will not succeed.

Plug.Conn.put_status(
  _conn :: %{:body_params => %PhoenixAppWeb.Schemas.UserRequest{:user => _, _ => _}, _ => _},
  :created
)

breaks the contract
(t(), status()) :: t()
________________________________________________________________________________
lib/phoenix_app_web/controllers/user_controller.ex:96:no_return
Function update/2 has no local return.
________________________________________________________________________________
lib/phoenix_app_web/controllers/user_controller.ex:98:call
The function call will not succeed.

Phoenix.Controller.render(
  _conn :: %{:body_params => %PhoenixAppWeb.Schemas.UserRequest{:user => _, _ => _}, _ => _},
  <<115, 104, 111, 119, 46, 106, 115, 111, 110>>,
  [{:user, _}, ...]
)

will never return since the success typing is:
(
  atom()
  | %{
      :__struct__ => Phoenix.Socket | Plug.Conn,
      :assigns => map(),
      :private => %{atom() => _},
      :adapter => {atom(), _},
      :before_send => [(_ -> any())],
      :body_params => %Plug.Conn.Unfetched{:aspect => atom(), binary() => _},
      :channel => atom(),
      :channel_pid => pid(),
      :cookies => %Plug.Conn.Unfetched{:aspect => atom(), binary() => binary()},
      :endpoint => atom(),
      :halted => _,
      :handler => atom(),
      :host => binary(),
      :id => nil | binary(),
      :join_ref => _,
      :joined => boolean(),
      :method => binary(),
      :owner => pid(),
      :params => %Plug.Conn.Unfetched{:aspect => atom(), binary() => _},
      :path_info => [binary()],
      :path_params => %{binary() => binary() | [any()] | map()},
      :port => char(),
      :pubsub_server => atom(),
      :query_params => %Plug.Conn.Unfetched{
        :aspect => atom(),
        binary() => binary() | [any()] | map()
      },
      :query_string => binary(),
      :ref => _,
      :remote_ip =>
        {byte(), byte(), byte(), byte()}
        | {char(), char(), char(), char(), char(), char(), char(), char()},
      :req_cookies => %Plug.Conn.Unfetched{:aspect => atom(), binary() => binary()},
      :req_headers => [{_, _}],
      :request_path => binary(),
      :resp_body =>
        nil
        | binary()
        | maybe_improper_list(
            binary() | maybe_improper_list(any(), binary() | []) | byte(),
            binary() | []
          ),
      :resp_cookies => %{binary() => map()},
      :resp_headers => [{_, _}],
      :scheme => :http | :https,
      :script_name => [binary()],
      :secret_key_base => nil | binary(),
      :serializer => atom(),
      :state => :chunked | :file | :sent | :set | :set_chunked | :set_file | :unset,
      :status => nil | non_neg_integer(),
      :topic => binary(),
      :transport => atom(),
      :transport_pid => pid()
    },
  atom() | binary(),
  atom() | binary() | maybe_improper_list() | map()
) :: %Plug.Conn{
  :adapter => {atom(), _},
  :assigns => %{atom() => _},
  :before_send => [(_ -> any())],
  :body_params => %Plug.Conn.Unfetched{:aspect => atom(), binary() => _},
  :cookies => %Plug.Conn.Unfetched{:aspect => atom(), binary() => binary()},
  :halted => _,
  :host => binary(),
  :method => binary(),
  :owner => pid(),
  :params => %Plug.Conn.Unfetched{:aspect => atom(), binary() => _},
  :path_info => [binary()],
  :path_params => %{binary() => binary() | [any()] | map()},
  :port => char(),
  :private => %{atom() => _},
  :query_params => %Plug.Conn.Unfetched{
    :aspect => atom(),
    binary() => binary() | [any()] | map()
  },
  :query_string => binary(),
  :remote_ip =>
    {byte(), byte(), byte(), byte()}
    | {char(), char(), char(), char(), char(), char(), char(), char()},
  :req_cookies => %Plug.Conn.Unfetched{:aspect => atom(), binary() => binary()},
  :req_headers => [{_, _}],
  :request_path => binary(),
  :resp_body =>
    nil
    | binary()
    | maybe_improper_list(
        binary() | maybe_improper_list(any(), binary() | []) | byte(),
        binary() | []
      ),
  :resp_cookies => %{binary() => map()},
  :resp_headers => [{_, _}],
  :scheme => :http | :https,
  :script_name => [binary()],
  :secret_key_base => nil | binary(),
  :state => :sent,
  :status => nil | non_neg_integer()
}

and the contract is
(Plug.Conn.t(), binary() | atom(), Keyword.t() | map() | binary() | atom()) ::
  Plug.Conn.t()
________________________________________________________________________________
lib/phoenix_app_web/controllers/user_controller_with_struct_specs.ex:72:call
The function call will not succeed.

Plug.Conn.put_status(
  _conn :: %{:body_params => %PhoenixAppWeb.Schemas.UserRequest{:user => _, _ => _}, _ => _},
  :created
)

breaks the contract
(t(), status()) :: t()
________________________________________________________________________________
done (warnings were emitted)

[moxley]

The resolution for this issue could happen in three phases:

Phase 1: When params are being casted, put them in the conn.private assigns, as mentioned by @bforchhammer. Allow the controller actions to fetch them from the private assigns using convenience functions, such as OpenApiSpex.body_params(conn) and OpenApiSpex.params(conn).

Phase 2: (Optional) Add plug logic that replaces the second (params) argument of the controller's def {action}(conn, params) function with the casted params and body params, similar to the solutions described by @hauleth and @bforchhammer.

Phase 3: Stop having OpenApiSpex modify conn.body_params and conn.params. This would be a breaking change.

For Phase 1, a controller action might access casted params like this:

def update(conn, _) do
  # Request body params fetched by OpenApiSpex.body_params(conn)
  # This function might be imported automatically with `use OpenApiSpex.ControllerSpecs`
  %UserParams{
    name: name,
    email: email,
    birthday: %Date{} = birthday
  } = OpenApiSpex.body_params(conn)

  # Path params, query params, and header params, merged together and fetched by OpenApiSpex.params(conn)
  # This function might be imported automatically with `use OpenApiSpex.ControllerSpecs`
  %{id: id} = OpenApiSpex.params(conn)
end

Phase 1 would resolve the bug described in this issue. Phase 2 is for convenience. Phase 3 is cleanup.

[riccardomanfrin]

I don't know how you plan to do this, but for phase 1 and 3, I did it this way

[lucacorti]

@moxley Is 1 implemented yet or still pending implementation?

3 is going to be a breaking change needing a major release, but we could start with a deprecation warning hinting at migratiing to 1 so that an upgrade path is available today and then release the breaking change with the major.

[mbuhot]

relevant: #398