Add a get_kind method to Pool

[nitnelave]

This PR addresses #1225

It adds a get_kind method to Pool, going through SharedPool and ConnectionOptions to get the (dynamic for Any) type of the connection.

This will allow query builders to build the query based on which driver (and so which dialect) is used.

Note: The sqlx-core/src/any/kind.rs was moved to sqlx-core/src/any_kind.rs because it's now needed for every DB, so it shouldn't be guarded by the any feature.

I didn't add any tests, because I'm not entirely sure where to add them, and there is not really any logic to test. The test (if any) should probably only check the interface, maybe in a test where we have a &any_pool and we check its .get_kind()?

[nitnelave]

Note that I'm not entirely sure how other libraries can build on that with the feature guards. I.e. if we take sea-query, we can add a dependency on sqlx, but how do you say things like "if sqlx is used by another dependency with this feature, then compile that": it looks more like every crate that wants to use both sea-query and sqlx will need to do the (trivial) mapping themselves.

Another way to do it would be to extract the AnyKind enum to another crate. Then at least only the mysql/sqlite/... feature guards would have to be coordinated between sqlx and sea-query. WDYT?

Note that it shouldn't block merging this, since it at least allows to do the binding in the crates that depend on both.

[nitnelave]

Can somebody review/merge this PR?

[Dessix]

The only thing I'd recommend adding here is a derive for Copy, Clone, PartialEq, Eq on AnyKind, since it can currently only be passed around by reference thanks to a lack of a Copy implementation, and can't be equated without a match.

[abonander]

@nitnelave I second the suggestion by @Dessix

Also it's a bit weird for any_kind() to live on ConnectOptions; I think I would prefer it to live on the Database trait as either an associated const or a stateless method:

pub trait Database {
    // ...

    // pick one
    const ANY_KIND: AnyKind;

    fn any_kind() -> AnyKind;
}

[nitnelave]

Sorry, i was on holidays the past 2 weeks. I'll have a look at it next week and apply the suggestions.

[nitnelave]

Thanks for the review!

@abonander It's not actually stateless: for most DBs it is, but for the Any driver it depends on the dynamic DB type (the whole point of this PR).

I also added the method to the Pool, just forwarding it to the connection options in order to have a nicer interface.

@Dessix Added the derives.

[Dessix]

@nitnelave This needs to be compliant with disablement of the any feature, as the sqlx-core::any module is entirely omitted if this feature is disabled. Additionally, you should be using the relatively new sqlx-core::any::AnyKind instead of creating the module sqlx-core::any_kind, as this type has now been exposed by that module in the main branch.

I'd suggest you do this by:

• restarting from HEAD which has significant changes to the any module
• exposing get_kind as you have
• Annotating get_kind and use clauses for conditional compilation based on the any feature being enabled. If the destination crate for some of these changes is not sqlx-core, you'll likely need cfg(feature = "sqlx-core/any") instead of just cfg(feature = "any") in those cases.

[nitnelave]

@Dessix I was trying to have the function be there regardless of whether the any feature is enabled (it could still be used for static polymorphism over the other engines) but I guess the main use case is going to be with any.

Updated as per your comment, ready for review!

[nitnelave]

Gentle ping :)

[Dessix]

Assuming I haven't missed anything, it should build with both any and not-any feature configurations, so I approve ^^

[abonander]

This is technically a backwards-incompatible change and so will need to be part of 0.6.0

[Dessix]

Additive changes are backward-compatible, unless you're describing trait-ABI compatibility? Since there is no official ABI for Rust yet, I don't think we need to consider that?

Ah, nevermind- this trait is unsealed...

It could be done without breakage by making a Kinded trait under the any module and exposing that for the implementations in question?

[abonander]

You could just have an inherent method kind(&self) -> AnyKind on AnyConnectOptions and then an inherent method on Pool<Any> that forwards to that. Would that suit your use case?

[Dessix]

Might be worth just releasing it as a breaking-change; it's not like it will benefit the performance semantics for anyone who would receive a patch update.

[nitnelave]

So what's the way forward? Should we release this as a breaking change, or implement @abonander's proposal? (sorry I lost a bit of context on that PR over the months so I'd have to get back in to understand his proposal)

[abonander]

I would prefer to just have it be an inherent method on Pool<Any> if that suits the use-case. I don't think it's necessary to change a fundamental trait like this.

[nitnelave]

Alright, I don't think I can make it any less intrusive like that, PTAL

[nitnelave]

Updated to minimize the changes, including not changing any traits. On the other hand, it only works for Any, so you can't use it to write code that is generic over the DB engine (I mean, statically generic).

PTAL, it's ready for review.

[abonander]

Looks good to me, just a few nits left.

[nitnelave]

Stop making me reduce the patch, there won't be anything left! :D
Just kidding, thanks for helping me find a more succinct way of expressing the patch.

[nitnelave]

Should we merge this? It's been approved for a week.

[abonander]

Yeah, sorry. I was waiting for it to go green but got busy and forgot.

[nitnelave]

Thanks!