Skip to content
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

Propagate opts when process child #3309

Closed
mojidabckuu opened this issue May 21, 2020 · 15 comments
Closed

Propagate opts when process child #3309

mojidabckuu opened this issue May 21, 2020 · 15 comments

Comments

@mojidabckuu
Copy link

Environment

  • Elixir version (elixir -v): 1.10.3
  • Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): Postgres
  • Ecto version (mix deps): 3.4.4
  • Database adapter and version (mix deps): postgrex 0.15.4
  • Operating system: Mac OS

Current behavior

If I try to inject Repo and want to extend "Shared options" passed to the repo to apply it for each insert/update/delete statements then it is not passed to child inserts
Having my repo like this

defmodule EctoOpts.Repo do
  use Ecto.Repo,
    otp_app: :ecto_opts,
    adapter: Ecto.Adapters.Postgres

  defoverridable([insert: 2])

  def insert(struct, opts) do
    IO.puts "#{inspect opts}"
    super(struct, opts)
  end
end

and having the test like

test "do it" do
  user_attrs = %{
    "posts" => [
      %{}
    ]
  }

  %EctoOpts.User{}
  |> EctoOpts.User.changeset(user_attrs)
  |> Repo.insert(foobar: :enable)
end

then I get next output

> Executing task: mix test test/ecto_opts/test_opts_case_test.exs <

00:19:34.403 [info]  Already up
[foobar: :enable] # which is User insert call
[] # which is Post insert call

Is it works like this for some reason? In my project I made a workaround playing with Process.put/Process.get combo to propagate the setting for child inserts and then restore the state after action was done, but looks dirty imho.

Expected behavior

The opts are available recursively to each action call

@wojtekmach
Copy link
Member

Post is an association on the User, right? Internal operations like inserting association don't go through the main API (that you're overriding) so that's why it doesn't work. Your best bet is using default_options/1 callback.

@mojidabckuu
Copy link
Author

@wojtekmach If it doesn't go though the main API that I am overriding then why it is called twice?

[foobar: :enable] #Ecto.Changeset<action: nil, changes: %{posts: [#Ecto.Changeset<action: :insert, changes: %{}, errors: [], data: #EctoOpts.Post<>, valid?: true>]}, errors: [], data: #EctoOpts.User<>, valid?: true>
[] #Ecto.Changeset<action: :insert, changes: %{user_id: 7}, errors: [], data: #EctoOpts.Post<>, valid?: true>

@wojtekmach
Copy link
Member

you showed how you're inserting user, could you show how you're inserting the post?

@mojidabckuu
Copy link
Author

@wojtekmach I am not inserting the post manually :) it is a part of cast_assoc/3 in the User

  user_attrs = %{
    "posts" => [
      %{}
    ]
  }

  %EctoOpts.User{}
  |> EctoOpts.User.changeset(user_attrs)
  |> Repo.insert(foobar: :enable)

User.ex

defmodule EctoOpts.User do
  use Ecto.Schema

  import Ecto.Changeset

  schema "users" do
    has_many :posts, EctoOpts.Post
  end
  def changeset(struct, attrs) do
    struct
    |> cast(attrs, [])
    |> cast_assoc(:posts)
  end
end

Post.ex

defmodule EctoOpts.Post do
  use Ecto.Schema

  import Ecto.Changeset

  schema "posts" do
    belongs_to :user, EctoOpts.User
  end

  def changeset(struct, attrs) do
    struct
    |> cast(attrs, [])
  end
end

@wojtekmach
Copy link
Member

never mind, you're using associations. yeah, we don't guarantee that the options are passed through down to associations.

@josevalim
Copy link
Member

Right. We don’t guarantee neither insert (It May work in some cases) will be called nor pass the options downstream. It is easier if you tell us generally about your use case.

@mojidabckuu
Copy link
Author

mojidabckuu commented May 21, 2020

Right! Why guarantee? It is just about pass the options. Are all nested assoc inserts done within same process? Barely viewing the code seems it is.
The default_options/1 provides only global setting per operation for the repo which is in terms of current issue isn't enough.
The idea behind is to collect the changes made by repo per schema. And Repo seems the best entry point to do this. The best format to control the collect behaviour is use opts variable and adjust the flow to track/to do not track changes for example in tests, per case, etc.

@mojidabckuu
Copy link
Author

@josevalim Just did it :)

@wojtekmach
Copy link
Member

you're still not telling us your use case, what options do you want to pass - what will you do with them?

@mojidabckuu
Copy link
Author

mojidabckuu commented May 21, 2020

Hmmm
The case is to take changes and store them somewhere(other db, file, whatever) but on recursive basis, all associated changes are tracked. The tracking depends on an option to disable/enable to be excluded with factories, tests, other specific change cases when it is not required.

# Main app repo
defmodule MyApp.Repo do
  use Ecto.Repo ...

  use MyTracker.Repo
end

# An extension to override main repo
defmodule MyTracker.Repo do
 defmacro __using__(opts) do
  defoverridable([insert: 2])

  defp do_tracking(%Ecto.Changeset{} = cs) do
    # Basicly get .changes and store them(somewhere).
  end

  def insert(struct, opts) do
    if Keyword.get(opts, :tracking_enabled, false) do
      do_tracking(struct)
    end
    super(struct, opts)
  end
 end
end

# Expects to create User in my Repo, track User and all nested structs downside the changes tree
def do_something_important_here1(attrs) do
  MyApp.User
  |> MyApp.User.changeset(attrs)
  |> MyApp.Repo.insert([tracking_enabled: true])
end

# Expects to do not track anything and User is created
def do_something_important_here2(attrs) do
  MyApp.User
  |> MyApp.User.changeset(attrs)
  |> MyApp.Repo.insert()
end

@josevalim
Copy link
Member

We were used to pass all options but some actions caused issues when passed down, such as :on_conflict and returning, so we made it opt-in. Generally speaking, your pattern of overriding insert/2 is not officially supported, as insert/2 is not guaranteed to be re-entrant, so we would need another mechanism to support this. This particular feature can most likely be supported with changeset+prepare_changes, which I think is a clearer mechanism than overriding the repo.

@mojidabckuu
Copy link
Author

@josevalim thanks for the answer!

Seems it would be legal to inject aka inherit Ecto.Changeset, let's say as MyApp.Ecto.Changeset, override cast/3 to automatically append current changeset to prepare_changes queue. For sure I have then edit all Repo.delete/2 usage and wrap it into changeset to get prepare changes call.

@mojidabckuu
Copy link
Author

Still curios about the guarantee when associated insert isn't called. Does it go through associations and just applies exactly the same steps as parent's changeset?
Reading the code from the first sight always leads to this line which takes the current repo and applies the action which triggers Schema's action which triggers prepare_changes. Could you please provide an example for a such case or point to the code?

@josevalim
Copy link
Member

We call it for insert but you can see some association operations, such as delete_all and update_all, bypass the repo. But the point is we cannot guarantee this will always work, the fact we do call it is just a convenient implementation detail.

@mojidabckuu
Copy link
Author

The first issue I see is how to track deletion, the plan is to query inside overridden delete/2 related schemas to delete and track the deletion. May I say it covers the case? Feels like yes.
The same is applicable for update/2 where it is possible to extract the diff(what is inserted/deleted) and track delete/insert as well.

I assume that when you mentioned delete_all and update_all they are related to delete/nilify callbacks because I found only this and this. For sure it would be great to have a single entry point for repo manipulation because it allows to get the returning which could be useful, I guess.

May be now I started to understand what you were trying to say. If "guarantee" is about it could break your logic later because of some internal changes then alright, it is time to take prepare_changeset solution which is less elegant comparing to the Repo pattern. Moreover it increases refactoring to existing project. If "guarantee" resides in hidden, non-controllable cascade calls then seems it is possible to handle them until Ecto becomes API stable v2 with more guarantees :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants