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.

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

Allow suppressing association key is nil warning when preloading data #4584

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/ecto/repo.ex
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,10 @@ defmodule Ecto.Repo do
* `:on_preloader_spawn` - when preloads are done in parallel, this function
will be called in the processes that perform the preloads. This can be useful
for context propagation for traces.
* `:warn_if_nil_keys` - When set to `false`, suppresses the warning
when preloading associations where the parent's association key is nil.
Useful when working with unpersisted data (e.g., in changesets).
Defaults to `true`.

See the ["Shared options"](#module-shared-options) section at the module
documentation for more options.
Expand Down
20 changes: 14 additions & 6 deletions lib/ecto/repo/preloader.ex
Original file line number Diff line number Diff line change
Expand Up @@ -251,38 +251,46 @@ defmodule Ecto.Repo.Preloader do
defp fetch_ids(structs, module, assoc, {_adapter_meta, opts}) do
%{field: field, owner_key: owner_key, cardinality: card} = assoc
force? = Keyword.get(opts, :force, false)
warn_if_nil_keys? = Keyword.get(opts, :warn_if_nil_keys, true)

Enum.reduce structs, {[], [], []}, fn
Enum.reduce(structs, {[], [], []}, fn
nil, acc ->
acc

struct, {fetch_ids, loaded_ids, loaded_structs} ->
assert_struct!(module, struct)
%{^owner_key => id, ^field => value} = struct
loaded? = Ecto.assoc_loaded?(value) and not force?

if loaded? and is_nil(id) and not Ecto.Changeset.Relation.empty?(assoc, value) do
Logger.warning """
if loaded? and is_nil(id) and not Ecto.Changeset.Relation.empty?(assoc, value) and warn_if_nil_keys? do
Logger.warning("""
association `#{field}` for `#{inspect(module)}` has a loaded value but \
its association key `#{owner_key}` is nil. This usually means one of:

* `#{owner_key}` was not selected in a query
* the struct was set with default values for `#{field}` which now you want to override

If this is intentional, set force: true to disable this warning
"""
If you want to override the data, set force: true.

If you are intentionally preloading data that has not yet been committed (e.g. a new struct
with id: nil), you can set warn_if_nil_keys: false to disable this warning.
""")
end

cond do
card == :one and loaded? ->
{fetch_ids, [id | loaded_ids], [value | loaded_structs]}

card == :many and loaded? ->
{fetch_ids, [{id, length(value)} | loaded_ids], value ++ loaded_structs}

is_nil(id) ->
{fetch_ids, loaded_ids, loaded_structs}

true ->
{[id | fetch_ids], loaded_ids, loaded_structs}
end
end
end)
end

defp fetch_query(ids, assoc, _repo_name, query, _prefix, related_key, _take, _tuplet)
Expand Down
20 changes: 20 additions & 0 deletions test/ecto/repo_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -2007,6 +2007,26 @@
TestRepo.preload(%MySchema{id: 1}, children: intersect_all(query, ^query))
end
end

test "suppresses nil key warnings with warn_if_nil_keys option" do
parent = %MySchema{id: nil, children: [%MySchemaChild{a: "child1"}, %MySchemaChild{a: "child2"}]}

log = ExUnit.CaptureLog.capture_log(fn ->
TestRepo.preload(parent, :children)
end)

assert log =~ "association `children` for"
assert log =~ "its association key `id` is nil"

log = ExUnit.CaptureLog.capture_log(fn ->
TestRepo.preload(parent, :children, warn_if_nil_keys: false)
end)
refute log =~ "association `children` for"

result = TestRepo.preload(parent, :children, warn_if_nil_keys: false)
assert length(result.children) == 2
assert Enum.map(result.children, & &1.a) == ["child1", "child2"]
end
end

describe "checkout" do
Expand Down Expand Up @@ -2072,7 +2092,7 @@
def ensure_all_started(_, _), do: raise("not implemented")
end

defmodule NoTransactionRepo do

Check warning on line 2095 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.14.5, 23.3.4.20)

function all_by/3 required by behaviour Ecto.Repo is not implemented (in module Ecto.RepoTest.NoTransactionRepo)
use Ecto.Repo, otp_app: :ecto, adapter: NoTransactionAdapter
end

Expand Down
Loading