Use of `build( ... )` with Ecto 3.4+ leads to many association preload warnings

[jeremy-hanna]

Summary Relating to some changes in Ecto for this bug, a warning is emitted on the attempted Preload of associations that exist a built struct without a parent_id on the associations.

This is similar to this issue: https://github.com/thoughtbot/ex_machina/issues/336

In future versions of Ecto, it looks like this will change further and error.

Steps to reproduce I set up a simple project environment for this. You can see the simple test cases here

Effectively: (1) make a relation using ecto
(2) make a factory for the records
(3) Factory.build(:post, %{comments: [build(:comment)]}) |> Repo.Preload(:comments) will emit a warning as the comment (which was previously ignored pre-3.4) as it is not of type #Ecto.Association.NotLoaded<association :comments is not loaded>, now checks if there is a parent fk id on the associated record

Expectation I think I would still expect this to work, and possibly maintain a fake but consistent parent fk id between these associated records

[germsvel]

@jeremy-hanna thanks so much for opening such a detailed and helpful issue! 🙏

This is still an issue that I'm not sure how to resolve, so I'm very open to ideas!

The problem is that build functions don't have any knowledge of Ecto or associations. It's only when using ExMachina.Ecto that we get access to insert functions (which know about Ecto and associations).

I'm also curious, what is the scenario when you try to preload an association on a struct that isn't persisted? In other words, when might you use this?

Factory.build(:post, %{comments: [build(:comment)]}) |> Repo.Preload(:comments)

And why wouldn't it be this?

Factory.insert(:post, %{comments: [insert(:comment)]}) |> Repo.Preload(:comments)

[jeremy-hanna]

Hey thanks for the reply

The test scenario is a bit contrived to keep the logic observable in one file but what we've run into with our test suite is building a record and then passing that into a context function that will attempt to preload subsequent records for verification like the following

test "it can update a title" do
  post = Factory.build(:post, %{comments: [build(:comment)]})

  assert %Post{} = MyApp.Posts.update_title(post, "A new title")
end

defmodule MyApp.Posts do

  ...

  def update_title(%Post{} = post, a_new_title) do
    post
    |> Repo.preload(:comments)
    |> Post.changeset(%{title: a_new_title})   # this changeset validates something against the preloaded assoc
    |> Changeset.apply_changes()
  end
end

If I'm testing a changeset is valid or not I would prefer to not have a persisted record. Again this example is pretty contrived, but we do some pretty deeply nested changeset validation with associated records which we either provide or preload (which if provided in the struct would no-op pre-3.4) associations

As for solutions... I also don't know how to resolve this. I think this direction in Ecto breaks a lot of our currently implemented code as we rely on this lazy preload pretty heavily for upserting records. This also might just be a thing where our use of ExMachina is wrong and possibly there should be a disclaimer haha

There wasn't any issues or documentation around this and it took a while to figure out what was happening so I figured I would open a thread on it.

[robacarp]

I have a sneaking feeling this isn't going to scale well, but here's how I'm dealing with this problem. Perhaps this can be better represented in ExMachina.Ecto but perhaps there are a myriad of other concerns to doing it this way which I'm ignorant of:

• the factory itself renders an empty array onto the association field
• with_ helpers merge in a built list which overwrites the association

Some psuedocode to show what I mean:

def post_factory do
  %Post{
    comments: []
  }
end

def with_comments(post = %Post{}, count \\ 2) do
  %{post | comments: build_list(count, :comment)}
end

test "post has comments" do
  build(:post) |> with_comments(2)
end

[germsvel]

Thanks @robacarp and @jeremy-hanna for commenting on this. I just wanted to loop back and say that I still haven't figured out how to solve this, but it is something I'd love for us to figure out.

The problem is that ExMachina's build functions comes from ExMachina's portion that does not know about Ecto. One option would be to have ExMachina.Ecto define a new build function that knows more about Ecto than ExMachina's build. But I haven't worked out all the implications of that yet.

If either of you have ideas, I'd love to hear them.

[robacarp]

Sure, yeah. I think it makes sense for ExMachina.Ecto.build to overlay (and perhaps call back to) ExMachina.build. That would give devs the ability to choose the older if they're so inclined.

It could be a frustrating breaking change to roll out though, because presumably everyone has already written import ExMachina.Ecto into their apps and suddenly this would change the behavior of every build method being used. If it's the right solution it's probably worth biting off, but it'll cause some headaches I'm sure.

Whether or not that's the least surprising behavior remains to be evaluated. I ran into this when writing a View test, so I'm expecting my structs to be all preloaded up already. Maybe the right solution is to recommend calling Repo.preload in the with_comments/1 decorator above -- maybe that's what I'd want later when I'm testing a controller instead?

[baldwindavid]

@germsvel I rather like that build doesn't know or care about Ecto. However, one thing I have missed in ExMachina that ruby's FactoryBot (and Thoughtbot's) has is something similar to build_stubbed to produce a graph of records that "seem" like they are persisted. That can be very helpful in cutting down on the need to hit the database. Perhaps rather than changing build, a build_stubbed function is introduced that is fully aware of Ecto.

[baldwindavid]

Here is a very limited and opinionated build_stubbed strategy I've been using... https://gist.github.com/baldwindavid/780449125ac4e9d064662895a0d65703

It does the following:

• Adds inserted_at and updated_at timestamps to all records in the tree if there is not already one
• Adds an id (specifically a UUID) to all records in the tree if there is not already one
• For belongs_to associations only: sets the foreign key of the child record to the associated parent ID

It doesn't handle syncing up IDs for any other associations. I just haven't needed it.

Example usage for the following account factory...

def account_factory do
  %Account{
    property: build(:property)
  }
end

account = build_stubbed(:account)

The account should have both the property and property_id set for the built property.

Example with an explicitly passed in property...

property = build_stubbed(:property)
account = build_stubbed(:account, property: property)

The account should have both the property and property_id set and it should correspond with the property and property_id passed in. It should not override the property's already set id, inserted_at, and updated_at.