Skip to content

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

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

Open
wants to merge 2 commits 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
76 changes: 75 additions & 1 deletion test/ecto/repo_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@

schema "my_schema_child" do
field :a, :string
belongs_to :my_schema, MySchema
belongs_to :my_schema, Ecto.RepoTest.MySchema
belongs_to :my_schema_no_pk, MySchemaNoPK, references: :n, foreign_key: :n
end

Expand Down Expand Up @@ -2007,6 +2007,80 @@
TestRepo.preload(%MySchema{id: 1}, children: intersect_all(query, ^query))
end
end

test "preload assigns belongs_to assoc" do
struct = %MySchemaWithAssoc{
id: 1,
parent_id: 1
}

assert %Ecto.Association.NotLoaded{} = struct.parent

mock_results = {
1,
[
[1, 2]
]
}

Process.put(:test_repo_all_results, mock_results)

result = TestRepo.preload(struct, :parent)

assert_received {:all, _query}

Check failure on line 2030 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.18.1, 27.2, lint)

test preload preload assigns belongs_to assoc (Ecto.RepoTest)

Check failure on line 2030 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.14.5, 24.3.4.17)

test preload preload assigns belongs_to assoc (Ecto.RepoTest)

Check failure on line 2030 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.17.3, 25.0.4)

test preload preload assigns belongs_to assoc (Ecto.RepoTest)

Check failure on line 2030 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.17.3, 27.1)

test preload preload assigns belongs_to assoc (Ecto.RepoTest)

assert result.parent == %MyParent{n: 2}
end
Comment on lines +2011 to +2033
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @josevalim, thanks for the insight here – you're right that this case fails. I dug into how the preload algorithm is working, and I understand now how it's building up a map of assocs using ids as keys, and in this case nil keys are causing things to break. We'd need to refactor this quite a bit to get nil keys to work. I think a good first step might be to add simple test cases for preloading behavior (I didn't find this anywhere in the test cases), but I'm not sure the best way to do this. I tried using this :test_repo_all_results to give the preloader some mock data... I'm having trouble getting it to work, but what do you think of this approach? Is there a better way to test Repo.preload?

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to have proper integration tests but the issue is that I am unsure if this can work at all. If all of the primary/foreign keys are nil, we are simply incapable of putting things back to their places :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm imagining a way it should be possible to assign things using an alternative lookup mechanism than an id, if we know the structure is always nested structs/lists, we can find things using access patterns (keys, list positions)?

In the meantime I'm trying to get a basic unit test working here where we have numeric ids, it feels my test case here follows existing examples of getting mock data from a query but I can't get this to actually work...


test "preload assigns nested has_many->belongs_to assocs" do
structs = [
%MySchema{
id: nil,
children: [
%MySchemaChild{
id: nil,
my_schema_id: 123,
my_schema: %Ecto.Association.NotLoaded{}
}
]
},
%MySchema{
id: nil,
children: [
%MySchemaChild{
id: nil,
my_schema_id: 456,
my_schema: %Ecto.Association.NotLoaded{}
}

Check failure on line 2054 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.18.1, 27.2, lint)

test preload preload assigns nested has_many->belongs_to assocs (Ecto.RepoTest)

Check failure on line 2054 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.14.5, 24.3.4.17)

test preload preload assigns nested has_many->belongs_to assocs (Ecto.RepoTest)

Check failure on line 2054 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.17.3, 25.0.4)

test preload preload assigns nested has_many->belongs_to assocs (Ecto.RepoTest)

Check failure on line 2054 in test/ecto/repo_test.exs

View workflow job for this annotation

GitHub Actions / unit test (1.17.3, 27.1)

test preload preload assigns nested has_many->belongs_to assocs (Ecto.RepoTest)
]
}
]

{result, _log} =
ExUnit.CaptureLog.with_log(fn ->
TestRepo.preload(structs, children: :my_schema)
end)

assert [
%{
children: [
%{
my_schema_id: 123,
my_schema: %{id: 123}
}
]
},
%{
children: [
%{
my_schema_id: 456,
my_schema: %{id: 456}
}
]
}
] =
result
end
end

describe "checkout" do
Expand Down
Loading