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

Author: mbuhot

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 """

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

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
+end

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
+end

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
+end

test/paths_test.exs

@@ -11,4 +11,4 @@ defmodule OpenApiSpex.PathsTest do
       } = paths
     end
   end
-end
+end

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" => %{