Allow suppressing association key is nil warning when preloading data

[tomconroy]

Sometimes we want to preload data inside a changeset. (In our case, snapshotting values like "price" in an order item from product selections)

changeset = struct
|> cast_assoc(:children)

changed_data = changeset
|> apply_changes()
|> Repo.preload([children: :nested_child])

changeset
|> put_computed_data(changed_data)

The data after apply_changes may have new children (with id: nil) like below.

%Child{
  id: nil, 
  nested_child_id: 123, 
  nested_child: %Ecto.Association.NotLoaded{}
}

Currently with the code above we get a warning message:

[warning] association `children` for `MyStruct` has a loaded value but its association key `id` is nil. This usually means one of:

  * `id` was not selected in a query
  * the struct was set with default values for `children` which now you want to override

If this is intentional, set force: true to disable this warning

We don't want to preload it with force: true in this case, because the data is changing and we don't want to override it, we just want a way to suppress this warning.

[greg-rychlewski]

Hi @tomconroy ,

Could you let me know if I understand your situation. Do you want to do this:

1. Taking an existing parent struct that has been loaded from the db
2. Take the existing associated values of :children from the database.
3. Perform a change on :children based on input from a form

[greg-rychlewski]

It looks to me like you want to reverse your operations. First preload and then cast assoc. But I'm not confident I'm understanding your situation properly.

[tomconroy]

We are certainly preloading before cast. The key thing is the apply_changes results in ecto structs with the new children, but without ids because they are yet to be persisted. We want to do operations on those new children which require their own nested preloads. Doing preload(force: true) would override the new children since they're not persisted, the current error message is a bit misleading, and we want to just allow this behavior as-is without the warning.

[greg-rychlewski]

Thanks. I think it's starting to become a bit clearer to me. In regards to this:

We want to do operations on those new children which require their own nested preloads

What would be the reason for trying to do nested preloads on new children? It seems they are not persisted so the preload would never return anything. Or do I misunderstand?

p.s. The reason I am poking around with questions is just so I can understand whether what you want to do below is the right way forward or if you are trying to do something that already has a correct solution

the current error message is a bit misleading, and we want to just allow this behavior as-is without the warning

[tomconroy]

I see why it's confusing, maybe this illustrates more clearly?

struct = %MyStruct{
  children: [%Child{
    id: nil, 
    nested_child_id: 123, 
    nested_child: %Ecto.Association.NotLoaded{}
  }]
}

preloaded_struct = Repo.preload(struct, [children: :nested_child]

preloaded_struct.children |> Enum.map(fn child -> child.nested_child.some_key end)

If we preload this with force: true, the children are going to be reloaded from the db. But we want preload to keep the existing loaded assocs, and only load ones that are %NotLoaded{}

[greg-rychlewski]

But we want preload to keep the existing loaded assocs, and only load ones that are %NotLoaded{}

This is supposed to be the default behaviour. So I think there must be something a bit different between your situation and the test in this PR.

Maybe I can explain why the behaviour in the PR happens and you can let me know if it makes sense

parent = %MySchema{id: nil, children: [%MySchemaChild{a: "child1"}, %MySchemaChild{a: "child2"}]}

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

The reason this has the warning is because MySchema is not loaded from the database. It's just a struct that was created and it is missing the id used in the association. If MyStruct was loaded from the db it would have a value of id and you would not get a warning.

[tomconroy]

Yes I understand that, in our case the struct is created through the apply_changes function after cast_assoc (with new items not yet persisted) – I want to pass this struct along to functions that expect that struct to have its assocs preloaded.

The %MyStruct{id: nil} is a representation of what happens when you do apply_changes with new items.

[greg-rychlewski]

So the issue is that you do not want to load the data from the database but you still want to preload onto the struct?

[tomconroy]

I want to keep the existing behavior (only load %Assocation.NotLoaded{} data from the db) but allow already-loaded data to have nil ids without printing a warning

[greg-rychlewski]

I think we are talking past each other. load means that it came from the database so it would not have a nil id.

[tomconroy]

Sorry, I use "loaded" to mean more narrowly "a value is that not Association.NotLoaded" rather than implying it came from the db

ecto/lib/ecto.ex

Lines 575 to 578 in 2e66ee8

	def assoc_loaded?(%Ecto.Association.NotLoaded{}), do: false
	def assoc_loaded?(list) when is_list(list), do: true
	def assoc_loaded?(%_{}), do: true
	def assoc_loaded?(nil), do: true

[greg-rychlewski]

Thanks for the clarification. So I think we are back to my original question:

What would be the reason for trying to do nested preloads on new children? It seems they are not persisted so the preload would never return anything. Or do I misunderstand?

When I say "new children" above you can think of it as: What is your use case for trying to preload on a struct that has not been persisted yet (does not have an id)? I'm not sure if this is a flow that is supposed to be supported. But I am really trying to understand just so I am not brushing it off.

[tomconroy]

I mentioned our use-case in the original post

snapshotting values like "price" in an order item from product selections

I can elaborate on what I mean: our OrderItem points to a product as well as list of product selections, those selections indicate price multipliers (which are editable in the store admin). We want to calculate the price for the OrderItem and persist it (snapshot it) based on its assocs, inside the changeset, after the selections have been cast (or the product changes).

This is one of a few different cases where we're doing this. We're also (e.g.) calculating discounts as order_items change, based on the subtotal of the order_items. The function that calculates the subtotal is expecting plain structs, not changesets. (FWIW, I used to have the logic in two places, one that calculates using structs, and one using changesets, but it's much better to have just one of these...)

[tfwright]

This thread popped up while I was researching a related issue. I have a function that abstracts some logic that works on a changeset. It requires access to associated data so it needs to ensure the changeset's data has been preloaded.

I was attempting to use get_field which actually raises an error when the associated data is not loaded. That is not exactly related to this specific PR and not to derail this discussion, but I wanted to flag that I have a similar question about the "blessed" way to access associated data in changesets, without knowing whether that data has been preloaded yet.

[tomconroy]

@tfwright definitely related! Before arriving at apply_changes I had a lot of get_field and so on (that's what I mean by a parallel version that operates on changesets rather than structs). I have been working on a larger preloading abstraction (connected to Dataloader patterns) which allows functions to declare+assert their preload requirements, that is expecting/requiring you to preload as early as possible instead of where needed (avoiding n+1 queries)

[greg-rychlewski]

I can elaborate on what I mean: our OrderItem points to a product as well as list of product selections, those selections indicate price multipliers (which are editable in the store admin). We want to calculate the price for the OrderItem and persist it (snapshot it) based on its assocs, inside the changeset, after the selections have been cast (or the product changes).

Thanks. I will try to abstract/simplify what you wrote here. Can you let me know if this is accurate:

You are creating some new entities, and these entities are associated to something like a configuration table. And you want to use the values in that configuration table to calculate other values in the entity before persisting it?

[tomconroy]

That's accurate! That entity (the order item) has a changeset that casts the user's selections for the configuration, then we take the values from the configuration's associations to put calculated values on the entity.

[greg-rychlewski]

And just want to make sure: you need to preload from the parent struct? It wouldn’t make sense to preload directly from the children? Because that would also get rid of the warning.

Besides the above I can’t think of another way to solve your problem. So if you cant do the above I think this change is ok. Just want to see what @josevalim thinks in case he knows a reason not to do it. Also to see his opinion if he is concerned we will have to allow finer granularity than Boolean in the future

[tomconroy]

Well, the cast_assoc may create new nested assocs with their own missing ids. To be clear, this PR doesn't change any real behavior in ecto, only allows suppressing the warning message in this niche use-case.

[greg-rychlewski]

Got it. Makes sense to me!

I want to wait for Jose before going ahead with the merge. I want to be consistent with whatever policy on warning suppression he thinks is best or has been in place in the past.

The reason is because we have multiple warnings and maybe all of them have situations where it makes sense to suppress them or conditionally suppress them. This can snowball if we are not careful, so just want to make sure we are moving forward in a way that let's use maintain our sanity/consistency.