improvement: more consistency for route & query params

this also solves cases where route params did not properly inherit
the corresponding types of their inputs

closes #338

Author: zachdaniel

lib/ash_json_api/json_schema/open_api.ex

@@ -1286,7 +1286,7 @@ if Code.ensure_loaded?(OpenApiSpex) do
         description: action_description(action, route, resource),
         operationId: route.name,
         tags: [to_string(AshJsonApi.Resource.Info.type(resource))],
-        parameters: path_parameters(path_params, action) ++ query_parameters(route, resource),
+        parameters: parameters(route, resource, path_params),
         responses: %{
           :default => %Reference{
             "$ref": "#/components/responses/errors"
@@ -1309,33 +1309,12 @@ if Code.ensure_loaded?(OpenApiSpex) do
       end
     end
 
-    @spec path_parameters(path_params :: [String.t()], action :: Actions.action()) ::
-            [Parameter.t()]
-    defp path_parameters(path_params, action) do
-      Enum.map(path_params, fn param ->
-        description =
-          action.arguments
-          |> Enum.find(&(to_string(&1.name) == param))
-          |> case do
-            %{description: description} when is_binary(description) -> description
-            _ -> nil
-          end
-
-        %Parameter{
-          name: param,
-          description: description,
-          in: :path,
-          required: true,
-          schema: %Schema{type: :string}
-        }
-      end)
-    end
-
-    @spec query_parameters(
+    @spec parameters(
             Route.t(),
-            resource :: module
+            resource :: module,
+            route_params :: [String.t()]
           ) :: [Parameter.t()]
-    defp query_parameters(%{type: :index} = route, resource) do
+    defp parameters(%{type: :index} = route, resource, route_params) do
       Enum.filter(
         [
           filter_parameter(resource, route),
@@ -1346,13 +1325,14 @@ if Code.ensure_loaded?(OpenApiSpex) do
         ],
         & &1
       )
-      |> Enum.concat(read_argument_parameters(route, resource))
+      |> Enum.concat(read_argument_parameters(route, resource, route_params))
       |> Enum.map(fn param ->
         Map.update!(param, :name, &to_string/1)
       end)
+      |> apply_route_params(route_params)
     end
 
-    defp query_parameters(%{type: type}, _resource)
+    defp parameters(%{type: type}, _resource, _route_params)
          when type in [
                 :post_to_relationship,
                 :patch_relationship,
@@ -1361,19 +1341,21 @@ if Code.ensure_loaded?(OpenApiSpex) do
       []
     end
 
-    defp query_parameters(%{type: type} = route, resource) when type in [:get, :related] do
+    defp parameters(%{type: type} = route, resource, route_params)
+         when type in [:get, :related] do
       [include_parameter(resource), fields_parameter(resource)]
       |> Enum.filter(& &1)
-      |> Enum.concat(read_argument_parameters(route, resource))
+      |> Enum.concat(read_argument_parameters(route, resource, route_params))
       |> Enum.reverse()
       |> Enum.map(fn param ->
         Map.update!(param, :name, &to_string/1)
       end)
       |> Enum.uniq_by(& &1.name)
       |> Enum.reverse()
+      |> apply_route_params(route_params)
     end
 
-    defp query_parameters(route, resource) do
+    defp parameters(route, resource, route_params) do
       action = Ash.Resource.Info.action(resource, route.action)
 
       query_params =
@@ -1384,28 +1366,52 @@ if Code.ensure_loaded?(OpenApiSpex) do
           if argument do
             argument
           else
-            if name in action.accept do
+            if name in Map.get(action, :accept, []) do
               Ash.Resource.Info.attribute(resource, name)
             else
               nil
             end
           end
         end)
+        |> Enum.concat(
+          Enum.map(route_params, fn route_param ->
+            case Enum.find(action.arguments, &(to_string(&1.name) == route_param)) do
+              nil ->
+                if Enum.any?(Map.get(action, :accept, []), &(to_string(&1) == route_param)) do
+                  Ash.Resource.Info.attribute(resource, route_param)
+                else
+                  nil
+                end
+
+              argument ->
+                argument
+            end
+          end)
+        )
         |> Enum.filter(& &1)
         |> Enum.map(fn argument_or_attribute ->
           schema = resource_write_attribute_type(argument_or_attribute, resource, action.type)
 
+          location =
+            if to_string(argument_or_attribute.name) in route_params do
+              :path
+            else
+              :query
+            end
+
+          style =
+            if schema.type == :object && location == :query do
+              :deepObject
+            else
+              :form
+            end
+
           %Parameter{
             name: to_string(argument_or_attribute.name),
-            in: :query,
-            description:
-              argument_or_attribute.description || to_string(argument_or_attribute.name),
-            required: !argument_or_attribute.allow_nil?,
-            style:
-              case schema.type do
-                :object -> :deepObject
-                _ -> :form
-              end,
+            in: location,
+            description: argument_or_attribute.description,
+            required: location == :path || !argument_or_attribute.allow_nil?,
+            style: style,
             schema: schema
           }
         end)
@@ -1423,6 +1429,26 @@ if Code.ensure_loaded?(OpenApiSpex) do
       end)
       |> Enum.uniq_by(& &1.name)
       |> Enum.reverse()
+      |> apply_route_params(route_params)
+    end
+
+    defp apply_route_params(params, route_params) do
+      param_names = Enum.map(params, & &1.name)
+
+      route_params_to_add =
+        route_params
+        |> Enum.reject(&(&1 in param_names))
+        |> Enum.map(fn name ->
+          %Parameter{
+            name: name,
+            in: :path,
+            required: true,
+            style: :form,
+            schema: %Schema{type: :string}
+          }
+        end)
+
+      route_params_to_add ++ Enum.sort_by(params, &(&1.in == :query))
     end
 
     @spec filter_parameter(resource :: module, route :: AshJsonApi.Resource.Route.t()) ::
@@ -1636,26 +1662,36 @@ if Code.ensure_loaded?(OpenApiSpex) do
       }
     end
 
-    @spec read_argument_parameters(Route.t(), resource :: module) :: [Parameter.t()]
-    defp read_argument_parameters(route, resource) do
+    @spec read_argument_parameters(Route.t(), resource :: module, route_params :: [String.t()]) ::
+            [Parameter.t()]
+    defp read_argument_parameters(route, resource, route_params) do
       action = Ash.Resource.Info.action(resource, route.action)
 
       action.arguments
       |> Enum.filter(& &1.public?)
-      |> without_path_arguments(route)
       |> Enum.map(fn argument ->
         schema = resource_attribute_type(argument, resource)
 
+        location =
+          if to_string(argument.name) in route_params do
+            :path
+          else
+            :query
+          end
+
+        style =
+          if schema.type == :object && location == :query do
+            :deepObject
+          else
+            :form
+          end
+
         %Parameter{
           name: argument.name,
-          in: :query,
-          description: argument.description || to_string(argument.name),
-          required: !argument.allow_nil?,
-          style:
-            case schema.type do
-              :object -> :deepObject
-              _ -> :form
-            end,
+          in: location,
+          description: argument.description,
+          required: location == :path || !argument.allow_nil?,
+          style: style,
           schema: schema
         }
       end)

test/acceptance/open_api_test.exs

@@ -58,6 +58,7 @@ defmodule Test.Acceptance.OpenApiTest do
         route :post, "/say_hello/:to", :say_hello
         route :post, "/trigger_job", :trigger_job, query_params: [:job_id]
         route :post, "/trigger_job/:job_id", :trigger_job
+        route :post, "/path_with_enum/:enum", :takes_enum
         route(:get, "returns_map", :returns_map)
         route(:get, "/get_foo", :get_foo)
         post_to_relationship :posts
@@ -94,6 +95,14 @@ defmodule Test.Acceptance.OpenApiTest do
         end)
       end
 
+      action :takes_enum do
+        argument(:enum, :atom, constraints: [one_of: [:one, :two]])
+
+        run(fn _input, _ ->
+          :ok
+        end)
+      end
+
       action :returns_map, :map do
         constraints(fields: [a: [type: :integer], b: [type: :string, allow_nil?: false]])
 
@@ -320,7 +329,7 @@ defmodule Test.Acceptance.OpenApiTest do
   end
 
   test "API routes are mapped to OpenAPI Operations", %{open_api_spec: %OpenApi{} = api_spec} do
-    assert map_size(api_spec.paths) == 12
+    assert map_size(api_spec.paths) == 13
 
     assert %{"/authors" => _, "/authors/{id}" => _, "/posts" => _, "/posts/{id}" => _} =
              api_spec.paths
@@ -439,7 +448,6 @@ defmodule Test.Acceptance.OpenApiTest do
              %Parameter{
                name: "job_id",
                in: :query,
-               description: "job_id",
                required: false,
                schema: %Schema{type: :string},
                style: :form
@@ -452,8 +460,8 @@ defmodule Test.Acceptance.OpenApiTest do
              %Parameter{
                name: "job_id",
                in: :path,
-               description: nil,
                required: true,
+               style: :form,
                schema: %Schema{type: :string}
              }
            ]
@@ -469,6 +477,22 @@ defmodule Test.Acceptance.OpenApiTest do
              }
   end
 
+  test "generic route paths have enums", %{
+    open_api_spec: %OpenApi{} = api_spec
+  } do
+    assert generic_action_schema = api_spec.paths["/authors/path_with_enum/{enum}"].post
+
+    assert generic_action_schema.parameters == [
+             %Parameter{
+               name: "enum",
+               in: :path,
+               required: true,
+               schema: %OpenApiSpex.Schema{enum: ["one", "two"], type: :string},
+               style: :form
+             }
+           ]
+  end
+
   test "API routes use `name` as operationId", %{
     open_api_spec: %OpenApi{} = api_spec
   } do
@@ -805,7 +829,7 @@ defmodule Test.Acceptance.OpenApiTest do
       %Parameter{} = filter = operation.parameters |> Enum.find(&(&1.name == "id"))
       assert filter.in == :path
       assert filter.required == true
-      assert filter.style == nil
+      assert filter.style == :form
       %Schema{} = schema = filter.schema
       assert schema.type == :string
     end