-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support multiple types of DBs at once #3278
Conversation
Signed-off-by: Raphael Höser <raphael@hoeser.info>
Signed-off-by: Raphael Höser <raphael@hoeser.info>
…y! macro. Signed-off-by: Raphael Höser <raphael@hoeser.info>
Signed-off-by: Raphael Höser <raphael@hoeser.info>
This PR right now only holds a rough draft. Right now I only implmented the How this currently looks like: query!(
"SQLite",
"CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)"
).execute(&sqlite).await.unwrap();
let row = query!("PostgreSQL", "SELECT 1 as ID").fetch_one(&pg).await.unwrap(); If you now set the environment variables If you have ideas for improving this, please let me know. |
This is cool to see! I'm wondering two things about the
The reason I ask these is because I bet you if these things are possible, it would then be possible to allow the caller define any variables they like, even having multiple databases with the same driver. I.E., |
Yes, this is totally possible and I want to implement this in the future. It should even be possible to infer the actual expected varaible at compiletime via the type of the database driver (via
Also this is possible and I thought about this too. Right now this PR already kind of does this, because the names are only for differntiating and the code doesn't check that a DATABASE_URL_POSTGRES actually holds a postgres URL. You could just as well (using the patternmatching from point 1) use DATABASE_URL_FILESYSTEM and DATABASE_URL_SERVER for e.g. sqlite and postgres.
Sadly IMO this will not be possible (at least as I implemented it right now), because it will always use the first URL that matches the scheme for the DB when resolving the URL, so you can't have two sqlite DBs. IMO the most urgent usecase is supporting multiple types of DBs (as this is my biggest painpoint). Maybe we can find a way to support multiple DBs of the same type at a later point in time. |
Signed-off-by: Raphael Höser <raphael@hoeser.info>
Signed-off-by: Raphael Höser <raphael@hoeser.info>
I'm working on a version that uses types instead of strings for selecting the DB backend here: Snapstromegon#1 I'm still running into an issue when multiple backends are compatible with a query and any help is welcome. |
…ferenced Signed-off-by: Raphael Höser <raphael@hoeser.info>
Signed-off-by: Raphael Höser <raphael@hoeser.info>
Switch to a type based implementation
Hi all, I just merged my implementation for a type driven query provider selection. With this change you can use the query!(
Sqlite,
"CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)"
).execute(&sqlite).await.unwrap();
query!(
Postgres,
"CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)"
).execute(&pg).await.unwrap(); I only implemented the declarative macro rule changes for query!, but it should work basically the same for all other macros. I'd love to have some feedback from someone related to the project about what would need to be added to this PR so a feature like this can be merged. |
@abonander I saw that you were commenting in the similar PR #3397. What do you think about the approach presented here? Maybe we can introduce a solution based on this for the "simple" cases and one based on the toml files for the more complex ones? |
Sorry, I just don't think this is the right approach. Having multiple ways to do something is just more to teach and more possible confusion for the user. This would also be rather annoying to use because you'd have to remember to specify the driver every time. I'm not even sure how usage of two different drivers in the same context is supposed to work; does one of the queries just become a no-op or what? That would be a nightmare to teach. With the Later on, we can support the ability to create shadowed versions of the macros with a prefixed name, using a specific config. This would let you mix and match drivers and databases within the same crate to your heart's content, in a way that IDE autocompletion could actually possibly assist with.
That's covered by configuring the macros to emit code using the The I appreciate the effort, but unfortunately because I fundamentally disagree with the direction here, I'm going to close. |
I fully agree here.
As per my implementation only if you want to actually use multiple drivers, so it's an extra thing you need to do if you want to use this feature, which IMO needs to be done anyways.
No, if the query gets called, it gets executed. This could be useful e.g. for tools that migrate data from an Sqlite DB to a Postgres one.
If the sqlx.toml gets introduced, why not completely remove the env var entirely and move everything into the toml? To me it feels again like two ways of doing things if the env var persists. Aside from that I'll need to take a look at how exactly it will work with the sub-crates and your current solution around sqlx.toml. Personally I love that there's some movement around this, as this is right now one of my main blockers for an app I'm building. |
No, because the |
Okay, I didn't completely state the idea. Instead of having a sqlx.toml that defines the env var to use, it could have a place for putting the db url that supports using env vars for templating (like with the .env syntax). That way you could define in the simplest case: Either way, I'm fine with it. |
This PR is meant as a starting point and baseline for discussions.
It implements support for multiple types of DBs by using mutliple "DATABASE_URL_*" environment variables, falling back to the current "DATABASE_URL" variable.
This doesn't allow using multiple DBs of the same type, but two different DBs as long as they don't share a driver (so e.g. Postgres and SQLite).
fixes #121