Ensure path parameters are declared in each Operation

`Paths.from_router` now returns an ok/error tuple.
The error case will occurr if any operation fails to
declare a path parameter that is present in the phoenix router.

Author: mbuhot

CHANGELOG.md

@@ -1,3 +1,9 @@
+# 4.0.0 (in development)
+
+ - `Paths.from_router/1` now returns an `:ok/:error` tuple and validates that
+   all path parameters are declared in the OpenAPI `Operation`.
+   Use `Paths.from_router!` for a version that will raise immediately on any missing parameters.
+
 # 3.2.1
 
 Patch release for documentation updates and improved error rendering Plug when using `CastAndValidate`.

README.md

@@ -44,7 +44,7 @@ defmodule MyApp.ApiSpec do
         version: "1.0"
       },
       # populate the paths from a phoenix router
-      paths: Paths.from_router(MyAppWeb.Router)
+      paths: Paths.from_router!(MyAppWeb.Router)
     }
     |> OpenApiSpex.resolve_schema_modules() # discover request/response schemas from path specs
   end

examples/phoenix_app/lib/phoenix_app_web/api_spec.ex

@@ -7,8 +7,8 @@ defmodule PhoenixAppWeb.ApiSpec do
         title: "Phoenix App",
         version: "1.0"
       },
-      paths: Paths.from_router(PhoenixAppWeb.Router)
+      paths: Paths.from_router!(PhoenixAppWeb.Router)
     }
     |> OpenApiSpex.resolve_schema_modules()
   end
-end
+end

lib/open_api_spex/operation.ex

@@ -52,11 +52,39 @@ 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, route.opts)
+    check_all_path_params_declared(operation, route.path)
+  end
+
+  defp check_all_path_params_declared(operation, route_path) do
+    pattern = ~r/{(.*?)}/
+    expected_path_params =
+      pattern
+      |> Regex.scan(route_path)
+      |> Enum.map(fn [_match, param] -> param end)
+
+    actual_path_params =
+      operation.parameters
+      |> Enum.filter(& &1.in == :path)
+      |> Enum.map(& &1.name |> to_string)
+
+    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 :: %{
+          verb: atom,
+          path: String.t(),
+          plug: atom,
+          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

@@ -16,20 +16,32 @@ defmodule OpenApiSpex.Paths do
   @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.map(fn route -> %{route | path: open_api_path(route.path)} end)
     |> 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.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} -> {:error, reason}
+      paths -> {:ok, paths}
+    end
+  end
+
+  @spec from_router!(module) :: t
+  def from_router!(router) do
+    {:ok, paths} = from_router(router)
+    paths
   end
 
   @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

@@ -5,10 +5,10 @@ defmodule OpenApiSpex.PathsTest do
 
   describe "Paths" do
     test "from_router" do
-      paths = Paths.from_router(Router)
+      paths = Paths.from_router!(Router)
       assert %{
         "/api/users" => %PathItem{},
       } = 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" => %{