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

Proposal: AnyPool compatibility with query builders (e.g. sea-query) #1225

Open
nitnelave opened this issue May 14, 2021 · 11 comments
Open

Proposal: AnyPool compatibility with query builders (e.g. sea-query) #1225

nitnelave opened this issue May 14, 2021 · 11 comments

Comments

@nitnelave
Copy link
Contributor

Hey,

When using the AnyPool connection, you have to adapt the query to the various quirks of the DB (e.g. sqlite and autoincrement). Since you cannot write truly generic queries, that severely limits the usefulness of AnyPool.

However, there are some crates specifically made to solve this problem: query builders (e.g. sea-query). They can generate the query for various DB. Since we can't extract the DB type from AnyPool, it would be great if it was aware of sea-query and could realize the query with the right engine.

(Note that query builders don't work with the compile-time verification, but it's not a problem since it doesn't work with AnyPool anyway, and it's optional)

My proposal, in more concrete terms:

  • a sea-query feature guard.
  • a method next to query and query_as that takes a sea-query and builds it with the correct builder, for all DB backends (supported), and delegation from AnyPool to the correct (dynamic) backend.

It would require calling the to_string function with the correct builder, for all query types. I also created an issue in sea-query to add a trait for all buildable query types, to facilitate this work.

If you agree with the plan, I can help put it into action.

@tyt2y3
Copy link

tyt2y3 commented May 14, 2021

sea-query author here.

I think you can write a generalized Executor with an enum to hold the database type and handle accordingly.

That say, from my understanding, Any is the common subset of any supported SQL engine. So (I guess), the SQL statement we pass to Any should also be the common subset of all SQL dialects. That means without identifier quotes (because MySQL and Postgres are not compatible) and other quirks. There could probably be an AnyQueryBuilder to output this dialect.

Otherwise, I think it is better for you to have an Executor wrapping the concrete database implementations (this is the approach we are taking in SeaORM). That way, database specific features can be used.

@nitnelave
Copy link
Contributor Author

There isn't always a common subset that's sufficient to represent all the queries (see AUTOINCREMENT for instance). And identifier quotes can be useful as well.

Any is not really exactly the common subset, but rather a dynamic dispatch over the specific engine: it seems simple enough to do a dynamic dispatch over the specific query builder to represent the query as string as needed, no?

@tyt2y3
Copy link

tyt2y3 commented May 14, 2021

@nitnelave I agree that it'd be nice to have an AnyQuery trait for down stream crates to implement that can be accepted by query_any() and query_any_as().

There are some differences at the protocol level to look out too. For example, MySQL has last_insert_id() at the connection, while Postgres rely on returning statement.

@nitnelave
Copy link
Contributor Author

I'm not sure we're talking about the same thing:

  • If I understand correctly, you're talking about queries that are turned into the "common SQL subset" language, which can be executed by Any. In that case, that can be a sea-query only change, providing an AnyQueryBuilder with the query realized as a String.
  • What I'm suggesting is instead to realize the query into a string differently based on which engine it is, dynamically. I.e. if the AnyPool contains a MySQL connection, then it would call the MySQLQueryBuilder from sea-query, and so on. That allows the query builder to take advantage of dialect-specific features.

@tyt2y3
Copy link

tyt2y3 commented May 14, 2021

Yes I understand your point. Let me illustrate:

trait AnyQuery {
fn build(&self, type: AnyKind) -> String;
}

That way any struct implementing this interface can be accepted. Edited snippet.

@nitnelave
Copy link
Contributor Author

nitnelave commented May 14, 2021

Ah, yes, that would work :) Sorry for misunderstanding.

Something like this enum?

pub enum AnyKind {
#[cfg(feature = "postgres")]
Postgres,
#[cfg(feature = "mysql")]
MySql,
#[cfg(feature = "sqlite")]
Sqlite,
#[cfg(feature = "mssql")]
Mssql,
}

@tyt2y3
Copy link

tyt2y3 commented May 14, 2021

Yes, I'd comfortably implement the AnyQuery trait as a driver behind a feature guard.

@nitnelave
Copy link
Contributor Author

I put together a draft of PR.

I ran into the limitation that the trait couldn't return String, but had to return &str, which might complicate implementation.
I'm going to try to implement it in sea-query to see if it's feasible. Maybe if we store the realized string in the query as an Option? I have to try it.

@tyt2y3
Copy link

tyt2y3 commented May 15, 2021

Isn't it the other way around, owned object easier to handle than ref?

Btw, it's better to have consent from the maintainer whether it aligns with the overall design.

@nitnelave
Copy link
Contributor Author

Okay, I've tried very hard to make it work with both &str and String without just converting everything to String, and I just can't figure it out, without propagating a generics parameter all the way to the Query type (which would be an interface break). There are ownership problems when you create the string deep inside the framework code.

A simpler solution is this interface:

let query : &str = sqlx::Select(...).build(any_pool.kind());
sqlx::query(&query).execute(&any_pool);
  • Anyway, we need to own the query outside of the statement, because otherwise it doesn't live long enough to execute
  • That greatly simplifies the change: we just need to expose the engine kind from the AnyPool and that's it. The rest can be easily implemented in sea_query with a switch on the pool kind.

WDYT?

@tyt2y3
Copy link

tyt2y3 commented May 17, 2021

Yes, I think an interface addition is a small enough change.
Hopefully this will get merged so you can implement your use case.

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

Successfully merging a pull request may close this issue.

2 participants