From f14596b2e801e4019e710da8957dfbba0ac67c85 Mon Sep 17 00:00:00 2001 From: Amir Hasanbasic Date: Thu, 23 Nov 2023 14:24:23 +0100 Subject: [PATCH 1/2] Add tests and fix parameter exploding issue --- .../plug_app/lib/plug_app/user_handler.ex | 33 ++++++++- examples/plug_app/mix.exs | 3 +- examples/plug_app/mix.lock | 1 + examples/plug_app/test/user_handler_test.exs | 21 +++++- lib/open_api_spex/cast_parameters.ex | 58 ++++++++++++++++ lib/open_api_spex/schema.ex | 17 +++++ test/cast_parameters_test.exs | 69 +++++++++++++++++++ 7 files changed, 198 insertions(+), 4 deletions(-) diff --git a/examples/plug_app/lib/plug_app/user_handler.ex b/examples/plug_app/lib/plug_app/user_handler.ex index 8a598245..292b87f9 100644 --- a/examples/plug_app/lib/plug_app/user_handler.ex +++ b/examples/plug_app/lib/plug_app/user_handler.ex @@ -58,7 +58,31 @@ defmodule PlugApp.UserHandler do description: "Show a user by ID", operationId: "UserHandler.Show", parameters: [ - parameter(:id, :path, %Schema{type: :integer, minimum: 1}, "User ID", example: 123) + parameter(:id, :path, %Schema{type: :integer, minimum: 1}, "User ID", example: 123), + parameter(:qux, :query, %Schema{type: :string}, "qux param", required: false), + parameter( + :some, + :query, + %Schema{ + type: :object, + oneOf: [ + %Schema{ + type: :object, + properties: %{foo: %Schema{type: :string}, bar: %Schema{type: :string}}, + required: [:foo] + }, + %Schema{ + type: :object, + properties: %{foo: %Schema{type: :string}, baz: %Schema{type: :string}}, + required: [:baz] + } + ] + }, + "Some query parameter ", + explode: true, + style: :form, + required: true + ) ], responses: %{ 200 => response("User", "application/json", Schemas.UserResponse) @@ -79,7 +103,12 @@ defmodule PlugApp.UserHandler do end end - def show(conn = %Plug.Conn{assigns: %{user: user}}, _opts) do + # def show(conn = %Plug.Conn{assigns: %{user: user}}, _opts) do + def show(conn, _opts) do + IO.inspect(conn.params) + + user = Accounts.get_user!(conn.params.id) + conn |> put_resp_header("content-type", "application/json") |> send_resp(200, render(user)) diff --git a/examples/plug_app/mix.exs b/examples/plug_app/mix.exs index 0d6d4216..9d5858e9 100644 --- a/examples/plug_app/mix.exs +++ b/examples/plug_app/mix.exs @@ -29,7 +29,8 @@ defmodule PlugApp.Mixfile do {:plug, "~> 1.0"}, {:ecto, "~> 2.2"}, {:sqlite_ecto2, "~> 2.4"}, - {:jason, "~> 1.0"} + {:jason, "~> 1.0"}, + {:cors_plug, "~> 3.0"} # {:dep_from_hexpm, "~> 0.3.0"}, # {:dep_from_git, git: "https://github.com/elixir-lang/my_dep.git", tag: "0.1.0"}, ] diff --git a/examples/plug_app/mix.lock b/examples/plug_app/mix.lock index 1ed6ae45..0919b578 100644 --- a/examples/plug_app/mix.lock +++ b/examples/plug_app/mix.lock @@ -1,5 +1,6 @@ %{ "connection": {:hex, :connection, "1.0.4", "a1cae72211f0eef17705aaededacac3eb30e6625b04a6117c1b2db6ace7d5976", [:mix], [], "hexpm", "4a0850c9be22a43af9920a71ab17c051f5f7d45c209e40269a1938832510e4d9"}, + "cors_plug": {:hex, :cors_plug, "3.0.3", "7c3ac52b39624bc616db2e937c282f3f623f25f8d550068b6710e58d04a0e330", [:mix], [{:plug, "~> 1.13", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "3f2d759e8c272ed3835fab2ef11b46bddab8c1ab9528167bd463b6452edf830d"}, "cowboy": {:hex, :cowboy, "2.9.0", "865dd8b6607e14cf03282e10e934023a1bd8be6f6bacf921a7e2a96d800cd452", [:make, :rebar3], [{:cowlib, "2.11.0", [hex: :cowlib, repo: "hexpm", optional: false]}, {:ranch, "1.8.0", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "2c729f934b4e1aa149aff882f57c6372c15399a20d54f65c8d67bef583021bde"}, "cowboy_telemetry": {:hex, :cowboy_telemetry, "0.4.0", "f239f68b588efa7707abce16a84d0d2acf3a0f50571f8bb7f56a15865aae820c", [:rebar3], [{:cowboy, "~> 2.7", [hex: :cowboy, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "7d98bac1ee4565d31b62d59f8823dfd8356a169e7fcbb83831b8a5397404c9de"}, "cowlib": {:hex, :cowlib, "2.11.0", "0b9ff9c346629256c42ebe1eeb769a83c6cb771a6ee5960bd110ab0b9b872063", [:make, :rebar3], [], "hexpm", "2b3e9da0b21c4565751a6d4901c20d1b4cc25cbb7fd50d91d2ab6dd287bc86a9"}, diff --git a/examples/plug_app/test/user_handler_test.exs b/examples/plug_app/test/user_handler_test.exs index 50866244..6774f0da 100644 --- a/examples/plug_app/test/user_handler_test.exs +++ b/examples/plug_app/test/user_handler_test.exs @@ -46,7 +46,7 @@ defmodule UserHandlerTest do } do %{resp_body: body} = conn = - conn(:get, "/api/users/#{user_id}") + conn(:get, "/api/users/#{user_id}?foo=asd") |> Router.call(@opts) assert %{status: 200} = conn @@ -57,6 +57,25 @@ defmodule UserHandlerTest do assert_schema(json_response, "UserResponse", api_spec) end + + test "responds with 422 when there is no either foo nor bar in query params", %{ + user: %{id: user_id}, + api_spec: _api_spec + } do + %{resp_body: body} = + conn = + conn(:get, "/api/users/#{user_id}") + |> Router.call(@opts) + + assert %{status: 422} = conn + + json_response = Jason.decode!(body) + + IO.inspect(json_response) + # assert %{} = json_response + + # assert_schema(json_response, "UserResponse", api_spec) + end end describe "POST /api/users" do diff --git a/lib/open_api_spex/cast_parameters.ex b/lib/open_api_spex/cast_parameters.ex index 1155cb79..d8338042 100644 --- a/lib/open_api_spex/cast_parameters.ex +++ b/lib/open_api_spex/cast_parameters.ex @@ -69,11 +69,13 @@ defmodule OpenApiSpex.CastParameters do properties: parameters |> Map.new(fn p -> {p.name, Parameter.schema(p)} end), required: parameters |> Enum.filter(& &1.required) |> Enum.map(& &1.name) } + # |> maybe_combine_oneOfs(parameters, components) |> maybe_add_additional_properties(components), parameters_contexts(parameters) } end + # Extract context information from parameters, useful later when casting defp parameters_contexts(parameters) do Map.new(parameters, fn parameter -> @@ -127,6 +129,7 @@ defmodule OpenApiSpex.CastParameters do location, schema.properties |> Map.keys() |> Enum.map(&Atom.to_string/1) ) + |> maybe_combine_params(schema, parameters_contexts) |> pre_parse_parameters(parameters_contexts, parsers) |> case do {:error, _} = err -> err @@ -134,6 +137,45 @@ defmodule OpenApiSpex.CastParameters do end end + # in caase some parameters have explode: true we want to search for those + # fields in parameters and combine the parameters in a single struct + # so that the casting can do further work + defp maybe_combine_params(%{} = parameters, %{} = schema, %{} = parameters_contexts) do + Enum.reduce(parameters_contexts, parameters, fn + {key, %{explode: true}}, parameters -> + # we have exploding property, we need to search for it's possible fields + # and add them under the key into the parameters struct. + # do we leave the fields in the params as well? not sure. + schema_of_exploding_property = Map.get(schema.properties, String.to_existing_atom(key), %{}) + + fields = + Schema.properties(schema_of_exploding_property) ++ + Schema.possible_properties(schema_of_exploding_property) + + {struct_params, found_keys} = + Enum.reduce(fields, {Map.new(), []}, fn {field_key, _}, {struct_params, found_keys} -> + param_field_key = field_key |> Atom.to_string() + val = Map.get(parameters, param_field_key) + + {new_params, new_found_keys} = + unless is_nil(val) do + {Map.put(struct_params, param_field_key, val), [param_field_key | found_keys]} + else + {struct_params, found_keys} + end + + {new_params, new_found_keys} + end) + + parameters + |> Map.drop(found_keys) + |> Map.put(key, struct_params) + + _, parameters -> + parameters + end) + end + defp pre_parse_parameters(%{} = parameters, %{} = parameters_context, parsers) do Enum.reduce_while(parameters, Map.new(), fn {key, value}, acc -> case pre_parse_parameter(value, Map.get(parameters_context, key, %{}), parsers) do @@ -208,4 +250,20 @@ defmodule OpenApiSpex.CastParameters do _ -> schema end end + + defp maybe_combine_oneOfs(schema, parameters, components) do + # check if any params have explode, + # if so add the properties of it's schema to the top level + # and remove the key for that + %{} + end + + defp create_one_of_schemas(parameters) do + if Enum.any?(parameters, fn p -> + p.explode == true and is_list(Parameter.schema(p).oneOf) + end) do + # in this case we need to create multiple schemas. Each of the schemas + # has to have properties defined in other parameters + add required properties + end + end end diff --git a/lib/open_api_spex/schema.ex b/lib/open_api_spex/schema.ex index a608a85f..096d180a 100644 --- a/lib/open_api_spex/schema.ex +++ b/lib/open_api_spex/schema.ex @@ -344,6 +344,23 @@ defmodule OpenApiSpex.Schema do def properties(_), do: [] + @doc """ + Get the names of all properties possible for polymorphic schemas using `oneOf`. + + This is different from properties/1 in that it returns properties that *might* + be a part of the schema sometimes based on the discriminator. + """ + + def possible_properties(%Schema{oneOf: schemas}) when is_list(schemas) do + Enum.flat_map(schemas, &properties/1) |> Enum.uniq() + end + + def possible_properties(%Schema{anyOf: schemas}) when is_list(schemas) do + Enum.flat_map(schemas, &properties/1) |> Enum.uniq() + end + + def possible_properties(_), do: [] + @doc """ Generate example value from a `%Schema{}` struct. diff --git a/test/cast_parameters_test.exs b/test/cast_parameters_test.exs index b881a914..a887f8b1 100644 --- a/test/cast_parameters_test.exs +++ b/test/cast_parameters_test.exs @@ -1,6 +1,8 @@ defmodule OpenApiSpex.CastParametersTest do use ExUnit.Case + require IEx + alias OpenApiSpex.{ CastParameters, Components, @@ -191,6 +193,23 @@ defmodule OpenApiSpex.CastParametersTest do [%OpenApiSpex.Cast.Error{format: "application/json", reason: :invalid_format}]} = CastParameters.cast(conn, operation, spec) end + + test "cast param with oneof and valid args" do + {operation, spec} = oneof_query_spec_operation() + + filter_params = URI.encode_query(%{size: "XL"}) + + conn = + :get + |> Plug.Test.conn("/api/users?#{filter_params}") + |> Plug.Conn.put_req_header("content-type", "application/json") + |> Plug.Conn.fetch_query_params() + + # require IEx + # IEx.pry() + + assert {:ok, _} = CastParameters.cast(conn, operation, spec) + end end defp create_conn() do @@ -294,4 +313,54 @@ defmodule OpenApiSpex.CastParametersTest do {operation, spec} end + + defp oneof_query_spec_operation() do + schema = %Schema{ + type: :object, + title: "Filters", + oneOf: [ + %Schema{ + type: :object, + properties: %{ + size: %Schema{type: :string, pattern: "^XS|S|M|L|XL$"}, + color: %Schema{type: :string} + }, + required: [:size] + }, + %Schema{ + type: :object, + properties: %{ + size: %Schema{type: :string, pattern: "^XS|S|M|L|XL$"}, + color: %Schema{type: :string} + }, + required: [:color] + } + ], + example: %{size: "XL"} + } + + parameter = %Parameter{ + in: :query, + name: :filter, + required: false, + schema: %Reference{"$ref": "#/components/schemas/Filters"}, + explode: true, + style: :form, + required: true + } + + operation = %Operation{ + parameters: [parameter], + responses: %{ + 200 => %Schema{type: :object} + } + } + + spec = + spec_with_components(%Components{ + schemas: %{"Filters" => schema} + }) + + {operation, spec} + end end From e8f2137e7a569b967d7271105bd44ca4b16caced Mon Sep 17 00:00:00 2001 From: Amir Hasanbasic Date: Wed, 3 Jan 2024 16:45:45 +0100 Subject: [PATCH 2/2] Remove matching params before constructing exploded params --- .../plug_app/lib/plug_app/user_handler.ex | 5 +-- examples/plug_app/mix.exs | 3 +- examples/plug_app/mix.lock | 1 - lib/open_api_spex/cast_parameters.ex | 42 +++++++++---------- test/cast_parameters_test.exs | 5 --- 5 files changed, 22 insertions(+), 34 deletions(-) diff --git a/examples/plug_app/lib/plug_app/user_handler.ex b/examples/plug_app/lib/plug_app/user_handler.ex index 292b87f9..50061033 100644 --- a/examples/plug_app/lib/plug_app/user_handler.ex +++ b/examples/plug_app/lib/plug_app/user_handler.ex @@ -103,10 +103,7 @@ defmodule PlugApp.UserHandler do end end - # def show(conn = %Plug.Conn{assigns: %{user: user}}, _opts) do - def show(conn, _opts) do - IO.inspect(conn.params) - + def show(conn = %Plug.Conn{assigns: %{user: user}}, _opts) do user = Accounts.get_user!(conn.params.id) conn diff --git a/examples/plug_app/mix.exs b/examples/plug_app/mix.exs index 9d5858e9..0d6d4216 100644 --- a/examples/plug_app/mix.exs +++ b/examples/plug_app/mix.exs @@ -29,8 +29,7 @@ defmodule PlugApp.Mixfile do {:plug, "~> 1.0"}, {:ecto, "~> 2.2"}, {:sqlite_ecto2, "~> 2.4"}, - {:jason, "~> 1.0"}, - {:cors_plug, "~> 3.0"} + {:jason, "~> 1.0"} # {:dep_from_hexpm, "~> 0.3.0"}, # {:dep_from_git, git: "https://github.com/elixir-lang/my_dep.git", tag: "0.1.0"}, ] diff --git a/examples/plug_app/mix.lock b/examples/plug_app/mix.lock index 0919b578..1ed6ae45 100644 --- a/examples/plug_app/mix.lock +++ b/examples/plug_app/mix.lock @@ -1,6 +1,5 @@ %{ "connection": {:hex, :connection, "1.0.4", "a1cae72211f0eef17705aaededacac3eb30e6625b04a6117c1b2db6ace7d5976", [:mix], [], "hexpm", "4a0850c9be22a43af9920a71ab17c051f5f7d45c209e40269a1938832510e4d9"}, - "cors_plug": {:hex, :cors_plug, "3.0.3", "7c3ac52b39624bc616db2e937c282f3f623f25f8d550068b6710e58d04a0e330", [:mix], [{:plug, "~> 1.13", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm", "3f2d759e8c272ed3835fab2ef11b46bddab8c1ab9528167bd463b6452edf830d"}, "cowboy": {:hex, :cowboy, "2.9.0", "865dd8b6607e14cf03282e10e934023a1bd8be6f6bacf921a7e2a96d800cd452", [:make, :rebar3], [{:cowlib, "2.11.0", [hex: :cowlib, repo: "hexpm", optional: false]}, {:ranch, "1.8.0", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "2c729f934b4e1aa149aff882f57c6372c15399a20d54f65c8d67bef583021bde"}, "cowboy_telemetry": {:hex, :cowboy_telemetry, "0.4.0", "f239f68b588efa7707abce16a84d0d2acf3a0f50571f8bb7f56a15865aae820c", [:rebar3], [{:cowboy, "~> 2.7", [hex: :cowboy, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "7d98bac1ee4565d31b62d59f8823dfd8356a169e7fcbb83831b8a5397404c9de"}, "cowlib": {:hex, :cowlib, "2.11.0", "0b9ff9c346629256c42ebe1eeb769a83c6cb771a6ee5960bd110ab0b9b872063", [:make, :rebar3], [], "hexpm", "2b3e9da0b21c4565751a6d4901c20d1b4cc25cbb7fd50d91d2ab6dd287bc86a9"}, diff --git a/lib/open_api_spex/cast_parameters.ex b/lib/open_api_spex/cast_parameters.ex index d8338042..1b4285b1 100644 --- a/lib/open_api_spex/cast_parameters.ex +++ b/lib/open_api_spex/cast_parameters.ex @@ -69,7 +69,6 @@ defmodule OpenApiSpex.CastParameters do properties: parameters |> Map.new(fn p -> {p.name, Parameter.schema(p)} end), required: parameters |> Enum.filter(& &1.required) |> Enum.map(& &1.name) } - # |> maybe_combine_oneOfs(parameters, components) |> maybe_add_additional_properties(components), parameters_contexts(parameters) } @@ -141,7 +140,23 @@ defmodule OpenApiSpex.CastParameters do # fields in parameters and combine the parameters in a single struct # so that the casting can do further work defp maybe_combine_params(%{} = parameters, %{} = schema, %{} = parameters_contexts) do - Enum.reduce(parameters_contexts, parameters, fn + # first filter out from parameters fields that match non exploding properties. + # we do this because we want to keep the original parameters struct intact + # and not remove fields that are not part of the exploding property + + non_exploding_matches = Enum.reduce(parameters, Map.new(), fn {key, value}, acc -> + case Map.get(parameters_contexts, key, %{}) do + %{explode: false} -> + Map.put(acc, key, value) + + _ -> + acc + end + end) + + possible_exploding_matches = Enum.reject(parameters, &Enum.member?(non_exploding_matches, &1)) |> Map.new() + + combined_params = Enum.reduce(parameters_contexts, possible_exploding_matches, fn {key, %{explode: true}}, parameters -> # we have exploding property, we need to search for it's possible fields # and add them under the key into the parameters struct. @@ -153,18 +168,15 @@ defmodule OpenApiSpex.CastParameters do Schema.possible_properties(schema_of_exploding_property) {struct_params, found_keys} = - Enum.reduce(fields, {Map.new(), []}, fn {field_key, _}, {struct_params, found_keys} -> + Enum.reduce(fields, {Map.new(), []}, fn {field_key, _default}, {struct_params, found_keys} -> param_field_key = field_key |> Atom.to_string() val = Map.get(parameters, param_field_key) - {new_params, new_found_keys} = unless is_nil(val) do {Map.put(struct_params, param_field_key, val), [param_field_key | found_keys]} else {struct_params, found_keys} end - - {new_params, new_found_keys} end) parameters @@ -174,6 +186,8 @@ defmodule OpenApiSpex.CastParameters do _, parameters -> parameters end) + + Map.merge(non_exploding_matches, combined_params) end defp pre_parse_parameters(%{} = parameters, %{} = parameters_context, parsers) do @@ -250,20 +264,4 @@ defmodule OpenApiSpex.CastParameters do _ -> schema end end - - defp maybe_combine_oneOfs(schema, parameters, components) do - # check if any params have explode, - # if so add the properties of it's schema to the top level - # and remove the key for that - %{} - end - - defp create_one_of_schemas(parameters) do - if Enum.any?(parameters, fn p -> - p.explode == true and is_list(Parameter.schema(p).oneOf) - end) do - # in this case we need to create multiple schemas. Each of the schemas - # has to have properties defined in other parameters + add required properties - end - end end diff --git a/test/cast_parameters_test.exs b/test/cast_parameters_test.exs index a887f8b1..fb250e05 100644 --- a/test/cast_parameters_test.exs +++ b/test/cast_parameters_test.exs @@ -1,8 +1,6 @@ defmodule OpenApiSpex.CastParametersTest do use ExUnit.Case - require IEx - alias OpenApiSpex.{ CastParameters, Components, @@ -205,9 +203,6 @@ defmodule OpenApiSpex.CastParametersTest do |> Plug.Conn.put_req_header("content-type", "application/json") |> Plug.Conn.fetch_query_params() - # require IEx - # IEx.pry() - assert {:ok, _} = CastParameters.cast(conn, operation, spec) end end