From ad6dfdb8f7b44c4ddb7a2fd7fd965a58107c5d53 Mon Sep 17 00:00:00 2001 From: Roland Tritsch Date: Sun, 8 May 2022 18:34:45 +0100 Subject: [PATCH 1/8] The test that shows the problem. --- test/plug/cast_test.exs | 39 ++++++++++++++++++++------------ test/support/noapi_controller.ex | 15 ++++++++++++ test/support/router.ex | 1 + 3 files changed, 41 insertions(+), 14 deletions(-) create mode 100644 test/support/noapi_controller.ex diff --git a/test/plug/cast_test.exs b/test/plug/cast_test.exs index 478bfc2e..4b1c2c35 100644 --- a/test/plug/cast_test.exs +++ b/test/plug/cast_test.exs @@ -5,8 +5,19 @@ defmodule OpenApiSpex.Plug.CastTest do alias OpenApiSpexTest.ApiSpec + describe "valid operation (not annotated)" do + test "should work" do + conn = + :get + |> Plug.Test.conn("/api/noapi") + |> OpenApiSpexTest.Router.call([]) + + assert conn.status == 200 + end + end + describe "query params - basics" do - test "Valid Param" do + test "valid Param" do conn = :get |> Plug.Test.conn("/api/users?validParam=true") @@ -16,7 +27,7 @@ defmodule OpenApiSpex.Plug.CastTest do end @tag :capture_log - test "Invalid value" do + test "invalid value" do conn = :get |> Plug.Test.conn("/api/users?validParam=123") @@ -26,7 +37,7 @@ defmodule OpenApiSpex.Plug.CastTest do end @tag :capture_log - test "Invalid Param" do + test "invalid param" do conn = :get |> Plug.Test.conn("/api/users?validParam=123") @@ -83,7 +94,7 @@ defmodule OpenApiSpex.Plug.CastTest do end describe "query params - param with custom error handling" do - test "Valid Param" do + test "valid param" do conn = :get |> Plug.Test.conn("/api/custom_error_users?validParam=true") @@ -93,7 +104,7 @@ defmodule OpenApiSpex.Plug.CastTest do end @tag :capture_log - test "Invalid value" do + test "invalid value" do conn = :get |> Plug.Test.conn("/api/custom_error_users?validParam=123") @@ -103,7 +114,7 @@ defmodule OpenApiSpex.Plug.CastTest do end @tag :capture_log - test "Invalid Param" do + test "invalid param" do conn = :get |> Plug.Test.conn("/api/custom_error_users?validParam=123") @@ -115,7 +126,7 @@ defmodule OpenApiSpex.Plug.CastTest do end describe "body params" do - test "Valid Request" do + test "valid request" do request_body = %{ "user" => %{ "id" => 123, @@ -154,7 +165,7 @@ defmodule OpenApiSpex.Plug.CastTest do end @tag :capture_log - test "Invalid Request" do + test "invalid request" do request_body = %{ "user" => %{ "id" => 123, @@ -185,7 +196,7 @@ defmodule OpenApiSpex.Plug.CastTest do end describe "oneOf body params" do - test "Valid Request" do + test "valid request" do request_body = %{ "pet" => %{ "pet_type" => "Dog", @@ -215,7 +226,7 @@ defmodule OpenApiSpex.Plug.CastTest do end @tag :capture_log - test "Invalid Request" do + test "invalid request" do request_body = %{ "pet" => %{ "pet_type" => "Human", @@ -241,7 +252,7 @@ defmodule OpenApiSpex.Plug.CastTest do } end - test "Header params" do + test "header params" do conn = :post |> Plug.Test.conn("/api/pets/1/adopt") @@ -257,7 +268,7 @@ defmodule OpenApiSpex.Plug.CastTest do } end - test "Optional param" do + test "optional param" do conn = :post |> Plug.Test.conn("/api/pets/1/adopt?status=adopted") @@ -273,7 +284,7 @@ defmodule OpenApiSpex.Plug.CastTest do } end - test "Cookie params" do + test "cookie params" do conn = :post |> Plug.Test.conn("/api/pets/1/adopt") @@ -290,7 +301,7 @@ defmodule OpenApiSpex.Plug.CastTest do } end - test "Discriminator with mapping" do + test "discriminator with mapping" do body = Jason.encode!(%{ appointment_type: "grooming", diff --git a/test/support/noapi_controller.ex b/test/support/noapi_controller.ex new file mode 100644 index 00000000..6e2691a2 --- /dev/null +++ b/test/support/noapi_controller.ex @@ -0,0 +1,15 @@ +defmodule OpenApiSpexTest.NoApiController do + @moduledoc false + + use Phoenix.Controller + use OpenApiSpex.ControllerSpecs + + plug OpenApiSpex.Plug.CastAndValidate, json_render_error_v2: true + + operation :noapi, false + + def noapi(conn, _opts) do + conn + |> put_status(200) + end +end diff --git a/test/support/router.ex b/test/support/router.ex index a9d9192a..8fce8fe8 100644 --- a/test/support/router.ex +++ b/test/support/router.ex @@ -12,6 +12,7 @@ defmodule OpenApiSpexTest.Router do scope "/api" do pipe_through :api resources "/users", OpenApiSpexTest.UserController, only: [:create, :index, :show] + get "/noapi", OpenApiSpexTest.NoApiController, :noapi # Used by ParamsTest resources "/custom_error_users", OpenApiSpexTest.CustomErrorUserController, only: [:index] From 29fd3a8f405891f564915896c0de7dcafa3aabd6 Mon Sep 17 00:00:00 2001 From: Roland Tritsch Date: Sun, 8 May 2022 18:35:18 +0100 Subject: [PATCH 2/8] The fix for the bug. --- lib/open_api_spex/plug/cast_and_validate.ex | 44 ++++++++++++++------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/lib/open_api_spex/plug/cast_and_validate.ex b/lib/open_api_spex/plug/cast_and_validate.ex index 7feadfca..cebab92c 100644 --- a/lib/open_api_spex/plug/cast_and_validate.ex +++ b/lib/open_api_spex/plug/cast_and_validate.ex @@ -1,6 +1,6 @@ defmodule OpenApiSpex.Plug.CastAndValidate do @moduledoc """ - Module plug that will cast and validate the `Conn.params` and `Conn.body_params` according to the schemas defined for the operation. + Module plug that will cast and validate the `Conn.params` and `Conn.body_params` according to the schemas defined for the operation. The operation_id can be given at compile time as an argument to `init`: @@ -90,25 +90,41 @@ defmodule OpenApiSpex.Plug.CastAndValidate do # This caching is to improve performance of extracting Operation specs # at runtime when they're using the @doc-based syntax. - operation = + operation_lookup = case operation_lookup[{controller, action}] do nil -> - operation_id = controller.open_api_operation(action).operationId - - PutApiSpec.get_and_cache_controller_action( - conn, - operation_id, - {controller, action} - ) + operation = controller.open_api_operation(action) + + if operation do + operation = + PutApiSpec.get_and_cache_controller_action( + conn, + operation.operationId, + {controller, action} + ) + + {:found_it, operation} + else + # this is the case when operation: false was used + {:skip_it, nil} + end operation -> - operation + {:found_it, operation} end - if operation.operationId do - call(conn, Map.put(opts, :operation_id, operation.operationId)) - else - raise "operationId was not found in action API spec" + case operation_lookup do + {:skip_it, _} -> + conn + + {:found_it, nil} -> + raise "operationId was not found in action API spec" + + {:found_it, operation} -> + call(conn, opts |> Map.put(:operation_id, operation.operationId)) + + _ -> + raise "Unexpected case detected" end end From df06ae6f30a33e0d627724241464769ba9a65012 Mon Sep 17 00:00:00 2001 From: Roland Tritsch Date: Sun, 19 Jun 2022 07:22:42 +0100 Subject: [PATCH 3/8] PR feedback: Formatting. --- lib/open_api_spex/plug/cast_and_validate.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/open_api_spex/plug/cast_and_validate.ex b/lib/open_api_spex/plug/cast_and_validate.ex index 9f5df41c..3c7105ca 100644 --- a/lib/open_api_spex/plug/cast_and_validate.ex +++ b/lib/open_api_spex/plug/cast_and_validate.ex @@ -1,6 +1,6 @@ defmodule OpenApiSpex.Plug.CastAndValidate do @moduledoc """ - Module plug that will cast and validate the `Conn.params` and `Conn.body_params` according to the schemas defined for the operation. + Module plug that will cast and validate the `Conn.params` and `Conn.body_params` according to the schemas defined for the operation. The operation_id can be given at compile time as an argument to `init`: From 8d50e1c634758574e03604d337faf8b6d6a13cad Mon Sep 17 00:00:00 2001 From: Roland Tritsch Date: Sun, 19 Jun 2022 07:39:25 +0100 Subject: [PATCH 4/8] Lowercase all test descriptions (for consistency and readability). --- test/plug/cast_test.exs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/plug/cast_test.exs b/test/plug/cast_test.exs index fba22cfb..0831d6d8 100644 --- a/test/plug/cast_test.exs +++ b/test/plug/cast_test.exs @@ -17,7 +17,7 @@ defmodule OpenApiSpex.Plug.CastTest do end describe "query params - basics" do - test "valid Param" do + test "valid param" do conn = :get |> Plug.Test.conn("/api/users?validParam=true") @@ -29,7 +29,7 @@ defmodule OpenApiSpex.Plug.CastTest do assert OpenApiSpex.params(conn) == %{validParam: true} end - test "Valid Param with replace_params false" do + test "valid param with replace_params false" do conn = :get |> Plug.Test.conn("/api/users_no_replace?validParam=true") @@ -218,7 +218,7 @@ defmodule OpenApiSpex.Plug.CastTest do } end - test "Valid Request with replace_params false" do + test "valid request with replace_params false" do request_body = %{ "user" => %{ "id" => 123, From 216faf6c92771c8071c63d373d33924a7c399ca1 Mon Sep 17 00:00:00 2001 From: Roland Tritsch Date: Sun, 19 Jun 2022 08:17:33 +0100 Subject: [PATCH 5/8] Adding test-case for struct based definition. --- test/plug/cast_test.exs | 11 ++++++++++- .../noapi_controller_with_struct_specs.ex | 18 ++++++++++++++++++ test/support/router.ex | 1 + 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/support/noapi_controller_with_struct_specs.ex diff --git a/test/plug/cast_test.exs b/test/plug/cast_test.exs index 0831d6d8..dda9014f 100644 --- a/test/plug/cast_test.exs +++ b/test/plug/cast_test.exs @@ -6,7 +6,7 @@ defmodule OpenApiSpex.Plug.CastTest do alias OpenApiSpexTest.ApiSpec describe "valid operation (not annotated)" do - test "should work" do + test "should work (with exdoc)" do conn = :get |> Plug.Test.conn("/api/noapi") @@ -14,6 +14,15 @@ defmodule OpenApiSpex.Plug.CastTest do assert conn.status == 200 end + + test "should work (with struct)" do + conn = + :get + |> Plug.Test.conn("/api/noapi_with_struct") + |> OpenApiSpexTest.Router.call([]) + + assert conn.status == 200 + end end describe "query params - basics" do diff --git a/test/support/noapi_controller_with_struct_specs.ex b/test/support/noapi_controller_with_struct_specs.ex new file mode 100644 index 00000000..71b2094e --- /dev/null +++ b/test/support/noapi_controller_with_struct_specs.ex @@ -0,0 +1,18 @@ +defmodule OpenApiSpexTest.NoApiControllerWithStructSpecs do + @moduledoc false + + use Phoenix.Controller + + plug OpenApiSpex.Plug.CastAndValidate, json_render_error_v2: true + + def open_api_operation(action) do + apply(__MODULE__, :"#{action}_operation", []) + end + + def noapi_operation(), do: nil + + def noapi(conn, _opts) do + conn + |> put_status(200) + end +end diff --git a/test/support/router.ex b/test/support/router.ex index c5127eb6..217f9e70 100644 --- a/test/support/router.ex +++ b/test/support/router.ex @@ -13,6 +13,7 @@ defmodule OpenApiSpexTest.Router do pipe_through :api resources "/users", OpenApiSpexTest.UserController, only: [:create, :index, :show] get "/noapi", OpenApiSpexTest.NoApiController, :noapi + get "/noapi_with_struct", OpenApiSpexTest.NoApiControllerWithStructSpecs, :noapi resources "/users_no_replace", OpenApiSpexTest.UserNoRepalceController, only: [:create, :index] From ab022d315c3e0b7b33ab0d9a0d2321d4c35654f4 Mon Sep 17 00:00:00 2001 From: Roland Tritsch Date: Mon, 20 Jun 2022 16:59:00 +0100 Subject: [PATCH 6/8] PR feedback: Make test desc more descriptive. --- test/plug/cast_test.exs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/plug/cast_test.exs b/test/plug/cast_test.exs index dda9014f..5de77730 100644 --- a/test/plug/cast_test.exs +++ b/test/plug/cast_test.exs @@ -6,7 +6,7 @@ defmodule OpenApiSpex.Plug.CastTest do alias OpenApiSpexTest.ApiSpec describe "valid operation (not annotated)" do - test "should work (with exdoc)" do + test "does not raise in error when the operation is nil set with the operation macro" do conn = :get |> Plug.Test.conn("/api/noapi") @@ -15,7 +15,7 @@ defmodule OpenApiSpex.Plug.CastTest do assert conn.status == 200 end - test "should work (with struct)" do + test "does not raise in error when the operation is nil set with the open_api_operation function" do conn = :get |> Plug.Test.conn("/api/noapi_with_struct") From afbbb28ebbc493c6c4a3283ddcb882563da310fe Mon Sep 17 00:00:00 2001 From: Roland Tritsch Date: Tue, 21 Jun 2022 15:45:39 +0100 Subject: [PATCH 7/8] PR feedback: Just raise a MatchError instead (if/when this happens). --- lib/open_api_spex/plug/cast_and_validate.ex | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/open_api_spex/plug/cast_and_validate.ex b/lib/open_api_spex/plug/cast_and_validate.ex index 3c7105ca..e8af40fc 100644 --- a/lib/open_api_spex/plug/cast_and_validate.ex +++ b/lib/open_api_spex/plug/cast_and_validate.ex @@ -138,9 +138,6 @@ defmodule OpenApiSpex.Plug.CastAndValidate do {:found_it, operation} -> call(conn, opts |> Map.put(:operation_id, operation.operationId)) - - _ -> - raise "Unexpected case detected" end end From e862a68a84355b1eb7ba83b0737be0a35c45b69b Mon Sep 17 00:00:00 2001 From: Roland Tritsch Date: Tue, 21 Jun 2022 15:47:32 +0100 Subject: [PATCH 8/8] PR Feedback: Typo. --- test/plug/cast_test.exs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/plug/cast_test.exs b/test/plug/cast_test.exs index 5de77730..3c9448b6 100644 --- a/test/plug/cast_test.exs +++ b/test/plug/cast_test.exs @@ -6,7 +6,7 @@ defmodule OpenApiSpex.Plug.CastTest do alias OpenApiSpexTest.ApiSpec describe "valid operation (not annotated)" do - test "does not raise in error when the operation is nil set with the operation macro" do + test "does not raise an error when the operation is nil set with the operation macro" do conn = :get |> Plug.Test.conn("/api/noapi") @@ -15,7 +15,7 @@ defmodule OpenApiSpex.Plug.CastTest do assert conn.status == 200 end - test "does not raise in error when the operation is nil set with the open_api_operation function" do + test "does not raise an error when the operation is nil set with the open_api_operation function" do conn = :get |> Plug.Test.conn("/api/noapi_with_struct")