From 950ecabcca30c0342be0f41ce4d247a026377ace Mon Sep 17 00:00:00 2001 From: Srikanth Kyatham Date: Mon, 17 Nov 2025 18:02:16 +0200 Subject: [PATCH 1/3] fix: fixing keyerror in GET /:id with includes --- lib/ash_json_api/controllers/helpers.ex | 4 +- ...t_with_invalid_filter_and_include_test.exs | 248 ++++++++++++++++++ 2 files changed, 250 insertions(+), 2 deletions(-) create mode 100644 test/acceptance/get_with_invalid_filter_and_include_test.exs diff --git a/lib/ash_json_api/controllers/helpers.ex b/lib/ash_json_api/controllers/helpers.ex index 91b0e8f..93373ba 100644 --- a/lib/ash_json_api/controllers/helpers.ex +++ b/lib/ash_json_api/controllers/helpers.ex @@ -689,7 +689,7 @@ defmodule AshJsonApi.Controllers.Helpers do case query do {:error, error} -> - {:error, Request.add_error(request, error, :filter)} + Request.add_error(request, error, :filter) {:ok, query} -> query = @@ -787,7 +787,7 @@ defmodule AshJsonApi.Controllers.Helpers do case query do {:error, error} -> - {:error, Request.add_error(request, error, :filter)} + Request.add_error(request, error, :filter) {:ok, query} -> query = diff --git a/test/acceptance/get_with_invalid_filter_and_include_test.exs b/test/acceptance/get_with_invalid_filter_and_include_test.exs new file mode 100644 index 0000000..6ecfdc6 --- /dev/null +++ b/test/acceptance/get_with_invalid_filter_and_include_test.exs @@ -0,0 +1,248 @@ +# SPDX-FileCopyrightText: 2019 ash_json_api contributors +# +# SPDX-License-Identifier: MIT + +defmodule Test.Acceptance.GetWithInvalidFilterAndIncludeTest do + use ExUnit.Case, async: true + + @moduledoc """ + This test module verifies the fix for a bug where GET /:id routes with includes + would crash with a KeyError instead of returning a JSON:API error document. + + The bug occurred when fetch_record_from_path/3 encountered an invalid filter + and returned {:error, Request.add_error(...)} instead of just Request.add_error(...). + This caused chain/3 to receive a tuple instead of a Request struct, leading to + a KeyError when trying to access request.errors. + """ + + defmodule Membership do + use Ash.Resource, + domain: Test.Acceptance.GetWithInvalidFilterAndIncludeTest.Domain, + data_layer: Ash.DataLayer.Ets, + extensions: [AshJsonApi.Resource] + + ets do + private?(true) + end + + json_api do + type "membership" + end + + attributes do + uuid_primary_key(:id) + attribute(:role, :string, public?: true) + end + + actions do + default_accept(:*) + defaults([:create, :read, :update, :destroy]) + end + + relationships do + belongs_to(:user, Test.Acceptance.GetWithInvalidFilterAndIncludeTest.User, public?: true) + end + end + + defmodule User do + use Ash.Resource, + domain: Test.Acceptance.GetWithInvalidFilterAndIncludeTest.Domain, + data_layer: Ash.DataLayer.Ets, + extensions: [AshJsonApi.Resource] + + ets do + private?(true) + end + + json_api do + type "user" + + routes do + base "/users" + get(:read) + index(:read) + end + + includes memberships: [] + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string, public?: true) + attribute(:email, :string, public?: true) + end + + actions do + default_accept(:*) + defaults([:create, :read, :update, :destroy]) + end + + relationships do + has_many(:memberships, Membership, public?: true, destination_attribute: :user_id) + 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 User + resource Membership + 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 + User + |> Ash.Query.for_read(:read) + |> Ash.read!() + |> Enum.each(&Ash.destroy!(&1)) + rescue + _ -> :ok + end + end) + + :ok + end + + describe "GET /:id with invalid filter and includes" do + test "returns JSON:API error document instead of crashing with KeyError" do + # Create a user with memberships + user = + User + |> Ash.Changeset.for_create(:create, %{name: "John Doe", email: "john@example.com"}) + |> Ash.create!() + + _membership = + Membership + |> Ash.Changeset.for_create(:create, %{role: "admin", user_id: user.id}) + |> Ash.create!() + + # This request triggers a filter error in fetch_record_from_path when the ID + # format is invalid. The bug would cause a KeyError because fetch_record_from_path + # returned {:error, request} instead of just request, and chain/3 would try to + # access request.errors on the tuple. + # + # With the fix, this returns a proper 404 error document + response = + Domain + |> get("/users/invalid-id-format?include=memberships", status: 404) + + # Verify we get a proper JSON:API error response, not a crash + assert is_map(response.resp_body) + assert Map.has_key?(response.resp_body, "errors") + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + # Verify the error has proper JSON:API structure + error = hd(errors) + assert is_map(error) + assert Map.has_key?(error, "code") + assert Map.has_key?(error, "title") + assert Map.has_key?(error, "detail") + assert Map.has_key?(error, "status") + + # The key point: we got a proper error response (404), not a 500 crash + assert error["status"] == "404" + assert error["code"] == "not_found" + end + + test "returns JSON:API error for non-existent ID with includes" do + # Use a valid UUID format but non-existent ID + non_existent_id = Ash.UUID.generate() + + response = + Domain + |> get("/users/#{non_existent_id}?include=memberships", status: 404) + + # Verify we get a proper 404 JSON:API error response + assert is_map(response.resp_body) + assert Map.has_key?(response.resp_body, "errors") + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + error = hd(errors) + assert error["status"] == "404" + assert error["code"] == "not_found" + end + + test "successfully returns data with valid ID and includes" do + # Create a user with memberships + user = + User + |> Ash.Changeset.for_create(:create, %{name: "Jane Smith", email: "jane@example.com"}) + |> Ash.create!() + + membership = + Membership + |> Ash.Changeset.for_create(:create, %{role: "member", user_id: user.id}) + |> Ash.create!() + + # This should work correctly + response = + Domain + |> get("/users/#{user.id}?include=memberships", status: 200) + + # Verify successful response + assert is_map(response.resp_body) + assert Map.has_key?(response.resp_body, "data") + data = response.resp_body["data"] + assert data["id"] == user.id + assert data["type"] == "user" + + # Verify included memberships + assert Map.has_key?(response.resp_body, "included") + included = response.resp_body["included"] + assert is_list(included) + assert length(included) > 0 + + membership_data = Enum.find(included, &(&1["type"] == "membership")) + assert membership_data + assert membership_data["id"] == membership.id + assert membership_data["attributes"]["role"] == "member" + end + end + + describe "GET /:id with complex scenarios" do + test "handles non-existent ID with complex includes gracefully" do + # Use a non-existent ID to trigger the error path in fetch_record_from_path + # This exercises the code path where the lookup fails and we need to add an error + non_existent_id = Ash.UUID.generate() + + # Before the fix, this would crash with KeyError when chain/3 tried to access + # request.errors on {:error, request}. After the fix, it returns proper 404. + response = + Domain + |> get("/users/#{non_existent_id}?include=memberships", status: 404) + + # Should return error document, not crash + assert is_map(response.resp_body) + assert Map.has_key?(response.resp_body, "errors") + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + error = hd(errors) + assert error["status"] == "404" + assert error["code"] == "not_found" + end + end +end From 15f5809f595a68492e7cc2c6855438ad15021bb2 Mon Sep 17 00:00:00 2001 From: Srikanth Kyatham Date: Mon, 17 Nov 2025 18:18:03 +0200 Subject: [PATCH 2/3] chore: fixing credo check --- lib/ash_json_api/json_schema/open_api.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ash_json_api/json_schema/open_api.ex b/lib/ash_json_api/json_schema/open_api.ex index 12a38fe..fd17826 100644 --- a/lib/ash_json_api/json_schema/open_api.ex +++ b/lib/ash_json_api/json_schema/open_api.ex @@ -34,8 +34,8 @@ if Code.ensure_loaded?(OpenApiSpex) do end """ alias Ash.Query.Aggregate - alias AshJsonApi.Resource.Route alias Ash.Resource.{Actions, Relationships} + alias AshJsonApi.Resource.Route alias OpenApiSpex.{ Info, From b50a2c20414d05e80a2312852fa0f147a487d4bf Mon Sep 17 00:00:00 2001 From: Srikanth Kyatham Date: Tue, 18 Nov 2025 06:27:22 +0200 Subject: [PATCH 3/3] chore: moving get invalid filter tests to get_related_tests --- test/acceptance/get_related_test.exs | 91 +++++++ ...t_with_invalid_filter_and_include_test.exs | 248 ------------------ 2 files changed, 91 insertions(+), 248 deletions(-) delete mode 100644 test/acceptance/get_with_invalid_filter_and_include_test.exs diff --git a/test/acceptance/get_related_test.exs b/test/acceptance/get_related_test.exs index 2b4ce7f..69d3140 100644 --- a/test/acceptance/get_related_test.exs +++ b/test/acceptance/get_related_test.exs @@ -154,6 +154,97 @@ defmodule Test.Acceptance.GetRelatedTest do :ok end + describe "GET /:id with invalid filter and includes" do + test "returns JSON:API error document instead of crashing with KeyError" do + # Create a post with comments + post = + Post + |> Ash.Changeset.for_create(:create, %{name: "John Doe", content: "Test post"}) + |> Ash.create!() + + _comment = + Comment + |> Ash.Changeset.for_create(:create, %{ + name: "Test comment", + content: "comment", + post_id: post.id + }) + |> Ash.create!() + + # This request triggers a filter error in fetch_record_from_path when the ID + # format is invalid. The bug would cause a KeyError because fetch_record_from_path + # returned {:error, request} instead of just request, and chain/3 would try to + # access request.errors on the tuple. + # + # With the fix, this returns a proper 404 error document + response = + Domain + |> get("/posts/invalid-id-format/comments?include=post", status: 404) + + # Verify we get a proper JSON:API error response, not a crash + assert is_map(response.resp_body) + assert Map.has_key?(response.resp_body, "errors") + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + # Verify the error has proper JSON:API structure + error = hd(errors) + assert is_map(error) + assert Map.has_key?(error, "code") + assert Map.has_key?(error, "title") + assert Map.has_key?(error, "detail") + assert Map.has_key?(error, "status") + + # The key point: we got a proper error response (404), not a 500 crash + assert error["status"] == "404" + assert error["code"] == "not_found" + end + + test "returns JSON:API error for non-existent ID with includes" do + # Use a valid UUID format but non-existent ID + non_existent_id = Ash.UUID.generate() + + response = + Domain + |> get("/posts/#{non_existent_id}/comments?include=post", status: 404) + + # Verify we get a proper 404 JSON:API error response + assert is_map(response.resp_body) + assert Map.has_key?(response.resp_body, "errors") + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + error = hd(errors) + assert error["status"] == "404" + assert error["code"] == "not_found" + end + + test "handles non-existent ID with complex includes gracefully" do + # Use a non-existent ID to trigger the error path in fetch_record_from_path + # This exercises the code path where the lookup fails and we need to add an error + non_existent_id = Ash.UUID.generate() + + # Before the fix, this would crash with KeyError when chain/3 tried to access + # request.errors on {:error, request}. After the fix, it returns proper 404. + response = + Domain + |> get("/posts/#{non_existent_id}/comments?include=post", status: 404) + + # Should return error document, not crash + assert is_map(response.resp_body) + assert Map.has_key?(response.resp_body, "errors") + errors = response.resp_body["errors"] + assert is_list(errors) + assert length(errors) > 0 + + error = hd(errors) + assert error["status"] == "404" + assert error["code"] == "not_found" + end + end + describe "index endpoint" do setup do post = diff --git a/test/acceptance/get_with_invalid_filter_and_include_test.exs b/test/acceptance/get_with_invalid_filter_and_include_test.exs deleted file mode 100644 index 6ecfdc6..0000000 --- a/test/acceptance/get_with_invalid_filter_and_include_test.exs +++ /dev/null @@ -1,248 +0,0 @@ -# SPDX-FileCopyrightText: 2019 ash_json_api contributors -# -# SPDX-License-Identifier: MIT - -defmodule Test.Acceptance.GetWithInvalidFilterAndIncludeTest do - use ExUnit.Case, async: true - - @moduledoc """ - This test module verifies the fix for a bug where GET /:id routes with includes - would crash with a KeyError instead of returning a JSON:API error document. - - The bug occurred when fetch_record_from_path/3 encountered an invalid filter - and returned {:error, Request.add_error(...)} instead of just Request.add_error(...). - This caused chain/3 to receive a tuple instead of a Request struct, leading to - a KeyError when trying to access request.errors. - """ - - defmodule Membership do - use Ash.Resource, - domain: Test.Acceptance.GetWithInvalidFilterAndIncludeTest.Domain, - data_layer: Ash.DataLayer.Ets, - extensions: [AshJsonApi.Resource] - - ets do - private?(true) - end - - json_api do - type "membership" - end - - attributes do - uuid_primary_key(:id) - attribute(:role, :string, public?: true) - end - - actions do - default_accept(:*) - defaults([:create, :read, :update, :destroy]) - end - - relationships do - belongs_to(:user, Test.Acceptance.GetWithInvalidFilterAndIncludeTest.User, public?: true) - end - end - - defmodule User do - use Ash.Resource, - domain: Test.Acceptance.GetWithInvalidFilterAndIncludeTest.Domain, - data_layer: Ash.DataLayer.Ets, - extensions: [AshJsonApi.Resource] - - ets do - private?(true) - end - - json_api do - type "user" - - routes do - base "/users" - get(:read) - index(:read) - end - - includes memberships: [] - end - - attributes do - uuid_primary_key(:id) - attribute(:name, :string, public?: true) - attribute(:email, :string, public?: true) - end - - actions do - default_accept(:*) - defaults([:create, :read, :update, :destroy]) - end - - relationships do - has_many(:memberships, Membership, public?: true, destination_attribute: :user_id) - 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 User - resource Membership - 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 - User - |> Ash.Query.for_read(:read) - |> Ash.read!() - |> Enum.each(&Ash.destroy!(&1)) - rescue - _ -> :ok - end - end) - - :ok - end - - describe "GET /:id with invalid filter and includes" do - test "returns JSON:API error document instead of crashing with KeyError" do - # Create a user with memberships - user = - User - |> Ash.Changeset.for_create(:create, %{name: "John Doe", email: "john@example.com"}) - |> Ash.create!() - - _membership = - Membership - |> Ash.Changeset.for_create(:create, %{role: "admin", user_id: user.id}) - |> Ash.create!() - - # This request triggers a filter error in fetch_record_from_path when the ID - # format is invalid. The bug would cause a KeyError because fetch_record_from_path - # returned {:error, request} instead of just request, and chain/3 would try to - # access request.errors on the tuple. - # - # With the fix, this returns a proper 404 error document - response = - Domain - |> get("/users/invalid-id-format?include=memberships", status: 404) - - # Verify we get a proper JSON:API error response, not a crash - assert is_map(response.resp_body) - assert Map.has_key?(response.resp_body, "errors") - errors = response.resp_body["errors"] - assert is_list(errors) - assert length(errors) > 0 - - # Verify the error has proper JSON:API structure - error = hd(errors) - assert is_map(error) - assert Map.has_key?(error, "code") - assert Map.has_key?(error, "title") - assert Map.has_key?(error, "detail") - assert Map.has_key?(error, "status") - - # The key point: we got a proper error response (404), not a 500 crash - assert error["status"] == "404" - assert error["code"] == "not_found" - end - - test "returns JSON:API error for non-existent ID with includes" do - # Use a valid UUID format but non-existent ID - non_existent_id = Ash.UUID.generate() - - response = - Domain - |> get("/users/#{non_existent_id}?include=memberships", status: 404) - - # Verify we get a proper 404 JSON:API error response - assert is_map(response.resp_body) - assert Map.has_key?(response.resp_body, "errors") - errors = response.resp_body["errors"] - assert is_list(errors) - assert length(errors) > 0 - - error = hd(errors) - assert error["status"] == "404" - assert error["code"] == "not_found" - end - - test "successfully returns data with valid ID and includes" do - # Create a user with memberships - user = - User - |> Ash.Changeset.for_create(:create, %{name: "Jane Smith", email: "jane@example.com"}) - |> Ash.create!() - - membership = - Membership - |> Ash.Changeset.for_create(:create, %{role: "member", user_id: user.id}) - |> Ash.create!() - - # This should work correctly - response = - Domain - |> get("/users/#{user.id}?include=memberships", status: 200) - - # Verify successful response - assert is_map(response.resp_body) - assert Map.has_key?(response.resp_body, "data") - data = response.resp_body["data"] - assert data["id"] == user.id - assert data["type"] == "user" - - # Verify included memberships - assert Map.has_key?(response.resp_body, "included") - included = response.resp_body["included"] - assert is_list(included) - assert length(included) > 0 - - membership_data = Enum.find(included, &(&1["type"] == "membership")) - assert membership_data - assert membership_data["id"] == membership.id - assert membership_data["attributes"]["role"] == "member" - end - end - - describe "GET /:id with complex scenarios" do - test "handles non-existent ID with complex includes gracefully" do - # Use a non-existent ID to trigger the error path in fetch_record_from_path - # This exercises the code path where the lookup fails and we need to add an error - non_existent_id = Ash.UUID.generate() - - # Before the fix, this would crash with KeyError when chain/3 tried to access - # request.errors on {:error, request}. After the fix, it returns proper 404. - response = - Domain - |> get("/users/#{non_existent_id}?include=memberships", status: 404) - - # Should return error document, not crash - assert is_map(response.resp_body) - assert Map.has_key?(response.resp_body, "errors") - errors = response.resp_body["errors"] - assert is_list(errors) - assert length(errors) > 0 - - error = hd(errors) - assert error["status"] == "404" - assert error["code"] == "not_found" - end - end -end