Add change_order option to has_many

[greg-rychlewski]

Closes #4713

I had a few different thoughts how to do this...some of the alternatives I thought about:

• Accepting a list of :insert, :update, :delete and letting the user choose which order they want. I thought it might get messy though validating that the list makes sense (i.e. if there are duplicates of certain atoms or things are missing) and also if potentially people will want to handle :replace specially
• Accepting a sorting function the user defines. I thought it might be better to start simpler and add this later if we get requests for more flexibility. There are some gotchas with translating :replace into repo actions that might be tricky to handle
• The name of the option value :unique_safe I thought about making it something like :remove_first or :delete_first or :delete_upsert_insert. The first two I discarded because I thought they were a bit confusing/not completely accurate. The last one I thought is a bit long and opens it up to having all the permutations as option names. I decided to try to find some kind of short user-friendly name that encompasses the specific purpose.

[greg-rychlewski]

As a side note I don't think on_replace: :nilify is working properly for has_many. When I tried to do a test for this change with that option it basically didn't register the update. And when I investigated the changes were empty. So I think we are not adding the foreign key change into the changes map.

If it's alright I can fix that in a follow up.

[greg-rychlewski]

Actually nm I'm just dumb and didn't set up the test data correctly. Added the test

[josevalim]

@greg-rychlewski could this be a changeset function instead? Something when casting associations or a pass we do on the changeset data after cast/put_assoc?

[greg-rychlewski]

@josevalim What about something like this

cast_assoc(changeset, :assocs, change_order: :unique_safe)

We would still do the sorting where it is in this PR but now the user has the flexibility to change the order instead of it being hard coded onto the schema.

The reason I'm not sure about or a pass we do on the changeset data after cast/put_assoc? is because of the interactions with :sort_param. Since they are orthogonal concerns the two orders should be allowed to be different. But if they are different then they both can't be satisfied after calling cast_assoc

[josevalim]

I was thinking something like: cast_assoc(changeset, :assocs) |> reorder_assoc(:assocs). For put, we could make it emit the correct order if we want (I believe the order is not guaranteed). For cast, we actually don't to change the order, because the order is often the one coming from the UI/interface.

[greg-rychlewski]

Ah ok. Just to make sure I understand correctly you are saying reorder_assoc always affects the database operation order right? So cast
_assoc ignores it when you’re passing around the struct but when we are doing the database operations would respect it?

[josevalim]

So there are two possible ways for the changesets for an assoc to be created:

1. cast_assoc, which often preserves UI order
2. put_assoc, which we can probably change the order to whatever we like

So my idea is that you can call reorder_assoc before you write to the database. We can provide a default function (no argument, but you can also give a custom one). Does that make sense?

[greg-rychlewski]

Oh yes I got your idea now. Thanks!

[greg-rychlewski]

@josevalim I'm having a bit of doubt and was curious what you thought. I started to implement it and then I thought that changes is already a public field so people could basically just use Enum.sort instead of reorder_assoc. We have the default function which we could set as the one in this PR but that kind of feels weird to make the default of a generic reorder function so specific to this one issue.

Do you think it would make sense to expose specific sort functions for users to apply themselves? I can't think of a good name right now but the idea would be something like Ecto.Changeset.sort_delete_update_insert which would take a list of changesets and order them the way this PR is. It seems to me the value is taking care of the complexities with :replace that users might forget about/not know how to do.