Add insert callback

[tlvenn]

Hi,

Little background on what I am trying to solve. Basically i have a classical parent <--> child association except that the parent also has some direct associations to some child.

I am building from the parent and pipe into an helper to associate the child. The thing is I cannot actually insert the pointer now because the child does not exist yet and it seems like the only way to address this would be to be able to update the parent once the parent with its child has been inserted.

To that effect, I created a macro which add a virtual field in my schema where I can store callbacks.

and then I define in my factory:

def insert_full(%{__factory_callbacks__: cbs} = struct) do
    Enum.reduce(cbs, insert(struct), &(&1.(&2)))
  end

It works but I don't like having 2 versions of insert. If the lib would provide an overridable fun like after_insert then I could execute my callbacks transparently without introducing another insert function.

[tlvenn]

Hi @germsvel, any thoughts on the above please ?

[germsvel]

Hi @tlvenn, do you have any sample code that you could past here? I'm not entirely clear on the issue you're running into. Seeing the schemas and factories related to what you're talking about might give me a better picture.

[tlvenn]

Yes sure thing @germsvel.

So in my case for example, I have an Asset struct and an AssetVersion struct. As you can guess conceptually, an asset has multiple versions. Now beyond the asset having a collection of versions, the asset also has potentially 2 direct associations to a version. One for the current version and one for the pending version.

Now when I try to create a factory around Asset and AssetVersion, I am doing the following:

asset = build(:asset)
   |> with_pending_version(build_asset_version(user, status: :validated))
   |> for_user(user, :user_photo)
   |> insert_full

The issue is it's impossible to create at the same time the parent and the child and having references going both ways (the child having fk to the parent and the parent having the child id for current_version_id or pending_version_id.

So I ended up solving this using callbacks as follow by having those 2 helpers in my main factory:

defp add_callback(%{__factory_callbacks__: cbs} = struct, cb) do
   %{struct | __factory_callbacks__: cbs ++ [cb]}
end

def insert_full(%{__factory_callbacks__: cbs} = struct) do
  Enum.reduce(cbs, insert(struct), &(&1.(&2)))
end

And my Asset schema has the following field (we could have a macro to do this):

field :__factory_callbacks__, :any, virtual: true, default: []

Then the code for with_pending_version do the following:

def with_pending_version(%Asset{} = asset, %AssetVersion{} = version) do
   asset
   |> insert_into(:versions, version)
   |> add_callback(fn struct ->
       Repo.update!(change(struct, %{pending_version_id: version.id}))
       |> Map.put(:pending_version, version)
    end)
end

[germsvel]

@tlvenn thanks for posting that info. I think I'm understanding what you're saying, but if it seems like I'm not, please let me know.

I am trying to think of the benefits of introducing something like what you're talking about. I could see how having callbacks could be beneficial, but I'm trying to think of how we could solve this in other ways. I don't really like the idea of introducing a virtual field in the schemas just to handle the callbacks. And whenever possible, I try to avoid macros.

One possible solution that doesn't require changes in ex_machina would be to do something like this, correct?

# factory
def asset_factory do
  %Asset{
    name: "random asset"
    # more fields
  }
end

def version_factory do
  %AssetVersion{
    some_field: "field in version"
    # more fields
  }
end

def with_pending_version(%Asset{} = asset) do
  version = insert(:version, asset: asset)

  asset
  |> insert_into(:versions, version)
  |> change(%{pending_version_id: version.id})
  |> Repo.update!
end

# test code
insert(:asset)
|> with_pending_version()

Some of the downsides here that I can think about are that you have to insert the asset before passing it to with_pending_version/1, and we removed the ability to pass in the version into the function. Of course we could allow it, but then we'd have to do more legwork before the method is called and both asset and version would have to be inserted,

# test code
asset = insert(:asset)
version = insert(:version, asset: asset)
asset = asset |> with_pending_versions(version)

# factory
def with_pending_version(%Asset{} = asset, %AsserVersion{} = version) do
  asset
  |> insert_into(:versions, version)
  |> change(%{pending_version_id: version.id})
  |> Repo.update!
end

Could you give me some feedback on why this solution might and might not work for your case?

[tlvenn]

Hi @germsvel , thanks for your feedback.

What you are doing would work but I dont feel like I should be forced to interact with the database while I am only doing composition at this point. From my point of view. only when I issue insert should the database be affected.

Now that also means that while my solution might be better in that regard, at the same time, it means that until I actually insert the whole thing in the DB, it is not fully correct from a data perspective. And in that sense, your solution is better.

I am wondering if there is not a way to actually achieve both. We should be able to assign the current_version_id and let ex_machina using Ecto metadata figures out on insert that it's dependent on the insertion of a child or a descendant and therefore, automatically defer that association until the initial insertion is done.

What do you think ?

[jhonathas]

I do not know if you continue with this problem, I ended up needing a callback and, I ended up including in ex_machina the following commit: https://github.com/jhonathas/ex_machina/commit/0586cbb51d1f54a324c3903d56d7aee14072b29e

I was going to open an issue now and, if it made sense, I would ask permission to do a pull_request.

The idea is that I need to do some things after the insert, and it would be bad to have to use with_ in all the places that I would give insert, since for all I would have wanted to do the same things. So, if you include a _callback along with _factory, ex_machina would do that by default, eg:

factory

def user_factory do    ..... end

def user_callback(struct) do    ..... end

The callback function does not change the result of the insert, it only performs some action.

If there was no callback function created, everything would work as it does now.

[germsvel]

Thanks for following up on this @jhonathas. I can see how a callback like that could work. It's actually very similar to an example we have in ExMachina.Strategy for when someone is implementing a custom strategy.

I took a look at https://github.com/jhonathas/ex_machina/commit/0586cbb51d1f54a324c3903d56d7aee14072b29e, and there's a couple of things I think we should change if we want to add this to ExMachina.

1. I think that we should call the callback function from ExMachina.EctoStrategy instead of ExMachina.Strategy since not all strategies that people implement will need a callback.

2. I also think naming it after_insert_factory_name(struct) (e.g. after_insert_user(struct)) would be better than just callback. We might want a before_insert_ later on, and because of point 1 we have more context on what the callback is coming after (i.e. after an insert)

I like this approach better over having a virtual field in the struct. cc: @tlvenn

What do you both think?

[jhonathas]

Hello @germsvel, how are you?

So, in relation to the name, I came to think of the after, instead of callback, but I did not follow this idea because I thought that we did not have a before for now, could confuse people - but I'm not opposed to changing the name, in inclusive, then I can think and build the solution with the before.

In relation to being within the strategy and not EctoStrategy is because I even thought of something that could be used, in a generic way, for any strategy. Therefore, being in the strategy and not in EctoStrategy, would be available for use by any custom strategy.

This function does not return anything, it only serves to use something created by ExMachina - in my case here I needed a statement of this because I needed to include the information that ExMachina created, in Elastic Search, then to have this direct behavior in the Factory was a way to gain time and diominate several lines of code. But let's talk to get a better format decided by all.

[germsvel]

Hi @jhonathas, I'm not sure I'm following what you're saying here:

but I'm not opposed to changing the name, in inclusive, then I can think and build the solution with the before.

I think you're saying that after_insert_* would be confusing since there's no before_insert_* callback. I think it's fine if we just introduce the after_insert_* and see if people have a need for a before_insert_*, at which point we could add that. But I am also okay if you wanted to introduce both at the same time.

I still think, however, that they should be in ExMachina.EctoStrategy, not in ExMachina.Strategy. Many strategies may not need callbacks, and those who implement a custom strategy can do so if they want to. But if we put it in ExMachina.Strategy those implementing custom strategies cannot remove the callback. I also think the name callback is less clear in its intention.

Regarding this,

This function does not return anything, it only serves to use something created by ExMachina

I think the generic after_insert_* should return the factory. I see that in your example, it was not needed, but I imagine many people might use the after_insert_* to add some additional dependencies to the factory struct, and so it would be better to return the factory from that function (in case it is modified). If someone is performing a pure side-effect (as it was in your case), returning the factory would not harm them either.

[jhonathas]

Hi @germsvel , got it. I'll make the changes and I'll push and communicate to you here, okay?

[germsvel]

Thanks @jhonathas! That sounds great!