From e6b0472b317b8506f5043dbc392fc26ac8103405 Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Tue, 12 May 2026 22:12:36 +0800 Subject: [PATCH 1/2] fix: skip standalone validation for embedded nested forms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nested forms backed by an embedded attribute/argument were validated twice: once against the embed's own create/update action, and again inline when the parent action cast the merged params. The standalone pass bypasses parent-level logic (e.g. a `cast_input/2` override on the embed that collapses blank input to `nil`), so an all-blank embed sub-form produced spurious "is required" errors even when the parent would have accepted the value as nil. After validating, clear `source.errors` and mark the source valid for forms with `embed?: true`. `carry_over_errors/2` replays any real errors the parent's validation surfaces with a matching path. Forms with `transform_params` set are skipped — unions rewrite params into an `%Ash.Union{}` before handing them up, so their standalone validation is the only line of defense. --- lib/ash_phoenix/form/form.ex | 24 +++++++++++-- test/form_test.exs | 60 +++++++++++++++++++++++++++++++ test/support/resources/address.ex | 26 ++++++++++++++ test/support/resources/author.ex | 1 + 4 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 test/support/resources/address.ex diff --git a/lib/ash_phoenix/form/form.ex b/lib/ash_phoenix/form/form.ex index 9f5b2fd3..1f5bb821 100644 --- a/lib/ash_phoenix/form/form.ex +++ b/lib/ash_phoenix/form/form.ex @@ -1602,7 +1602,7 @@ defmodule AshPhoenix.Form do opts = update_opts(opts, nil, params) new_form = - cond do + (cond do !opts[:create_action] && !opts[:read_action] -> raise AshPhoenix.Form.NoActionConfigured, path: form.name <> "[#{key}][#{index}]", @@ -1663,7 +1663,8 @@ defmodule AshPhoenix.Form do as: form.name <> "[#{key}][#{index}]", id: form.id <> "_#{key}_#{index}" ) - end + end) + |> clear_embed_source_errors(opts) Map.update(forms, key, [new_form], &(&1 ++ [new_form])) @@ -1682,6 +1683,7 @@ defmodule AshPhoenix.Form do ) |> Map.put(:name, form.name <> "[#{key}][#{index}]") |> Map.put(:id, form.id <> "_#{key}_#{index}") + |> clear_embed_source_errors(opts) Map.update(forms, key, [validated], fn nested_forms -> nested_forms ++ @@ -1722,6 +1724,7 @@ defmodule AshPhoenix.Form do accessing_from: opts[:managed_relationship], prepare_source: opts[:prepare_source] ) + |> clear_embed_source_errors(opts) Map.put(forms, key, new_form) else @@ -1758,6 +1761,7 @@ defmodule AshPhoenix.Form do as: form.name <> "[#{key}]", id: form.id <> "_#{key}" ) + |> clear_embed_source_errors(opts) Map.put(forms, key, new_form) end @@ -2393,6 +2397,22 @@ defmodule AshPhoenix.Form do end end + # The parent action casts and validates embedded params inline, so running + # the embed's own action would produce a second, duplicate validation that + # bypasses parent-level logic. We still need the standalone source for + # rendering; `carry_over_errors/2` replays any real errors from the parent. + # + # Skipped when `transform_params` is set — unions rewrite params into an + # `%Ash.Union{}` struct before handing them up, so the parent can't catch + # input errors and the standalone validation is the only line of defense. + defp clear_embed_source_errors(form, opts) do + if opts[:embed?] && form.source && !opts[:transform_params] do + %{form | source: %{form.source | errors: [], valid?: true}} + else + form + end + end + defp carry_over_errors(form, additional_errors \\ nil) do errors = List.wrap(form.source.errors) diff --git a/test/form_test.exs b/test/form_test.exs index 64891313..44f933e8 100644 --- a/test/form_test.exs +++ b/test/form_test.exs @@ -1464,6 +1464,66 @@ defmodule AshPhoenix.FormTest do assert is_map(all_raw_errors) assert Map.has_key?(all_raw_errors, []) end + + test "embedded forms are not validated in isolation when parent cast_input allows empty values" do + form = + Author + |> Form.for_create(:create, forms: [auto?: true]) + |> Form.add_form(:address, params: %{}) + |> Form.validate(%{ + "email" => "me@example.com", + "address" => %{"line1" => "", "city" => "", "postcode" => ""} + }) + + assert Form.errors(form, for_path: :all) == %{} + assert form.valid? + end + + test "embedded forms still surface parent errors for partial input" do + form = + Author + |> Form.for_create(:create, forms: [auto?: true]) + |> Form.add_form(:address, params: %{}) + |> Form.validate(%{ + "email" => "me@example.com", + "address" => %{"line1" => "1 Main St", "city" => "", "postcode" => ""} + }) + + errors = Form.errors(form, for_path: [:address]) + assert {:postcode, _} = List.keyfind(errors, :postcode, 0) + refute form.valid? + end + + test "manage_relationship forms still validate standalone on validate" do + form = + Comment + |> Form.for_create(:create, forms: [auto?: true]) + |> Form.add_form(:post, params: %{}) + |> Form.validate(%{"text" => "comment text", "post" => %{}}) + + assert Form.errors(form, for_path: [:post]) == [text: "is required"] + refute form.valid? + end + + test "update form with existing embedded data is valid without changes" do + author = %Author{ + id: Ash.UUID.generate(), + email: "me@example.com", + address: %AshPhoenix.Test.Address{ + line1: "1 Main St", + city: "Sydney", + postcode: "2000" + } + } + + form = + author + |> Form.for_update(:update, forms: [auto?: true]) + |> Form.validate(%{"email" => "new@example.com"}) + + assert Form.errors(form, for_path: :all) == %{} + assert form.valid? + end end describe "data" do diff --git a/test/support/resources/address.ex b/test/support/resources/address.ex new file mode 100644 index 00000000..236de5d2 --- /dev/null +++ b/test/support/resources/address.ex @@ -0,0 +1,26 @@ +# SPDX-FileCopyrightText: 2020 ash_phoenix contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshPhoenix.Test.Address do + @moduledoc false + use Ash.Resource, data_layer: :embedded + + attributes do + attribute :line1, :string, allow_nil?: false, public?: true + attribute :city, :string, allow_nil?: false, public?: true + attribute :postcode, :string, allow_nil?: false, public?: true + end + + defoverridable cast_input: 2 + + def cast_input(value, constraints) when is_map(value) and not is_struct(value) do + if Enum.all?(Map.values(value), &(&1 in [nil, ""])) do + {:ok, nil} + else + super(value, constraints) + end + end + + def cast_input(value, constraints), do: super(value, constraints) +end diff --git a/test/support/resources/author.ex b/test/support/resources/author.ex index e62876a5..65455147 100644 --- a/test/support/resources/author.ex +++ b/test/support/resources/author.ex @@ -16,6 +16,7 @@ defmodule AshPhoenix.Test.Author do attributes do uuid_primary_key(:id) attribute(:email, :string, allow_nil?: false, public?: true) + attribute(:address, AshPhoenix.Test.Address, allow_nil?: true, public?: true) end actions do From c9c35aa91dcc0c4c52fd4aa81a6c4611841cefcc Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Wed, 13 May 2026 13:46:29 +0800 Subject: [PATCH 2/2] refactor: tag union sub-forms with explicit `union?` flag `clear_embed_source_errors/2` used `!opts[:transform_params]` to detect union sub-forms. `transform_params` is also a public form option, so a caller setting it on a non-union embed (undocumented but reachable) would silently disable the exemption. Set `union?: true` in `unions/3` and check that instead. --- lib/ash_phoenix/form/auto.ex | 1 + lib/ash_phoenix/form/form.ex | 13 +++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/ash_phoenix/form/auto.ex b/lib/ash_phoenix/form/auto.ex index b4db7588..57a02868 100644 --- a/lib/ash_phoenix/form/auto.ex +++ b/lib/ash_phoenix/form/auto.ex @@ -212,6 +212,7 @@ defmodule AshPhoenix.Form.Auto do prepare_source: prepare_source, transform_params: transform_params, embed?: true, + union?: true, save_updates?: false, forms: Keyword.new( diff --git a/lib/ash_phoenix/form/form.ex b/lib/ash_phoenix/form/form.ex index 1f5bb821..ab229aaf 100644 --- a/lib/ash_phoenix/form/form.ex +++ b/lib/ash_phoenix/form/form.ex @@ -1602,7 +1602,7 @@ defmodule AshPhoenix.Form do opts = update_opts(opts, nil, params) new_form = - (cond do + cond do !opts[:create_action] && !opts[:read_action] -> raise AshPhoenix.Form.NoActionConfigured, path: form.name <> "[#{key}][#{index}]", @@ -1663,7 +1663,7 @@ defmodule AshPhoenix.Form do as: form.name <> "[#{key}][#{index}]", id: form.id <> "_#{key}_#{index}" ) - end) + end |> clear_embed_source_errors(opts) Map.update(forms, key, [new_form], &(&1 ++ [new_form])) @@ -2402,11 +2402,12 @@ defmodule AshPhoenix.Form do # bypasses parent-level logic. We still need the standalone source for # rendering; `carry_over_errors/2` replays any real errors from the parent. # - # Skipped when `transform_params` is set — unions rewrite params into an - # `%Ash.Union{}` struct before handing them up, so the parent can't catch - # input errors and the standalone validation is the only line of defense. + # Unions are excluded: they rewrite params into an `%Ash.Union{}` struct + # before handing them up, so the parent can't catch input errors and the + # standalone validation is the only line of defense. `union?` is set by + # `AshPhoenix.Form.Auto.unions/3` to mark these forms explicitly. defp clear_embed_source_errors(form, opts) do - if opts[:embed?] && form.source && !opts[:transform_params] do + if opts[:embed?] && !opts[:union?] && form.source do %{form | source: %{form.source | errors: [], valid?: true}} else form