Improve Ecto.InvalidChangesetError message for embeds #1796

wojtekmach · 2016-11-13T11:16:55Z

Discussed on ML: https://groups.google.com/forum/#!topic/elixir-ecto/dB8Iu02YQO4

Before:

could not perform insert because changeset is invalid.

* Changeset changes

%{posts: [#Ecto.Changeset<action: :insert, changes: %{}, errors: [title: {"can't be blank", [validation: :required]}], data: #Ecto.ErrorTest.Post<>, valid?: false>, #Ecto.Changeset<action: :insert, changes: %{comments: [#Ecto.Changeset<action: :insert, changes: %{}, errors: [text: {"can't be blank", [validation: :required]}], data: #Ecto.ErrorTest.Comment<>, valid?: false>]}, errors: [title: {"can't be blank", [validation: :required]}], data: #Ecto.ErrorTest.Post<>, valid?: false>]}

* Changeset params

%{"name" => "", "posts" => [%{}, %{"comments" => [%{"text" => ""}], "title" => ""}]}

* Changeset errors

[name: {"can't be blank", [validation: :required]}]

After:

could not perform insert because changeset is invalid.

Changeset changes

    %{posts: [%{comments: [%{}]}]}

Changeset params

    %{"name" => "", "posts" => [%{"comments" => [%{"text" => ""}], "title" => ""}]}

Changeset errors

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

(see an included temporary test that generates this message)

Note: I've initially used traverse_error/2 to showcase deeply nested changesets. Problem with just using traverse_error that is we'd lose distinction between changeset.errors and changeset.changes.foo.errors which may or may not be a good thing. Alternatively, something close to what was originally proposed on ML could be used, e.g.: (not sure about the exact format)

Changeset errors

    [name: {"can't be blank", [validation: :required]}]
    posts:
      [title: {"can't be blank", [validation: :required]}] # changeset.changes.posts[0]

      [title: {"can't be blank", [validation: :required]}] # changeset.changes.posts[1]
      comments:
        [title: {"can't be blank", [validation: :required]}]

elixir-bot · 2016-11-13T11:16:56Z

Hello, @wojtekmach! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

michalmuskala · 2016-11-13T11:55:19Z

My only problem with this is that the values we show here are different from the fields in the changeset itself. I worry that this could be confusing. Maybe we should somehow make it more obvious that it's not what is actually stored in the changeset as the data, but rather it's logical value/interpretation.

ericmj · 2016-11-13T13:22:49Z

It's a bit redundant to include "Changeset". What about removing it from each heading like this:

could not perform insert because changeset is invalid.

Changes

    %{posts: [%{comments: [%{}]}]}

Params

    %{"name" => "", "posts" => [%{"comments" => [%{"text" => ""}], "title" => ""}]}

Errors

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

This way I think it will also be less associated with the exact changeset fields. Do you think that helps @michalmuskala?

wojtekmach · 2016-11-14T09:10:32Z

Another solution is to include the Changeset in the exception message (although we rarely if ever do that in other exceptions) although that might be a bit too redundant:

josevalim · 2016-11-16T19:20:58Z

@michalmuskala @wojtekmach how should we proceed here then? :)

wojtekmach · 2016-11-21T13:10:22Z

@michalmuskala ping :) I like @ericmj proposal the most, I also provided an alternative (that would be my 2nd choice)

michalmuskala · 2016-11-21T13:21:19Z

I think I like the solution with printing the whole changeset too. It might be helpful in some occasions to see the whole, unprocessed data, that we show currently.

ericmj · 2016-11-21T13:42:07Z

It might be helpful in some occasions to see the whole, unprocessed data, that we show currently.

In those cases it is easy to inspect the changeset. I think in the majority of cases the changeset is not interesting.

michalmuskala · 2016-11-21T15:10:45Z

Yeah, that's true. I'm still worried that printing something under Changes: that is different from what is in changeset.changes will be confusing. I'm not sure how to make it "not confusing".

josevalim · 2016-11-25T08:02:52Z

@michalmuskala that's a good point. The changeset may also have important information, such as which action is going to be used. Maybe what we could do to improve this is by adding width: 80 when inspecting the changesets so at least we don't have a huge garbled message?

wojtekmach · 2016-11-25T09:17:18Z

@josevalim @ericmj @michalmuskala wdyt about the following? (I've added a commit) The top level changeset.action was always displayed in the first sentence of the error, but for embedded changesets I can see how it could be useful to see the actions at the bottom:

fishcakez · 2016-11-25T12:03:38Z

Not sure about this but could use :erlang.error/2 to put the changeset in the stacktrace.

…

On Friday, 25 November 2016, Wojtek Mach ***@***.***> wrote: @josevalim <https://github.com/josevalim> @ericmj <https://github.com/ericmj> @michalmuskala <https://github.com/michalmuskala> wdyt about the following (I've added a commit)? The top level changeset.action was always displayed in the first sentence of the error, but for embedded changesets I can see how it could be useful to see the actions at the bottom: [image: zrzut ekranu 2016-11-25 o 10 11 29] <https://cloud.githubusercontent.com/assets/76071/20620058/06780b7e-b2f8-11e6-8d07-3e707a1d78a0.png> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1796 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB6JTZZYrXnvQBhjCCESkM9RO2VCaWdjks5rBqefgaJpZM4Kwqrz> .

wojtekmach · 2016-11-30T23:22:31Z

~~I tried that but it looks like it's not converting \n in exception message, and the changeset (without pretty printing) suffers from the same "garbled message" problem.~~

~~(thanks for the tip about :erlang.error/2 though, I did not know about it! :))~~

fishcakez · 2016-11-30T23:25:53Z

@wojtekmach I meant to call it with the InvalidChangesetError exception as first argument, not the message. Like

:erlang.error(Ecto.InvalidChangesetError.exception(...), [changeset])

wojtekmach · 2016-11-30T23:26:45Z

ugh, doh. let me double check.

(edit) @fishcakez thanks, I've updated the screenshot above. Personally, I find the changeset being part of exception message (and be pretty printed) to be more readable

wojtekmach · 2016-12-09T17:38:40Z

hey everyone, just checking-in (sorry for nagging!): we clearly have a few different ways to display this as shown in the screenshots above, is there one way we could all agree on? Personally I think I like the 2nd screenshot from the top the most (pretty printing data and changeset), but I think any of the ones shown above would be an improvement over the original message.

ericmj · 2016-12-09T18:17:50Z

I agree with @wojtekmach, I prefer the 2nd screenshot.

josevalim · 2016-12-09T19:01:56Z

I am ok with the second screenshot as long as we call the first entry "Applied changes" or something similar.

josevalim · 2016-12-09T19:02:46Z

lib/ecto/exceptions.ex

+    |> Enum.map_join("\n", &"    " <> &1)
+  end
+
+  defp extract_changes(%Ecto.Changeset{changes: changes}) do


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

sounds good 👍

@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?

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.

@josevalim okay thanks for clarifying it

fixed in ebdf13c

michalmuskala · 2016-12-09T19:15:48Z

I'm fine with going with any of the proposed solutions. They are all better from what we have right now.

michalmuskala · 2016-12-10T17:36:40Z

lib/ecto/exceptions.ex

    end)
  end
+  defp extract_changes([%Ecto.Changeset{action: :delete} | tail]),
+    do: [extract_changes(tail)]


This shouldn't be wrapped in a list.

good catch, fixed

josevalim · 2016-12-11T10:52:17Z

❤️ 💚 💙 💛 💜

wojtekmach added 2 commits December 1, 2016 00:10

Improve Ecto.InvalidChangesetError message for embeds

de5d5d0

Prettier output

7680575

josevalim reviewed Dec 9, 2016

View reviewed changes

wojtekmach added 2 commits December 10, 2016 17:29

Rename: Changes -> Applied changes

4c8cde5

Don't include changes for related changests that are marked for deletion

ebdf13c

michalmuskala requested changes Dec 10, 2016

View reviewed changes

Fix handling of a list

2e7a7ea

michalmuskala approved these changes Dec 10, 2016

View reviewed changes

josevalim merged commit 4877374 into elixir-ecto:master Dec 11, 2016

wojtekmach deleted the wm-invalid-changeset-error-improvements branch December 12, 2016 10:31

bartekupartek pushed a commit to bartekupartek/ecto that referenced this pull request Mar 19, 2019

Improve Ecto.InvalidChangesetError message for embeds (elixir-ecto#1796)

8c70f77

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Ecto.InvalidChangesetError message for embeds #1796

Improve Ecto.InvalidChangesetError message for embeds #1796

wojtekmach commented Nov 13, 2016

elixir-bot commented Nov 13, 2016

michalmuskala commented Nov 13, 2016

ericmj commented Nov 13, 2016

wojtekmach commented Nov 14, 2016

josevalim commented Nov 16, 2016

wojtekmach commented Nov 21, 2016

michalmuskala commented Nov 21, 2016

ericmj commented Nov 21, 2016

michalmuskala commented Nov 21, 2016

josevalim commented Nov 25, 2016

wojtekmach commented Nov 25, 2016 •

edited

fishcakez commented Nov 25, 2016 via email

wojtekmach commented Nov 30, 2016 •

edited

fishcakez commented Nov 30, 2016 •

edited

wojtekmach commented Nov 30, 2016 •

edited

wojtekmach commented Dec 9, 2016

ericmj commented Dec 9, 2016

josevalim commented Dec 9, 2016 •

edited

josevalim Dec 9, 2016

wojtekmach Dec 10, 2016

wojtekmach Dec 10, 2016 •

edited

josevalim Dec 10, 2016

wojtekmach Dec 10, 2016

wojtekmach Dec 10, 2016

michalmuskala commented Dec 9, 2016

michalmuskala Dec 10, 2016

wojtekmach Dec 10, 2016

josevalim commented Dec 11, 2016

Improve Ecto.InvalidChangesetError message for embeds #1796

Improve Ecto.InvalidChangesetError message for embeds #1796

Conversation

wojtekmach commented Nov 13, 2016

elixir-bot commented Nov 13, 2016

michalmuskala commented Nov 13, 2016

ericmj commented Nov 13, 2016

wojtekmach commented Nov 14, 2016

josevalim commented Nov 16, 2016

wojtekmach commented Nov 21, 2016

michalmuskala commented Nov 21, 2016

ericmj commented Nov 21, 2016

michalmuskala commented Nov 21, 2016

josevalim commented Nov 25, 2016

wojtekmach commented Nov 25, 2016 • edited

fishcakez commented Nov 25, 2016 via email

wojtekmach commented Nov 30, 2016 • edited

fishcakez commented Nov 30, 2016 • edited

wojtekmach commented Nov 30, 2016 • edited

wojtekmach commented Dec 9, 2016

ericmj commented Dec 9, 2016

josevalim commented Dec 9, 2016 • edited

josevalim Dec 9, 2016

Choose a reason for hiding this comment

wojtekmach Dec 10, 2016

Choose a reason for hiding this comment

wojtekmach Dec 10, 2016 • edited

Choose a reason for hiding this comment

josevalim Dec 10, 2016

Choose a reason for hiding this comment

wojtekmach Dec 10, 2016

Choose a reason for hiding this comment

wojtekmach Dec 10, 2016

Choose a reason for hiding this comment

michalmuskala commented Dec 9, 2016

michalmuskala Dec 10, 2016

Choose a reason for hiding this comment

wojtekmach Dec 10, 2016

Choose a reason for hiding this comment

josevalim commented Dec 11, 2016

wojtekmach commented Nov 25, 2016 •

edited

wojtekmach commented Nov 30, 2016 •

edited

fishcakez commented Nov 30, 2016 •

edited

wojtekmach commented Nov 30, 2016 •

edited

josevalim commented Dec 9, 2016 •

edited

wojtekmach Dec 10, 2016 •

edited