Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve Ecto.InvalidChangesetError message for embeds #1796

Merged
merged 5 commits into from
Dec 11, 2016
Merged

Improve Ecto.InvalidChangesetError message for embeds #1796

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
de5d5d0
Improve Ecto.InvalidChangesetError message for embeds
wojtekmach Nov 13, 2016
7680575
Prettier output
wojtekmach Nov 25, 2016
4c8cde5
Rename: Changes -> Applied changes
wojtekmach Dec 10, 2016
ebdf13c
Don't include changes for related changests that are marked for deletion
wojtekmach Dec 10, 2016
2e7a7ea
Fix handling of a list
wojtekmach Dec 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 34 additions & 6 deletions lib/ecto/exceptions.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,22 +81,50 @@ defmodule Ecto.InvalidChangesetError do
defexception [:action, :changeset]

def message(%{action: action, changeset: changeset}) do
changes = extract_changes(changeset)
errors = Ecto.Changeset.traverse_errors(changeset, & &1)

"""
could not perform #{action} because changeset is invalid.

* Changeset changes
Applied changes

#{pretty changes}

#{inspect changeset.changes}
Params

* Changeset params
#{pretty changeset.params}

#{inspect changeset.params}
Errors

* Changeset errors
#{pretty errors}

#{inspect changeset.errors}
Changeset

#{pretty changeset}
"""
end

defp pretty(term) do
inspect(term, pretty: true)
|> String.split("\n")
|> Enum.map_join("\n", &" " <> &1)
end

defp extract_changes(%Ecto.Changeset{changes: changes}) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the changeset is marked as action: :delete, should we maybe remove it from showing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim do you mean the "main" changeset is marked as delete or one of the embeds changesets? If the main changeset is marked as delete, do you mean that we wouldn't show the last part:

     ** (Ecto.InvalidChangesetError) could not perform delete because changeset is invalid.

     Applied changes

         %{posts: [nil, %{}, %{title: "new"}, %{title: nil}, %{title: "new name"}]}

     Params

         %{"posts" => [%{"title" => "new"}, %{"id" => "2", "title" => nil},
            %{"id" => "3", "title" => "new name"}]}

     Errors

         %{posts: [%{}, %{}, %{},
            %{title: [{"can't be blank", [validation: :required]}]}, %{}]}

     (Changeset used to be here)

or hide "Applied changes" section or something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that, when we hide the changeset, we can no longer know if the changeset is going to be inserted/updated/deleted. This way, if something was meant to be deleted but we still include it, it can be confusing. So I thought we should hide the changes not for the main changeset but any of the children changeset that is marked as delete.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim okay thanks for clarifying it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in ebdf13c

Enum.reduce(changes, %{}, fn({key, value}, acc) ->
case value do
%Ecto.Changeset{action: :delete} -> acc
_ -> Map.put(acc, key, extract_changes(value))
end
end)
end
defp extract_changes([%Ecto.Changeset{action: :delete} | tail]),
do: [extract_changes(tail)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be wrapped in a list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, fixed

defp extract_changes([%Ecto.Changeset{} = changeset | tail]),
do: [extract_changes(changeset) | extract_changes(tail)]
defp extract_changes(other),
do: other
end

defmodule Ecto.CastError do
Expand Down
2 changes: 1 addition & 1 deletion test/ecto/repo_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ defmodule Ecto.RepoTest do
end

test "insert!, update!, insert_or_update! and delete! fail on invalid changeset" do
invalid = %Ecto.Changeset{valid?: false, data: %MySchema{}}
invalid = %Ecto.Changeset{valid?: false, data: %MySchema{}, types: %{}}

assert_raise Ecto.InvalidChangesetError,
~r"could not perform insert because changeset is invalid", fn ->
Expand Down