diff --git a/lib/ash_json_api/controllers/helpers.ex b/lib/ash_json_api/controllers/helpers.ex index f710aae..91b0e8f 100644 --- a/lib/ash_json_api/controllers/helpers.ex +++ b/lib/ash_json_api/controllers/helpers.ex @@ -924,7 +924,7 @@ defmodule AshJsonApi.Controllers.Helpers do _ -> Request.add_error( request, - "id path parameter not present in get route: #{request.url}", + AshJsonApi.Error.InvalidPathParam.exception(parameter: "id", url: request.url), :id_path_param ) end diff --git a/lib/ash_json_api/error/conflicting_params.ex b/lib/ash_json_api/error/conflicting_params.ex new file mode 100644 index 0000000..2b88cf4 --- /dev/null +++ b/lib/ash_json_api/error/conflicting_params.ex @@ -0,0 +1,37 @@ +# SPDX-FileCopyrightText: 2019 ash_json_api contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshJsonApi.Error.ConflictingParams do + @moduledoc """ + Returned when path parameters and query parameters have conflicting names + """ + + use Splode.Error, + class: :invalid, + fields: [:conflicting_keys, :detail] + + def message(error) do + error.detail + end + + def exception(opts) do + opts + |> Keyword.put_new(:detail, "conflict path and query params") + |> Keyword.drop([:conflicting_keys]) + |> super() + end + + defimpl AshJsonApi.ToJsonApiError do + def to_json_api_error(error) do + %AshJsonApi.Error{ + id: Ash.UUID.generate(), + status_code: 400, + code: "invalid_query", + title: "InvalidQuery", + detail: error.detail, + meta: %{} + } + end + end +end diff --git a/lib/ash_json_api/error/error.ex b/lib/ash_json_api/error/error.ex index 78026b3..ef3f2de 100644 --- a/lib/ash_json_api/error/error.ex +++ b/lib/ash_json_api/error/error.ex @@ -35,6 +35,11 @@ defmodule AshJsonApi.Error do [error] end + def to_json_api_errors(domain, resource, error, type) when is_binary(error) do + unknown_error = AshJsonApi.Error.UnknownError.exception(message: error) + to_json_api_errors(domain, resource, unknown_error, type) + end + def to_json_api_errors(domain, resource, error, type) do if AshJsonApi.ToJsonApiError.impl_for(error) do error diff --git a/lib/ash_json_api/error/invalid_filter.ex b/lib/ash_json_api/error/invalid_filter.ex new file mode 100644 index 0000000..ac639ca --- /dev/null +++ b/lib/ash_json_api/error/invalid_filter.ex @@ -0,0 +1,47 @@ +# SPDX-FileCopyrightText: 2019 ash_json_api contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshJsonApi.Error.InvalidFilter do + @moduledoc """ + Returned when a filter parameter is invalid or malformed + """ + + use Splode.Error, + class: :invalid, + fields: [:filter, :detail, :source_parameter] + + def message(error) do + error.detail + end + + def exception(opts) do + filter = opts[:filter] + + detail = + case filter do + nil -> "Invalid filter" + filter -> "Invalid filter: #{filter}" + end + + opts + |> Keyword.put_new(:detail, detail) + |> Keyword.put_new(:source_parameter, "filter") + |> Keyword.drop([:filter]) + |> super() + end + + defimpl AshJsonApi.ToJsonApiError do + def to_json_api_error(error) do + %AshJsonApi.Error{ + id: Ash.UUID.generate(), + status_code: 400, + code: "invalid_filter", + title: "InvalidFilter", + detail: error.detail, + source_parameter: error.source_parameter, + meta: %{} + } + end + end +end diff --git a/lib/ash_json_api/error/invalid_path_param.ex b/lib/ash_json_api/error/invalid_path_param.ex new file mode 100644 index 0000000..66a63e5 --- /dev/null +++ b/lib/ash_json_api/error/invalid_path_param.ex @@ -0,0 +1,48 @@ +# SPDX-FileCopyrightText: 2019 ash_json_api contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshJsonApi.Error.InvalidPathParam do + @moduledoc """ + Returned when a required path parameter is missing or invalid + """ + + use Splode.Error, + class: :invalid, + fields: [:parameter, :url, :detail] + + def message(error) do + error.detail + end + + def exception(opts) do + parameter = opts[:parameter] + url = opts[:url] + + detail = + case {parameter, url} do + {nil, nil} -> "Required path parameter not present" + {param, nil} -> "#{param} path parameter not present" + {nil, url} -> "Required path parameter not present in route: #{url}" + {param, url} -> "#{param} path parameter not present in route: #{url}" + end + + opts + |> Keyword.put_new(:detail, detail) + |> Keyword.drop([:parameter, :url]) + |> super() + end + + defimpl AshJsonApi.ToJsonApiError do + def to_json_api_error(error) do + %AshJsonApi.Error{ + id: Ash.UUID.generate(), + status_code: 400, + code: "invalid_path_param", + title: "InvalidPathParam", + detail: error.detail, + meta: %{} + } + end + end +end diff --git a/lib/ash_json_api/error/invalid_sort.ex b/lib/ash_json_api/error/invalid_sort.ex new file mode 100644 index 0000000..f576d38 --- /dev/null +++ b/lib/ash_json_api/error/invalid_sort.ex @@ -0,0 +1,49 @@ +# SPDX-FileCopyrightText: 2019 ash_json_api contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshJsonApi.Error.InvalidSort do + @moduledoc """ + Returned when a sort parameter is invalid or malformed + """ + + use Splode.Error, + class: :invalid, + fields: [:sort, :field, :detail, :source_parameter] + + def message(error) do + error.detail + end + + def exception(opts) do + sort = opts[:sort] + field = opts[:field] + + detail = + cond do + field -> "Invalid sort field: #{field}" + sort -> "Invalid sort: #{sort}" + true -> "Invalid sort parameter" + end + + opts + |> Keyword.put_new(:detail, detail) + |> Keyword.put_new(:source_parameter, "sort") + |> Keyword.drop([:sort, :field]) + |> super() + end + + defimpl AshJsonApi.ToJsonApiError do + def to_json_api_error(error) do + %AshJsonApi.Error{ + id: Ash.UUID.generate(), + status_code: 400, + code: "invalid_sort", + title: "InvalidSort", + detail: error.detail, + source_parameter: error.source_parameter, + meta: %{} + } + end + end +end diff --git a/lib/ash_json_api/error/missing_schema.ex b/lib/ash_json_api/error/missing_schema.ex new file mode 100644 index 0000000..1fb7234 --- /dev/null +++ b/lib/ash_json_api/error/missing_schema.ex @@ -0,0 +1,36 @@ +# SPDX-FileCopyrightText: 2019 ash_json_api contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshJsonApi.Error.MissingSchema do + @moduledoc """ + Returned when a required schema is not found for validation + """ + + use Splode.Error, + class: :invalid, + fields: [:detail] + + def message(error) do + error.detail + end + + def exception(opts) do + opts + |> Keyword.put_new(:detail, "No schema found for validation") + |> super() + end + + defimpl AshJsonApi.ToJsonApiError do + def to_json_api_error(error) do + %AshJsonApi.Error{ + id: Ash.UUID.generate(), + status_code: 400, + code: "missing_schema", + title: "MissingSchema", + detail: error.detail, + meta: %{} + } + end + end +end diff --git a/lib/ash_json_api/error/unknown_error.ex b/lib/ash_json_api/error/unknown_error.ex new file mode 100644 index 0000000..7db24f8 --- /dev/null +++ b/lib/ash_json_api/error/unknown_error.ex @@ -0,0 +1,39 @@ +# SPDX-FileCopyrightText: 2019 ash_json_api contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshJsonApi.Error.UnknownError do + @moduledoc """ + Returned when an unexpected error occurs that doesn't fit other categories + """ + + use Splode.Error, + class: :unknown, + fields: [:detail] + + def message(error) do + error.detail + end + + def exception(opts) do + message = opts[:message] || "An unknown error occurred" + + opts + |> Keyword.put_new(:detail, message) + |> Keyword.drop([:message]) + |> super() + end + + defimpl AshJsonApi.ToJsonApiError do + def to_json_api_error(error) do + %AshJsonApi.Error{ + id: Ash.UUID.generate(), + status_code: 500, + code: "unknown_error", + title: "UnknownError", + detail: error.detail, + meta: %{} + } + end + end +end diff --git a/lib/ash_json_api/request.ex b/lib/ash_json_api/request.ex index 81554d8..1c90ca1 100644 --- a/lib/ash_json_api/request.ex +++ b/lib/ash_json_api/request.ex @@ -349,15 +349,21 @@ defmodule AshJsonApi.Request do end defp validate_params(%{query_params: query_params, path_params: path_params} = request) do - if Enum.any?(Map.keys(query_params), &Map.has_key?(path_params, &1)) do - add_error(request, "conflict path and query params", request.route.type) + conflicting_keys = Enum.filter(Map.keys(query_params), &Map.has_key?(path_params, &1)) + + if conflicting_keys != [] do + add_error( + request, + AshJsonApi.Error.ConflictingParams.exception(conflicting_keys: conflicting_keys), + request.route.type + ) else request end end defp validate_href_schema(%{schema: nil} = request) do - add_error(request, "no schema found", request.route.type) + add_error(request, AshJsonApi.Error.MissingSchema.exception([]), request.route.type) end defp validate_href_schema( @@ -394,14 +400,22 @@ defmodule AshJsonApi.Request do case public_related(resource, path) do nil -> - add_error(request, "Invalid filter included", request.route.type) + add_error( + request, + AshJsonApi.Error.InvalidFilter.exception(filter: relationship_path), + request.route.type + ) related -> if AshJsonApi.Resource.Info.derive_filter?(related) do path = Enum.map(path, &String.to_existing_atom/1) %{request | filter_included: Map.put(request.filter_included, path, filter_statement)} else - add_error(request, "Invalid filter included", request.route.type) + add_error( + request, + AshJsonApi.Error.InvalidFilter.exception(filter: relationship_path), + request.route.type + ) end end end) @@ -425,14 +439,22 @@ defmodule AshJsonApi.Request do case public_related(resource, path) do nil -> - add_error(request, "Invalid sort included", request.route.type) + add_error( + request, + AshJsonApi.Error.InvalidSort.exception(sort: relationship_path), + request.route.type + ) related -> if AshJsonApi.Resource.Info.derive_sort?(related) do path = Enum.map(path, &String.to_existing_atom/1) %{request | sort_included: Map.put(request.sort_included, path, sort_included)} else - add_error(request, "Invalid sort included", request.route.type) + add_error( + request, + AshJsonApi.Error.InvalidSort.exception(sort: relationship_path), + request.route.type + ) end end end) @@ -765,7 +787,7 @@ defmodule AshJsonApi.Request do defp parse_filter(%{query_params: %{"filter" => filter}} = request) do if request.action.type == :read && request.route.derive_filter? && AshJsonApi.Resource.Info.derive_filter?(request.resource) do - add_error(request, "invalid filter", request.route.type) + add_error(request, AshJsonApi.Error.InvalidFilter.exception([]), request.route.type) else %{request | arguments: Map.put(request.arguments, :filter, filter)} end @@ -800,7 +822,11 @@ defmodule AshJsonApi.Request do %{request | sort: [{calc.name, order} | request.sort]} true -> - add_error(request, "invalid sort #{field}", request.route.type) + add_error( + request, + AshJsonApi.Error.InvalidSort.exception(field: field), + request.route.type + ) end end) end @@ -812,7 +838,7 @@ defmodule AshJsonApi.Request do defp parse_sort(%{query_params: %{"sort" => sort}} = request) do if request.action.type == :read && request.route.derive_sort? && AshJsonApi.Resource.Info.derive_sort?(request.resource) do - add_error(request, "invalid sort string", request.route.type) + add_error(request, AshJsonApi.Error.InvalidSort.exception(sort: sort), request.route.type) else %{request | arguments: Map.put(request.arguments, :sort, sort)} end diff --git a/test/acceptance/error_validation_test.exs b/test/acceptance/error_validation_test.exs new file mode 100644 index 0000000..6966899 --- /dev/null +++ b/test/acceptance/error_validation_test.exs @@ -0,0 +1,214 @@ +# SPDX-FileCopyrightText: 2019 ash_json_api contributors +# +# SPDX-License-Identifier: MIT + +defmodule Test.Acceptance.ErrorValidationTest do + use ExUnit.Case, async: true + + defmodule TestPost do + use Ash.Resource, + domain: Test.Acceptance.ErrorValidationTest.Domain, + data_layer: Ash.DataLayer.Ets, + extensions: [AshJsonApi.Resource] + + ets do + private?(true) + end + + json_api do + type "post" + + routes do + base "/posts" + + # Route with filtering/sorting disabled for testing InvalidFilter/InvalidSort + index :read, derive_filter?: false, derive_sort?: false, route: "/no_filter_sort" + + # Route with filtering/sorting enabled for testing invalid field names + index :read, derive_filter?: true, derive_sort?: true, route: "/with_filter_sort" + end + end + + attributes do + uuid_primary_key(:id) + attribute(:title, :string, allow_nil?: false, public?: true) + attribute(:content, :string, public?: true) + end + + actions do + defaults([:read, :create, :update, :destroy]) + end + end + + defmodule Domain do + use Ash.Domain, + otp_app: :ash_json_api, + extensions: [AshJsonApi.Domain] + + json_api do + authorize? false + log_errors? false + end + + resources do + resource TestPost + end + end + + defmodule Router do + use AshJsonApi.Router, domain: Domain + end + + import AshJsonApi.Test + + setup do + Application.put_env(:ash_json_api, Domain, json_api: [test_router: Router]) + + on_exit(fn -> + try do + TestPost + |> Ash.Query.for_read(:read) + |> Ash.read!() + |> Enum.each(&Ash.destroy!(&1)) + rescue + _ -> :ok + end + end) + + :ok + end + + describe "InvalidFilter errors" do + test "returns proper error when filter is invalid type on derive_filter?: true route" do + # This triggers the error: derive_filter?: true but filter is array (not string or map) + response = + Domain + |> get("/posts/with_filter_sort?filter[]=invalid", status: 400) + + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + error = Enum.find(errors, &(&1["code"] == "invalid_filter")) + assert error, "Expected to find an 'invalid_filter' error" + assert error["title"] == "InvalidFilter" + assert error["detail"] == "Invalid filter" + assert error["source"]["parameter"] == "filter" + assert error["status"] == "400" + end + end + + describe "InvalidSort errors" do + test "returns proper error when sort is invalid type on derive_sort?: true route" do + # This triggers the error: derive_sort?: true but sort is array (not string) + response = + Domain + |> get("/posts/with_filter_sort?sort[]=title", status: 400) + + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + error = Enum.find(errors, &(&1["code"] == "invalid_sort")) + assert error, "Expected to find an 'invalid_sort' error" + assert error["title"] == "InvalidSort" + assert String.contains?(error["detail"], "Invalid sort") + assert error["source"]["parameter"] == "sort" + assert error["status"] == "400" + end + + test "returns proper error for invalid field in sort string" do + response = + Domain + |> get("/posts/with_filter_sort?sort=invalid_field_name", status: 400) + + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + error = Enum.find(errors, &(&1["code"] == "invalid_sort")) + assert error, "Expected to find an 'invalid_sort' error" + assert error["title"] == "InvalidSort" + assert String.contains?(error["detail"], "Invalid sort field: invalid_field_name") + assert error["source"]["parameter"] == "sort" + assert error["status"] == "400" + end + end + + describe "Direct function tests for complex scenarios" do + test "ConflictingParams error creation and JSON:API conversion" do + # Test the error struct directly + error = AshJsonApi.Error.ConflictingParams.exception(conflicting_keys: ["name", "id"]) + + # Test ToJsonApiError protocol + json_error = AshJsonApi.ToJsonApiError.to_json_api_error(error) + + assert json_error.status_code == 400 + assert json_error.code == "invalid_query" + assert json_error.title == "InvalidQuery" + assert json_error.detail == "conflict path and query params" + assert is_binary(json_error.id) + end + + test "MissingSchema error creation and JSON:API conversion" do + error = AshJsonApi.Error.MissingSchema.exception([]) + + json_error = AshJsonApi.ToJsonApiError.to_json_api_error(error) + + assert json_error.status_code == 400 + assert json_error.code == "missing_schema" + assert json_error.title == "MissingSchema" + assert json_error.detail == "No schema found for validation" + assert is_binary(json_error.id) + end + + test "InvalidPathParam error creation and JSON:API conversion" do + error = AshJsonApi.Error.InvalidPathParam.exception(parameter: "id", url: "/test/url") + + json_error = AshJsonApi.ToJsonApiError.to_json_api_error(error) + + assert json_error.status_code == 400 + assert json_error.code == "invalid_path_param" + assert json_error.title == "InvalidPathParam" + + assert String.contains?( + json_error.detail, + "id path parameter not present in route: /test/url" + ) + + assert is_binary(json_error.id) + end + + test "UnknownError creation and JSON:API conversion" do + error = AshJsonApi.Error.UnknownError.exception(message: "Something unexpected happened") + + json_error = AshJsonApi.ToJsonApiError.to_json_api_error(error) + + assert json_error.status_code == 500 + assert json_error.code == "unknown_error" + assert json_error.title == "UnknownError" + assert json_error.detail == "Something unexpected happened" + assert is_binary(json_error.id) + end + + test "Binary error fallback uses UnknownError" do + # Test the binary error handler directly + domain = nil + resource = nil + binary_error = "some unexpected string error" + operation_type = :read + + result = AshJsonApi.Error.to_json_api_errors(domain, resource, binary_error, operation_type) + + assert is_list(result) + assert length(result) == 1 + + [json_error] = result + assert json_error.status_code == 500 + assert json_error.code == "unknown_error" + assert json_error.title == "UnknownError" + assert json_error.detail == "some unexpected string error" + assert is_binary(json_error.id) + end + end +end diff --git a/test/acceptance/route_test.exs b/test/acceptance/route_test.exs index 07c65ae..215a991 100644 --- a/test/acceptance/route_test.exs +++ b/test/acceptance/route_test.exs @@ -136,4 +136,26 @@ defmodule Test.Acceptance.RouteTest do assert String.contains?(source_pointer, "from"), "Expected source pointer '#{source_pointer}' to contain field name 'from'" end + + test "conflict path and query params should return proper JSON API error" do + # This triggers the "conflict path and query params" string error + # by having both a path param (:to) and query param (?to=) with the same name + response = + Domain + |> get("/say_hello/fred?to=john", status: 400) + + assert %{"errors" => errors} = response.resp_body + assert is_list(errors) + assert length(errors) > 0 + + # Find the conflict error + conflict_error = Enum.find(errors, &(&1["code"] == "invalid_query")) + assert conflict_error, "Expected to find an 'invalid_query' error" + + # Verify the error details + assert conflict_error["title"] == "InvalidQuery" + assert conflict_error["detail"] == "conflict path and query params" + assert conflict_error["status"] == "400" + assert is_binary(conflict_error["id"]) + end end