Repository: elixir-ecto/ecto
Pull request #2129 (2017-07-17 10:18:52)

Remove the field from changes if it does not pass validate_required
based on the discussion in #1874

ebert[bot]: Hello, @slashmili! This is your first Pull Request that will be reviewed by [Ebert](https://ebertapp.io/), 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](https://ebertapp.io/github/elixir-ecto/ecto/pulls/2129). Please check our [documentation](https://docs.ebertapp.io) for more information. 
josevalim: Thank you! I have added one comment!
slashmili: @josevalim is it good enough?

Not sure if you are ok with this pipe operator in middle of the code 😬 
```diff
+    new_errors = fields_with_errors
+                 |> Enum.map(&({&1, {message, [validation: :required]}}))
```
ebert[bot]: Ebert has finished reviewing this Pull Request and has found:

* 1 possible new issue (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2129.

<!-- Ebert auto generated comment -->


Patch:
diff --git a/lib/ecto/changeset.ex b/lib/ecto/changeset.ex
index 24f684a3..d7fd79f9 100644
--- a/lib/ecto/changeset.ex
+++ b/lib/ecto/changeset.ex
@@ -970,7 +970,7 @@ defmodule Ecto.Changeset do
 
   The given `function` is invoked with the change value only if there
   is a change for the given `key`. Note that the value of the change
-  can still be `nil` (unless the field was marked as required on `cast/4`).
+  can still be `nil` (unless the field was marked as required on `validate_required/3`).
 
   ## Examples
 
@@ -1359,20 +1359,23 @@ defmodule Ecto.Changeset do
 
   """
   @spec validate_required(t, list | atom, Keyword.t) :: t
-  def validate_required(%{required: required, errors: errors} = changeset, fields, opts \\ []) do
+  def validate_required(%{required: required, errors: errors, changes: changes} = changeset, fields, opts \\ []) do
     message = message(opts, "can't be blank")
     fields  = List.wrap(fields)
 
-    new_errors =
+    fields_with_errors =
       for field <- fields,
           missing?(changeset, field),
           ensure_field_exists!(changeset, field),
           is_nil(errors[field]),
-          do: {field, {message, [validation: :required]}}
+          do: field
 
-    case new_errors do
+    case fields_with_errors do
       [] -> %{changeset | required: fields ++ required}
-      _  -> %{changeset | required: fields ++ required, errors: new_errors ++ errors, valid?: false}
+      _  ->
+        new_errors = Enum.map(fields_with_errors, &{&1, {message, [validation: :required]}})
+        changes = Map.drop(changes, fields_with_errors)
+        %{changeset | changes: changes, required: fields ++ required, errors: new_errors ++ errors, valid?: false}
     end
   end
 
diff --git a/lib/ecto/changeset/relation.ex b/lib/ecto/changeset/relation.ex
index 8cafe477..89e47b42 100644
--- a/lib/ecto/changeset/relation.ex
+++ b/lib/ecto/changeset/relation.ex
@@ -29,6 +29,7 @@ defmodule Ecto.Changeset.Relation do
   """
   def empty?(%{cardinality: _}, %NotLoaded{}), do: true
   def empty?(%{cardinality: :many}, []), do: true
+  def empty?(%{cardinality: :many}, changes), do: Enum.all?(changes, & &1.action in [:replace, :delete])
   def empty?(%{cardinality: :one}, nil), do: true
   def empty?(%{}, _), do: false
 
diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex
index 3ab553b2..5d71cfcf 100644
--- a/lib/ecto/query.ex
+++ b/lib/ecto/query.ex
@@ -107,7 +107,7 @@ defmodule Ecto.Query do
       query = from u in query, select: u.name
 
   Composing queries uses the same syntax as creating a query.
-  The difference is that, instead of passing a schema like `Weather`
+  The difference is that, instead of passing a schema like `User`
   on the right side of `in`, we passed the query itself.
 
   Any value can be used on the right-side of `in` as long as it implements


Test patch:
diff --git a/test/ecto/changeset/belongs_to_test.exs b/test/ecto/changeset/belongs_to_test.exs
index 70754e6e..30036cf8 100644
--- a/test/ecto/changeset/belongs_to_test.exs
+++ b/test/ecto/changeset/belongs_to_test.exs
@@ -64,7 +64,7 @@ defmodule Ecto.Changeset.BelongsToTest do
 
   test "cast belongs_to with invalid params" do
     changeset = cast(%Author{}, %{"profile" => %{name: nil}}, :profile)
-    assert changeset.changes.profile.changes == %{name: nil}
+    assert changeset.changes.profile.changes == %{}
     assert changeset.changes.profile.errors  == [name: {"can't be blank", [validation: :required]}]
     assert changeset.changes.profile.action  == :insert
     refute changeset.changes.profile.valid?
diff --git a/test/ecto/changeset/has_assoc_test.exs b/test/ecto/changeset/has_assoc_test.exs
index dbc01a01..ec54a948 100644
--- a/test/ecto/changeset/has_assoc_test.exs
+++ b/test/ecto/changeset/has_assoc_test.exs
@@ -89,7 +89,7 @@ defmodule Ecto.Changeset.HasAssocTest do
 
   test "cast has_one with invalid params" do
     changeset = cast(%Author{}, %{"profile" => %{name: nil}}, :profile)
-    assert changeset.changes.profile.changes == %{name: nil}
+    assert changeset.changes.profile.changes == %{}
     assert changeset.changes.profile.errors  == [name: {"can't be blank", [validation: :required]}]
     assert changeset.changes.profile.action  == :insert
     refute changeset.changes.profile.valid?
@@ -444,6 +444,10 @@ defmodule Ecto.Changeset.HasAssocTest do
     assert changeset.changes == %{posts: []}
     assert changeset.errors == [posts: {"can't be blank", [validation: :required]}]
 
+    changeset = cast(%Author{posts: [%Post{title: "hello", id: 1}]}, %{posts: []}, :posts, required: true)
+    assert changeset.required == [:posts]
+    assert changeset.errors == [posts: {"can't be blank", [validation: :required]}]
+
     changeset = cast(%Author{posts: []}, %{}, :posts, required: true)
     assert changeset.required == [:posts]
     assert changeset.changes == %{}
diff --git a/test/ecto/changeset_test.exs b/test/ecto/changeset_test.exs
index 36eaf6c4..1a465ed9 100644
--- a/test/ecto/changeset_test.exs
+++ b/test/ecto/changeset_test.exs
@@ -687,6 +687,7 @@ defmodule Ecto.ChangesetTest do
       |> validate_required([:title, :body], message: "is blank")
     refute changeset.valid?
     assert changeset.required == [:title, :body]
+    assert changeset.changes == %{}
     assert changeset.errors == [title: {"is blank", [validation: :required]}, body: {"is blank", [validation: :required]}]
 
     # When unknown field