From d61d03fd198d07290b011ef5552fb1e6f098f20e Mon Sep 17 00:00:00 2001 From: Michael Buhot Date: Sat, 20 Apr 2019 14:29:34 +1000 Subject: [PATCH] Ensure path parameters are declared in each Operation Paths.from_router will raise an exception if an operation fails to declare a path parameter that exists in the route --- lib/open_api_spex/operation.ex | 38 +++++++++++++++++-- lib/open_api_spex/path_item.ex | 57 ++++++++++++++++++---------- lib/open_api_spex/paths.ex | 29 ++++++++------ test/operation_test.exs | 27 ++++++++++--- test/path_item_test.exs | 4 +- test/paths_test.exs | 2 +- test/plug/cast_and_validate_test.exs | 3 -- 7 files changed, 114 insertions(+), 46 deletions(-) diff --git a/lib/open_api_spex/operation.ex b/lib/open_api_spex/operation.ex index e9a16275..952f6181 100644 --- a/lib/open_api_spex/operation.ex +++ b/lib/open_api_spex/operation.ex @@ -52,11 +52,43 @@ defmodule OpenApiSpex.Operation do } @doc """ - Constructs an Operation struct from the plug and opts specified in the given route + Constructs an Operation struct from the plug and opts specified in the given route. + + If any path parameters declared in the `route.path` do not have corresponding + parameters defined in the `Operation`, the result is an error tuple with a message + descring which parameters are missing. """ - @spec from_route(PathItem.route) :: t + @spec from_route(PathItem.route) :: {:ok, t} | {:error, String.t()} def from_route(route) do - from_plug(route.plug, route.opts) + operation = from_plug(route.plug, Map.get(route, :opts, [])) + + case route do + %{path: path} -> check_all_path_params_declared(operation, path) + _ -> {:ok, operation} + end + end + + defp check_all_path_params_declared(operation, route_path) do + {expected_path_params, _} = Plug.Router.Utils.build_path_match(route_path) + + actual_path_params = + operation.parameters + |> Enum.filter(&(&1.in == :path)) + |> Enum.map(& &1.name) + + missing_params = expected_path_params -- actual_path_params + + case missing_params do + [] -> + {:ok, operation} + + _ -> + message = + "Operation for route: #{inspect(route_path)} " <> + "did not declare path parameters: #{inspect(missing_params)}" + + {:error, message} + end end @doc """ diff --git a/lib/open_api_spex/path_item.ex b/lib/open_api_spex/path_item.ex index 8e893bf6..4f1bb720 100644 --- a/lib/open_api_spex/path_item.ex +++ b/lib/open_api_spex/path_item.ex @@ -4,6 +4,7 @@ defmodule OpenApiSpex.PathItem do """ alias OpenApiSpex.{Operation, Server, Parameter, PathItem, Reference} + defstruct [ :"$ref", :summary, @@ -29,31 +30,36 @@ defmodule OpenApiSpex.PathItem do but they will not know which operations and parameters are available. """ @type t :: %__MODULE__{ - "$ref": String.t | nil, - summary: String.t | nil, - description: String.t | nil, - get: Operation.t | nil, - put: Operation.t | nil, - post: Operation.t | nil, - delete: Operation.t | nil, - options: Operation.t | nil, - head: Operation.t | nil, - patch: Operation.t | nil, - trace: Operation.t | nil, - servers: [Server.t] | nil, - parameters: [Parameter.t | Reference.t] | nil - } + "$ref": String.t() | nil, + summary: String.t() | nil, + description: String.t() | nil, + get: Operation.t() | nil, + put: Operation.t() | nil, + post: Operation.t() | nil, + delete: Operation.t() | nil, + options: Operation.t() | nil, + head: Operation.t() | nil, + patch: Operation.t() | nil, + trace: Operation.t() | nil, + servers: [Server.t()] | nil, + parameters: [Parameter.t() | Reference.t()] | nil + } @typedoc """ Represents a route from in a Plug/Phoenix application. Eg from the generated `__routes__` function in a Phoenix.Router module. """ - @type route :: %{verb: atom, plug: atom, opts: any} + @type route :: %{ + required(:verb) => atom, + required(:plug) => atom, + optional(:path) => String.t(), + optional(:opts) => any + } @doc """ Builds a PathItem struct from a list of routes that share a path. """ - @spec from_routes([route]) :: nil | t + @spec from_routes([route]) :: {:ok, nil | t} | {:error, Sting.t()} def from_routes(routes) do Enum.each(routes, fn route -> Code.ensure_loaded(route.plug) @@ -64,9 +70,22 @@ defmodule OpenApiSpex.PathItem do |> from_valid_routes() end - @spec from_valid_routes([route]) :: nil | t - defp from_valid_routes([]), do: nil + @spec from_valid_routes([route]) :: {:ok, nil | t} | {:error, String.t()} + defp from_valid_routes([]), do: {:ok, nil} + defp from_valid_routes(routes) do - struct(PathItem, Enum.map(routes, &{&1.verb, Operation.from_route(&1)})) + routes + |> Enum.map(fn route -> {route.verb, Operation.from_route(route)} end) + |> Enum.reduce_while( + %PathItem{}, + fn + {verb, {:ok, operation}}, acc -> {:cont, Map.put(acc, verb, operation)} + {_verb, {:error, reason}}, _acc -> {:halt, {:error, reason}} + end + ) + |> case do + {:error, reason} -> {:error, reason} + path_item = %PathItem{} -> {:ok, path_item} + end end end diff --git a/lib/open_api_spex/paths.ex b/lib/open_api_spex/paths.ex index 4dfa5792..aafa9b73 100644 --- a/lib/open_api_spex/paths.ex +++ b/lib/open_api_spex/paths.ex @@ -11,25 +11,30 @@ defmodule OpenApiSpex.Paths do The path is appended to the URL from the Server Object in order to construct the full URL. The Paths MAY be empty, due to ACL constraints. """ - @type t :: %{String.t => PathItem.t} + @type t :: %{String.t() => PathItem.t()} @doc """ Create a Paths map from the routes in the given router module. """ - @spec from_router(module) :: t + @spec from_router(module) :: {:ok, t} | {:error, String.t()} def from_router(router) do router.__routes__() - |> Enum.group_by(fn route -> route.path end) - |> Enum.map(fn {k, v} -> {open_api_path(k), PathItem.from_routes(v)} end) - |> Enum.filter(fn {_k, v} -> !is_nil(v) end) - |> Map.new() + |> Enum.group_by(fn route -> open_api_path(route.path) end) + |> Enum.map(fn {path, routes} -> {path, PathItem.from_routes(routes)} end) + |> Enum.reduce_while(%{}, fn + {_path, {:error, reason}}, _acc -> {:halt, {:error, reason}} + {_path, {:ok, nil}}, acc -> {:cont, acc} + {path, {:ok, path_item}}, acc -> {:cont, Map.put(acc, path, path_item)} + end) + |> case do + {:error, reason} -> raise reason + paths -> paths + end end - @spec open_api_path(String.t) :: String.t + @spec open_api_path(String.t()) :: String.t() defp open_api_path(path) do - path - |> String.split("/") - |> Enum.map(fn ":"<>segment -> "{#{segment}}"; segment -> segment end) - |> Enum.join("/") + pattern = ~r{:([^/]+)} + Regex.replace(pattern, path, "{\\1}") end -end \ No newline at end of file +end diff --git a/test/operation_test.exs b/test/operation_test.exs index 58001263..994807fe 100644 --- a/test/operation_test.exs +++ b/test/operation_test.exs @@ -3,14 +3,29 @@ defmodule OpenApiSpex.OperationTest do alias OpenApiSpex.Operation alias OpenApiSpexTest.UserController - describe "Operation" do - test "from_route" do - route = %{plug: UserController, opts: :show} - assert Operation.from_route(route) == UserController.show_operation() + describe "Operation.from_route" do + test "succeeds when all path params present" do + route = %{plug: UserController, opts: :show, path: "/users/:id"} + assert Operation.from_route(route) == {:ok, UserController.show_operation()} end - test "from_plug" do + test "Fails on missing path params" do + path = "/teams/:missing_team_id_param/users/:id" + route = %{plug: UserController, opts: :show, path: path} + assert {:error, message} = Operation.from_route(route) + assert message =~ "missing_team_id_param" + end + + test "Allows additional path params not known to phoenix" do + path = "/no/path/params" + route = %{plug: UserController, opts: :show, path: path} + assert {:ok, _operation} = Operation.from_route(route) + end + end + + describe "Operation.from_plug" do + test "invokes the plug to get the Operation" do assert Operation.from_plug(UserController, :show) == UserController.show_operation() end end -end \ No newline at end of file +end diff --git a/test/path_item_test.exs b/test/path_item_test.exs index fc221b30..de9de7de 100644 --- a/test/path_item_test.exs +++ b/test/path_item_test.exs @@ -10,11 +10,11 @@ defmodule OpenApiSpex.PathItemTest do route.path == "/api/users", do: route - path_item = PathItem.from_routes(routes) + {:ok, path_item} = PathItem.from_routes(routes) assert path_item == %PathItem{ get: UserController.index_operation(), post: UserController.create_operation() } end end -end \ No newline at end of file +end diff --git a/test/paths_test.exs b/test/paths_test.exs index f7401291..38496f2b 100644 --- a/test/paths_test.exs +++ b/test/paths_test.exs @@ -11,4 +11,4 @@ defmodule OpenApiSpex.PathsTest do } = paths end end -end \ No newline at end of file +end diff --git a/test/plug/cast_and_validate_test.exs b/test/plug/cast_and_validate_test.exs index f24037a9..98c46760 100644 --- a/test/plug/cast_and_validate_test.exs +++ b/test/plug/cast_and_validate_test.exs @@ -55,9 +55,6 @@ defmodule OpenApiSpex.Plug.CastAndValidateTest do end describe "body params" do - # TODO Fix this test. The datetime should be parsed, but it isn't. - # https://github.com/open-api-spex/open_api_spex/issues/90 - @tag :skip test "Valid Request" do request_body = %{ "user" => %{