-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Enable db structure dump
for new db layer
#163
Conversation
provider = slice.container.providers.find_and_load_provider(:db) | ||
next unless provider | ||
|
||
database_url = provider.source.send(:database_url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid unnecessary connections to the database by reaching inside the provider source and calling its #database_url
method (which we presume to be present). Right now #database_url
is a private method, but we can make it public before this PR merges.
In addition, this method of accessing a provider object itself is not proper public API within dry-system. I think we need to streamline it:
- Have
Dry::System::ProviderRegistrar#[]
invokefind_and_load_provider
so callers don't need to worry about that step (and to ensure consistent behaviour for provider registrars that aren't finalised) - Classify
Dry::System::ProviderRegistrar#[]
as@api public
- Classify
Dry::System::Provider#source
as@api public
- Possibly even consider having
Dry::System::Provider
delegate missing methods to its source for nicer ergonomics, though I think we can skip this for now.
Why have I needed to do this?
Without this change, we had to establish a database connection just to get a slice's database_url, which we already know from config alone! This is wasteful and pointless. Worse still, we would actually connect multiple times to the same database for an app with multiple slices using the same database URL.
Why do we need to call this provider.source.database_url
, though? This is to make sure we're using the "source of truth" for this database URL, which is the provider source itself, since it contains the logic to determine this URL when it eventually does connect.
It also makes testing this CLI command much harder (i.e. forcing us to have running database servers).
@solnic @flash-gordon @alassek — I'd be interested in your thoughts on this kind of provider usage. This feels reasonable to me, but it does make providers potentially "richer" objects in regular usage. I think this is safe though, since you couldn't confidently call methods against a provider's source unless you were already confident about what it should be.
An alternative would be to find some other home for this database_url logic, e.g. a static method somewhere in hanami-db itself. For this to work, though, we'd still need to send config from the provider to that method, since a user may choose to override the database_url directly in their own provider code. This is why I decided to stick with the provider as the source of truth and just use that directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a few days wait, I've gone ahead and merged this. I think it's a reasonable approach, and since this is all internal (i.e. no public API commitments), we can change it in the future if we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timriley I cannot see any problem with your approach. Is setting a URL is the only way to configure a DB connection?
d9cfff7
to
ea3b979
Compare
Because we’re no longer preparing the db provider, we’re not creating non-existing sqlite files until we really need to, so they may not exist at this point.
fe7bda1
to
683b169
Compare
I forgot to push some expanded tests before merging. FYI, they are here: cf0f88f |
@timriley I think this may indicate the need for an abstract DB Config object, which is the direction I went with Since we will undoubtedly need this anyway for hanami/hanami#1389 and hanami/hanami#1409, perhaps we should move the |
@alassek Can you please share more of how this looked for you? In terms of having an explicit config object, we'll already have this in the provider's own In what we have laid out so far, this feels like the right place to have it, right next to the code that needs to consume that config. Improve provider config is what I want to look at next. My first step was to allow provider sources to declare their own custom config classes (which is a dry-configurable feature). This would mean that e.g.
This class could also provider reader methods for getting the config back out in various structures that are useful to us. If there's anything else you think we should be considering at this stage, I'd be very happy to hear it! FWIW, I'm actually done with all the |
This is all I meant by having a configuration object, I was thinking of So rather than keeping Hanami.app.configure_provider :db do
config.rom.plugin :sql, :some_plugin do |plugin|
# plugin config goes here
# this block can then later be evaluated in the context of the provider,
# so it can access app resources if needed
end
end I like this approach for ROM plugins. Perhaps we could have something similar for adapter configuration Hanami.app.configure_provider do
config.adapter :sql do |sql|
sql.extensions += [:pg_array, :pg_json]
end
end |
Enable the
db structure dump
command for our new database layer, and add tests covering its use with Postgres. Support for other database types will follow later (see #157, #158).As part of this change, make
Hanami::Providers::DB#database_url
public API, and access it throughslice.container.providers[:db].source.database_url
(withDry::System::Provider#source
soon to be tagged as public API too).This ensures that the canonical databases across an app can be identified via their database URL strings alone, without having to establish a full connection to the database. This means all the
db
CLI commands (not juststructure dump
) can operate more efficiently. It also makes them easier to test inside the repo, since we can avoid providing the live database servers for them to connect to.Resolves #154.