Repo.config and dynamic instances

[Saša Jurić]

While adding the support for dynamic repos to the Oban library, I bumped into a subtle problem. I'm not sure what would be the proper course of action, or if the problem should even be solved in Ecto, so I'm starting a discussion here.

The issue is triggered by the Oban library in this place: https://github.com/sorentwo/oban/blob/3431a674d2980e599a433adce98cd90d1b004b19/lib/oban/notifier.ex#L246

Essentially, Oban needs to establish a separate db connection for notifications. To do that, it invokes `Repo.config` on the repo module passed by the client code to obtain the conn params. At this point the dynamic instance is correctly set, but regardless, `config/0` won't include the db connection settings (e.g. database, password, or url). From what I can tell, `config` will simply invoke the `init` callback, which will return only the "statical" part of the config, i.e. it won't include instance-specific settings, such as db conn params, even if the dynamic instance is correctly set in the caller process.

To make this work in my client project I had to resort to some trickery:

```

defmodule MyDynamicRepo do

def init(_, config) do

with repo_pid when is_pid(repo_pid) <- get_dynamic_repo(),

{:ok, url} <- discover_url(repo_pid) do

{:ok, Keyword.put(config, :url, url)}

else

{:ok, config}

end

end

# ...

end

```

This feels less than perfect for two reasons. First, I needed to establish some url discovery mechanism. Secondly, since `init` may be invoked when the app is not fully started (from migration tasks), the supporting processes might not be started, and so the code in `discover_url` needs to include some extra safeguard checks. I must say that this solution doesn't seem obvious at all.

The question is how could we improve this? One idea that comes to mind is to push this logic to Ecto. Basically, if dynamic instance is set, Ecto could honor it and issue a call into some process (or do a registry lookup) to get the instance specific settings.

Alternatively, perhaps another function could be introduced, e.g. `runtime_config` which would fetch the setting from the running repo instance. The problem with `config` is that it seems to be designed to also support the cases where it is invoked without a running repo. OTOH, sometimes the code wants to get the settings of the running repo instance (as is the case with Oban). Having a dedicated function for that could be a clearer solution which doesn't involve weird edge cases that can happen when the repo is not running.

[José Valim]

Hi Sasa, thanks for the proposal!

Correct. The config is used for things like mix ecto.create, which is when you don't have an app/repo running.

Honestly, this does not feel like an Ecto problem. We have been working hard to remove those singleton entry-points and you are one one of the main advocates for this work. :) Therefore adding such runtime_config feels like a step backwards. Since you can initialize a repo with any option, it should be your responsibility to pass these options around for other interested parties. So I would say Oban should be configurable here and allow you to pass the notifications config too, and all you have to do is to mirror them. :)

--
You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/63cc3fe7-764d-4597-a9d7-d2d33e995f5an%40googlegroups.com.

[Saša Jurić]

> So I would say Oban should be configurable here and allow you to pass
> the notifications config too, and all you have to do is to mirror them. :)

The thing is that Oban is a 3rd party library that hooks into a running
Repo instance. The client developer needs to first start the repo, and
then an Oban instance, passing the repo module to Oban. I recently did a
PR which allows passing the dynamic instance pid resolver. Now you're
suggesting that Oban users should also pass the connection params that
were already passed to repo (which is running). This will work, but the
UX feels needlessly unfriendly, both to lib authors and the client code
devs.

> Therefore adding such runtime_config feels like a step backwards.

I don't see why. It's just a convenience function which can resolve the
correct running repo instance (which is already done in a bunch of
places throughout Ecto), and can return the repo config of that instance.